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

@@ -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

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

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

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

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 =