From 48a4955e50825a02836fcb5cc0fd86f47dc8b760 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Sun, 25 Feb 2024 15:25:26 +0100 Subject: [PATCH] fix EIP-7044 implementation when using batch verification (#5953) In #5120, EIP-7044 support got added to the state transition function to force `CAPELLA_FORK_VERSION` to be used when validiting `VoluntaryExit` messages, irrespective of their `epoch`. In #5637, similar logic was added when batch verifying BLS signatures, which is used during gossip validation (libp2p gossipsub, and req/resp). However, that logic did not match the one introduced in #5120, and only uses `CAPELLA_FORK_VERSION` when a `VoluntaryExit`'s `epoch` was set to a value `>= CAPELLA_FORK_EPOCH`. Otherwise, `BELLATRIX_FORK_VERSION` would still be used when validating `VoluntaryExit`, e.g., with `epoch` set to `0`, as is the case in this Holesky block: - https://holesky.beaconcha.in/slot/1076985#voluntary-exits Extracting the correct logic from #5120 into a function, and reusing it when verifying BLS signatures fixes this issue, and also leverages the exhaustive EF test suite that covers the (correct) #5120 logic. This fix only affects networks that have EIP-7044 applied (post-Deneb). Without the fix, Deneb blocks with a `VoluntaryExit` with `epoch` set to `< CAPELLA_FORK_EPOCH` incorrectly fail to validate despite being valid. Incorrect blocks that contain a malicious `VoluntaryExit` with `epoch` set to `< CAPELLA_FORK_EPOCH` and signed using `BELLATRIX_FORK_VERSION` _would_ pass the BLS verification stage, but subsequently fail the state transition logic. Such blocks would still correctly be labeled invalid. --- .../block_clearance.nim | 2 +- beacon_chain/spec/signatures.nim | 14 +++++++ beacon_chain/spec/signatures_batch.nim | 37 +++++++++---------- beacon_chain/spec/state_transition_block.nim | 13 ++----- 4 files changed, 37 insertions(+), 29 deletions(-) diff --git a/beacon_chain/consensus_object_pools/block_clearance.nim b/beacon_chain/consensus_object_pools/block_clearance.nim index c3c778f27..4b2a5e37f 100644 --- a/beacon_chain/consensus_object_pools/block_clearance.nim +++ b/beacon_chain/consensus_object_pools/block_clearance.nim @@ -285,7 +285,7 @@ proc addHeadBlockWithParent*( var sigs: seq[SignatureSet] if (let e = sigs.collectSignatureSets( signedBlock, dag.db.immutableValidators, - dag.clearanceState, dag.cfg.genesisFork(), dag.cfg.capellaFork(), + dag.clearanceState, dag.cfg.genesisFork(), dag.cfg.CAPELLA_FORK_VERSION, cache); e.isErr()): # A PublicKey or Signature isn't on the BLS12-381 curve info "Unable to load signature sets", diff --git a/beacon_chain/spec/signatures.nim b/beacon_chain/spec/signatures.nim index 8c185e1f5..5d5a946b3 100644 --- a/beacon_chain/spec/signatures.nim +++ b/beacon_chain/spec/signatures.nim @@ -216,6 +216,20 @@ func compute_voluntary_exit_signing_root*( fork, DOMAIN_VOLUNTARY_EXIT, epoch, genesis_validators_root) compute_signing_root(voluntary_exit, domain) +func voluntary_exit_signature_fork*( + consensusFork: static ConsensusFork, + state_fork: Fork, + capella_fork_version: Version): Fork = + when consensusFork >= ConsensusFork.Deneb: + # Always use Capella fork version, disregarding `VoluntaryExit` epoch + # [Modified in Deneb:EIP7044] + Fork( + previous_version: capella_fork_version, + current_version: capella_fork_version, + epoch: GENESIS_EPOCH) # irrelevant when current/previous identical + else: + state_fork + func get_voluntary_exit_signature*( fork: Fork, genesis_validators_root: Eth2Digest, voluntary_exit: VoluntaryExit, diff --git a/beacon_chain/spec/signatures_batch.nim b/beacon_chain/spec/signatures_batch.nim index 3ad13d630..128c69ec4 100644 --- a/beacon_chain/spec/signatures_batch.nim +++ b/beacon_chain/spec/signatures_batch.nim @@ -235,7 +235,7 @@ proc collectSignatureSets*( validatorKeys: openArray[ImmutableValidatorData2], state: ForkedHashedBeaconState, genesis_fork: Fork, - capella_fork: Fork, + capella_fork_version: Version, cache: var StateCache): Result[void, cstring] = ## Collect all signature verifications that process_block would normally do ## except deposits, in one go. @@ -385,25 +385,24 @@ proc collectSignatureSets*( # SSZ deserialization guarantees that blocks received from random sources # including peer or RPC # have at most MAX_VOLUNTARY_EXITS voluntary exits. - for i in 0 ..< signed_block.message.body.voluntary_exits.len: - # don't use "items" for iterating over large type - # due to https://github.com/nim-lang/Nim/issues/14421 - # fixed in 1.4.2 - template volex: untyped = signed_block.message.body.voluntary_exits[i] - let key = validatorKeys.load(volex.message.validator_index).valueOr: - return err("collectSignatureSets: invalid voluntary exit") + if signed_block.message.body.voluntary_exits.len > 0: + let voluntary_exit_fork = withConsensusFork(state.kind): + consensusFork.voluntary_exit_signature_fork(fork, capella_fork_version) + for i in 0 ..< signed_block.message.body.voluntary_exits.len: + # don't use "items" for iterating over large type + # due to https://github.com/nim-lang/Nim/issues/14421 + # fixed in 1.4.2 + template volex: untyped = signed_block.message.body.voluntary_exits[i] + let key = validatorKeys.load(volex.message.validator_index).valueOr: + return err("collectSignatureSets: invalid voluntary exit") - sigs.add voluntary_exit_signature_set( - # https://eips.ethereum.org/EIPS/eip-7044 - # https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.4/specs/deneb/beacon-chain.md#modified-process_voluntary_exit - (if state.kind >= ConsensusFork.Capella: - capella_fork - else: - fork), - genesis_validators_root, volex.message, key, - volex.signature.load.valueOr do: - return err( - "collectSignatureSets: cannot load voluntary exit signature")) + sigs.add voluntary_exit_signature_set( + # https://eips.ethereum.org/EIPS/eip-7044 + # https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.7/specs/deneb/beacon-chain.md#modified-process_voluntary_exit + voluntary_exit_fork, genesis_validators_root, volex.message, key, + volex.signature.load.valueOr do: + return err( + "collectSignatureSets: cannot load voluntary exit signature")) block: when signed_block is phase0.SignedBeaconBlock: diff --git a/beacon_chain/spec/state_transition_block.nim b/beacon_chain/spec/state_transition_block.nim index 337a6d4ca..03cca1154 100644 --- a/beacon_chain/spec/state_transition_block.nim +++ b/beacon_chain/spec/state_transition_block.nim @@ -379,16 +379,11 @@ proc check_voluntary_exit*( # Verify signature if skipBlsValidation notin flags: - let exitSignatureFork = - when typeof(state).kind >= ConsensusFork.Deneb: - Fork( - previous_version: cfg.CAPELLA_FORK_VERSION, - current_version: cfg.CAPELLA_FORK_VERSION, - epoch: cfg.CAPELLA_FORK_EPOCH) - else: - state.fork + const consensusFork = typeof(state).kind + let voluntary_exit_fork = consensusFork.voluntary_exit_signature_fork( + state.fork, cfg.CAPELLA_FORK_VERSION) if not verify_voluntary_exit_signature( - exitSignatureFork, state.genesis_validators_root, voluntary_exit, + voluntary_exit_fork, state.genesis_validators_root, voluntary_exit, validator[].pubkey, signed_voluntary_exit.signature): return err("Exit: invalid signature")