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

Root next_sync_committee in attested_header #2932

Merged
merged 8 commits into from
Jul 12, 2022
Merged

Root next_sync_committee in attested_header #2932

merged 8 commits into from
Jul 12, 2022

Conversation

etan-status
Copy link
Contributor

LightClientUpdate structures currently use different merkle proof root
depending on the presence of finalized_header. By always rooting it in
the same state (the attested_header.state_root), logic gets simpler.

Caveats:

  • In periods of extended non-finality, update.finalized_header may now
    be outdated by several sync committee periods. The old implementation
    rejected such updates as the next_sync_committee in them was stale,
    but the new implementation can properly handle this case.
  • The next_sync_committee can no longer be considered finalized based
    on is_finality_update. Instead, waiting until finalized_header is
    in the attested_header's sync committee period is now necessary.
  • Because update.finalized_header > store.finalized_header no longer
    holds (for updates with finality), an is_better_update helper is
    added to improve best_valid_update tracking (in the past, finalized
    updates with supermajority participation would always directly apply)

This PR builds on prior work from:

`LightClientUpdate` structures currently use different merkle proof root
depending on the presence of `finalized_header`. By always rooting it in
the same state (the `attested_header.state_root`), logic gets simpler.

Caveats:
- In periods of extended non-finality, `update.finalized_header` may now
  be outdated by several sync committee periods. The old implementation
  rejected such updates as the `next_sync_committee` in them was stale,
  but the new implementation can properly handle this case.
- The `next_sync_committee` can no longer be considered finalized based
  on `is_finality_update`. Instead, waiting until `finalized_header` is
  in the `attested_header`'s sync committee period is now necessary.
- Because `update.finalized_header > store.finalized_header` no longer
  holds (for updates with finality), an `is_better_update` helper is
  added to improve `best_valid_update` tracking (in the past, finalized
  updates with supermajority participation would always directly apply)

This PR builds on prior work from:
- @hwwhww at #2829
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.

epic work!

I haven't looked into the new test cases. will do another round.

specs/altair/sync-protocol.md Show resolved Hide resolved
specs/altair/sync-protocol.md Show resolved Hide resolved
specs/altair/sync-protocol.md Outdated Show resolved Hide resolved
specs/altair/sync-protocol.md Show resolved Hide resolved
specs/altair/sync-protocol.md 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 Show resolved Hide resolved
specs/altair/sync-protocol.md Show resolved Hide resolved
specs/altair/sync-protocol.md Show resolved Hide resolved
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! Great work 👍👍

@@ -42,13 +42,18 @@ def get_sync_aggregate(spec, state, signature_slot=None):
committee_indices = compute_committee_indices(spec, signature_state)
Copy link
Contributor

Choose a reason for hiding this comment

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

note to myself: out of the scope of this PR, compute_committee_indices helper doesn't need spec parameter at all.

@hwwhww hwwhww merged commit 739587b into ethereum:dev Jul 12, 2022
@etan-status etan-status deleted the lc-attestedroot branch July 12, 2022 05:19
hujw77 added a commit to darwinia-network/darwinia-messages-sol that referenced this pull request Sep 28, 2022
hackfisher pushed a commit to darwinia-network/darwinia-messages-sol that referenced this pull request Nov 29, 2022
* replace import_next_sync_committee to sync_committee_period_update

* Root `next_sync_committee` in `attested_header`, ref: ethereum/consensus-specs#2932

* use body hash verify `latest_execution_payload_state_root`

* sync_committee_period_update after it's finalized

* revert

* test

* verify update does not skip a sync committee period

* test beacon light client updates

* skip import too old state

* test beacon light client match spec

* test beacon light client match spec

* test beacon api

* test beacon light client match prater

* test beacon light client match mainnet

* comment

* comment

* change interface

* update BeaconLCMandatoryReward

* deploy new beacon light client

* using lodestar ssz

* change interface

* beacon lc e2e test

* finish beacon lc e2e test

* update deps

* rm log

* rm log

* beacon light client migrator

* compile blc migrator

* add block_number in light client

* flatten

* deploy bnlc migrator

* bnlc migrate script

* update flatten

* test migrator

* migrate bnlc testnet

* root next_sync_committee in attested_header

* migrate bnlc on testnet

* match spec test

* migrate on mainnet

* migrate on mainnet
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.

2 participants