From 9e9db216c52209f2396ef3af0dfc34ece914de9c Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Tue, 23 Aug 2022 17:30:46 +0200 Subject: [PATCH] Harden block proposal against expired slashings/exits (#4013) * Harden block proposal against expired slashings/exits When a message is signed in a phase0 domain, it can no longer be validated under bellatrix due to the correct fork no longer being available in the `BeaconState`. To ensure that all slashing/exits are still valid, in this PR we re-run the checks in the state that we're proposing for, thus hardening against both signatures and other changes in the state that might have invalidated the message. * fix same message added multiple times in case of attestation slashing of multiple validators in one go --- AllTests-mainnet.md | 5 +- .../consensus_object_pools/exit_pool.nim | 59 +++++--- beacon_chain/validators/validator_duties.nim | 2 +- tests/test_exit_pool.nim | 132 +++++++++++++++--- tests/testblockutil.nim | 4 +- 5 files changed, 153 insertions(+), 49 deletions(-) diff --git a/AllTests-mainnet.md b/AllTests-mainnet.md index 7ec389dca9..b1939e9661 100644 --- a/AllTests-mainnet.md +++ b/AllTests-mainnet.md @@ -172,8 +172,9 @@ OK: 3/3 Fail: 0/3 Skip: 0/3 + addExitMessage/getAttesterSlashingMessage OK + addExitMessage/getProposerSlashingMessage OK + addExitMessage/getVoluntaryExitMessage OK ++ pre-pre-fork voluntary exit OK ``` -OK: 3/3 Fail: 0/3 Skip: 0/3 +OK: 4/4 Fail: 0/4 Skip: 0/4 ## Fee recipient management [Beacon Node] [Preset: mainnet] ```diff + Configuring the fee recpient [Beacon Node] [Preset: mainnet] OK @@ -595,4 +596,4 @@ OK: 1/1 Fail: 0/1 Skip: 0/1 OK: 9/9 Fail: 0/9 Skip: 0/9 ---TOTAL--- -OK: 332/337 Fail: 0/337 Skip: 5/337 +OK: 333/338 Fail: 0/338 Skip: 5/338 diff --git a/beacon_chain/consensus_object_pools/exit_pool.nim b/beacon_chain/consensus_object_pools/exit_pool.nim index 481a9de267..5e1fd2e8a3 100644 --- a/beacon_chain/consensus_object_pools/exit_pool.nim +++ b/beacon_chain/consensus_object_pools/exit_pool.nim @@ -16,11 +16,11 @@ import # Status libraries chronicles, # Internal - ../spec/helpers, - ../spec/datatypes/[phase0, altair], + ../spec/datatypes/base, + ../spec/[helpers, state_transition_block], "."/[attestation_pool, blockchain_dag] -export phase0, altair, merge, deques, sets, blockchain_dag +export base, deques, sets, blockchain_dag logScope: topics = "exitpool" @@ -125,8 +125,20 @@ func addMessage*(pool: var ExitPool, msg: SignedVoluntaryExit) = msg.message.validator_index) pool.voluntary_exits.addExitMessage(msg, VOLUNTARY_EXITS_BOUND) -func getExitMessagesForBlock( - subpool: var Deque, validators: auto, seen: var HashSet, output: var List) = +proc validateExitMessage( + cfg: RuntimeConfig, state: ForkyBeaconState, msg: ProposerSlashing): bool = + check_proposer_slashing(state, msg, {}).isOk +proc validateExitMessage( + cfg: RuntimeConfig, state: ForkyBeaconState, msg: AttesterSlashing): bool = + check_attester_slashing(state, msg, {}).isOk +proc validateExitMessage( + cfg: RuntimeConfig, state: ForkyBeaconState, msg: SignedVoluntaryExit): + bool = + check_voluntary_exit(cfg, state, msg, {}).isOk + +proc getExitMessagesForBlock( + subpool: var Deque, cfg: RuntimeConfig, state: ForkyBeaconState, + seen: var HashSet, output: var List) = # Approach taken here is to simply collect messages, effectively, a circular # buffer and only re-validate that they haven't already found themselves out # of the network eventually via some exit message at block construction time @@ -136,10 +148,9 @@ func getExitMessagesForBlock( # it's a different type, REJECT. Neither is worth packaging into BeaconBlock # messages we broadcast. # - # Beyond that, no other criterion of the exit messages' validity changes from - # when they were created, so given that we validated them to start with, they - # otherwise remain as valid as when we received them. There's no need to thus - # re-validate them on their way out. + # Beyond that, it may happen that messages were signed in an epoch pre-dating + # the current state by two or more forks - such messages can no longer be + # validated in the context of the given state and are therefore dropped. # # This overall approach handles a scenario wherein we receive an exit message # over gossip and put it in the pool; receive a block X, with that message in @@ -150,33 +161,35 @@ func getExitMessagesForBlock( while subpool.len > 0 and output.len < output.maxLen: # Prefer recent messages let exit_message = subpool.popLast() + # Re-check that message is still valid in the state that we're proposing + if not validateExitMessage(cfg, state, exit_message): + continue + var skip = false for slashed_index in getValidatorIndices(exit_message): - if validators.lenu64 <= slashed_index: - continue - if validators[slashed_index].exit_epoch != FAR_FUTURE_EPOCH: - continue if seen.containsOrIncl(slashed_index): - continue - - if not output.add exit_message: + skip = true break + if skip: + continue + + if not output.add exit_message: + break subpool.clear() -func getBeaconBlockExits*(pool: var ExitPool, state: ForkyBeaconState): BeaconBlockExits = +proc getBeaconBlockExits*( + pool: var ExitPool, cfg: RuntimeConfig, state: ForkyBeaconState): + BeaconBlockExits = var indices: HashSet[uint64] res: BeaconBlockExits getExitMessagesForBlock( - pool.attester_slashings, state.validators, indices, - res.attester_slashings) + pool.attester_slashings, cfg, state, indices, res.attester_slashings) getExitMessagesForBlock( - pool.proposer_slashings, state.validators, indices, - res.proposer_slashings) + pool.proposer_slashings, cfg, state, indices, res.proposer_slashings) getExitMessagesForBlock( - pool.voluntary_exits, state.validators, indices, - res.voluntary_exits) + pool.voluntary_exits, cfg, state, indices, res.voluntary_exits) res diff --git a/beacon_chain/validators/validator_duties.nim b/beacon_chain/validators/validator_duties.nim index e08963c826..ce08e167a6 100644 --- a/beacon_chain/validators/validator_duties.nim +++ b/beacon_chain/validators/validator_duties.nim @@ -477,7 +477,7 @@ proc makeBeaconBlockForHeadAndSlot*( let exits = withState(state): - node.exitPool[].getBeaconBlockExits(state.data) + node.exitPool[].getBeaconBlockExits(node.dag.cfg, state.data) effectiveExecutionPayload = if executionPayload.isSome: executionPayload.get diff --git a/tests/test_exit_pool.nim b/tests/test_exit_pool.nim index 58acac40c8..fa1d8c8cf5 100644 --- a/tests/test_exit_pool.nim +++ b/tests/test_exit_pool.nim @@ -7,26 +7,76 @@ {.used.} -import chronos -import ../beacon_chain/spec/[datatypes/base, forks, presets] -import ../beacon_chain/consensus_object_pools/[ - block_quarantine, blockchain_dag, exit_pool] -import "."/[testutil, testdbutil] +import + ../beacon_chain/spec/[ + datatypes/base, forks, presets, signatures, state_transition], + ../beacon_chain/consensus_object_pools/[ + block_quarantine, blockchain_dag, exit_pool], + "."/[testutil, testblockutil, testdbutil] + +func makeSignedBeaconBlockHeader( + fork: Fork, genesis_validators_root: Eth2Digest, slot: Slot, + proposer_index: uint64, parent_root: Eth2Digest): SignedBeaconBlockHeader = + let tmp = BeaconBlockHeader( + slot: slot, proposer_index: proposer_index, parent_root: parent_root) + + SignedBeaconBlockHeader( + message: tmp, + signature: get_block_signature( + fork, genesis_validators_root, slot, hash_tree_root(tmp), + MockPrivKeys[proposer_index]).toValidatorSig()) + +func makeIndexedAttestation( + fork: Fork, genesis_validators_root: Eth2Digest, slot: Slot, + validator_index: uint64, beacon_block_root: Eth2Digest): IndexedAttestation = + let tmp = AttestationData(slot: slot, beacon_block_root: beacon_block_root) + + IndexedAttestation( + data: tmp, + attesting_indices: List[uint64, Limit MAX_VALIDATORS_PER_COMMITTEE](@[validator_index]), + signature: get_attestation_signature( + fork, genesis_validators_root, tmp, + MockPrivKeys[validator_index]).toValidatorSig) + +func makeSignedVoluntaryExit( + fork: Fork, genesis_validators_root: Eth2Digest, epoch: Epoch, + validator_index: uint64): SignedVoluntaryExit = + let tmp = VoluntaryExit(epoch: epoch, validator_index: validator_index) + + SignedVoluntaryExit( + message: tmp, + signature: get_voluntary_exit_signature( + fork, genesis_validators_root, tmp, + MockPrivKeys[validator_index]).toValidatorSig) suite "Exit pool testing suite": setup: let + cfg = block: + var tmp = defaultRuntimeConfig + tmp.ALTAIR_FORK_EPOCH = Epoch(tmp.SHARD_COMMITTEE_PERIOD) + tmp.BELLATRIX_FORK_EPOCH = Epoch(tmp.SHARD_COMMITTEE_PERIOD) + 1 + tmp + validatorMonitor = newClone(ValidatorMonitor.init()) dag = init( - ChainDAGRef, defaultRuntimeConfig, makeTestDB(SLOTS_PER_EPOCH * 3), + ChainDAGRef, cfg, makeTestDB(SLOTS_PER_EPOCH * 3), validatorMonitor, {}) + fork = dag.forkAtEpoch(Epoch(0)) + genesis_validators_root = dag.genesis_validators_root pool = newClone(ExitPool.init(dag)) test "addExitMessage/getProposerSlashingMessage": for i in 0'u64 .. MAX_PROPOSER_SLASHINGS + 5: for j in 0'u64 .. i: - let msg = ProposerSlashing(signed_header_1: SignedBeaconBlockHeader( - message: BeaconBlockHeader(proposer_index: j))) + let + msg = ProposerSlashing( + signed_header_1: + makeSignedBeaconBlockHeader( + fork, genesis_validators_root, Slot(1), j, makeFakeHash(0)), + signed_header_2: + makeSignedBeaconBlockHeader( + fork, genesis_validators_root, Slot(1), j, makeFakeHash(1))) if i == 0: check not pool[].isSeen(msg) @@ -35,18 +85,19 @@ suite "Exit pool testing suite": check: pool[].isSeen(msg) withState(dag.headState): check: - pool[].getBeaconBlockExits(state.data).proposer_slashings.lenu64 == + pool[].getBeaconBlockExits(cfg, state.data).proposer_slashings.lenu64 == min(i + 1, MAX_PROPOSER_SLASHINGS) - pool[].getBeaconBlockExits(state.data).proposer_slashings.len == 0 + pool[].getBeaconBlockExits(cfg, state.data).proposer_slashings.len == 0 test "addExitMessage/getAttesterSlashingMessage": for i in 0'u64 .. MAX_ATTESTER_SLASHINGS + 5: for j in 0'u64 .. i: - let msg = AttesterSlashing( - attestation_1: IndexedAttestation(attesting_indices: - List[uint64, Limit MAX_VALIDATORS_PER_COMMITTEE](@[j])), - attestation_2: IndexedAttestation(attesting_indices: - List[uint64, Limit MAX_VALIDATORS_PER_COMMITTEE](@[j]))) + let + msg = AttesterSlashing( + attestation_1: makeIndexedAttestation( + fork, genesis_validators_root, Slot(1), j, makeFakeHash(0)), + attestation_2: makeIndexedAttestation( + fork, genesis_validators_root, Slot(1), j, makeFakeHash(1))) if i == 0: check not pool[].isSeen(msg) @@ -55,22 +106,61 @@ suite "Exit pool testing suite": check: pool[].isSeen(msg) withState(dag.headState): check: - pool[].getBeaconBlockExits(state.data).attester_slashings.lenu64 == + pool[].getBeaconBlockExits(cfg, state.data).attester_slashings.lenu64 == min(i + 1, MAX_ATTESTER_SLASHINGS) - pool[].getBeaconBlockExits(state.data).attester_slashings.len == 0 + pool[].getBeaconBlockExits(cfg, state.data).attester_slashings.len == 0 test "addExitMessage/getVoluntaryExitMessage": + # Need to advance state or it will not accept voluntary exits + var + cache: StateCache + info: ForkedEpochInfo + process_slots( + dag.cfg, dag.headState, + Epoch(dag.cfg.SHARD_COMMITTEE_PERIOD).start_slot + 1, cache, info, + {}).expect("ok") + let + fork = dag.forkAtEpoch(dag.headState.get_current_epoch()) + for i in 0'u64 .. MAX_VOLUNTARY_EXITS + 5: for j in 0'u64 .. i: - let msg = SignedVoluntaryExit(message: VoluntaryExit(validator_index: j)) - + # Cannot exit until + let msg = makeSignedVoluntaryExit( + fork, genesis_validators_root, dag.headState.get_current_epoch(), j) if i == 0: check not pool[].isSeen(msg) pool[].addMessage(msg) check: pool[].isSeen(msg) + withState(dag.headState): check: - pool[].getBeaconBlockExits(state.data).voluntary_exits.lenu64 == + pool[].getBeaconBlockExits(cfg, state.data).voluntary_exits.lenu64 == min(i + 1, MAX_VOLUNTARY_EXITS) - pool[].getBeaconBlockExits(state.data).voluntary_exits.len == 0 + pool[].getBeaconBlockExits(cfg, state.data).voluntary_exits.len == 0 + + test "pre-pre-fork voluntary exit": + var + cache: StateCache + info: ForkedEpochInfo + process_slots( + dag.cfg, dag.headState, + Epoch(dag.cfg.SHARD_COMMITTEE_PERIOD).start_slot + 1, cache, info, + {}).expect("ok") + + let msg = makeSignedVoluntaryExit( + fork, genesis_validators_root, dag.headState.get_current_epoch(), 0) + + pool[].addMessage(msg) + check: pool[].isSeen(msg) + + process_slots( + dag.cfg, dag.headState, + (Epoch(dag.cfg.SHARD_COMMITTEE_PERIOD) + 1).start_slot + 1, cache, info, + {}).expect("ok") + + withState(dag.headState): + check: + # Message signed with a (fork-2) domain can no longer be added as that + # fork is not present in the BeaconState and thus fails transition + pool[].getBeaconBlockExits(cfg, state.data).voluntary_exits.lenu64 == 0 diff --git a/tests/testblockutil.nim b/tests/testblockutil.nim index f093331689..796e5f8294 100644 --- a/tests/testblockutil.nim +++ b/tests/testblockutil.nim @@ -23,14 +23,14 @@ const MockPubKeys* = MockPubKeysT() # https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.3/tests/core/pyspec/eth2spec/test/helpers/keys.py -func `[]`*(_: MockPrivKeysT, index: ValidatorIndex): ValidatorPrivKey = +func `[]`*(_: MockPrivKeysT, index: ValidatorIndex|uint64): ValidatorPrivKey = # 0 is not a valid BLS private key - 1000 helps interop with rust BLS library, # lighthouse. EF tests use 1 instead of 1000. var bytes = (index.uint64 + 1000'u64).toBytesLE() static: doAssert sizeof(bytes) <= sizeof(result) copyMem(addr result, addr bytes, sizeof(bytes)) -func `[]`*(_: MockPubKeysT, index: ValidatorIndex): ValidatorPubKey = +func `[]`*(_: MockPubKeysT, index: ValidatorIndex|uint64): ValidatorPubKey = MockPrivKeys[index].toPubKey().toPubKey() func makeFakeHash*(i: int): Eth2Digest =