Skip to content

Commit

Permalink
fix EIP-7044 implementation when using batch verification (#5953)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
etan-status authored Feb 25, 2024
1 parent d09bf3b commit f54fa08
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 30 deletions.
2 changes: 1 addition & 1 deletion beacon_chain/consensus_object_pools/block_clearance.nim
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
14 changes: 14 additions & 0 deletions beacon_chain/spec/signatures.nim
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
39 changes: 19 additions & 20 deletions beacon_chain/spec/signatures_batch.nim
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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")

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
(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"))
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.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:
Expand Down
13 changes: 4 additions & 9 deletions beacon_chain/spec/state_transition_block.nim
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down

0 comments on commit f54fa08

Please sign in to comment.