Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update process_deposit() to actually check is_valid_merkle_branch() #705

Merged
merged 1 commit into from
Jan 30, 2020
Merged

update process_deposit() to actually check is_valid_merkle_branch() #705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 8 additions & 15 deletions beacon_chain/spec/beaconstate.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,27 +47,20 @@ func decrease_balance*(
else:
state.balances[index] - delta

# https://github.com/ethereum/eth2.0-specs/blob/v0.8.4/specs/core/0_beacon-chain.md#deposits
func process_deposit*(
# https://github.com/ethereum/eth2.0-specs/blob/v0.9.4/specs/core/0_beacon-chain.md#deposits
proc process_deposit*(
state: var BeaconState, deposit: Deposit, flags: UpdateFlags = {}): bool {.nbench.}=
# Process an Eth1 deposit, registering a validator or increasing its balance.

# Verify the Merkle branch
# TODO enable this check, but don't use doAssert
if not is_valid_merkle_branch(
hash_tree_root(deposit.getDepositMessage),
deposit.proof,
DEPOSIT_CONTRACT_TREE_DEPTH,
state.eth1_deposit_index,
if skipValidation notin flags and not is_valid_merkle_branch(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, what are the use cases for skipValidation? for tests it should be cheap enough to calculate..

Copy link
Contributor Author

@tersec tersec Jan 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually more expensive asymmetric cryptography, especially BLS signing and verification. In this case, the rest of the test-genesis-beacon-block infrastructure wasn't properly creating deposit proofs either, and it's not clear that's relevant to any other nim-beacon-chain functionality for release, so this enables the check for all external deposits (which includes those from the EF test suite).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant in this particular case - generating the merkle tree for the deposit data should be quick - it was disabled mainly because during interop, clients were not generating it correctly yet..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this particular case, yes, generating the Merkle tree for the deposit data should be quick, but nim-beacon-chain stlll cannot create correct trees in this case. This PR splits off the verification-of-other-trees functionality to address a specific fuzzing result, and apparently specific not-actually-run EF test, in a way that doesn't hinder verifying all such trees when nim-beacon-chain itself can correctly create them for mocking purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, this lives in https://github.com/status-im/nim-beacon-chain/blob/devel/tests/mocking/merkle_minimal.nim, https://github.com/status-im/nim-beacon-chain/blob/devel/tests/mocking/mock_deposits.nim, and https://github.com/status-im/nim-beacon-chain/blob/devel/tests/mocking/mock_genesis.nim, but that's inconsistent with the correct approach in a few ways.

Most of the mock-genesis-code doesn't even try to use that though - https://github.com/status-im/nim-beacon-chain/blob/devel/tests/testblockutil.nim#L47 has makeDeposit() and makeInitialDeposits() which simply don't attempt to do anything here.

But all of that's a bigger PR than necessary to fix this particular bug, and there are higher priorities at which I was and am aiming.

hash_tree_root(deposit.data),
deposit.proof,
DEPOSIT_CONTRACT_TREE_DEPTH + 1,
state.eth1_deposit_index,
state.eth1_data.deposit_root,
):
## TODO: a notice-like mechanism which works in a func
## here and elsewhere, one minimal approach is a check-if-true
## and return false iff so.
## obviously, better/more principled ones exist, but
## generally require broader rearchitecting, and this is what
## mostly happens now, just haphazardly.
discard
return false

# Deposits must be processed in order
state.eth1_deposit_index += 1
Expand Down
5 changes: 3 additions & 2 deletions research/state_sim.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# beacon_chain
# Copyright (c) 2019 Status Research & Development GmbH
# Copyright (c) 2019-2020 Status Research & Development GmbH
# Licensed and distributed under either of
# * MIT license (license terms in the root directory or at https://opensource.org/licenses/MIT).
# * Apache v2 license (license terms in the root directory or at https://www.apache.org/licenses/LICENSE-2.0).
Expand Down Expand Up @@ -78,7 +78,8 @@ cli do(slots = SLOTS_PER_EPOCH * 6,

let
genesisState =
initialize_beacon_state_from_eth1(Eth2Digest(), 0, deposits, flags)
initialize_beacon_state_from_eth1(
Eth2Digest(), 0, deposits, {skipValidation})
genesisBlock = get_initial_beacon_block(genesisState)

echo "Starting simulation..."
Expand Down
7 changes: 4 additions & 3 deletions tests/test_beaconstate.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# beacon_chain
# Copyright (c) 2018 Status Research & Development GmbH
# Copyright (c) 2018-2020 Status Research & Development GmbH
# Licensed and distributed under either of
# * MIT license (license terms in the root directory or at https://opensource.org/licenses/MIT).
# * Apache v2 license (license terms in the root directory or at https://www.apache.org/licenses/LICENSE-2.0).
Expand All @@ -10,11 +10,12 @@
import
times, unittest,
./testutil, ./testblockutil,
../beacon_chain/spec/[beaconstate, datatypes, digest]
../beacon_chain/spec/[beaconstate, datatypes, digest],
../beacon_chain/extras

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, {}), {})
makeInitialDeposits(SLOTS_PER_EPOCH, {}), {skipValidation})
check: state.validators.len == SLOTS_PER_EPOCH
6 changes: 3 additions & 3 deletions tests/test_state_transition.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# beacon_chain
# Copyright (c) 2018 Status Research & Development GmbH
# Copyright (c) 2018-2020 Status Research & Development GmbH
# Licensed and distributed under either of
# * MIT license (license terms in the root directory or at https://opensource.org/licenses/MIT).
# * Apache v2 license (license terms in the root directory or at https://www.apache.org/licenses/LICENSE-2.0).
Expand All @@ -11,7 +11,7 @@ import
unittest,
./testutil, ./testblockutil,
../beacon_chain/spec/[beaconstate, datatypes, digest, validator],
../beacon_chain/[state_transition, ssz]
../beacon_chain/[extras, state_transition, ssz]

suite "Block processing" & preset():
## For now just test that we can compile and execute block processing with
Expand All @@ -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(), {})
makeInitialDeposits(), {skipValidation})
genesisBlock = get_initial_beacon_block(genesisState)
genesisRoot = hash_tree_root(genesisBlock.message)

Expand Down