Always use committee index 0 when getting attestation data

[chong-he]

Issue Addressed

• Validator client does not ask for committee index 0 on attestation data request #8046

Proposed Changes

Split the function publish_attestations_and_aggregates into publish_attestations and handle_aggregates, so that for attestations, only 1 task is spawned.

Additional Info

Tested with Kurtosis on minimal preset, which has a max_committees_per_slot of 4. The number of times the VC calls the BN for the endpoint /eth/v1/validator/attestation_data is given by the metric: http_api_paths_times_count{path="v1/validator/attestation_data"}

For 100 slots of data:
Before: http_api_paths_times_count{path="v1/validator/attestation_data"} 398
After: http_api_paths_times_count{path="v1/validator/attestation_data"} 100

From the test output, we see that the number of calls to the BN is reduced. On mainnet, the max_committee_per_slot is 64, the VC should only call the BN only once per slot for this endpoint

[mergify]

Some required checks have failed. Could you please take a look @chong-he? 🙏

[dapplion]

This feature conflicted in the past with DVT setups which rely on the committee index. Is that not the case anymore after Electra?

[chong-he]

This feature conflicted in the past with DVT setups which rely on the committee index. Is that not the case anymore after Electra?

Not sure which issue are you referring to about the DVT conflict?

For post-Electra, when VC requests to the BN, it must have a committee_index of 0:
https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Validator/produceAttestationData

[KaloyanTanev]

@dapplion I can speak of Charon here. We did have conflicts indeed, but it was fixed in v1.4.3 (~5 months ago), a week after Electra fork.

Edit: To be precise, we had issues when a DV cluster had mix of VCs, some asking for attestation data with committee index 0, while others for the actual committee index.

[macladson]

Do we have to handle attestations pre-Electra as well? Seems like we would only request committee 0 even pre-Electra, but I could be misunderstanding the fork flow.

Note that this will spawn twice as many tasks (one for attestations, one for aggregates) but I actually think that in this case it will improve performance despite the context-switching overhead since these tasks are likely IO bound and so can yield quicker than before. (Plus of course the huge reduction in BN calls makes this significantly better of course)

[eserilev]

Do we have to handle attestations pre-Electra as well? Seems like we would only request committee 0 even pre-Electra, but I could be misunderstanding the fork flow.

no we don't need to handle constructing pre-electra attestations. we actually removed a lot of pre-electra attestation logic when we moved to SingleAttestation

[michaelsproul]

Seeing very high values for the vc_attestation_service_task_times{attestations_http_get} metric on Hoodi (like 4s+ for the 99th percentile, and 1.5-2s for the median). I think this PR could help speed that up a lot (by reducing the number of requests), so will prioritise review and merge.

[mergify]

This pull request has merge conflicts. Could you please resolve them @chong-he? 🙏

[michaelsproul]

This change did help a lot with reducing the time the VC needs to fetch attestations from the BN. Data taken from a Hoodi VC with 10k keys, connected to an overworked BN.

[michaelsproul]

Marking this as backwards-incompat, as it will technically break pre-Electra devnets.

[mergify]

Some required checks have failed. Could you please take a look @chong-he? 🙏

[eserilev]

Nice work, LGTM!!

[mergify]

Merge Queue Status

✅ The pull request has been merged

This pull request spent 41 minutes 6 seconds in the queue, including 38 minutes 38 seconds running CI.
The checks were run on draft #8519.

Required conditions to merge

• check-success=local-testnet-success
• check-success=test-suite-success