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

Add functions for deriving light client data #2938

Merged
merged 19 commits into from
Jul 20, 2022
Merged

Add functions for deriving light client data #2938

merged 19 commits into from
Jul 20, 2022

Conversation

etan-status
Copy link
Contributor

Adds create_light_client_bootstrap and create_light_client_update
functions as a reference implementation for serving light client data.
This also enables a new test harness to verify that light client data
gets applied to a LightClientStore as expected.

Adds `create_light_client_bootstrap` and `create_light_client_update`
functions as a reference implementation for serving light client data.
This also enables a new test harness to verify that light client data
gets applied to a `LightClientStore` as expected.
@etan-status
Copy link
Contributor Author

This may also progress #2842

@dapplion
Copy link
Collaborator

🤩

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.

Good to have these new tests 👏

I left some comments and will review the tests later.

specs/altair/sync-protocol.md Outdated Show resolved Hide resolved
specs/altair/sync-protocol.md Outdated Show resolved Hide resolved
specs/altair/sync-protocol.md Outdated Show resolved Hide resolved
specs/altair/sync-protocol.md Outdated Show resolved Hide resolved

```yaml
{
update: string -- name of the `*.ssz_snappy` file to load
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest adding a note about the suffix rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far, the suffixes are just an implementation detail to avoid name clashes, the test runner does not need to know about their meaning, i.e., they can just take the filename from here and run the test without further processing the name.


def emit_slot(test, spec, state):
current_slot = state.slot
yield from []
Copy link
Contributor

Choose a reason for hiding this comment

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

we can just

  • remove this line and
  • remove yield from from every emit_slot call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is possible, but allowing the consistent yield from notion (even if not technically necessary) make the functions that append to the test script (setup_test, emit_slot, emit_update, finish_test) similar in use, which helps with readability of the usage code.

etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Jul 15, 2022
The EF test format for the LC sync protocol is modified to verify checks
after each step: ethereum/consensus-specs#2938 -
The test runner is updated accordingly.
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.

lgtm

Thank you @etan-status 🎉

Comment on lines +187 to +188
elif out_kind == "cfg":
output_part(out_kind, name, dump_yaml_fn(data, name, file_mode, cfg_yaml))
Copy link
Contributor

@hwwhww hwwhww Jul 19, 2022

Choose a reason for hiding this comment

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

Note for other reviewers:

  1. If the test case uses with_config_overrides, it will output config.yaml file with this new file format. The new format is closer to the config files (mainnet.yaml & minimal.yaml). 👍
  2. It outputs more than the *_FORK_EPOCH overridden. For Bellatrix tests:
PRESET_BASE: 'minimal'
MIN_GENESIS_ACTIVE_VALIDATOR_COUNT: 64
MIN_GENESIS_TIME: 1578009600
GENESIS_FORK_VERSION: 0x00000001
GENESIS_DELAY: 300
SECONDS_PER_SLOT: 6
SECONDS_PER_ETH1_BLOCK: 14
MIN_VALIDATOR_WITHDRAWABILITY_DELAY: 256
SHARD_COMMITTEE_PERIOD: 64
ETH1_FOLLOW_DISTANCE: 16
EJECTION_BALANCE: 16000000000
MIN_PER_EPOCH_CHURN_LIMIT: 4
CHURN_LIMIT_QUOTIENT: 32
PROPOSER_SCORE_BOOST: 40
INACTIVITY_SCORE_BIAS: 4
INACTIVITY_SCORE_RECOVERY_RATE: 16
ALTAIR_FORK_VERSION: 0x01000001
ALTAIR_FORK_EPOCH: 0
TERMINAL_TOTAL_DIFFICULTY: 115792089237316195423570985008687907853269984665640564039457584007913129638912
TERMINAL_BLOCK_HASH: 0x0000000000000000000000000000000000000000000000000000000000000000
TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH: 18446744073709551615
BELLATRIX_FORK_VERSION: 0x02000001
BELLATRIX_FORK_EPOCH: 0
  • p.s. deposit contract configurations are not included because deposit-contract.md is not part of executable pyspec.

@hwwhww hwwhww merged commit 1d2ef9f into ethereum:dev Jul 20, 2022
@etan-status etan-status deleted the lc-testsuite branch July 20, 2022 11:09
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Jul 23, 2022
The EF test format for the LC sync protocol is modified to verify checks
after each step: ethereum/consensus-specs#2938 -
The test runner is updated accordingly.
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.

None yet

4 participants