Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] Generate Hive Test Fixtures #3374

Draft
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented May 18, 2023

Introduces a new generator type, api, that can be used to produce hive test fixtures.

The fixtures specify a post state file that is used by hive as checkpoint sync start point for the consensus client.

The produced tests contain a hive.yaml file that lists a series of verifications/actions that hive must perform after the client is started. At the moment, only a single Beacon API verification is supported, but this list of actions can be expanded to perform P2P tasks.

The config file configs/hive.yaml is introduced, and its only purpose is to change the fork versions in order for the clients to accept the configuration files (otherwise the consensus client (prysm) fails with "cannot add config with conflicting fork version schedule").

Decorator hive_state is also introduced and performs two tasks:

  • Changes the state's genesis time from zero to MIN_GENESIS_TIME in order for clients to accept the genesis state and post state as checkpoint sync, otherwise client (prysm) fails with "panic: zero genesis time".
  • Yields the start state as genesis.ssz_snappy, which is then used by hive to configure the consensus client

setup.py was also modified in order to set PRESET_BASE in the resulting python module to the actual value found in the config yaml file (it was previously forced to the preset name value).

Open questions:

  • The new preset file configs/hive.yaml should only be used to generate tests in the api generator, but I'm not sure how to put a limit to this.
  • There might be a simpler way to modify the fork version values without having to create an entire config file only for this.

Requires ethereum/hive#784 to run the fixtures.

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marioevz this is cool!

The new preset file configs/hive.yaml should only be used to generate tests in the api generator, but I’m not sure how to put a limit to this.

My intuition is to remove HIVE from ALL_PRESETS and add a is_hive flag to run_state_test_generators params.

And do something like:

def run_state_test_generators(runner_name: str,
                              all_mods: Dict[str, Dict[str, str]],
                              presets: Iterable[PresetBaseName] = ALL_PRESETS,
                              forks: Iterable[SpecForkName] = TESTGEN_FORKS,
                              is_hive: bool = False) -> None:
    """
    if is_hive:
        presets = list(presets) + [HIVE]  # ALL_PRESETS is a tuple  
    for preset_name in presets:
        ...

And then, set is_hive=True in the api generator.


There might be a simpler way to modify the fork version values without having to create an entire config file only for this.

We use with_config_overrides or spec_configured_state_test decorator to override certain configurations.

with_config_overrides

For example, light client sync tests use it: consensus-specs/test_sync.py at 696eac61275c5139d1b02be11b5c21e569f95d8e · ethereum/consensus-specs · GitHub

It would output the custom config file per test case: consensus-spec-tests/config.yaml at master · ethereum/consensus-spec-tests · GitHub

spec_configured_state_test

For example, merge block tests: consensus-specs/test_validate_merge_block.py at 696eac61275c5139d1b02be11b5c21e569f95d8e · ethereum/consensus-specs · GitHub

It doesn’t handle the config automatically, so we only use it in the unit tests.

Comment on lines 1 to 4
# Mainnet config

# Extends the mainnet preset
PRESET_BASE: 'mainnet'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Mainnet config
# Extends the mainnet preset
PRESET_BASE: 'mainnet'
# Hive config
# Extends the hive preset
PRESET_BASE: 'hive'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a small modification in setup.py for this change to work: It was failing because there is no hive kzg setup, so I added EQUIVALENT_KZG_SETUPS map to re-utilize the existing mainnet kzg setup.

return cls("justified")


@dataclass(kw_only=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kw_only is new in Python3.10 but eth2spec's minimum requirement is Python3.9.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed! :)

Also removed the method key since the YAML library tags the type of each exported class, and I could use this tag in hive to detect the object type, so it worked out quite nicely.



@dataclass
class EthV1BeaconStatesFinalityCheckpoints:
Copy link
Contributor

@djrtwo djrtwo May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do you create a data-class per beacon-api that you test?

How does this show up in the yaml outputs? Can you share a sample output?
I'm assuming that the intention is to utilize the class name (e.g. EthV2DebugBeaconStates) to dictate the api method to test, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, yes, it's basically a list of actions/verifications in YAML format, and the hive simulator is basically just a parser that sets up the client and follows each step from this list one by one:

- !EthV1BeaconStatesFinalityCheckpoints
  id: head
  finalized: false
  execution_optimistic: null
  data:
    previous_justified: {epoch: 0, root: '0000000000000000000000000000000000000000000000000000000000000000'}
    current_justified: {epoch: 0, root: '0000000000000000000000000000000000000000000000000000000000000000'}
    finalized: {epoch: 0, root: '0000000000000000000000000000000000000000000000000000000000000000'}
- !EthV1BeaconStatesFork
  id: head
  finalized: false
  execution_optimistic: null
  data: {previous_version: 0200000a, current_version: 0300000a, epoch: 0}

On hive's end, these look like this: https://github.com/ethereum/hive/blob/572390c1559196597a8bbdf6c1e404c34d2caad8/simulators/beacon/api/verifications.go#L520-L561

My plan was to add not only verifications but also actions like send an specific message through P2P and expect a certain output to verify.

@djrtwo
Copy link
Contributor

djrtwo commented Jul 11, 2023

@marioevz Where do you stand on this? Do you want to get this cleaned up to get merged? Or want to leave as draft for now

@marioevz
Copy link
Member Author

@marioevz Where do you stand on this? Do you want to get this cleaned up to get merged? Or want to leave as draft for now

Sorry I didn't make the time to finish this up yet, the problem at the moment is that the hive part is not really working yet, but I've been working on execution layer and haven't been able to work on the fixes yet, so there might be some small modifications to this PR based on fixes on the hive side.

Let me try to clean it up this week and have it merged, if there are any modifications based on the eventual hive fixes, I'll open up a follow up PR if that's ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants