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

integrate light client into beacon node #3557

Merged
merged 12 commits into from
Jun 7, 2022
Merged

Conversation

etan-status
Copy link
Contributor

@etan-status etan-status commented Mar 30, 2022

Adds a LightClient instance to the beacon node as preparation to
accelerate syncing in the future (optimistic sync).

  • --light-client-enable turns on the feature
  • --light-client-trusted-block-root configures block to start from

If no block root is configured, light client tracks DAG finalizedHead.

@github-actions
Copy link

github-actions bot commented Mar 30, 2022

Unit Test Results

     12 files  ±0     842 suites  ±0   56m 57s ⏱️ + 1m 4s
1 699 tests ±0  1 647 ✔️ ±0    52 💤 ±0  0 ±0 
9 893 runs  ±0  9 765 ✔️ ±0  128 💤 ±0  0 ±0 

Results for commit 24e77a9. ± Comparison against base commit f929980.

♻️ This comment has been updated with latest results.

@etan-status
Copy link
Contributor Author

  • add comment for values iterator purpose
  • proc -> func for callback implementations
  • len.uint -> lenu64 for length logging
  • a .. (b - 1) -> a ..< b for conciseness
  • queryed -> queried typo

@etan-status
Copy link
Contributor Author

Note that this PR does not yet take into account GossipSub data. Will submit that separately as it's already quite noisy. GossipSub data can be used to keep store.optimistic_header updated.

@etan-status
Copy link
Contributor Author

  • Rebase to unstable (genesisValidatorsRoot -> genesis_validators_Root).

@etan-status etan-status marked this pull request as draft May 4, 2022 13:19
Adds a `LightClient` instance to the beacon node as preparation to
accelerate syncing in the future (optimistic sync).

- `--light-client-enable` turns on the feature
- `--light-client-trusted-block-root` configures block to start from

If no block root is configured, light client tracks DAG `finalizedHead`.
@etan-status etan-status changed the title add LightClientStore and keep it in sync integrate light client into beacon node May 31, 2022
@etan-status
Copy link
Contributor Author

etan-status commented May 31, 2022

@etan-status etan-status marked this pull request as ready for review May 31, 2022 12:40
@etan-status etan-status requested a review from tersec June 1, 2022 01:16
lightClientEnable* {.
hidden
desc: "BETA: Accelerate sync using light client."
name: "light-client-enable" .}: Option[bool]
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this Option/applyConfigDefault help?

template applyConfigDefault(field: untyped): untyped =
if config.`field`.isNone:
if not metadata.configDefaults.`field`.isZeroMemory:
info "Applying network config default",
eth2Network = config.eth2Network,
`field` = metadata.configDefaults.`field`
config.`field` = some metadata.configDefaults.`field`
applyConfigDefault(lightClientEnable)
applyConfigDefault(serveLightClientData)
applyConfigDefault(importLightClientData)

show the mechanism, but it's not clear to me why this version (see also lightClientTrustedBlockRoot) is better than the more straightforward Option-free variation which sets some appropriate default and avoids the Option-.get() indirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had it at default-on for kiln/ropsten/prater, but you are right that now with the default off the straight-forward bool would also work. I guess when optimistic sync is fully implemented, that this would need to be changed to detect the case of "no user preference" vs "user selected off" and apply network specific default in first case. I can get rid of it once more and it can then be reintroduced later, or we can just keep it as is, up to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, keep as is.

@etan-status
Copy link
Contributor Author

During the Ropsten change from Altair -> Bellatrix, noticed in the logs that the LC optimistic update for the first slot was not picked up on Gossip, due to the subscription on old net being dropped immediately after bellatrix. Light client uses attested_header to derive fork digest, but sends the message based on a future signature_slot, so the topics need to be available some longer. Will change it so that Altair light client subscriptions stay around for the first bellatrix epoch as well, to avoid missing the first slot.

@etan-status etan-status merged commit 72a46bd into unstable Jun 7, 2022
@etan-status etan-status deleted the dev/etan/lc-store branch June 7, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants