From c762c0232d787f5ffe788fdbe46ab48bd144c3f5 Mon Sep 17 00:00:00 2001 From: Dustin Brody Date: Wed, 4 Dec 2019 10:49:59 +0000 Subject: [PATCH] render get_beacon_proposer_index(...) return value an option and remove overly aggressive assertion formerly causing crashes when missing validators (#617) * render get_beacon_proposer_index(...) return value an option and remove overly aggressive assertion formerly causing crashes when missing validators * follow addBlock(...) after refactoring * [skip ci] fix typo --- beacon_chain/beacon_node.nim | 11 ++++++--- beacon_chain/spec/beaconstate.nim | 21 ++++++++++++---- beacon_chain/spec/state_transition_block.nim | 20 ++++++++++----- beacon_chain/spec/validator.nim | 9 ++++--- tests/mocking/mock_blocks.nim | 26 +++++++++++--------- tests/testblockutil.nim | 5 ++-- 6 files changed, 59 insertions(+), 33 deletions(-) diff --git a/beacon_chain/beacon_node.nim b/beacon_chain/beacon_node.nim index 6cb223e3a..7897c5904 100644 --- a/beacon_chain/beacon_node.nim +++ b/beacon_chain/beacon_node.nim @@ -645,9 +645,12 @@ proc handleProposal(node: BeaconNode, head: BlockRef, slot: Slot): # revisit this - we should be able to advance behind var cache = get_empty_per_epoch_cache() node.blockPool.withState(node.stateCache, BlockSlot(blck: head, slot: slot)): - let - proposerIdx = get_beacon_proposer_index(state, cache) - validator = node.getAttachedValidator(state, proposerIdx) + let proposerIdx = get_beacon_proposer_index(state, cache) + if proposerIdx.isNone: + debug "Missing proposer index" + return head + + let validator = node.getAttachedValidator(state, proposerIdx.get) if validator != nil: return await proposeBlock(node, validator, head, slot) @@ -655,7 +658,7 @@ proc handleProposal(node: BeaconNode, head: BlockRef, slot: Slot): trace "Expecting block proposal", headRoot = shortLog(head.root), slot = shortLog(slot), - proposer = shortLog(state.validators[proposerIdx].pubKey), + proposer = shortLog(state.validators[proposerIdx.get].pubKey), cat = "consensus", pcs = "wait_for_proposal" diff --git a/beacon_chain/spec/beaconstate.nim b/beacon_chain/spec/beaconstate.nim index ceaac348e..6d20db3e8 100644 --- a/beacon_chain/spec/beaconstate.nim +++ b/beacon_chain/spec/beaconstate.nim @@ -6,7 +6,7 @@ # at your option. This file may not be copied, modified, or distributed except according to those terms. import - tables, algorithm, math, sequtils, + tables, algorithm, math, sequtils, options, json_serialization/std/sets, chronicles, stew/bitseqs, ../extras, ../ssz, ./crypto, ./datatypes, ./digest, ./helpers, ./validator @@ -171,14 +171,19 @@ proc slash_validator*(state: var BeaconState, slashed_index: ValidatorIndex, decrease_balance(state, slashed_index, validator.effective_balance div MIN_SLASHING_PENALTY_QUOTIENT) + # The rest doesn't make sense without there being any proposer index, so skip + let proposer_index = get_beacon_proposer_index(state, stateCache) + if proposer_index.isNone: + debug "No beacon proposer index and probably no active validators" + return + let - proposer_index = get_beacon_proposer_index(state, stateCache) # Spec has whistleblower_index as optional param, but it's never used. - whistleblower_index = proposer_index + whistleblower_index = proposer_index.get whistleblowing_reward = (validator.effective_balance div WHISTLEBLOWER_REWARD_QUOTIENT).Gwei proposer_reward = whistleblowing_reward div PROPOSER_REWARD_QUOTIENT - increase_balance(state, proposer_index, proposer_reward) + increase_balance(state, proposer_index.get, proposer_reward) # TODO: evaluate if spec bug / underflow can be triggered doAssert(whistleblowing_reward >= proposer_reward, "Spec bug: underflow in slash_validator") increase_balance( @@ -492,6 +497,12 @@ proc process_attestation*( # reused when looking for suitable blocks to include in attestations. # TODO don't log warnings when looking for attestations (return # Result[void, cstring] instead of logging in check_attestation?) + + let proposer_index = get_beacon_proposer_index(state, stateCache) + if proposer_index.isNone: + debug "No beacon proposer index and probably no active validators" + return false + if check_attestation(state, attestation, flags, stateCache): let attestation_slot = attestation.data.slot @@ -499,7 +510,7 @@ proc process_attestation*( data: attestation.data, aggregation_bits: attestation.aggregation_bits, inclusion_delay: state.slot - attestation_slot, - proposer_index: get_beacon_proposer_index(state, stateCache).uint64, + proposer_index: proposer_index.get.uint64, ) if attestation.data.target.epoch == get_current_epoch(state): diff --git a/beacon_chain/spec/state_transition_block.nim b/beacon_chain/spec/state_transition_block.nim index 64ea746df..1778f1a7b 100644 --- a/beacon_chain/spec/state_transition_block.nim +++ b/beacon_chain/spec/state_transition_block.nim @@ -32,8 +32,8 @@ # improvements to be made - other than that, keep things similar to spec for # now. -import # TODO - cleanup imports - algorithm, collections/sets, chronicles, sequtils, sets, tables, +import + algorithm, collections/sets, chronicles, options, sequtils, sets, tables, ../extras, ../ssz, metrics, beaconstate, crypto, datatypes, digest, helpers, validator @@ -76,10 +76,13 @@ proc process_block_header*( signature: BlsValue[Signature](kind: OpaqueBlob) ) - # Verify proposer is not slashed - let proposer = - state.validators[get_beacon_proposer_index(state, stateCache)] + let proposer_index = get_beacon_proposer_index(state, stateCache) + if proposer_index.isNone: + debug "Block header: proposer missing" + return false + + let proposer = state.validators[proposer_index.get] if proposer.slashed: notice "Block header: proposer slashed" return false @@ -105,7 +108,12 @@ proc process_randao( let epoch = state.get_current_epoch() proposer_index = get_beacon_proposer_index(state, stateCache) - proposer = addr state.validators[proposer_index] + + if proposer_index.isNone: + debug "Proposer index missing, probably along with any active validators" + return false + + let proposer = addr state.validators[proposer_index.get] # Verify that the provided randao value is valid if skipValidation notin flags: diff --git a/beacon_chain/spec/validator.nim b/beacon_chain/spec/validator.nim index 28919ec13..b19143f79 100644 --- a/beacon_chain/spec/validator.nim +++ b/beacon_chain/spec/validator.nim @@ -148,11 +148,12 @@ func get_empty_per_epoch_cache*(): StateCache = # https://github.com/ethereum/eth2.0-specs/blob/v0.9.2/specs/core/0_beacon-chain.md#compute_proposer_index func compute_proposer_index(state: BeaconState, indices: seq[ValidatorIndex], - seed: Eth2Digest, stateCache: var StateCache): ValidatorIndex = + seed: Eth2Digest, stateCache: var StateCache): Option[ValidatorIndex] = # Return from ``indices`` a random index sampled by effective balance. const MAX_RANDOM_BYTE = 255 - doAssert len(indices) > 0 + if len(indices) == 0: + return none(ValidatorIndex) # TODO fixme; should only be run once per slot and cached # There's exactly one beacon proposer per slot. @@ -175,12 +176,12 @@ func compute_proposer_index(state: BeaconState, indices: seq[ValidatorIndex], state.validators[candidate_index].effective_balance if effective_balance * MAX_RANDOM_BYTE >= MAX_EFFECTIVE_BALANCE * random_byte: - return candidate_index + return some(candidate_index) i += 1 # https://github.com/ethereum/eth2.0-specs/blob/v0.9.2/specs/core/0_beacon-chain.md#get_beacon_proposer_index func get_beacon_proposer_index*(state: BeaconState, stateCache: var StateCache): - ValidatorIndex = + Option[ValidatorIndex] = # Return the beacon proposer index at the current slot. let epoch = get_current_epoch(state) diff --git a/tests/mocking/mock_blocks.nim b/tests/mocking/mock_blocks.nim index a07f9a690..b82c28b2c 100644 --- a/tests/mocking/mock_blocks.nim +++ b/tests/mocking/mock_blocks.nim @@ -6,6 +6,7 @@ # at your option. This file may not be copied, modified, or distributed except according to those terms. import + options, # Specs ../../beacon_chain/spec/[datatypes, crypto, helpers, validator], # Internals @@ -60,20 +61,21 @@ proc signMockBlock*( blck: var BeaconBlock ) = - var proposer_index: ValidatorIndex var emptyCache = get_empty_per_epoch_cache() - if blck.slot == state.slot: - proposer_index = get_beacon_proposer_index(state, emptyCache) - else: - # Stub to get proposer index of future slot - # Note: this relies on ``let`` deep-copying the state - # i.e. BeaconState should have value semantics - # and not contain ref objects or pointers - var stubState = state - process_slots(stub_state, blck.slot) - proposer_index = get_beacon_proposer_index(stub_state, emptyCache) + let proposer_index = + if blck.slot == state.slot: + get_beacon_proposer_index(state, emptyCache) + else: + # Stub to get proposer index of future slot + # Note: this relies on ``let`` deep-copying the state + # i.e. BeaconState should have value semantics + # and not contain ref objects or pointers + var stubState = state + process_slots(stub_state, blck.slot) + get_beacon_proposer_index(stub_state, emptyCache) - signMockBlockImpl(state, blck, proposer_index) + # In tests, just let this throw if appropriate + signMockBlockImpl(state, blck, proposer_index.get) proc mockBlock*( state: BeaconState, diff --git a/tests/testblockutil.nim b/tests/testblockutil.nim index d59bc7182..87231fb3e 100644 --- a/tests/testblockutil.nim +++ b/tests/testblockutil.nim @@ -6,7 +6,7 @@ # at your option. This file may not be copied, modified, or distributed except according to those terms. import - stew/endians2, + options, stew/endians2, chronicles, eth/trie/[db], ../beacon_chain/[beacon_chain_db, block_pool, extras, ssz, state_transition, validator_pool], @@ -87,7 +87,8 @@ proc addBlock*( let # Index from the new state, but registry from the old state.. hmm... - proposer = state.validators[proposer_index] + # In tests, let this throw + proposer = state.validators[proposer_index.get] privKey = hackPrivKey(proposer) # TODO ugly hack; API needs rethinking