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

Compute recent lightclient updates #4969

Merged
merged 13 commits into from
Jan 31, 2024

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Dec 1, 2023

Current lightclient implementation is reactive, it produces updates in response to external queries.

This PR allows the beacon node to eagerly produce lightclient updates immediately after processing each block. This approach reduces the overall cost to serve lightclient data since each update is computed at most once. Lightclient updates need access to full states to compute proofs, by computing the updates immediately after block production block replay is never necessary in normal network conditions.

In summary lightclient updates are produced in two steps:

  1. cache state proofs from the attested block
  2. use the next block's sync aggregate to produce the update

image

to not delay the import_block routine, the second step in done in a separate thread with access to the network senders to push gossip updates in a latter PR.

Since this PR only produces recent updates, it's not necessary to persist them to disk. Gossip validation rules are simplified significantly since now you just need an equality check with the most recently produced update.

Issue Addressed

Closes #4926

Next steps

@jimmygchen
Copy link
Member

Hey @dapplion thanks for this PR. I haven't got a chance to look at the gossip verification changes yet - will continue tomorrow, but overall looks great so far!

@jimmygchen jimmygchen added under-review A reviewer has only partially completed a review. and removed ready-for-review The code is ready for review labels Dec 11, 2023
@jimmygchen
Copy link
Member

Nice updates, thanks ☺️ I've left a few more questions & comments, but I think we're close!

I've got a few more things in mind, but happy to address these in a separate PRS:

  • It would be useful to capture some metrics on computing light client data, this will help evaluate the readiness to make light client data serving the default behaviour.
  • Nit: we've unified existing code to separate the "light client" into two words (i.e. light_client or LightClient), we should keep the consistency
  • We don't have any tests rn, we should add a test on the beacon_chain to verify the light client data computation.

@jimmygchen jimmygchen added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Jan 2, 2024
@dapplion
Copy link
Collaborator Author

Where should we place such test? Could be something like

Great updates! I think we can add the test to a new file dedicated for light clients under beacon_node/beacon_chain/tests. It should be pretty easy to achieve with BeaconChainHarness, we could use harness.extend_chain with some blocks and access the beacon chain using harness.chain. light_client_server_cache. This would be useful as well when we make the light client server fork-aware.

Adding a test in beacon_node/beacon_chain/tests won't be complete as the harness does not start the extra thread that calls compute_light_client_updates in the beacon_node builder.

compute_light_client_updates(

The chain instance inside the harness has None as light_client_server_tx so no update is ever produced. Is there a way to recreate the more "complete" beacon node setup without repeating code in the harness?

@jimmygchen jimmygchen added work-in-progress PR is a work-in-progress and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 30, 2024
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jan 30, 2024
@jimmygchen
Copy link
Member

jimmygchen commented Jan 30, 2024

@dapplion and I got a passing test in the simulator, which shows that it's constantly producing optimistic updates and finality updates as the chain progresses! 🎉

I haven't looked at the metrics yet on how efficient the caching is, but I think we'll be able to start running some tests on a testnet once we have #4946 (Capella & Deneb light client support) merged as well.

Ready for a final review!

Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

I've re-reviewed the PR again - I think there's no changes to the regular code path when ligth-client-server is disabled, and we've got altair light client updates available according to the simulator tests, so I'm happy with the changes now 👍 thanks @dapplion!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 30, 2024
Copy link
Collaborator Author

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Can't approve my own PR, but the last string of commits from @jimmygchen look great https://github.com/sigp/lighthouse/pull/4969/files/b41ed0759964990bec486e223cc3a25ae2c93377..a63a4886d140d19506c73ee54c4f6971654db9ad

thanks for nailing the tests!

@jimmygchen
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Jan 31, 2024

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

mergify bot added a commit that referenced this pull request Jan 31, 2024
@michaelsproul
Copy link
Member

@Mergifyio requeue

Copy link

mergify bot commented Jan 31, 2024

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

Copy link

mergify bot commented Jan 31, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at b035638

mergify bot added a commit that referenced this pull request Jan 31, 2024
@mergify mergify bot merged commit b035638 into sigp:unstable Jan 31, 2024
29 checks passed
danielrachi1 pushed a commit to danielrachi1/lighthouse that referenced this pull request Feb 14, 2024
* Compute recent lightclient updates

* Review PR

* Merge remote-tracking branch 'upstream/unstable' into lc-prod-recent-updates

* Review PR

* consistent naming

* add metrics

* revert dropping reprocessing queue

* Update light client optimistic update re-processing logic. (sigp#7)

* Add light client server simulator tests. Co-authored by @dapplion.

* Merge branch 'unstable' into fork/dapplion/lc-prod-recent-updates

* Fix lint

* Enable light client server in simulator test.

* Fix test for light client optimistic updates and finality updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
light-client ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Produce and cache light client data in the BeaconChain to support fast gossip validation and RPC processing
4 participants