From c01cf6601a9200d82b017af3b7ff131c2ce6b861 Mon Sep 17 00:00:00 2001 From: tersec Date: Mon, 14 Oct 2024 14:20:26 +0000 Subject: [PATCH] attestation and aggregated attestation BN fixes for Electra (#6641) * attestation and aggregated attestation BN fixes for Electra * disable electra transition for this PR * use cleaned-up verification function everywhere --- Makefile | 2 - .../attestation_pool.nim | 44 +++++++------- .../consensus_object_pools/spec_cache.nim | 2 +- beacon_chain/spec/validator.nim | 9 +++ beacon_chain/validators/beacon_validators.nim | 26 ++++---- scripts/launch_local_testnet.sh | 6 +- tests/test_attestation_pool.nim | 60 +++++++++++++++++-- 7 files changed, 107 insertions(+), 42 deletions(-) diff --git a/Makefile b/Makefile index 14db6ea6b..6d3508f4b 100644 --- a/Makefile +++ b/Makefile @@ -92,8 +92,6 @@ TOOLS_CSV := $(subst $(SPACE),$(COMMA),$(TOOLS)) deps \ update \ test \ - clean_eth2_network_simulation_all \ - eth2_network_simulation \ clean \ libbacktrace \ book \ diff --git a/beacon_chain/consensus_object_pools/attestation_pool.nim b/beacon_chain/consensus_object_pools/attestation_pool.nim index f1523768b..3becea2a3 100644 --- a/beacon_chain/consensus_object_pools/attestation_pool.nim +++ b/beacon_chain/consensus_object_pools/attestation_pool.nim @@ -394,17 +394,21 @@ proc addAttestation( func getAttestationCandidateKey( data: AttestationData, - committee_bits: AttestationCommitteeBits = - default(AttestationCommitteeBits)): Eth2Digest = + committee_index: Opt[CommitteeIndex]): Eth2Digest = # Some callers might have used for the key just htr(data), so rather than # risk some random regression (one was caught in test suite, but there is # not any particular reason other code could not have manually calculated # the key, too), special-case the phase0 case as htr(data). - if committee_bits == static(default(typeof(committee_bits))): + if committee_index.isNone: # i.e. no committees selected, so it can't be an actual Electra attestation hash_tree_root(data) else: - hash_tree_root([hash_tree_root(data), hash_tree_root(committee_bits)]) + hash_tree_root([hash_tree_root(data), hash_tree_root(committee_index.get.uint64)]) + +func getAttestationCandidateKey( + attestationDataRoot: Eth2Digest, committee_index: CommitteeIndex): + Eth2Digest = + hash_tree_root([attestationDataRoot, hash_tree_root(committee_index.uint64)]) proc addAttestation*( pool: var AttestationPool, @@ -437,8 +441,8 @@ proc addAttestation*( # TODO withValue is an abomination but hard to use anything else too without # creating an unnecessary AttestationEntry on the hot path and avoiding # multiple lookups - template addAttToPool(attCandidates: untyped, entry: untyped) = - let attestation_data_root = hash_tree_root(entry.data) + template addAttToPool(attCandidates: untyped, entry: untyped, committee_index: untyped) = + let attestation_data_root = getAttestationCandidateKey(entry.data, committee_index) attCandidates[candidateIdx.get()].withValue(attestation_data_root, entry) do: if not addAttestation(entry[], attestation, signature): @@ -453,7 +457,7 @@ proc addAttestation*( template addAttToPool(_: phase0.Attestation) {.used.} = let newAttEntry = Phase0AttestationEntry( data: attestation.data, committee_len: attestation.aggregation_bits.len) - addAttToPool(pool.phase0Candidates, newAttEntry) + addAttToPool(pool.phase0Candidates, newAttEntry, Opt.none CommitteeIndex) pool.addForkChoiceVotes( attestation.data.slot, attesting_indices, attestation.data.beacon_block_root, wallTime) @@ -474,7 +478,7 @@ proc addAttestation*( let newAttEntry = ElectraAttestationEntry( data: data, committee_len: attestation.aggregation_bits.len) - addAttToPool(pool.electraCandidates, newAttEntry) + addAttToPool(pool.electraCandidates, newAttEntry, Opt.some committee_index) pool.addForkChoiceVotes( attestation.data.slot, attesting_indices, attestation.data.beacon_block_root, wallTime) @@ -497,7 +501,7 @@ func covers*( return false pool.phase0Candidates[candidateIdx.get()].withValue( - getAttestationCandidateKey(data), entry): + getAttestationCandidateKey(data, Opt.none CommitteeIndex), entry): if entry[].covers(bits): return true @@ -955,7 +959,11 @@ proc getElectraAttestationsForBlock*( # and score possible on-chain attestations while collecting candidates # (previous loop) and reavaluate cache key definition let - key = (entry.data.beacon_block_root, entry.data.slot) + entry2 = block: + var e2 = entry.data + e2.index = 0 + e2 + key = (hash_tree_root(entry2), entry.data.slot) newAtt = entry[].toElectraAttestation(entry[].aggregates[j]) candidatesPerBlock.withValue(key, candidate): @@ -988,25 +996,22 @@ proc getElectraAttestationsForBlock*( # Sort candidates by score use slot as a tie-breaker candidates.sort() - # Consolidate attestation aggregates with disjoint comittee bits into single + # Consolidate attestation aggregates with disjoint committee bits into single # attestation var res: seq[electra.Attestation] for a in candidatesPerBlock.values(): - if a.len > 1: - let - att = compute_on_chain_aggregate(a).valueOr: - continue + let att = compute_on_chain_aggregate(a).valueOr: + continue res.add(att) - #no on chain candidates + # no on-chain candidates else: res.add(a) if res.lenu64 == MAX_ATTESTATIONS_ELECTRA: break - let - packingDur = Moment.now() - startPackingTick + let packingDur = Moment.now() - startPackingTick debug "Packed attestations for block", newBlockSlot, packingDur, totalCandidates, attestations = res.len() @@ -1051,8 +1056,7 @@ func getElectraAggregatedAttestation*( return Opt.none(electra.Attestation) pool.electraCandidates[candidateIdx.get].withValue( - attestationDataRoot, entry): - + getAttestationCandidateKey(attestationDataRoot, committeeIndex), entry): if entry.data.index == committeeIndex.distinctBase: entry[].updateAggregates() diff --git a/beacon_chain/consensus_object_pools/spec_cache.nim b/beacon_chain/consensus_object_pools/spec_cache.nim index 33e462a4b..c6bca31f8 100644 --- a/beacon_chain/consensus_object_pools/spec_cache.nim +++ b/beacon_chain/consensus_object_pools/spec_cache.nim @@ -237,7 +237,7 @@ func get_attesting_indices_one*(shufflingRef: ShufflingRef, Opt[ValidatorIndex] = # A variation on get_attesting_indices that returns the validator index only # if only one validator index is set - static: doAssert not on_chain, "only on_chain supported" + static: doAssert not on_chain, "only not on_chain supported" var res = Opt.none(ValidatorIndex) let committee_index = ? get_committee_index_one(committee_bits) diff --git a/beacon_chain/spec/validator.nim b/beacon_chain/spec/validator.nim index edac4a78d..803fc8cb0 100644 --- a/beacon_chain/spec/validator.nim +++ b/beacon_chain/spec/validator.nim @@ -577,13 +577,22 @@ proc compute_on_chain_aggregate*( for i, a in aggregates: totalLen += a.aggregation_bits.len + # TODO doesn't work if a committee is skipped var aggregation_bits = ElectraCommitteeValidatorsBits.init(totalLen) var pos = 0 + var prev_committee_index: Opt[CommitteeIndex] for i, a in aggregates: let committee_index = ? get_committee_index_one(a.committee_bits) first = pos == 0 + when false: + if prev_committee_index.isNone: + prev_committee_index = Opt.some committee_index + elif committee_index.distinctBase <= prev_committee_index.get.distinctBase: + continue + prev_committee_index = Opt.some committee_index + for b in a.aggregation_bits: aggregation_bits[pos] = b pos += 1 diff --git a/beacon_chain/validators/beacon_validators.nim b/beacon_chain/validators/beacon_validators.nim index 4f814835a..06bf9dcb1 100644 --- a/beacon_chain/validators/beacon_validators.nim +++ b/beacon_chain/validators/beacon_validators.nim @@ -1592,7 +1592,7 @@ proc signAndSendAggregate( if not is_aggregator(shufflingRef, slot, committee_index, selectionProof): return - template signAndSendAggregate() = + template signAndSendAggregatedAttestations() = msg.signature = block: let res = await validator.getAggregateAndProofSignature( fork, genesis_validators_root, msg.message) @@ -1612,31 +1612,29 @@ proc signAndSendAggregate( if node.dag.cfg.consensusForkAtEpoch(slot.epoch) >= ConsensusFork.Electra: # https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.8/specs/electra/validator.md#construct-aggregate # https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.8/specs/electra/validator.md#aggregateandproof - var - msg = electra.SignedAggregateAndProof( - message: electra.AggregateAndProof( - aggregator_index: distinctBase validator_index, - selection_proof: selectionProof)) + var msg = electra.SignedAggregateAndProof( + message: electra.AggregateAndProof( + aggregator_index: distinctBase validator_index, + selection_proof: selectionProof)) msg.message.aggregate = node.attestationPool[].getElectraAggregatedAttestation( - slot, default(Eth2Digest), committee_index).valueOr: + slot, committee_index).valueOr: return - signAndSendAggregate() + signAndSendAggregatedAttestations() else: # https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.8/specs/phase0/validator.md#construct-aggregate # https://github.com/ethereum/consensus-specs/blob/v1.4.0/specs/phase0/validator.md#aggregateandproof - var - msg = phase0.SignedAggregateAndProof( - message: phase0.AggregateAndProof( - aggregator_index: distinctBase validator_index, - selection_proof: selectionProof)) + var msg = phase0.SignedAggregateAndProof( + message: phase0.AggregateAndProof( + aggregator_index: distinctBase validator_index, + selection_proof: selectionProof)) msg.message.aggregate = node.attestationPool[].getPhase0AggregatedAttestation( slot, committee_index).valueOr: return - signAndSendAggregate() + signAndSendAggregatedAttestations() proc sendAggregatedAttestations( node: BeaconNode, head: BlockRef, slot: Slot) = diff --git a/scripts/launch_local_testnet.sh b/scripts/launch_local_testnet.sh index 3e2142a38..8327b5d03 100755 --- a/scripts/launch_local_testnet.sh +++ b/scripts/launch_local_testnet.sh @@ -104,7 +104,7 @@ DL_GETH="0" : ${BEACON_NODE_COMMAND:="./build/nimbus_beacon_node$EXE_EXTENSION"} : ${DENEB_FORK_EPOCH:=0} -: ${ELECTRA_FORK_EPOCH:=5000} +: ${ELECTRA_FORK_EPOCH:=500} #NIMBUS EL VARS RUN_NIMBUS_ETH1="0" @@ -216,6 +216,10 @@ while true; do DENEB_FORK_EPOCH="$2" shift 2 ;; + --electra-fork-epoch) + ELECTRA_FORK_EPOCH="$2" + shift 2 + ;; --stop-at-epoch) STOP_AT_EPOCH=$2 STOP_AT_EPOCH_FLAG="--debug-stop-at-epoch=$2" diff --git a/tests/test_attestation_pool.nim b/tests/test_attestation_pool.nim index 4364296c7..51ac3a6d5 100644 --- a/tests/test_attestation_pool.nim +++ b/tests/test_attestation_pool.nim @@ -24,7 +24,7 @@ import # Test utilities ./testutil, ./testdbutil, ./testblockutil, ./consensus_spec/fixtures_utils -from std/sequtils import toSeq +from std/sequtils import mapIt, toSeq from ./testbcutil import addHeadBlock func combine(tgt: var (phase0.Attestation | electra.Attestation), @@ -898,8 +898,21 @@ suite "Attestation pool electra processing" & preset(): attestations[0].aggregation_bits.countOnes() == 2 attestations[0].committee_bits.countOnes() == 2 - test "Aggregated attestations with disjoint comittee bits into a single on-chain aggregate" & preset(): + proc verifyAttestationSignature(attestation: electra.Attestation): bool = + withState(state[]): + when consensusFork == ConsensusFork.Electra: + let + fork = pool.dag.cfg.forkAtEpoch(forkyState.data.slot.epoch) + attesting_indices = get_attesting_indices( + forkyState.data, attestation.data, attestation.aggregation_bits, + attestation.committee_bits, cache) + verify_attestation_signature( + fork, pool.dag.genesis_validators_root, attestation.data, + attesting_indices.mapIt(forkyState.data.validators.item(it).pubkey), + attestation.signature) + else: + raiseAssert "must be electra" let bc0 = get_beacon_committee( @@ -921,6 +934,11 @@ suite "Attestation pool electra processing" & preset(): attestation_3 = makeElectraAttestation( state[], state[].latest_block_root, bc1[1], cache) + check: + verifyAttestationSignature(attestation_1) + verifyAttestationSignature(attestation_2) + verifyAttestationSignature(attestation_3) + pool[].addAttestation( attestation_1, @[bc0[0]], attestation_1.loadSig, attestation_1.data.slot.start_beacon_time) @@ -942,6 +960,8 @@ suite "Attestation pool electra processing" & preset(): let attestations = pool[].getElectraAttestationsForBlock(state[], cache) check: + verifyAttestationSignature(attestations[0]) + # A single final chain aggregated attestation should be created # with same data, 2 committee bits and 3 aggregation bits attestations.len == 1 @@ -966,11 +986,35 @@ suite "Attestation pool electra processing" & preset(): att3 = makeElectraAttestation( state[], state[].latest_block_root, bc0[3], cache) + proc verifyAttestationSignature(attestation: electra.Attestation): bool = + withState(state[]): + when consensusFork == ConsensusFork.Electra: + let + fork = pool.dag.cfg.forkAtEpoch(forkyState.data.slot.epoch) + attesting_indices = get_attesting_indices( + forkyState.data, attestation.data, attestation.aggregation_bits, + attestation.committee_bits, cache) + verify_attestation_signature( + fork, pool.dag.genesis_validators_root, attestation.data, + attesting_indices.mapIt(forkyState.data.validators.item(it).pubkey), + attestation.signature) + else: + raiseAssert "must be electra" + + check: + verifyAttestationSignature(att0) + verifyAttestationSignature(att0x) + verifyAttestationSignature(att1) + verifyAttestationSignature(att2) + verifyAttestationSignature(att3) + # Both attestations include member 2 but neither is a subset of the other att0.combine(att2) att1.combine(att2) check: + verifyAttestationSignature(att0) + verifyAttestationSignature(att1) not pool[].covers(att0.data, att0.aggregation_bits) pool[].addAttestation( @@ -978,17 +1022,23 @@ suite "Attestation pool electra processing" & preset(): pool[].addAttestation( att1, @[bc0[1], bc0[2]], att1.loadSig, att1.data.slot.start_beacon_time) + for att in pool[].electraAttestations(Opt.none Slot, Opt.none CommitteeIndex): + check: verifyAttestationSignature(att) + check: process_slots( defaultRuntimeConfig, state[], getStateField(state[], slot) + MIN_ATTESTATION_INCLUSION_DELAY, cache, info, {}).isOk() + for att in pool[].electraAttestations(Opt.none Slot, Opt.none CommitteeIndex): + check: verifyAttestationSignature(att) + check: pool[].getElectraAttestationsForBlock(state[], cache).len() == 1 # Can get either aggregate here, random! - pool[].getElectraAggregatedAttestation( - 1.Slot, hash_tree_root(att0.data), 0.CommitteeIndex).isSome() + verifyAttestationSignature(pool[].getElectraAggregatedAttestation( + 1.Slot, hash_tree_root(att0.data), 0.CommitteeIndex).get) # Add in attestation 3 - both aggregates should now have it added pool[].addAttestation( @@ -999,6 +1049,7 @@ suite "Attestation pool electra processing" & preset(): check: attestations.len() == 1 attestations[0].aggregation_bits.countOnes() == 6 + #TODO: verifyAttestationSignature(attestations[0]) fails # Can get either aggregate here, random! pool[].getElectraAggregatedAttestation( 1.Slot, hash_tree_root(attestations[0].data), 0.CommitteeIndex).isSome() @@ -1013,6 +1064,7 @@ suite "Attestation pool electra processing" & preset(): check: attestations.len() == 1 attestations[0].aggregation_bits.countOnes() == 4 + verifyAttestationSignature(attestations[0]) pool[].getElectraAggregatedAttestation( 1.Slot, hash_tree_root(attestations[0].data), 0.CommitteeIndex).isSome()