Add distributed flag to VC to enable support for DVT

[AgeManning]

Am interested in feedback for this PR.

There are efforts towards building out Distributed Validator Technology (DVT). We want to support this effort and make Lighthouse maximally compatible with what is being built in the space.

For an immediate (perhaps temporary) path forward, I'm suggesting we group some of the required functionality under a --distributed CLI flag, which is counter to some opinions expressed in #4248.

I've added additional reasoning to the above issue, but in the current context, there is desire to add a minimal change to the VC which I think is fine to add behind either a CLI flag or feature flag. I expect more changes to occur and my thinking is we can use this banner flag to incorporate any future changes we need to support DVT under this CLI flag.

For this PR the change is to compute sync aggregates for duties per slot, rather than to pre-compute them for each sync committee period.

[xenowits]

Thanks @AgeManning , looks good!

Seems calculating sync committee aggregations per slot is still in progress, happy to review it when implemented 🙌

[michaelsproul]

I've added a commit 8d19615 to compute sync committee selection proofs with 1-slot lookahead when --distributed is provided.

The only change for non-distributed validator clients is that selection proofs will no longer be computed in epoch-sized batches. Rather each batch is now just a single slot. I don't think this will have a substantial performance impact (positive or negative), but will be deploying to one of our testnet nodes with 10k+ keys to make sure.

[michaelsproul]

Looks like this is working as intended. Proofs are computed at the start of the previous slot (7104128=06:25:36):

Dec 05 06:25:36.353 DEBG Calculating sync selection proofs pre_compute_slot: 7104129, current_slot: 7104128, period: 867, service: duties

A validator is found to be a sync aggregator (the slot here is the proof slot, which is the slot prior, i.e. the current slot):

Dec 05 06:25:36.354 DEBG Validator is sync aggregator subnet_id: 3, slot: 7104128, validator_index: 115668, service: duties

Then, 4s into the slot 7104129, the sync aggregate is successfully published:

Dec 05 06:25:56.674 INFO Successfully published sync contributions, slot: 7104129, num_signers: 93, beacon_block_root: 0xeb8b…481e, subnet: 3, service: sync_committee

[michaelsproul]

@xenowits It would be great if you could take this PR for a spin with Obol and provide feedback 🙏

[xenowits]

Hey @michaelsproul , that's some great work!

Unfortunately, i'm out for the next two weeks. I'll review this PR on my side & pass this to the rest of my team to test.

[xenowits]

Hey @michaelsproul , I have reviewed it & the changes look solid. We'll be running this branch internally on one of our clusters & test it.

[michaelsproul]

@xenowits Great to hear! Let us know how it goes, and whether you need any other tweaks

[dB2510]

Hey @michaelsproul @AgeManning 👋

We tested these changes with our clusters and aggregations don't work as it seems like lighthouse VC is not calling selection endpoints to get aggregated selection proof as described in #4248. Will the changes to call those endpoints be included in a separate pull request, since this PR does not query those selection endpoints?

I am referring to these endpoints:

• https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Validator/submitBeaconCommitteeSelections
• https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Validator/submitSyncCommitteeSelections

Thanks

[AgeManning]

Hey @dB2510

Yep, these need to still be added. I recently saw a talk about running obol which indicated the lodestar vc was required to run an obol node. Do you know which other clients support these endpoints, is it just lodestar?

They are in the specification, I'll have a chat with the others about the best path forward from here. Supporting these endpoints will be a future PR.

[michaelsproul]

What about the builder registration overrides (#4306), do you still need that too?

It would be good to have a complete list of features required by Obol so we can know the fill scope of changes.

[michaelsproul]

@xenowits Confirmed via DM that this PR is a good step forward for Obol support, and that we should merge it to allow further testing.

The other features desired for Obol DVT are:

• Add DVT "selections" endpoints to VC #4248
• Handle unknown head during attestation publishing #5010 (nice to have, prevents error logs)

I'm still double-checking whether we need (update: we do):

• add builder registration overrides  #4306

[dapplion]

Adding the --distributed flag is good step, this PR can be merged as is. All logic is correct.

Just merged unstable to fix conflicts

[michaelsproul]

I've merged unstable again and updated the CLI tests. Let's merge once CI passes

[michaelsproul]

@Mergifyio queue

[mergify]

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.

[michaelsproul]

@Mergifyio requeue

[mergify]

requeue

✅ This pull request will be re-embarked automatically

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

[mergify]

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.

[michaelsproul]

@Mergifyio requeue

[mergify]

requeue

✅ This pull request will be re-embarked automatically

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

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 49536ff

[OisinKyne]

Hey @AgeManning, appreciate the chat last week. Here’s a synopsis of where I believe we are that (though you wrote the PR so likely have the specifics).



“it seems like lighthouse VC is not calling selection endpoints to get aggregated selection proof as described in #4248. Will the changes to call those endpoints be included in a separate pull request, since this PR does not query those selection endpoints?”



This comment above is the last key update for us on your PR. This was the other thing that’s not that important and is now closed. (So Michaels comment further down of: “I'm still double-checking whether we need (update: we do):” is no longer the case).

Calling those endpoints (attest aggregate, sync aggregate) during the flow and using the returned selection proof rather than the one generated locally to hash and determine if you’re an aggregator should be what’s left IIUC. Here is our original implementation note, and here is a kurtosis repo that might help with testing, though we’ll also happily test once you have a PR or an unstable build :)

Let me know if I can answer any other follow ups. Cheers.