From b9e122a60c6dc4cf479f0be8e238a8b06d4ea036 Mon Sep 17 00:00:00 2001 From: tersec Date: Thu, 12 Nov 2020 19:24:07 +0000 Subject: [PATCH] address some TODOs (#2005) --- beacon_chain/spec/beaconstate.nim | 3 +-- beacon_chain/spec/crypto.nim | 9 --------- beacon_chain/spec/datatypes.nim | 1 - beacon_chain/spec/state_transition.nim | 10 ---------- beacon_chain/spec/state_transition_epoch.nim | 5 +++-- tests/mocking/mock_attestations.nim | 2 +- tests/official/test_fixture_sanity_blocks.nim | 7 +++++-- tests/test_state_transition.nim | 9 ++++++--- 8 files changed, 16 insertions(+), 30 deletions(-) diff --git a/beacon_chain/spec/beaconstate.nim b/beacon_chain/spec/beaconstate.nim index a2f183a88..41ff9226f 100644 --- a/beacon_chain/spec/beaconstate.nim +++ b/beacon_chain/spec/beaconstate.nim @@ -90,7 +90,7 @@ proc process_deposit*(preset: RuntimePreset, let pubkey = deposit.data.pubkey - pubkey_inited = pubkey.initPubKey # TODO replicate previous PR semantics, check later + pubkey_inited = pubkey.initPubKey amount = deposit.data.amount var index = -1 @@ -445,7 +445,6 @@ proc is_valid_indexed_attestation*( # Verify aggregate signature if skipBLSValidation notin flags: - # TODO: fuse loops with blsFastAggregateVerify let pubkeys = mapIt( indexed_attestation.attesting_indices, state.validators[it].pubkey) if not verify_attestation_signature( diff --git a/beacon_chain/spec/crypto.nim b/beacon_chain/spec/crypto.nim index 09c44c753..5f491d19e 100644 --- a/beacon_chain/spec/crypto.nim +++ b/beacon_chain/spec/crypto.nim @@ -151,15 +151,6 @@ proc blsVerify*( # TODO: chronicles warning return false - # TODO: remove fully if the comment below is not true anymore and - # and we don't need this workaround - # # TODO bls_verify_multiple(...) used to have this workaround, and now it - # # lives here. No matter the signature, there's also no meaningful way to - # # verify it -- it's a kind of vacuous truth. No pubkey/sig pairs. Sans a - # # getBytes() or similar mechanism, pubKey == default(ValidatorPubKey) is - # # a way to create many false positive matches. This seems odd. - # if pubkey.getBytes() == default(ValidatorPubKey).getBytes(): - # return true realkey.get.blsValue.verify(message, signature.blsValue) func blsSign*(privkey: ValidatorPrivKey, message: openArray[byte]): ValidatorSig = diff --git a/beacon_chain/spec/datatypes.nim b/beacon_chain/spec/datatypes.nim index 22d0a408d..22f8869e4 100644 --- a/beacon_chain/spec/datatypes.nim +++ b/beacon_chain/spec/datatypes.nim @@ -444,7 +444,6 @@ type message*: AggregateAndProof signature*: ValidatorSig - # TODO to be replaced with some magic hash caching HashedBeaconState* = object data*: BeaconState root*: Eth2Digest # hash_tree_root(data) diff --git a/beacon_chain/spec/state_transition.nim b/beacon_chain/spec/state_transition.nim index 37ebcd18e..da18952da 100644 --- a/beacon_chain/spec/state_transition.nim +++ b/beacon_chain/spec/state_transition.nim @@ -225,16 +225,6 @@ proc state_transition*( false -proc state_transition*( - preset: RuntimePreset, - state: var HashedBeaconState, signedBlock: SomeSignedBeaconBlock, - flags: UpdateFlags, rollback: RollbackHashedProc): bool {.nbench.} = - # TODO consider moving this to testutils or similar, since non-testing - # and fuzzing code should always be coming from block pool which should - # always be providing cache or equivalent - var cache = StateCache() - state_transition(preset, state, signedBlock, cache, flags, rollback) - # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0/specs/phase0/validator.md#preparing-for-a-beaconblock proc makeBeaconBlock*( preset: RuntimePreset, diff --git a/beacon_chain/spec/state_transition_epoch.nim b/beacon_chain/spec/state_transition_epoch.nim index 1d6ddd580..d318c734f 100644 --- a/beacon_chain/spec/state_transition_epoch.nim +++ b/beacon_chain/spec/state_transition_epoch.nim @@ -285,9 +285,10 @@ proc process_justification_and_finalization*(state: var BeaconState, ## Spec: ## state.justification_bits[1:] = state.justification_bits[:-1] ## state.justification_bits[0] = 0b0 - # TODO JUSTIFICATION_BITS_LENGTH is a constant in spec, move there or fix - # BitVector serialization in SSZ layer + + # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0/specs/phase0/beacon-chain.md#constants const JUSTIFICATION_BITS_LENGTH = 4 + state.justification_bits = (state.justification_bits shl 1) and cast[uint8]((2^JUSTIFICATION_BITS_LENGTH) - 1) diff --git a/tests/mocking/mock_attestations.nim b/tests/mocking/mock_attestations.nim index 92addbd4b..ad17b1e3e 100644 --- a/tests/mocking/mock_attestations.nim +++ b/tests/mocking/mock_attestations.nim @@ -140,7 +140,7 @@ proc add*(state: var HashedBeaconState, attestation: Attestation, slot: Slot) = signMockBlock(state.data, signedBlock) let success = state_transition( - defaultRuntimePreset, state, signedBlock, + defaultRuntimePreset, state, signedBlock, cache, flags = {skipStateRootValidation}, noRollback) doAssert success diff --git a/tests/official/test_fixture_sanity_blocks.nim b/tests/official/test_fixture_sanity_blocks.nim index e98034df8..47d744be5 100644 --- a/tests/official/test_fixture_sanity_blocks.nim +++ b/tests/official/test_fixture_sanity_blocks.nim @@ -39,6 +39,7 @@ proc runTest(testName, testDir, unitTestName: string) = preState = newClone(parseTest(testPath/"pre.ssz", SSZ, BeaconState)) hashedPreState = (ref HashedBeaconState)( data: preState[], root: hash_tree_root(preState[])) + cache = StateCache() # In test cases with more than 10 blocks the first 10 aren't 0-prefixed, # so purely lexicographic sorting wouldn't sort properly. @@ -48,11 +49,13 @@ proc runTest(testName, testDir, unitTestName: string) = if hasPostState: let success = state_transition( - defaultRuntimePreset, hashedPreState[], blck, flags = {}, noRollback) + defaultRuntimePreset, hashedPreState[], blck, cache, flags = {}, + noRollback) doAssert success, "Failure when applying block " & $i else: let success = state_transition( - defaultRuntimePreset, hashedPreState[], blck, flags = {}, noRollback) + defaultRuntimePreset, hashedPreState[], blck, cache, flags = {}, + noRollback) doAssert (i + 1 < numBlocks) or not success, "We didn't expect these invalid blocks to be processed" diff --git a/tests/test_state_transition.nim b/tests/test_state_transition.nim index 3c85e58ce..361035eea 100644 --- a/tests/test_state_transition.nim +++ b/tests/test_state_transition.nim @@ -41,7 +41,8 @@ suiteReport "Block processing" & preset(): previous_block_root = genesisBlock.root new_block = makeTestBlock(state[], previous_block_root, cache) - let block_ok = state_transition(defaultRuntimePreset, state[], new_block, {}, noRollback) + let block_ok = state_transition( + defaultRuntimePreset, state[], new_block, cache, {}, noRollback) check: block_ok @@ -61,7 +62,8 @@ suiteReport "Block processing" & preset(): for i in 1..SLOTS_PER_EPOCH: let new_block = makeTestBlock(state[], previous_block_root, cache) - let block_ok = state_transition(defaultRuntimePreset, state[], new_block, {}, noRollback) + let block_ok = state_transition( + defaultRuntimePreset, state[], new_block, cache, {}, noRollback) check: block_ok @@ -97,7 +99,8 @@ suiteReport "Block processing" & preset(): new_block = makeTestBlock(state[], previous_block_root, cache, attestations = @[attestation] ) - check state_transition(defaultRuntimePreset, state[], new_block, {}, noRollback) + check state_transition( + defaultRuntimePreset, state[], new_block, cache, {}, noRollback) check: # TODO epoch attestations can get multiplied now; clean up paths to