From 836f6984bb5b34f90e8b7326733b46a0aeee00cc Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Mon, 17 Jan 2022 12:19:58 +0100 Subject: [PATCH] move `state_transition` to `Result` (#3284) * better error messages in api * avoid `BlockData` copies when replaying blocks --- .../block_clearance.nim | 14 +- .../consensus_object_pools/blockchain_dag.nim | 52 ++--- beacon_chain/rpc/rpc_validator_api.nim | 9 +- beacon_chain/spec/forks.nim | 5 + beacon_chain/spec/state_transition.nim | 188 ++++++------------ beacon_chain/sync/sync_protocol.nim | 2 +- beacon_chain/validators/validator_duties.nim | 22 +- ncli/ncli.nim | 12 +- ncli/ncli_db.nim | 46 ++--- nfuzz/libnfuzz.nim | 3 +- .../altair/test_fixture_sanity_blocks.nim | 8 +- .../altair/test_fixture_sanity_slots.nim | 2 +- .../altair/test_fixture_sync_protocol.nim | 8 +- .../altair/test_fixture_transition.nim | 8 +- .../bellatrix/test_fixture_sanity_blocks.nim | 8 +- .../bellatrix/test_fixture_sanity_slots.nim | 2 +- .../bellatrix/test_fixture_transition.nim | 8 +- .../phase0/test_fixture_sanity_blocks.nim | 9 +- .../phase0/test_fixture_sanity_slots.nim | 2 +- tests/mocking/mock_blocks.nim | 2 +- tests/test_attestation_pool.nim | 22 +- tests/test_blockchain_dag.nim | 16 +- tests/test_gossip_validation.nim | 2 +- tests/test_helpers.nim | 5 +- tests/test_spec.nim | 21 +- tests/testblockutil.nim | 5 +- tests/teststateutil.nim | 4 +- vendor/nim-stew | 2 +- 28 files changed, 223 insertions(+), 264 deletions(-) diff --git a/beacon_chain/consensus_object_pools/block_clearance.nim b/beacon_chain/consensus_object_pools/block_clearance.nim index 3a04cba87..1e09764c7 100644 --- a/beacon_chain/consensus_object_pools/block_clearance.nim +++ b/beacon_chain/consensus_object_pools/block_clearance.nim @@ -109,14 +109,14 @@ proc checkStateTransition( doAssert v.addr == addr dag.clearanceState.data assign(dag.clearanceState, dag.headState) - logScope: - blockRoot = shortLog(signedBlock.root) - blck = shortLog(signedBlock.message) - - if not state_transition_block( + let res = state_transition_block( dag.cfg, dag.clearanceState.data, signedBlock, - cache, dag.updateFlags, restore): - info "Invalid block" + cache, dag.updateFlags, restore) + if res.isErr(): + info "Invalid block", + blockRoot = shortLog(signedBlock.root), + blck = shortLog(signedBlock.message), + error = res.error() err(BlockError.Invalid) else: diff --git a/beacon_chain/consensus_object_pools/blockchain_dag.nim b/beacon_chain/consensus_object_pools/blockchain_dag.nim index 94744d9ec..d637bb65d 100644 --- a/beacon_chain/consensus_object_pools/blockchain_dag.nim +++ b/beacon_chain/consensus_object_pools/blockchain_dag.nim @@ -810,9 +810,8 @@ proc getForkedBlock*(dag: ChainDAGRef, id: BlockId): Opt[ForkedTrustedSignedBeac return ok ForkedTrustedSignedBeaconBlock.init(data.get) proc getForkedBlock*(dag: ChainDAGRef, blck: BlockRef): ForkedTrustedSignedBeaconBlock = - let blck = dag.getForkedBlock(blck.bid) - if blck.isSome(): - return blck.get() + dag.getForkedBlock(blck.bid).expect( + "BlockRef block should always load, database corrupt?") proc get*(dag: ChainDAGRef, blck: BlockRef): BlockData = ## Retrieve the associated block body of a block reference @@ -841,10 +840,9 @@ proc advanceSlots( let preEpoch = getStateField(state.data, slot).epoch loadStateCache(dag, cache, state.blck, getStateField(state.data, slot).epoch) - doAssert process_slots( - dag.cfg, state.data, getStateField(state.data, slot) + 1, cache, info, - dag.updateFlags), - "process_slots shouldn't fail when state slot is correct" + process_slots( + dag.cfg, state.data, getStateField(state.data, slot) + 1, cache, info, + dag.updateFlags).expect("process_slots shouldn't fail when state slot is correct") if save: dag.putState(state) @@ -860,28 +858,36 @@ proc advanceSlots( proc applyBlock( dag: ChainDAGRef, - state: var StateData, blck: BlockData, flags: UpdateFlags, - cache: var StateCache, info: var ForkedEpochInfo): bool = + state: var StateData, blck: BlockRef, flags: UpdateFlags, + cache: var StateCache, info: var ForkedEpochInfo) = # Apply a single block to the state - the state must be positioned at the # parent of the block with a slot lower than the one of the block being # applied - doAssert state.blck == blck.refs.parent - - var statePtr = unsafeAddr state # safe because `restore` is locally scoped - func restore(v: var ForkedHashedBeaconState) = - doAssert (addr(statePtr.data) == addr v) - assign(statePtr[], dag.headState) + doAssert state.blck == blck.parent loadStateCache(dag, cache, state.blck, getStateField(state.data, slot).epoch) - let ok = withBlck(blck.data): + case dag.cfg.blockForkAtEpoch(blck.slot.epoch) + of BeaconBlockFork.Phase0: + let data = dag.db.getPhase0Block(blck.root).expect("block loaded") state_transition( - dag.cfg, state.data, blck, cache, info, - flags + dag.updateFlags + {slotProcessed}, restore) - if ok: - state.blck = blck.refs + dag.cfg, state.data, data, cache, info, + flags + dag.updateFlags + {slotProcessed}, noRollback).expect( + "Blocks from database must not fail to apply") + of BeaconBlockFork.Altair: + let data = dag.db.getAltairBlock(blck.root).expect("block loaded") + state_transition( + dag.cfg, state.data, data, cache, info, + flags + dag.updateFlags + {slotProcessed}, noRollback).expect( + "Blocks from database must not fail to apply") + of BeaconBlockFork.Bellatrix: + let data = dag.db.getMergeBlock(blck.root).expect("block loaded") + state_transition( + dag.cfg, state.data, data, cache, info, + flags + dag.updateFlags + {slotProcessed}, noRollback).expect( + "Blocks from database must not fail to apply") - ok + state.blck = blck proc updateStateData*( dag: ChainDAGRef, state: var StateData, bs: BlockSlot, save: bool, @@ -1034,9 +1040,7 @@ proc updateStateData*( # again. Also, because we're applying blocks that were loaded from the # database, we can skip certain checks that have already been performed # before adding the block to the database. - let ok = - dag.applyBlock(state, dag.get(ancestors[i]), {}, cache, info) - doAssert ok, "Blocks in database should never fail to apply.." + dag.applyBlock(state, ancestors[i], {}, cache, info) # ...and make sure to process empty slots as requested dag.advanceSlots(state, bs.slot, save, cache, info) diff --git a/beacon_chain/rpc/rpc_validator_api.nim b/beacon_chain/rpc/rpc_validator_api.nim index 04b21688a..11047e8d1 100644 --- a/beacon_chain/rpc/rpc_validator_api.nim +++ b/beacon_chain/rpc/rpc_validator_api.nim @@ -41,12 +41,11 @@ proc installValidatorApiHandlers*(rpcServer: RpcServer, node: BeaconNode) {. if proposer.isNone(): raise newException(CatchableError, "could not retrieve block for slot: " & $slot) - let message = await makeBeaconBlockForHeadAndSlot( + let res = await makeBeaconBlockForHeadAndSlot( node, randao_reveal, proposer.get(), graffiti, head, slot) - if message.isErr(): - raise newException(CatchableError, - "could not retrieve block for slot: " & $slot) - let blck = message.get() + if res.isErr(): + raise newException(CatchableError, res.error()) + let blck = res.get() case blck.kind of BeaconBlockFork.Phase0: return blck.phase0Data diff --git a/beacon_chain/spec/forks.nim b/beacon_chain/spec/forks.nim index 18460dae6..be0df4758 100644 --- a/beacon_chain/spec/forks.nim +++ b/beacon_chain/spec/forks.nim @@ -66,6 +66,11 @@ type altair.BeaconBlock | bellatrix.BeaconBlock + ForkySigVerifiedBeaconBlock* = + phase0.SigVerifiedBeaconBlock | + altair.SigVerifiedBeaconBlock | + bellatrix.SigVerifiedBeaconBlock + ForkyTrustedBeaconBlock* = phase0.TrustedBeaconBlock | altair.TrustedBeaconBlock | diff --git a/beacon_chain/spec/state_transition.nim b/beacon_chain/spec/state_transition.nim index 7439d274b..1204a7294 100644 --- a/beacon_chain/spec/state_transition.nim +++ b/beacon_chain/spec/state_transition.nim @@ -43,76 +43,47 @@ import chronicles, stew/results, - metrics, ../extras, ./datatypes/[phase0, altair, bellatrix], "."/[ beaconstate, eth2_merkleization, forks, helpers, signatures, state_transition_block, state_transition_epoch, validator] -export extras, phase0, altair, bellatrix +export results, extras, phase0, altair, bellatrix # https://github.com/ethereum/consensus-specs/blob/v1.1.8/specs/phase0/beacon-chain.md#beacon-chain-state-transition-function proc verify_block_signature( - #state: ForkyBeaconState, signed_block: SomeSomeSignedBeaconBlock): bool = - state: ForkyBeaconState, signed_block: SomeForkySignedBeaconBlock): bool = + state: ForkyBeaconState, signed_block: SomeForkySignedBeaconBlock): + Result[void, cstring] = let proposer_index = signed_block.message.proposer_index if proposer_index >= state.validators.lenu64: - notice "Invalid proposer index in block", - blck = shortLog(signed_block.message) - return false + return err("block: invalid proposer index") if not verify_block_signature( state.fork, state.genesis_validators_root, signed_block.message.slot, signed_block.root, state.validators[proposer_index].pubkey, signed_block.signature): - notice "Block: signature verification failed", - blck = shortLog(signedBlock) - return false + return err("block: signature verification failed") - true + ok() # https://github.com/ethereum/consensus-specs/blob/v1.1.8/specs/phase0/beacon-chain.md#beacon-chain-state-transition-function -proc verifyStateRoot(state: ForkyBeaconState, blck: phase0.BeaconBlock or phase0.SigVerifiedBeaconBlock or altair.BeaconBlock or altair.SigVerifiedBeaconBlock or bellatrix.BeaconBlock or bellatrix.SigVerifiedBeaconBlock or bellatrix.TrustedBeaconBlock): bool = +proc verifyStateRoot( + state: ForkyBeaconState, blck: ForkyBeaconBlock | ForkySigVerifiedBeaconBlock): + Result[void, cstring] = # This is inlined in state_transition(...) in spec. let state_root = hash_tree_root(state) if state_root != blck.state_root: - notice "Block: root verification failed", - block_state_root = shortLog(blck.state_root), state_root = shortLog(state_root) - false + err("block: state root verification failed") else: - true + ok() -func verifyStateRoot(state: phase0.BeaconState, blck: phase0.TrustedBeaconBlock): bool = +func verifyStateRoot( + state: ForkyBeaconState, blck: ForkyTrustedBeaconBlock): + Result[void, cstring] = # This is inlined in state_transition(...) in spec. - true - -func verifyStateRoot(state: altair.BeaconState, blck: altair.TrustedBeaconBlock): bool = - # This is inlined in state_transition(...) in spec. - true - -func verifyStateRoot(state: bellatrix.BeaconState, blck: bellatrix.TrustedBeaconBlock): bool = - # This is inlined in state_transition(...) in spec. - true - -# one of these can happen on the fork block itself (it's a phase 0 block which -# creates an Altair state) -func verifyStateRoot(state: altair.BeaconState, blck: phase0.TrustedBeaconBlock): bool = - # This is inlined in state_transition(...) in spec. - true - -func verifyStateRoot(state: bellatrix.BeaconState, blck: phase0.TrustedBeaconBlock): bool = - # This is inlined in state_transition(...) in spec. - true - -func verifyStateRoot(state: bellatrix.BeaconState, blck: altair.TrustedBeaconBlock): bool = - # This is inlined in state_transition(...) in spec. - true - -func verifyStateRoot(state: phase0.BeaconState, blck: altair.TrustedBeaconBlock): bool = - # This is inlined in state_transition(...) in spec. - true + ok() type RollbackProc* = proc() {.gcsafe, raises: [Defect].} @@ -213,14 +184,11 @@ proc maybeUpgradeState*( proc process_slots*( cfg: RuntimeConfig, state: var ForkedHashedBeaconState, slot: Slot, - cache: var StateCache, info: var ForkedEpochInfo, flags: UpdateFlags): bool = + cache: var StateCache, info: var ForkedEpochInfo, flags: UpdateFlags): + Result[void, cstring] = if not (getStateField(state, slot) < slot): if slotProcessed notin flags or getStateField(state, slot) != slot: - notice "Unusual request for a slot in the past", - state_root = shortLog(getStateRoot(state)), - current_slot = getStateField(state, slot), - target_slot = slot - return false + return err("process_slots: cannot rewind state to past slot") # Update the state so its slot matches that of the block while getStateField(state, slot) < slot: @@ -237,43 +205,28 @@ proc process_slots*( maybeUpgradeState(cfg, state) - true + ok() proc state_transition_block_aux( cfg: RuntimeConfig, state: var ForkyHashedBeaconState, - signedBlock: phase0.SignedBeaconBlock | phase0.SigVerifiedSignedBeaconBlock | - phase0.TrustedSignedBeaconBlock | altair.SignedBeaconBlock | - altair.SigVerifiedSignedBeaconBlock | altair.TrustedSignedBeaconBlock | - bellatrix.TrustedSignedBeaconBlock | bellatrix.SigVerifiedSignedBeaconBlock | - bellatrix.SignedBeaconBlock, - cache: var StateCache, flags: UpdateFlags): bool = + signedBlock: SomeForkySignedBeaconBlock, + cache: var StateCache, flags: UpdateFlags): Result[void, cstring] = # Block updates - these happen when there's a new block being suggested # by the block proposer. Every actor in the network will update its state # according to the contents of this block - but first they will validate # that the block is sane. - if not (skipBLSValidation in flags or - verify_block_signature(state.data, signedBlock)): - return false + if skipBLSValidation notin flags: + ? verify_block_signature(state.data, signedBlock) trace "state_transition: processing block, signature passed", signature = shortLog(signedBlock.signature), blockRoot = shortLog(signedBlock.root) - let res = process_block(cfg, state.data, signedBlock.message, flags, cache) + ? process_block(cfg, state.data, signedBlock.message, flags, cache) - if not res.isOk(): - debug "state_transition: process_block failed", - blck = shortLog(signedBlock.message), - slot = state.data.slot, - eth1_deposit_index = state.data.eth1_deposit_index, - deposit_root = shortLog(state.data.eth1_data.deposit_root), - error = res.error - return false - - if not (skipStateRootValidation in flags or - verifyStateRoot(state.data, signedBlock.message)): - return false + if skipStateRootValidation notin flags: + ? verifyStateRoot(state.data, signedBlock.message) # only blocks currently being produced have an empty state root - we use a # separate function for those @@ -281,7 +234,7 @@ proc state_transition_block_aux( "see makeBeaconBlock for block production" state.root = signedBlock.message.state_root - true + ok() type RollbackForkedHashedProc* = @@ -293,13 +246,9 @@ func noRollback*(state: var ForkedHashedBeaconState) = proc state_transition_block*( cfg: RuntimeConfig, state: var ForkedHashedBeaconState, - signedBlock: phase0.SignedBeaconBlock | phase0.SigVerifiedSignedBeaconBlock | - phase0.TrustedSignedBeaconBlock | - altair.SignedBeaconBlock | altair.SigVerifiedSignedBeaconBlock | - altair.TrustedSignedBeaconBlock | bellatrix.TrustedSignedBeaconBlock | - bellatrix.SigVerifiedSignedBeaconBlock | bellatrix.SignedBeaconBlock, + signedBlock: SomeForkySignedBeaconBlock, cache: var StateCache, flags: UpdateFlags, - rollback: RollbackForkedHashedProc): bool = + rollback: RollbackForkedHashedProc): Result[void, cstring] = ## `rollback` is called if the transition fails and the given state has been ## partially changed. If a temporary state was given to `state_transition`, ## it is safe to use `noRollback` and leave it broken, else the state @@ -310,21 +259,20 @@ proc state_transition_block*( # Ensure state_transition_block()-only callers trigger this maybeUpgradeStateToAltair(cfg, state) - let success = withState(state): + let res = withState(state): state_transition_block_aux(cfg, state, signedBlock, cache, flags) - if not success: + if res.isErr(): rollback(state) - return false - true + res proc state_transition*( cfg: RuntimeConfig, state: var ForkedHashedBeaconState, signedBlock: SomeForkySignedBeaconBlock, cache: var StateCache, info: var ForkedEpochInfo, flags: UpdateFlags, - rollback: RollbackForkedHashedProc): bool = + rollback: RollbackForkedHashedProc): Result[void, cstring] = ## Apply a block to the state, advancing the slot counter as necessary. The ## given state must be of a lower slot, or, in case the `slotProcessed` flag ## is set, can be the slot state of the same slot as the block (where the @@ -340,10 +288,10 @@ proc state_transition*( ## it is safe to use `noRollback` and leave it broken, else the state ## object should be rolled back to a consistent state. If the transition fails ## before the state has been updated, `rollback` will not be called. - if not process_slots( + ? process_slots( cfg, state, signedBlock.message.slot, cache, info, - flags + {skipLastStateRootCalculation}): - return false + flags + {skipLastStateRootCalculation}) + state_transition_block( cfg, state, signedBlock, cache, flags, rollback) @@ -359,7 +307,7 @@ template partialBeaconBlock( deposits: seq[Deposit], exits: BeaconBlockExits, sync_aggregate: SyncAggregate, - executionPayload: ExecutionPayload): phase0.BeaconBlock = + execution_payload: ExecutionPayload): phase0.BeaconBlock = phase0.BeaconBlock( slot: state.data.slot, proposer_index: proposer_index.uint64, @@ -385,9 +333,9 @@ proc makeBeaconBlock*( deposits: seq[Deposit], exits: BeaconBlockExits, sync_aggregate: SyncAggregate, - executionPayload: ExecutionPayload, + execution_payload: ExecutionPayload, rollback: RollbackHashedProc, - cache: var StateCache): Result[phase0.BeaconBlock, string] = + cache: var StateCache): Result[phase0.BeaconBlock, cstring] = ## Create a block for the given state. The latest block applied to it will ## be used for the parent_root value, and the slot will be take from ## state.slot meaning process_slots must be called up to the slot for which @@ -403,17 +351,12 @@ proc makeBeaconBlock*( let res = process_block(cfg, state.data, blck, {skipBlsValidation}, cache) if res.isErr: - warn "Unable to apply new block to state", - blck = shortLog(blck), - slot = state.data.slot, - eth1_deposit_index = state.data.eth1_deposit_index, - deposit_root = shortLog(state.data.eth1_data.deposit_root), - error = res.error rollback(state) - return err("Unable to apply new block to state: " & $res.error()) + return err(res.error()) state.root = hash_tree_root(state.data) blck.state_root = state.root + ok(blck) # https://github.com/ethereum/consensus-specs/blob/v1.1.8/specs/altair/validator.md#preparing-a-beaconblock @@ -428,7 +371,7 @@ template partialBeaconBlock( deposits: seq[Deposit], exits: BeaconBlockExits, sync_aggregate: SyncAggregate, - executionPayload: ExecutionPayload): altair.BeaconBlock = + execution_payload: ExecutionPayload): altair.BeaconBlock = altair.BeaconBlock( slot: state.data.slot, proposer_index: proposer_index.uint64, @@ -455,9 +398,9 @@ proc makeBeaconBlock*( deposits: seq[Deposit], exits: BeaconBlockExits, sync_aggregate: SyncAggregate, - executionPayload: ExecutionPayload, + execution_payload: ExecutionPayload, rollback: RollbackAltairHashedProc, - cache: var StateCache): Result[altair.BeaconBlock, string] = + cache: var StateCache): Result[altair.BeaconBlock, cstring] = ## Create a block for the given state. The latest block applied to it will ## be used for the parent_root value, and the slot will be take from ## state.slot meaning process_slots must be called up to the slot for which @@ -468,22 +411,17 @@ proc makeBeaconBlock*( var blck = partialBeaconBlock(cfg, state, proposer_index, randao_reveal, eth1_data, graffiti, attestations, deposits, - exits, sync_aggregate, executionPayload) + exits, sync_aggregate, execution_payload) let res = process_block(cfg, state.data, blck, {skipBlsValidation}, cache) if res.isErr: - warn "Unable to apply new block to state", - blck = shortLog(blck), - slot = state.data.slot, - eth1_deposit_index = state.data.eth1_deposit_index, - deposit_root = shortLog(state.data.eth1_data.deposit_root), - error = res.error rollback(state) - return err("Unable to apply new block to state: " & $res.error()) + return err(res.error()) state.root = hash_tree_root(state.data) blck.state_root = state.root + ok(blck) # https://github.com/ethereum/consensus-specs/blob/v1.1.3/specs/merge/validator.md#block-proposal @@ -498,7 +436,7 @@ template partialBeaconBlock( deposits: seq[Deposit], exits: BeaconBlockExits, sync_aggregate: SyncAggregate, - executionPayload: ExecutionPayload): bellatrix.BeaconBlock = + execution_payload: ExecutionPayload): bellatrix.BeaconBlock = bellatrix.BeaconBlock( slot: state.data.slot, proposer_index: proposer_index.uint64, @@ -513,7 +451,7 @@ template partialBeaconBlock( deposits: List[Deposit, Limit MAX_DEPOSITS](deposits), voluntary_exits: exits.voluntary_exits, sync_aggregate: sync_aggregate, - execution_payload: executionPayload)) + execution_payload: execution_payload)) proc makeBeaconBlock*( cfg: RuntimeConfig, @@ -526,9 +464,9 @@ proc makeBeaconBlock*( deposits: seq[Deposit], exits: BeaconBlockExits, sync_aggregate: SyncAggregate, - executionPayload: ExecutionPayload, + execution_payload: ExecutionPayload, rollback: RollbackMergeHashedProc, - cache: var StateCache): Result[bellatrix.BeaconBlock, string] = + cache: var StateCache): Result[bellatrix.BeaconBlock, cstring] = ## Create a block for the given state. The latest block applied to it will ## be used for the parent_root value, and the slot will be take from ## state.slot meaning process_slots must be called up to the slot for which @@ -539,22 +477,17 @@ proc makeBeaconBlock*( var blck = partialBeaconBlock(cfg, state, proposer_index, randao_reveal, eth1_data, graffiti, attestations, deposits, - exits, sync_aggregate, executionPayload) + exits, sync_aggregate, execution_payload) let res = process_block(cfg, state.data, blck, {skipBlsValidation}, cache) if res.isErr: - warn "Unable to apply new block to state", - blck = shortLog(blck), - slot = state.data.slot, - eth1_deposit_index = state.data.eth1_deposit_index, - deposit_root = shortLog(state.data.eth1_data.deposit_root), - error = res.error rollback(state) - return err("Unable to apply new block to state: " & $res.error()) + return err(res.error()) state.root = hash_tree_root(state.data) blck.state_root = state.root + ok(blck) proc makeBeaconBlock*( @@ -570,13 +503,13 @@ proc makeBeaconBlock*( sync_aggregate: SyncAggregate, executionPayload: ExecutionPayload, rollback: RollbackForkedHashedProc, - cache: var StateCache): Result[ForkedBeaconBlock, string] = + cache: var StateCache): Result[ForkedBeaconBlock, cstring] = ## Create a block for the given state. The latest block applied to it will ## be used for the parent_root value, and the slot will be take from ## state.slot meaning process_slots must be called up to the slot for which ## the block is to be created. - template makeBeaconBlock(kind: untyped): Result[ForkedBeaconBlock, string] = + template makeBeaconBlock(kind: untyped): Result[ForkedBeaconBlock, cstring] = # To create a block, we'll first apply a partial block to the state, skipping # some validations. @@ -590,17 +523,12 @@ proc makeBeaconBlock*( {skipBlsValidation}, cache) if res.isErr: - warn "Unable to apply new block to state", - blck = shortLog(blck), - slot = state.`kind Data`.data.slot, - eth1_deposit_index = state.`kind Data`.data.eth1_deposit_index, - deposit_root = shortLog(state.`kind Data`.data.eth1_data.deposit_root), - error = res.error rollback(state) - return err("Unable to apply new block to state: " & $res.error()) + return err(res.error()) state.`kind Data`.root = hash_tree_root(state.`kind Data`.data) blck.`kind Data`.state_root = state.`kind Data`.root + ok(blck) case state.kind diff --git a/beacon_chain/sync/sync_protocol.nim b/beacon_chain/sync/sync_protocol.nim index cfd217d51..e2ba83e2d 100644 --- a/beacon_chain/sync/sync_protocol.nim +++ b/beacon_chain/sync/sync_protocol.nim @@ -233,7 +233,7 @@ p2pProtocol BeaconSync(version = 1, case blck.kind of BeaconBlockFork.Phase0: await response.write(blck.phase0Data.asSigned) - of BeaconBlockFork.Altair, BeaconBlockFork.Bellatrix: + else: # Skipping all subsequent blocks should be OK because the spec says: # "Clients MAY limit the number of blocks in the response." # https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/p2p-interface.md#beaconblocksbyrange diff --git a/beacon_chain/validators/validator_duties.nim b/beacon_chain/validators/validator_duties.nim index de1ed372a..f46b51ee2 100644 --- a/beacon_chain/validators/validator_duties.nim +++ b/beacon_chain/validators/validator_duties.nim @@ -448,21 +448,21 @@ proc makeBeaconBlockForHeadAndSlot*(node: BeaconNode, # Advance to the given slot without calculating state root - we'll only # need a state root _with_ the block applied var info: ForkedEpochInfo - if not process_slots( - node.dag.cfg, stateData.data, slot, cache, info, - {skipLastStateRootCalculation}): - return ForkedBlockResult.err("Unable to advance state to slot") + + process_slots( + node.dag.cfg, stateData.data, slot, cache, info, + {skipLastStateRootCalculation}).expect("advancing 1 slot should not fail") let eth1Proposal = node.getBlockProposalEth1Data(stateData.data) if eth1Proposal.hasMissingDeposits: - error "Eth1 deposits not available. Skipping block proposal", slot + warn "Eth1 deposits not available. Skipping block proposal", slot return ForkedBlockResult.err("Eth1 deposits not available") let exits = withState(stateData.data): node.exitPool[].getBeaconBlockExits(state.data) - return makeBeaconBlock( + let res = makeBeaconBlock( node.dag.cfg, stateData.data, validator_index, @@ -479,8 +479,16 @@ proc makeBeaconBlockForHeadAndSlot*(node: BeaconNode, default(merge.ExecutionPayload), noRollback, # Temporary state - no need for rollback cache) + if res.isErr(): + # This is almost certainly a bug, but it's complex enough that there's a + # small risk it might happen even when most proposals succeed - thus we + # log instead of asserting + error "Cannot create block for proposal", + slot, head = shortLog(head), error = res.error() + return err($res.error) + return ok(res.get()) do: - warn "Cannot get proposal state - skipping block productioon, database corrupt?", + error "Cannot get proposal state - skipping block production, database corrupt?", head = shortLog(head), slot diff --git a/ncli/ncli.nim b/ncli/ncli.nim index 578a1c6d2..e64fe4e46 100644 --- a/ncli/ncli.nim +++ b/ncli/ncli.nim @@ -95,9 +95,11 @@ proc doTransition(conf: NcliConf) = var cache = StateCache() info = ForkedEpochInfo() - if not state_transition(getRuntimeConfig(conf.eth2Network), - stateY[], blckX, cache, info, flags, noRollback): - error "State transition failed" + let res = state_transition( + getRuntimeConfig(conf.eth2Network), stateY[], blckX, cache, info, + flags, noRollback) + if res.isErr(): + error "State transition failed", error = res.error() quit 1 else: saveSSZFile(conf.postState, stateY[]) @@ -126,9 +128,9 @@ proc doSlots(conf: NcliConf) = for i in 0'u64.. MAX_ATTESTATIONS, "6*SLOTS_PER_EPOCH validators > 128 mainnet MAX_ATTESTATIONS" @@ -283,7 +283,7 @@ suite "Attestation pool processing" & preset(): check: process_slots( defaultRuntimeConfig, state.data, getStateField(state.data, slot) + 1, - cache, info, {}) + cache, info, {}).isOk() let bc1 = get_beacon_committee(state[].data, @@ -298,10 +298,6 @@ suite "Attestation pool processing" & preset(): attestation0, @[bc0[0]], attestation0.loadSig, attestation0.data.slot.start_beacon_time) - discard process_slots( - defaultRuntimeConfig, state.data, - MIN_ATTESTATION_INCLUSION_DELAY.Slot + 1, cache, info, {}) - let attestations = pool[].getAttestationsForBlock(state.data, cache) check: @@ -328,7 +324,7 @@ suite "Attestation pool processing" & preset(): check: process_slots( defaultRuntimeConfig, state.data, - MIN_ATTESTATION_INCLUSION_DELAY.Slot + 1, cache, info, {}) + MIN_ATTESTATION_INCLUSION_DELAY.Slot + 1, cache, info, {}).isOk() let attestations = pool[].getAttestationsForBlock(state.data, cache) @@ -359,7 +355,7 @@ suite "Attestation pool processing" & preset(): check: process_slots( defaultRuntimeConfig, state.data, - MIN_ATTESTATION_INCLUSION_DELAY.Slot + 1, cache, info, {}) + MIN_ATTESTATION_INCLUSION_DELAY.Slot + 1, cache, info, {}).isOk() let attestations = pool[].getAttestationsForBlock(state.data, cache) @@ -389,7 +385,7 @@ suite "Attestation pool processing" & preset(): check: process_slots( defaultRuntimeConfig, state.data, - MIN_ATTESTATION_INCLUSION_DELAY.Slot + 1, cache, info, {}) + MIN_ATTESTATION_INCLUSION_DELAY.Slot + 1, cache, info, {}).isOk() let attestations = pool[].getAttestationsForBlock(state.data, cache) diff --git a/tests/test_blockchain_dag.nim b/tests/test_blockchain_dag.nim index 1a9364dae..b36c8cfd2 100644 --- a/tests/test_blockchain_dag.nim +++ b/tests/test_blockchain_dag.nim @@ -118,7 +118,7 @@ suite "Block pool processing" & preset(): check: process_slots( defaultRuntimeConfig, state[], getStateField(state[], slot) + 1, cache, - info, {}) + info, {}).isOk() let b4 = addTestBlock(state[], cache).phase0Data @@ -283,7 +283,7 @@ suite "Block pool altair processing" & preset(): check: process_slots( cfg, state[], cfg.ALTAIR_FORK_EPOCH.start_slot(), cache, - info, {}) + info, {}).isOk() state[].kind == BeaconStateFork.Altair @@ -362,7 +362,7 @@ suite "chain DAG finalization tests" & preset(): process_slots( defaultRuntimeConfig, tmpState[], getStateField(tmpState[], slot) + (5 * SLOTS_PER_EPOCH).uint64, - cache, info, {}) + cache, info, {}).isOk() let lateBlock = addTestBlock(tmpState[], cache).phase0Data block: @@ -472,7 +472,7 @@ suite "chain DAG finalization tests" & preset(): doAssert process_slots( defaultRuntimeConfig, prestate[], getStateField(prestate[], slot) + 1, - cache, info, {}) + cache, info, {}).isOk() # create another block, orphaning the head let blck = makeTestBlock(prestate[], cache).phase0Data @@ -502,7 +502,7 @@ suite "chain DAG finalization tests" & preset(): check: process_slots( defaultRuntimeConfig, dag.headState.data, Slot(SLOTS_PER_EPOCH * 6 + 2), - cache, info, {}) + cache, info, {}).isOk() var blck = makeTestBlock( dag.headState.data, cache, @@ -607,7 +607,7 @@ suite "Diverging hardforks": process_slots( phase0RuntimeConfig, tmpState[], getStateField(tmpState[], slot) + (3 * SLOTS_PER_EPOCH).uint64, - cache, info, {}) + cache, info, {}).isOk() # Because the first block is after the Altair transition, the only block in # common is the tail block @@ -629,7 +629,7 @@ suite "Diverging hardforks": process_slots( phase0RuntimeConfig, tmpState[], getStateField(tmpState[], slot) + SLOTS_PER_EPOCH.uint64, - cache, info, {}) + cache, info, {}).isOk() # There's a block in the shared-correct phase0 hardfork, before epoch 2 var @@ -641,7 +641,7 @@ suite "Diverging hardforks": process_slots( phase0RuntimeConfig, tmpState[], getStateField(tmpState[], slot) + (3 * SLOTS_PER_EPOCH).uint64, - cache, info, {}) + cache, info, {}).isOk() var b2 = addTestBlock(tmpState[], cache).phase0Data diff --git a/tests/test_gossip_validation.nim b/tests/test_gossip_validation.nim index 5b8ff14b2..4c04df1a3 100644 --- a/tests/test_gossip_validation.nim +++ b/tests/test_gossip_validation.nim @@ -52,7 +52,7 @@ suite "Gossip validation " & preset(): check: process_slots( defaultRuntimeConfig, state.data, getStateField(state.data, slot) + 1, - cache, info, {}) + cache, info, {}).isOk() test "Empty committee when no committee for slot": template committee(idx: uint64): untyped = diff --git a/tests/test_helpers.nim b/tests/test_helpers.nim index 3b0315bf1..260f24830 100644 --- a/tests/test_helpers.nim +++ b/tests/test_helpers.nim @@ -32,8 +32,9 @@ suite "Spec helpers": forked = newClone(initGenesisState()) cache = StateCache() info = ForkedEpochInfo() - doAssert process_slots(defaultRuntimeConfig, forked[], - Slot(100), cache, info, flags = {}) + process_slots( + defaultRuntimeConfig, forked[], Slot(100), cache, info, + flags = {}).expect("no failure") let state = forked[].phase0Data.data diff --git a/tests/test_spec.nim b/tests/test_spec.nim index 7ea28b423..eae406f5b 100644 --- a/tests/test_spec.nim +++ b/tests/test_spec.nim @@ -24,6 +24,21 @@ suite "Beacon state" & preset(): makeInitialDeposits(SLOTS_PER_EPOCH, {}), {})) check: state.validators.lenu64 == SLOTS_PER_EPOCH + test "process_slots": + var + cfg = defaultRuntimeConfig + state = (ref ForkedHashedBeaconState)( + kind: BeaconStateFork.Phase0, + phase0Data: initialize_hashed_beacon_state_from_eth1( + defaultRuntimeConfig, Eth2Digest(), 0, + makeInitialDeposits(SLOTS_PER_EPOCH, {}), {skipBlsValidation})) + cache: StateCache + info: ForkedEpochInfo + check: + process_slots(cfg, state[], Slot 1, cache, info, {}).isOk() + process_slots(cfg, state[], Slot 1, cache, info, {}).isErr() + process_slots(cfg, state[], Slot 1, cache, info, {slotProcessed}).isOk() + test "latest_block_root": var cfg = defaultRuntimeConfig @@ -38,7 +53,7 @@ suite "Beacon state" & preset(): check: # Works for genesis block state[].phase0Data.latest_block_root() == genBlock.root - process_slots(cfg, state[], Slot 1, cache, info, {}) + process_slots(cfg, state[], Slot 1, cache, info, {}).isOk() state[].phase0Data.latest_block_root() == genBlock.root let blck = addTestBlock( @@ -46,7 +61,7 @@ suite "Beacon state" & preset(): check: # Works for random blocks state[].phase0Data.latest_block_root() == blck.root - process_slots(cfg, state[], Slot 2, cache, info, {}) + process_slots(cfg, state[], Slot 2, cache, info, {}).isOk() state[].phase0Data.latest_block_root() == blck.root test "get_beacon_proposer_index": @@ -68,7 +83,7 @@ suite "Beacon state" & preset(): state[].phase0Data.data, cache, Epoch(2).start_slot()).isNone() check: - process_slots(cfg, state[], Epoch(1).start_slot(), cache, info, {}) + process_slots(cfg, state[], Epoch(1).start_slot(), cache, info, {}).isOk() get_beacon_proposer_index(state[].phase0Data.data, cache, Slot 1).isNone() get_beacon_proposer_index( state[].phase0Data.data, cache, Epoch(1).start_slot()).isSome() diff --git a/tests/testblockutil.nim b/tests/testblockutil.nim index 36dd1b79a..d8a36beea 100644 --- a/tests/testblockutil.nim +++ b/tests/testblockutil.nim @@ -88,8 +88,9 @@ proc addTestBlock*( # Create and add a block to state - state will advance by one slot! if nextSlot: var info = ForkedEpochInfo() - doAssert process_slots( - cfg, state, getStateField(state, slot) + 1, cache, info, flags) + process_slots( + cfg, state, getStateField(state, slot) + 1, cache, info, flags).expect( + "can advance 1") let proposer_index = get_beacon_proposer_index( diff --git a/tests/teststateutil.nim b/tests/teststateutil.nim index a82cf206d..36c86c4a6 100644 --- a/tests/teststateutil.nim +++ b/tests/teststateutil.nim @@ -73,8 +73,8 @@ proc getTestStates*( for i, epoch in stateEpochs: let slot = epoch.Epoch.start_slot if getStateField(tmpState[], slot) < slot: - doAssert process_slots( - cfg, tmpState[], slot, cache, info, {}) + process_slots( + cfg, tmpState[], slot, cache, info, {}).expect("no failure") if i mod 3 == 0: withState(tmpState[]): diff --git a/vendor/nim-stew b/vendor/nim-stew index 6ad35b876..b464505b4 160000 --- a/vendor/nim-stew +++ b/vendor/nim-stew @@ -1 +1 @@ -Subproject commit 6ad35b876fb6ebe0dfee0f697af173acc47906ee +Subproject commit b464505b4dcbe2ef52d19b2cfca8f2be4ebf2c7e