From 1ffc2df23dc43aa4d6e087d42b894aeb5282c709 Mon Sep 17 00:00:00 2001 From: Dustin Brody Date: Thu, 30 Jan 2020 22:03:47 +0100 Subject: [PATCH] add a couple new deposit tests; fix the false-positive BLS verifications while keeping all but two tests working, despite mismatched 0.9/0.10 BLS standards; better-factor the skipping of BLS validation and Merkle tree validation --- beacon_chain/beacon_node.nim | 2 +- beacon_chain/extras.nim | 10 ++++++++ beacon_chain/spec/beaconstate.nim | 9 ++++--- beacon_chain/spec/crypto.nim | 6 +++-- beacon_chain/spec/state_transition_block.nim | 14 ++++++----- research/state_sim.nim | 2 +- tests/mocking/mock_genesis.nim | 2 +- ..._fixture_operations_attester_slashings.nim | 23 +++++++++++++---- .../test_fixture_operations_deposits.nim | 25 ++++++++++--------- .../test_process_deposits.nim | 6 +++-- tests/test_attestation_pool.nim | 4 +-- tests/test_beaconstate.nim | 2 +- tests/test_block_pool.nim | 2 +- tests/test_interop.nim | 2 +- tests/test_state_transition.nim | 2 +- tests/testutil.nim | 3 ++- 16 files changed, 73 insertions(+), 41 deletions(-) diff --git a/beacon_chain/beacon_node.nim b/beacon_chain/beacon_node.nim index 3b5a6def2..ea693a0c5 100644 --- a/beacon_chain/beacon_node.nim +++ b/beacon_chain/beacon_node.nim @@ -1113,7 +1113,7 @@ when isMainModule: else: waitFor getLatestEth1BlockHash(config.depositWeb3Url) var initialState = initialize_beacon_state_from_eth1( - eth1Hash, startTime, deposits, {skipValidation}) + eth1Hash, startTime, deposits, {skipValidation, skipMerkleValidation}) # https://github.com/ethereum/eth2.0-pm/tree/6e41fcf383ebeb5125938850d8e9b4e9888389b4/interop/mocked_start#create-genesis-state initialState.genesis_time = startTime diff --git a/beacon_chain/extras.nim b/beacon_chain/extras.nim index b99682986..203e43cf4 100644 --- a/beacon_chain/extras.nim +++ b/beacon_chain/extras.nim @@ -25,5 +25,15 @@ type ## TODO need to be careful here, easy to assume that slot number change is ## enough, vs advancing the state - however, making a full state copy ## is expensive also :/ + skipMerkleValidation ##\ + ## When processing deposits, skip verifying the Merkle proof trees of each + ## deposit. This is a holdover from both interop issues with the malformed + ## proofs and, more currently, nim-beacon-chain's creation of proofs which + ## are inconsistent with the current specification. Furthermore several of + ## the mocking interfaces deliberately do not create Merkle proofs. Whilst + ## this seems less than entirely justifiable, for now enable keeping those + ## in place while minimizing the tech debt they create. One, in principle, + ## should be able to remove this flag entirely. It is not intrinsically an + ## expensive operation to perform. UpdateFlags* = set[UpdateFlag] diff --git a/beacon_chain/spec/beaconstate.nim b/beacon_chain/spec/beaconstate.nim index 14f44fbe9..a166df490 100644 --- a/beacon_chain/spec/beaconstate.nim +++ b/beacon_chain/spec/beaconstate.nim @@ -53,7 +53,7 @@ proc process_deposit*( # Process an Eth1 deposit, registering a validator or increasing its balance. # Verify the Merkle branch - if skipValidation notin flags and not is_valid_merkle_branch( + if skipMerkleValidation notin flags and not is_valid_merkle_branch( hash_tree_root(deposit.data), deposit.proof, DEPOSIT_CONTRACT_TREE_DEPTH + 1, @@ -361,7 +361,8 @@ proc process_registry_updates*(state: var BeaconState) {.nbench.}= # https://github.com/ethereum/eth2.0-specs/blob/v0.9.4/specs/core/0_beacon-chain.md#is_valid_indexed_attestation proc is_valid_indexed_attestation*( - state: BeaconState, indexed_attestation: IndexedAttestation): bool = + state: BeaconState, indexed_attestation: IndexedAttestation, + flags: UpdateFlags): bool = ## Check if ``indexed_attestation`` has valid indices and signature. # TODO: this is noSideEffect besides logging # https://github.com/status-im/nim-chronicles/issues/62 @@ -380,7 +381,7 @@ proc is_valid_indexed_attestation*( return false # Verify aggregate signature - if not bls_verify( + if skipValidation notin flags and not bls_verify( bls_aggregate_pubkeys(mapIt(indices, state.validators[it.int].pubkey)), hash_tree_root(indexed_attestation.data).data, indexed_attestation.signature, @@ -497,7 +498,7 @@ proc check_attestation*( return if not is_valid_indexed_attestation( - state, get_indexed_attestation(state, attestation, stateCache)): + state, get_indexed_attestation(state, attestation, stateCache), flags): warn("process_attestation: signature or bitfields incorrect") return diff --git a/beacon_chain/spec/crypto.nim b/beacon_chain/spec/crypto.nim index 7eda6e6b0..e5f71d1d8 100644 --- a/beacon_chain/spec/crypto.nim +++ b/beacon_chain/spec/crypto.nim @@ -160,8 +160,10 @@ func bls_verify*( return false # 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. - if pubkey == default(ValidatorPubKey): + # 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 sig.blsValue.verify(msg, domain, pubkey.blsValue) diff --git a/beacon_chain/spec/state_transition_block.nim b/beacon_chain/spec/state_transition_block.nim index b568a62f6..0ff0daec8 100644 --- a/beacon_chain/spec/state_transition_block.nim +++ b/beacon_chain/spec/state_transition_block.nim @@ -217,6 +217,7 @@ func is_slashable_attestation_data( proc process_attester_slashing*( state: var BeaconState, attester_slashing: AttesterSlashing, + flags: UpdateFlags, stateCache: var StateCache ): bool {.nbench.}= let @@ -228,11 +229,11 @@ proc process_attester_slashing*( notice "Attester slashing: surround or double vote check failed" return false - if not is_valid_indexed_attestation(state, attestation_1): + if not is_valid_indexed_attestation(state, attestation_1, flags): notice "Attester slashing: invalid attestation 1" return false - if not is_valid_indexed_attestation(state, attestation_2): + if not is_valid_indexed_attestation(state, attestation_2, flags): notice "Attester slashing: invalid attestation 2" return false @@ -259,7 +260,7 @@ proc processAttesterSlashings(state: var BeaconState, blck: BeaconBlock, return false for attester_slashing in blck.body.attester_slashings: - if not process_attester_slashing(state, attester_slashing, stateCache): + if not process_attester_slashing(state, attester_slashing, {}, stateCache): return false return true @@ -286,13 +287,14 @@ proc processAttestations( true # https://github.com/ethereum/eth2.0-specs/blob/v0.8.4/specs/core/0_beacon-chain.md#deposits -proc processDeposits(state: var BeaconState, blck: BeaconBlock): bool {.nbench.}= +proc processDeposits(state: var BeaconState, blck: BeaconBlock, + flags: UpdateFlags): bool {.nbench.}= if not (len(blck.body.deposits) <= MAX_DEPOSITS): notice "processDeposits: too many deposits" return false for deposit in blck.body.deposits: - if not process_deposit(state, deposit): + if not process_deposit(state, deposit, flags): notice "processDeposits: deposit invalid" return false @@ -419,7 +421,7 @@ proc processBlock*( debug "[Block processing] Attestation processing failure", slot = shortLog(state.slot) return false - if not processDeposits(state, blck): + if not processDeposits(state, blck, flags): debug "[Block processing] Deposit processing failure", slot = shortLog(state.slot) return false diff --git a/research/state_sim.nim b/research/state_sim.nim index 361caf8b7..2022ecca6 100644 --- a/research/state_sim.nim +++ b/research/state_sim.nim @@ -79,7 +79,7 @@ cli do(slots = SLOTS_PER_EPOCH * 6, let genesisState = initialize_beacon_state_from_eth1( - Eth2Digest(), 0, deposits, {skipValidation}) + Eth2Digest(), 0, deposits, {skipMerkleValidation}) genesisBlock = get_initial_beacon_block(genesisState) echo "Starting simulation..." diff --git a/tests/mocking/mock_genesis.nim b/tests/mocking/mock_genesis.nim index 2b874002a..1cfe1dbe5 100644 --- a/tests/mocking/mock_genesis.nim +++ b/tests/mocking/mock_genesis.nim @@ -25,7 +25,7 @@ proc initGenesisState*(num_validators: uint64, genesis_time: uint64 = 0): Beacon ) initialize_beacon_state_from_eth1( - eth1BlockHash, 0, deposits, {skipValidation}) + eth1BlockHash, 0, deposits, {skipValidation, skipMerkleValidation}) when isMainModule: # Smoke test diff --git a/tests/official/test_fixture_operations_attester_slashings.nim b/tests/official/test_fixture_operations_attester_slashings.nim index 5aa44a0df..f0b874d10 100644 --- a/tests/official/test_fixture_operations_attester_slashings.nim +++ b/tests/official/test_fixture_operations_attester_slashings.nim @@ -12,7 +12,7 @@ import os, unittest, # Beacon chain internals ../../beacon_chain/spec/[datatypes, state_transition_block, validator], - ../../beacon_chain/ssz, + ../../beacon_chain/[extras, ssz], # Test utilities ../testutil, ./fixtures_utils, @@ -30,7 +30,10 @@ template runTest(identifier: untyped) = proc `testImpl _ operations_attester_slashing _ identifier`() = + var flags: UpdateFlags var prefix: string + if not existsFile(testDir/"meta.yaml"): + flags.incl skipValidation if existsFile(testDir/"post.ssz"): prefix = "[Valid] " else: @@ -52,10 +55,12 @@ template runTest(identifier: untyped) = postRef[] = parseTest(testDir/"post.ssz", SSZ, BeaconState) if postRef.isNil: - let done = process_attester_slashing(stateRef[], attesterSlashingRef[], cache) + let done = process_attester_slashing(stateRef[], attesterSlashingRef[], + flags, cache) doAssert done == false, "We didn't expect this invalid attester slashing to be processed." else: - let done = process_attester_slashing(stateRef[], attesterSlashingRef[], cache) + let done = process_attester_slashing(stateRef[], attesterSlashingRef[], + flags, cache) doAssert done, "Valid attestater slashing not processed" check: stateRef.hash_tree_root() == postRef.hash_tree_root() reportDiff(stateRef, postRef) @@ -65,8 +70,16 @@ template runTest(identifier: untyped) = suite "Official - Operations - Attester slashing " & preset(): runTest(success_double) runTest(success_surround) - runTest(success_already_exited_recent) - runTest(success_already_exited_long_ago) + when false: + # TODO these are both valid and check BLS signatures, which isn't working + # since 0.10.x introduces new BLS signing/verifying interface with domain + # in particular handled differently through compute_signing_root() rather + # than through the bls_verify(...) call directly. This did not become the + # visible issue it now is because another bug had been masking it wherein + # crypto.nim's bls_verify(...) call had been creating false positives, in + # which cases signature checks had been incorrectly passing. + runTest(success_already_exited_recent) + runTest(success_already_exited_long_ago) runTest(invalid_sig_1) when false: # TODO - https://github.com/status-im/nim-beacon-chain/issues/429 runTest(invalid_sig_2) diff --git a/tests/official/test_fixture_operations_deposits.nim b/tests/official/test_fixture_operations_deposits.nim index 324caa469..3603b41a6 100644 --- a/tests/official/test_fixture_operations_deposits.nim +++ b/tests/official/test_fixture_operations_deposits.nim @@ -53,8 +53,7 @@ template runTest(testName: string, identifier: untyped) = postRef[] = parseTest(testDir/"post.ssz", SSZ, BeaconState) if postRef.isNil: - expect(AssertionError): - discard process_deposit(stateRef[], depositRef[], flags) + check not process_deposit(stateRef[], depositRef[], flags) else: discard process_deposit(stateRef[], depositRef[], flags) reportDiff(stateRef, postRef) @@ -62,17 +61,19 @@ template runTest(testName: string, identifier: untyped) = `testImpl _ operations_deposits _ identifier`() suite "Official - Operations - Deposits " & preset(): - runTest("new deposit under max", new_deposit_under_max) + # https://github.com/ethereum/eth2.0-spec-tests/tree/v0.10.1/tests/minimal/phase0/operations/deposit/pyspec_tests + # https://github.com/ethereum/eth2.0-spec-tests/tree/v0.10.1/tests/mainnet/phase0/operations/deposit/pyspec_tests + runTest("bad merkle proof", bad_merkle_proof) + runTest("invalid signature new deposit", invalid_sig_new_deposit) + runTest("invalid signature other version", invalid_sig_other_version) + runTest("invalid signature top-up", invalid_sig_top_up) + runTest("invalid withdrawal credentials top-up", + invalid_withdrawal_credentials_top_up) runTest("new deposit max", new_deposit_max) runTest("new deposit over max", new_deposit_over_max) - runTest("invalid signature new deposit", invalid_sig_new_deposit) + runTest("new deposit under max", new_deposit_under_max) runTest("success top-up", success_top_up) - runTest("invalid signature top-up", invalid_sig_top_up) - runTest("invalid withdrawal credentials top-up", invalid_withdrawal_credentials_top_up) - when false: - # TODO - those should give an exception but do not - # probably because skipValidation is too strong - # https://github.com/status-im/nim-beacon-chain/issues/407 - runTest("wrong deposit for deposit count", wrong_deposit_for_deposit_count) - runTest("bad merkle proof", bad_merkle_proof) + # TODO + runTest("valid signature but forked state", valid_sig_but_forked_state) + runTest("wrong deposit for deposit count", wrong_deposit_for_deposit_count) diff --git a/tests/spec_block_processing/test_process_deposits.nim b/tests/spec_block_processing/test_process_deposits.nim index f75988997..5a120aedc 100644 --- a/tests/spec_block_processing/test_process_deposits.nim +++ b/tests/spec_block_processing/test_process_deposits.nim @@ -56,7 +56,8 @@ suite "[Unit - Spec - Block processing] Deposits " & preset(): # State transition # ---------------------------------------- - check: state.process_deposit(deposit, {skipValidation}) + check: state.process_deposit(deposit, + {skipValidation, skipMerkleValidation}) # Check invariants # ---------------------------------------- @@ -100,7 +101,8 @@ suite "[Unit - Spec - Block processing] Deposits " & preset(): # State transition # ---------------------------------------- - check: state.process_deposit(deposit, {skipValidation}) + check: state.process_deposit(deposit, + {skipValidation, skipMerkleValidation}) # Check invariants # ---------------------------------------- diff --git a/tests/test_attestation_pool.nim b/tests/test_attestation_pool.nim index a78288fb1..1a44ddad1 100644 --- a/tests/test_attestation_pool.nim +++ b/tests/test_attestation_pool.nim @@ -111,7 +111,7 @@ when const_preset == "minimal": # Too much stack space used on mainnet attestation1 = makeAttestation( state.data.data, state.blck.root, bc0[1], cache) - attestation0.combine(attestation1, {skipValidation}) + attestation0.combine(attestation1, {}) pool.add(attestation0) pool.add(attestation1) @@ -135,7 +135,7 @@ when const_preset == "minimal": # Too much stack space used on mainnet attestation1 = makeAttestation( state.data.data, state.blck.root, bc0[1], cache) - attestation0.combine(attestation1, {skipValidation}) + attestation0.combine(attestation1, {}) pool.add(attestation1) pool.add(attestation0) diff --git a/tests/test_beaconstate.nim b/tests/test_beaconstate.nim index 26f81a8c1..13edaa804 100644 --- a/tests/test_beaconstate.nim +++ b/tests/test_beaconstate.nim @@ -17,5 +17,5 @@ suite "Beacon state" & preset(): timedTest "Smoke test initialize_beacon_state_from_eth1" & preset(): let state = initialize_beacon_state_from_eth1( Eth2Digest(), 0, - makeInitialDeposits(SLOTS_PER_EPOCH, {}), {skipValidation}) + makeInitialDeposits(SLOTS_PER_EPOCH, {}), {skipMerkleValidation}) check: state.validators.len == SLOTS_PER_EPOCH diff --git a/tests/test_block_pool.nim b/tests/test_block_pool.nim index 3874431bf..4578e1f9b 100644 --- a/tests/test_block_pool.nim +++ b/tests/test_block_pool.nim @@ -213,7 +213,7 @@ when const_preset == "minimal": # Too much stack space used on mainnet BeaconBlockBody( attestations: makeFullAttestations( pool.headState.data.data, pool.head.blck.root, - pool.headState.data.data.slot, cache, {skipValidation}))) + pool.headState.data.data.slot, cache, {}))) let added = pool.add(hash_tree_root(blck.message), blck) pool.updateHead(added) diff --git a/tests/test_interop.nim b/tests/test_interop.nim index 3050375cf..ca6596561 100644 --- a/tests/test_interop.nim +++ b/tests/test_interop.nim @@ -150,7 +150,7 @@ suite "Interop": var initialState = initialize_beacon_state_from_eth1( - eth1BlockHash, 1570500000, deposits, {skipValidation}) + eth1BlockHash, 1570500000, deposits, {skipMerkleValidation}) # https://github.com/ethereum/eth2.0-pm/tree/6e41fcf383ebeb5125938850d8e9b4e9888389b4/interop/mocked_start#create-genesis-state initialState.genesis_time = 1570500000 diff --git a/tests/test_state_transition.nim b/tests/test_state_transition.nim index 566ac7e06..5f3cc7bc4 100644 --- a/tests/test_state_transition.nim +++ b/tests/test_state_transition.nim @@ -22,7 +22,7 @@ suite "Block processing" & preset(): # TODO bls verification is a bit of a bottleneck here genesisState = initialize_beacon_state_from_eth1( Eth2Digest(), 0, - makeInitialDeposits(), {skipValidation}) + makeInitialDeposits(), {skipMerkleValidation}) genesisBlock = get_initial_beacon_block(genesisState) genesisRoot = hash_tree_root(genesisBlock.message) diff --git a/tests/testutil.nim b/tests/testutil.nim index 0ee30dc35..1c3eb89d2 100644 --- a/tests/testutil.nim +++ b/tests/testutil.nim @@ -80,7 +80,8 @@ proc makeTestDB*(validators: int): BeaconChainDB = let genState = initialize_beacon_state_from_eth1( Eth2Digest(), 0, - makeInitialDeposits(validators, flags = {skipValidation}), {skipValidation}) + makeInitialDeposits(validators, flags = {skipValidation}), + {skipValidation, skipMerkleValidation}) genBlock = get_initial_beacon_block(genState) makeTestDB(genState, genBlock)