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

Add GetAggregateAttestation V2 endpoint version #6511

Merged
merged 13 commits into from
Sep 6, 2024

Conversation

pedromiguelmiranda
Copy link
Contributor

Added new version (v2) of GetAggregateAttestation endpoint to support EIP-7549.
spec: ethereum/beacon-APIs#447

Copy link

github-actions bot commented Aug 27, 2024

Unit Test Results

         9 files  ±0    1 343 suites  ±0   47m 43s ⏱️ +33s
  5 141 tests +1    4 793 ✔️ +1  348 💤 ±0  0 ±0 
21 195 runs  +3  20 791 ✔️ +3  404 💤 ±0  0 ±0 

Results for commit a878da2. ± Comparison against base commit f9a4add.

♻️ This comment has been updated with latest results.

@tersec
Copy link
Contributor

tersec commented Sep 2, 2024

https://github.com/status-im/nimbus-eth2/actions/runs/10667055348/job/29568268145

2024-09-02T14:16:25.3542317Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/validator_client/attestation_service.nim(248, 63) template/generic instantiation of `async` from here
2024-09-02T14:16:25.3544530Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/validator_client/attestation_service.nim(283, 17) Error: type mismatch
2024-09-02T14:16:25.3546172Z Expression: getAggregatedAttestation(vc, slot, attestationRoot, committeeIndex, Best)
2024-09-02T14:16:25.3547078Z   [1] vc: ValidatorClientRef
2024-09-02T14:16:25.3547523Z   [2] slot: Slot
2024-09-02T14:16:25.3547931Z   [3] attestationRoot: Digest
2024-09-02T14:16:25.3548412Z   [4] committeeIndex: uint64
2024-09-02T14:16:25.3656072Z   [5] Best: ApiStrategyKind
2024-09-02T14:16:25.3656467Z 
2024-09-02T14:16:25.3656729Z Expected one of (first mismatch at [position]):
2024-09-02T14:16:25.3657569Z [3] proc getAggregatedAttestation(vc: ValidatorClientRef; slot: Slot;
2024-09-02T14:16:25.3658498Z                               committee_index: CommitteeIndex; root: Eth2Digest;
2024-09-02T14:16:25.3659324Z                               strategy: ApiStrategyKind): Future[Attestation]
2024-09-02T14:16:25.3659828Z 
2024-09-02T14:16:25.5278872Z make: *** [Makefile:447: nimbus_validator_client] Error 1
2024-09-02T14:16:25.5279590Z make: *** Waiting for unfinished jobs....

running make nimbus_validator_client locally can find this too

var res: Opt[electra.Attestation]
for _, entry in pool.electraCandidates[candidateIdx.get].mpairs():
if entry.data.index != committeeIndex.distinctBase:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

The point of the Electra changes is that attestations across committees can be aggregated.

https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.5/specs/phase0/validator.md#construct-aggregate (phase 0, i.e. pre-Electra) states that:

Construct aggregate

If the validator is selected to aggregate (is_aggregator()), they construct an aggregate attestation via the following.

Collect attestations seen via gossip during the slot that have an equivalent attestation_data to that constructed by the validator. If len(attestations) > 0, create an aggregate_attestation: Attestation with the following fields.

Data

Set aggregate_attestation.data = attestation_data where attestation_data is the AttestationData object that is the same for each individual attestation being aggregated.

Aggregation bits

Let aggregate_attestation.aggregation_bits be a Bitlist[MAX_VALIDATORS_PER_COMMITTEE] of length len(committee), where each bit set from each individual attestation is set to 0b1.

But the index field of https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.5/specs/phase0/beacon-chain.md#attestationdata is the committee index, until Electra. So this condition means that aggregation cannot happen across committees pre-Electra.

https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.5/specs/electra/validator.md#construct-attestation changes this:

Construct attestation

  • Set attestation_data.index = 0.
  • Let attestation.aggregation_bits be a Bitlist[MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT] of length len(committee), where the bit of the index of the validator in the committee is set to 0b1.
  • Let attestation.committee_bits be a Bitvector[MAX_COMMITTEES_PER_SLOT], where the bit at the index associated with the validator's committee is set to 0b1.

Note: Calling get_attesting_indices(state, attestation) should return a list of length equal to 1, containing validator_index.

And the procedure for https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.5/specs/electra/validator.md#attestation-aggregation similarly allows cross-committee index-aggregation.

Copy link
Contributor Author

@pedromiguelmiranda pedromiguelmiranda Sep 5, 2024

Choose a reason for hiding this comment

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

Yes, Electra changes will allow cross committee index-aggregationss, however, the new getAgregatedAttestationV2 endpoint version is only interested in attestations in its corresponding committee(*) (previous version, could aggregate attestations from multiple committees since committee_index is not in its params, and that 's not the objective wanted for the API according to the spec).

Also, and more important, we are still using the data.index field from phase0 (maybe as "transition/temp" structure while we still have 2 attestations pools):

  • template addAttToPool on _proc addAttestation uses data.index field to save the index obtained from get_committee_index_one
  • toElectraAttestation recovers this index and saves it on committee_bits while building the final electra aggregated attestation

This particular conditional statement in the code is to achieve the objective defined in (*) given the code particularites referred before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, indeed.

@tersec tersec merged commit 1ac9b85 into unstable Sep 6, 2024
12 checks passed
@tersec tersec deleted the dev/pedro/agg_endpoint_v2 branch September 6, 2024 12:15
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