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
5 changes: 3 additions & 2 deletions AllTests-mainnet.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ OK: 1/1 Fail: 0/1 Skip: 0/1
+ Aggregated attestations with disjoint comittee bits into a single on-chain aggregate [Pres OK
+ Attestations with disjoint comittee bits and equal data into single on-chain aggregate [Pr OK
+ Can add and retrieve simple electra attestations [Preset: mainnet] OK
+ Working with electra aggregates [Preset: mainnet] OK
```
OK: 3/3 Fail: 0/3 Skip: 0/3
OK: 4/4 Fail: 0/4 Skip: 0/4
## Attestation pool processing [Preset: mainnet]
```diff
+ Attestation from different branch [Preset: mainnet] OK
Expand Down Expand Up @@ -1040,4 +1041,4 @@ OK: 2/2 Fail: 0/2 Skip: 0/2
OK: 9/9 Fail: 0/9 Skip: 0/9

---TOTAL---
OK: 693/698 Fail: 0/698 Skip: 5/698
OK: 694/699 Fail: 0/699 Skip: 5/699
37 changes: 33 additions & 4 deletions beacon_chain/consensus_object_pools/attestation_pool.nim
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,16 @@ proc addForkChoiceVotes(
# hopefully the fork choice will heal itself over time.
error "Couldn't add attestation to fork choice, bug?", err = v.error()

func candidateIdx(pool: AttestationPool, slot: Slot): Opt[int] =
func candidateIdx(pool: AttestationPool, slot: Slot,
isElectra: bool = false): Opt[int] =
static: doAssert pool.phase0Candidates.len == pool.electraCandidates.len

let poolLength = if isElectra:
pool.electraCandidates.lenu64 else: pool.phase0Candidates.lenu64

if slot >= pool.startingSlot and
slot < (pool.startingSlot + pool.phase0Candidates.lenu64):
Opt.some(int(slot mod pool.phase0Candidates.lenu64))
slot < (pool.startingSlot + poolLength):
Opt.some(int(slot mod poolLength))
else:
Opt.none(int)

Expand Down Expand Up @@ -978,7 +983,8 @@ proc getElectraAttestationsForBlock*(
else:
default(seq[electra.Attestation])

func bestValidation(aggregates: openArray[Phase0Validation]): (int, int) =
func bestValidation(
aggregates: openArray[Phase0Validation | ElectraValidation]): (int, int) =
# Look for best validation based on number of votes in the aggregate
doAssert aggregates.len() > 0,
"updateAggregates should have created at least one aggregate"
Expand All @@ -993,6 +999,29 @@ func bestValidation(aggregates: openArray[Phase0Validation]): (int, int) =
bestIndex = i
(bestIndex, best)

func getElectraAggregatedAttestation*(
pool: var AttestationPool, slot: Slot,
attestationDataRoot: Eth2Digest, committeeIndex: CommitteeIndex):
Opt[electra.Attestation] =

let candidateIdx = pool.candidateIdx(slot)
if candidateIdx.isNone:
return Opt.none(electra.Attestation)

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.


entry.updateAggregates()

let (bestIndex, best) = bestValidation(entry.aggregates)

if res.isNone() or best > res.get().aggregation_bits.countOnes():
res = Opt.some(entry.toElectraAttestation(entry.aggregates[bestIndex]))

res

func getAggregatedAttestation*(
pool: var AttestationPool, slot: Slot, attestation_data_root: Eth2Digest):
Opt[phase0.Attestation] =
Expand Down
26 changes: 22 additions & 4 deletions beacon_chain/rpc/rest_validator_api.nim
Original file line number Diff line number Diff line change
Expand Up @@ -771,10 +771,12 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) =
makeAttestationData(epochRef, qhead.atSlot(qslot), qindex)
RestApiResponse.jsonResponse(adata)

# https://ethereum.github.io/beacon-APIs/#/Validator/getAggregatedAttestation
router.api2(MethodGet, "/eth/v1/validator/aggregate_attestation") do (
# https://ethereum.github.io/beacon-APIs/#/Validator/getAggregatedAttestationV2
router.api2(MethodGet, "/eth/v2/validator/aggregate_attestation") do (
attestation_data_root: Option[Eth2Digest],
committee_index: Option[CommitteeIndex],
slot: Option[Slot]) -> RestApiResponse:

let attestation =
block:
let qslot =
Expand All @@ -786,7 +788,18 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) =
return RestApiResponse.jsonError(Http400, InvalidSlotValueError,
$res.error())
res.get()
let qroot =
let committee_index =
block:
if committee_index.isNone():
return RestApiResponse.jsonError(Http400,
MissingCommitteeIndexValueError)
let res = committee_index.get()
if res.isErr():
return RestApiResponse.jsonError(Http400,
InvalidCommitteeIndexValueError,
$res.error())
res.get()
let root =
block:
if attestation_data_root.isNone():
return RestApiResponse.jsonError(Http400,
Expand All @@ -797,7 +810,12 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) =
InvalidAttestationDataRootValueError, $res.error())
res.get()
let res =
node.attestationPool[].getAggregatedAttestation(qslot, qroot)
block:
let contextFork = node.dag.cfg.consensusForkAtEpoch(epoch(qslot))
if contextFork >= ConsensusFork.Electra:
node.attestationPool[].getElectraAggregatedAttestation(qslot, root, committee_index)
else:
node.attestationPool[].getAggregatedAttestation(qslot, root)
if res.isNone():
return RestApiResponse.jsonError(Http400,
UnableToGetAggregatedAttestationError)
Expand Down
1 change: 1 addition & 0 deletions beacon_chain/spec/eth2_apis/rest_types.nim
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@ type
# Types based on the OAPI yaml file - used in responses to requests
GetBeaconHeadResponse* = DataEnclosedObject[Slot]
GetAggregatedAttestationResponse* = DataEnclosedObject[phase0.Attestation]
GetElectraAggregatedAttestationResponse* = DataEnclosedObject[electra.Attestation]
GetAttesterDutiesResponse* = DataRootEnclosedObject[seq[RestAttesterDuty]]
GetBlockAttestationsResponse* = DataEnclosedObject[seq[phase0.Attestation]]
GetBlockHeaderResponse* = DataOptimisticAndFinalizedObject[RestBlockHeaderInfo]
Expand Down
9 changes: 5 additions & 4 deletions beacon_chain/spec/eth2_apis/rest_validator_calls.nim
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,14 @@ proc produceAttestationDataPlain*(
meth: MethodGet.}
## https://ethereum.github.io/beacon-APIs/#/Validator/produceAttestationData

proc getAggregatedAttestationPlain*(
pedromiguelmiranda marked this conversation as resolved.
Show resolved Hide resolved
proc getAggregatedAttestationPlainV2*(
attestation_data_root: Eth2Digest,
slot: Slot
slot: Slot,
committee_index: CommitteeIndex
): RestPlainResponse {.
rest, endpoint: "/eth/v1/validator/aggregate_attestation"
rest, endpoint: "/eth/v2/validator/aggregate_attestation"
meth: MethodGet.}
## https://ethereum.github.io/beacon-APIs/#/Validator/getAggregatedAttestation
## https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Beacon/getPoolAttestationsV2

proc publishAggregateAndProofs*(
body: seq[phase0.SignedAggregateAndProof]
Expand Down
7 changes: 4 additions & 3 deletions beacon_chain/validator_client/api.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1662,6 +1662,7 @@ proc submitPoolSyncCommitteeSignature*(
proc getAggregatedAttestation*(
vc: ValidatorClientRef,
slot: Slot,
committee_index: CommitteeIndex,
root: Eth2Digest,
strategy: ApiStrategyKind
): Future[phase0.Attestation] {.async.} =
Expand All @@ -1678,7 +1679,7 @@ proc getAggregatedAttestation*(
OneThirdDuration,
ViableNodeStatus,
{BeaconNodeRole.AggregatedData},
getAggregatedAttestationPlain(it, root, slot)):
getAggregatedAttestationPlainV2(it, root, slot, committee_index)):
if apiResponse.isErr():
handleCommunicationError()
ApiResponse[GetAggregatedAttestationResponse].err(apiResponse.error)
Expand Down Expand Up @@ -1718,7 +1719,7 @@ proc getAggregatedAttestation*(
OneThirdDuration,
ViableNodeStatus,
{BeaconNodeRole.AggregatedData},
getAggregatedAttestationPlain(it, root, slot),
getAggregatedAttestationPlainV2(it, root, slot, committee_index),
getAggregatedAttestationDataScore(itresponse)):
if apiResponse.isErr():
handleCommunicationError()
Expand Down Expand Up @@ -1764,7 +1765,7 @@ proc getAggregatedAttestation*(
OneThirdDuration,
ViableNodeStatus,
{BeaconNodeRole.AggregatedData},
getAggregatedAttestationPlain(it, root, slot)):
getAggregatedAttestationPlainV2(it, root, slot, committee_index)):
if apiResponse.isErr():
handleCommunicationError()
false
Expand Down
1 change: 1 addition & 0 deletions beacon_chain/validator_client/attestation_service.nim
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ proc produceAndPublishAggregates(service: AttestationServiceRef,
let aggAttestation =
try:
await vc.getAggregatedAttestation(slot, attestationRoot,
committeeIndex,
ApiStrategyKind.Best)
except ValidatorApiError as exc:
warn "Unable to get aggregated attestation data", slot = slot,
Expand Down
30 changes: 26 additions & 4 deletions ncli/resttest-rules.json
Original file line number Diff line number Diff line change
Expand Up @@ -4480,7 +4480,7 @@
{
"topics": ["validator", "aggregate_attestation"],
"request": {
"url": "/eth/v1/validator/aggregate_attestation",
"url": "/eth/v2/validator/aggregate_attestation",
"headers": {"Accept": "application/json"}
},
"response": {
Expand All @@ -4492,7 +4492,7 @@
{
"topics": ["validator", "aggregate_attestation"],
"request": {
"url": "/eth/v1/validator/aggregate_attestation?slot=0",
"url": "/eth/v2/validator/aggregate_attestation?slot=0",
"headers": {"Accept": "application/json"}
},
"response": {
Expand All @@ -4504,7 +4504,7 @@
{
"topics": ["validator", "aggregate_attestation"],
"request": {
"url": "/eth/v1/validator/aggregate_attestation?slot=&attestation_data_root=",
"url": "/eth/v2/validator/aggregate_attestation?slot=&attestation_data_root=",
"headers": {"Accept": "application/json"}
},
"response": {
Expand All @@ -4516,7 +4516,29 @@
{
"topics": ["validator", "aggregate_attestation"],
"request": {
"url": "/eth/v1/validator/aggregate_attestation?slot=0&attestation_data_root=0x0000000000000000000000000000000000000000000000000000000000000000",
"url": "/eth/v2/validator/aggregate_attestation?slot=&attestation_data_root=&committee_index=",
"headers": {"Accept": "application/json"}
},
"response": {
"status": {"operator": "equals", "value": "400"},
"headers": [{"key": "Content-Type", "value": "application/json", "operator": "equals"}],
"body": [{"operator": "jstructcmpns", "value": {"code": 400, "message": ""}}]
}
},
{
"topics": ["validator", "aggregate_attestation"],
"request": {
"url": "/eth/v2/validator/aggregate_attestation?slot=0&attestation_data_root=0x0000000000000000000000000000000000000000000000000000000000000000",
"headers": {"Accept": "application/json"}
},
"response": {
"status": {"operator": "oneof", "value": ["400", "200"]}
}
},
{
"topics": ["validator", "aggregate_attestation"],
"request": {
"url": "/eth/v2/validator/aggregate_attestation?slot=0&attestation_data_root=0x0000000000000000000000000000000000000000000000000000000000000000&committee_index=0",
"headers": {"Accept": "application/json"}
},
"response": {
Expand Down
84 changes: 78 additions & 6 deletions tests/test_attestation_pool.nim
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ import
from std/sequtils import toSeq
from ./testbcutil import addHeadBlock

func combine(tgt: var phase0.Attestation, src: phase0.Attestation) =
func combine(tgt: var (phase0.Attestation | electra.Attestation),
src: phase0.Attestation | electra.Attestation) =
## Combine the signature and participation bitfield, with the assumption that
## the same data is being signed - if the signatures overlap, they are not
## combined.
Expand Down Expand Up @@ -849,10 +850,13 @@ suite "Attestation pool electra processing" & preset():
# We should now get both attestations for the block, but the aggregate
# should be the one with the most votes
pool[].getElectraAttestationsForBlock(state[], cache).len() == 2
# pool[].getAggregatedAttestation(2.Slot, 0.CommitteeIndex).
# get().aggregation_bits.countOnes() == 2
# pool[].getAggregatedAttestation(2.Slot, hash_tree_root(att2.data)).
# get().aggregation_bits.countOnes() == 2
pool[].getElectraAggregatedAttestation(2.Slot, combined[0].data.beacon_block_root,
0.CommitteeIndex).get().aggregation_bits.countOnes() == 2
pool[].getElectraAggregatedAttestation(2.Slot, hash_tree_root(att2.data), 0.CommitteeIndex).
get().aggregation_bits.countOnes() == 2
# requests to get and aggregate from different committees should be empty
pool[].getElectraAggregatedAttestation(
2.Slot, combined[0].data.beacon_block_root, 1.CommitteeIndex).isNone()

let
# Someone votes for a different root
Expand Down Expand Up @@ -949,4 +953,72 @@ suite "Attestation pool electra processing" & preset():
# with same data, 2 committee bits and 3 aggregation bits
attestations.len == 1
attestations[0].aggregation_bits.countOnes() == 3
attestations[0].committee_bits.countOnes() == 2
attestations[0].committee_bits.countOnes() == 2


test "Working with electra aggregates" & preset():
let
# Create an attestation for slot 1!
bc0 = get_beacon_committee(
state[], getStateField(state[], slot), 0.CommitteeIndex, cache)

var
att0 = makeElectraAttestation(
state[], state[].latest_block_root, bc0[0], cache)
att0x = att0
att1 = makeElectraAttestation(
state[], state[].latest_block_root, bc0[1], cache)
att2 = makeElectraAttestation(
state[], state[].latest_block_root, bc0[2], cache)
att3 = makeElectraAttestation(
state[], state[].latest_block_root, bc0[3], cache)

# Both attestations include member 2 but neither is a subset of the other
att0.combine(att2)
att1.combine(att2)

check:
not pool[].covers(att0.data, att0.aggregation_bits)

pool[].addAttestation(
att0, @[bc0[0], bc0[2]], att0.loadSig, att0.data.slot.start_beacon_time)
pool[].addAttestation(
att1, @[bc0[1], bc0[2]], att1.loadSig, att1.data.slot.start_beacon_time)

check:
process_slots(
defaultRuntimeConfig, state[],
getStateField(state[], slot) + MIN_ATTESTATION_INCLUSION_DELAY, cache,
info, {}).isOk()

check:
pool[].getElectraAttestationsForBlock(state[], cache).len() == 1
# Can get either aggregate here, random!
pool[].getElectraAggregatedAttestation(
1.Slot, att0.data.beacon_block_root, 0.CommitteeIndex).isSome()

# Add in attestation 3 - both aggregates should now have it added
pool[].addAttestation(
att3, @[bc0[3]], att3.loadSig, att3.data.slot.start_beacon_time)

block:
let attestations = pool[].getElectraAttestationsForBlock(state[], cache)
check:
attestations.len() == 1
attestations[0].aggregation_bits.countOnes() == 6
# Can get either aggregate here, random!
pool[].getElectraAggregatedAttestation(
1.Slot, attestations[0].data.beacon_block_root, 0.CommitteeIndex).isSome()

# Add in attestation 0 as single - attestation 1 is now a superset of the
# aggregates in the pool, so everything else should be removed
pool[].addAttestation(
att0x, @[bc0[0]], att0x.loadSig, att0x.data.slot.start_beacon_time)

block:
let attestations = pool[].getElectraAttestationsForBlock(state[], cache)
check:
attestations.len() == 1
attestations[0].aggregation_bits.countOnes() == 4
pool[].getElectraAggregatedAttestation(
1.Slot, attestations[0].data.beacon_block_root, 0.CommitteeIndex).isSome()
Loading