[Merged by Bors] - Reduce bandwidth over the VC<>BN API using dependant roots #4170

jimmygchen · 2023-04-06T05:11:11Z

Issue Addressed

Proposed Changes

See description in #4157.

In diagram form:

jimmygchen · 2023-04-06T05:39:18Z

I've pushed a first implementation, seems to work fine on a local testnet between 4 Lighthouse nodes. I'm going to do a bit more testing on this, and verify the bandwidth improvements.

jimmygchen · 2023-04-06T08:32:14Z

Some initial test numbers below from running local testnet, seems to be working fine and chain is finalizing.

Test Setup

local testnet, 4 beacon nodes
5000 validators split across 4 validator clients
Record for 1 minute
All POST requests to /eth/v1/validator/duties/attester/{epoch}

version	number of requests	total req/res size (kb)	initialization request size(kb)	subsequent request size (kb)
unstable	160	51,222	~320	~320
optimized	160	1,398	~320	~0.75

jimmygchen · 2023-04-14T06:56:03Z

Looking pretty good so far on a 5000 validator VC (see the drop below before vs after the deployment):

Attestations looks fine, will keep monitoring.

jimmygchen · 2023-04-26T08:00:07Z

Short update: I've been running some tests on this branch, and the VC seems to be querying attester duties correctly (on a local testnet) when the dependent_root is updated.

Test Setup

It is very difficult to verify this on the Goerli testnet because re-orgs that change the dependent_root slot are very rare, since intentional late block re-orgs are not allowed at epoch boundaries (spec). I wasn't able to find one looking at data between 14 April - 21 April, where I had the updated validator client deployed on Goerli.

However it is possible to verify this on a local testnet using @michaelsproul's suggestion:

we could probably make Lighthouse produce late blocks in slot 31 and re-org them at slot 0 if that would be useful

This test branch contains code that achieves the above. This would allow verifying the attestations work as expected when dependent_root is updated. I was able to observe the VC requesting a full update from BN when the dependent_root slot got re-org'd.

Additional Info

Assume the current epoch is N:

For duties in current epoch N to change, the proposer would have to re-org the last slot in epoch N-2 (reorg distance > 32)
For duties in next epoch N+1 to change, the last slot in previous epoch N-1 would have to be re-org'd, which means a reorg distance of at least 1 (if we're on slot 0 of current epoch).

…with

jimmygchen · 2023-04-27T01:17:40Z

In addition to the above re-org scenario, I've also observed two other scenarios that changes depedent_root, and both times the attester duties were updated correctly

if slot 31 arrives late, and block root of slot 30 would be used as dependent root updated once slot 31 is imported
if beacon node goes out of sync due to network issues, then latest block in the epoch would be used as dependent root until BN is sync'd up again.

More details on this HackMD page.

paulhauner

I'm impressed at how small this change ended up, nice work!

I have a few suggestions but nothing major!

paulhauner · 2023-05-02T01:45:33Z

validator_client/src/duties_service.rs

+    // request for extra data unless necessary in order to save on network bandwidth.
+    let uninitialized_validators =
+        get_uninitialized_validators(duties_service, &epoch, local_pubkeys);
+    let indices_to_request = if uninitialized_validators.len() >= INITIAL_DUTIES_QUERY_SIZE {


In the (hypothetical) case where INITIAL_DUTIES_QUERY_SIZE == 2 and uninitialized_validators == 1 I think we could fail to initialize that one uninitialized validator in the first call.

To avoid this, I think we'd want something like:

if !uninitialized_validators.is_empty() { uninitalized_validators } else { &local_indices[0..min(INITIAL_DUTIES_QUERY_SIZE, local_indices.len())] }

My suggestion removes the guarantee that we'll always request INITIAL_DUTIES_QUERY_SIZE validators, but it does ensure that we always query for all uninitialized validators. I think it's probably OK to sometimes query for less than INITIAL_DUTIES_QUERY_SIZE, especially since we current have it set to 1.

validator_client/src/duties_service.rs

paulhauner · 2023-05-02T07:20:19Z

validator_client/src/duties_service.rs

+    );
+
+    if duties_service.per_validator_metrics() {
+        update_per_validator_duty_metrics::<E>(current_slot, &new_duties);


Previously we were running this function for all duties and now we're only running it for new duties. I think that might pose a problem where we don't update the metrics to show the next epoch duties, once that next epoch arrives (assuming they haven't changed since the previous epoch).

I think we could solve this by iterating over duties_service.attesters instead.

Thanks, didn't think about this scenario! Will push a fix.

… suggestions Co-authored-by: Paul Hauner <paul@paulhauner.com>

…use into reduce-vc-bn-bandwith

paulhauner

Nice! We're almost there! Just a minor comment request and also something I missed earlier about updating the attestation slot metrics 🙏

validator_client/src/duties_service.rs

paulhauner · 2023-05-15T00:51:05Z

validator_client/src/duties_service.rs

+
+    if validators_to_update.is_empty() {
+        // No validators have conflicting (epoch, dependent_root) values or missing duties for the epoch.
+        return Ok(());


I think we need to make sure we run update_per_validator_duty_metrics so that we start reporting the next-epoch duties as the current_slot progresses past the current-epoch slot.

Since we've changed update_per_validator_duty_metrics, perhaps it makes sense to hoist it up into poll_beacon_attesters (perhaps after each call to poll_beacon_attesters_for_epoch in here)? That way we can exit this whenever we like and still be confident that update_per_validator_duty_metrics is being called.

Nice catch 🙏 , I've moved this to where you suggested!

paulhauner

Perfect, approved!

paulhauner · 2023-05-15T02:10:26Z

bors r+

## Issue Addressed #4157 ## Proposed Changes See description in #4157. In diagram form: ![reduce-attestation-bandwidth](https://user-images.githubusercontent.com/742762/230277084-f97301c1-0c5d-4fb3-92f9-91f99e4dc7d4.png) Co-authored-by: Jimmy Chen <jimmy@sigmaprime.io>

bors · 2023-05-15T04:26:05Z

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

## Issue Addressed sigp#4157 ## Proposed Changes See description in sigp#4157. In diagram form: ![reduce-attestation-bandwidth](https://user-images.githubusercontent.com/742762/230277084-f97301c1-0c5d-4fb3-92f9-91f99e4dc7d4.png) Co-authored-by: Jimmy Chen <jimmy@sigmaprime.io>

Reduce bandwidth over the VC<>BN API using dependant roots

9e47bed

jimmygchen self-assigned this Apr 6, 2023

jimmygchen added the optimization Something to make Lighthouse run more efficiently. label Apr 6, 2023

jimmygchen added 2 commits April 6, 2023 15:24

Remove extra comments

d905e8b

Update attester duties request indices condition

9cc8783

jimmygchen requested a review from paulhauner April 6, 2023 05:29

jimmygchen marked this pull request as ready for review April 6, 2023 05:29

jimmygchen added the work-in-progress PR is a work-in-progress label Apr 6, 2023

jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Apr 6, 2023

jimmygchen mentioned this pull request Apr 6, 2023

Reduce bandwidth over the VC<>BN API using dependant roots #4157

Closed

Merge remote-tracking branch 'origin/unstable' into reduce-vc-bn-band…

fa3c67a

…with

paulhauner added the v4.2.0 Q2 2023 label Apr 27, 2023

Merge branch 'unstable' into reduce-vc-bn-bandwith

a107882

paulhauner requested changes May 2, 2023

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 2, 2023

jimmygchen and others added 5 commits May 9, 2023 10:20

Merge branch 'unstable' into reduce-vc-bn-bandwith

1f56417

Optimize filtering by avoiding extra heap allocation, and apply other…

c120108

… suggestions Co-authored-by: Paul Hauner <paul@paulhauner.com>

Merge branch 'reduce-vc-bn-bandwith' of github.com:jimmygchen/lightho…

16797e0

…use into reduce-vc-bn-bandwith

Update metrics for all duties rather than just the new ones.

7d6c042

Ensure that we always query for all uninitialized validators.

cfaab2d

jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels May 15, 2023

paulhauner requested changes May 15, 2023

View reviewed changes

jimmygchen added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels May 15, 2023

Ensure duty metrics are always updated and update code comments

ab22458

jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels May 15, 2023

paulhauner approved these changes May 15, 2023

View reviewed changes

paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels May 15, 2023

bors bot changed the title ~~Reduce bandwidth over the VC<>BN API using dependant roots~~ [Merged by Bors] - Reduce bandwidth over the VC<>BN API using dependant roots May 15, 2023

bors bot closed this May 15, 2023

AgeManning mentioned this pull request Aug 22, 2023

Propose Jimmy Chen protocolguild/documentation#100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Reduce bandwidth over the VC<>BN API using dependant roots #4170

[Merged by Bors] - Reduce bandwidth over the VC<>BN API using dependant roots #4170

jimmygchen commented Apr 6, 2023

jimmygchen commented Apr 6, 2023

jimmygchen commented Apr 6, 2023 •

edited

Loading

jimmygchen commented Apr 14, 2023

jimmygchen commented Apr 26, 2023 •

edited

Loading

jimmygchen commented Apr 27, 2023 •

edited

Loading

paulhauner left a comment

paulhauner May 2, 2023

paulhauner May 2, 2023

jimmygchen May 9, 2023

paulhauner left a comment

paulhauner May 15, 2023

jimmygchen May 15, 2023

paulhauner left a comment

paulhauner commented May 15, 2023

bors bot commented May 15, 2023

[Merged by Bors] - Reduce bandwidth over the VC<>BN API using dependant roots #4170

[Merged by Bors] - Reduce bandwidth over the VC<>BN API using dependant roots #4170

Conversation

jimmygchen commented Apr 6, 2023

Issue Addressed

Proposed Changes

jimmygchen commented Apr 6, 2023

jimmygchen commented Apr 6, 2023 • edited Loading

Test Setup

jimmygchen commented Apr 14, 2023

jimmygchen commented Apr 26, 2023 • edited Loading

Test Setup

Additional Info

jimmygchen commented Apr 27, 2023 • edited Loading

paulhauner left a comment

Choose a reason for hiding this comment

paulhauner May 2, 2023

Choose a reason for hiding this comment

paulhauner May 2, 2023

Choose a reason for hiding this comment

jimmygchen May 9, 2023

Choose a reason for hiding this comment

paulhauner left a comment

Choose a reason for hiding this comment

paulhauner May 15, 2023

Choose a reason for hiding this comment

jimmygchen May 15, 2023

Choose a reason for hiding this comment

paulhauner left a comment

Choose a reason for hiding this comment

paulhauner commented May 15, 2023

bors bot commented May 15, 2023

jimmygchen commented Apr 6, 2023 •

edited

Loading

jimmygchen commented Apr 26, 2023 •

edited

Loading

jimmygchen commented Apr 27, 2023 •

edited

Loading