update process_deposit() to actually check is_valid_merkle_branch() #705
Conversation
…nless skipValidation specified
| deposit.proof, | ||
| DEPOSIT_CONTRACT_TREE_DEPTH, | ||
| state.eth1_deposit_index, | ||
| if skipValidation notin flags and not is_valid_merkle_branch( |
There was a problem hiding this comment.
hm, what are the use cases for skipValidation? for tests it should be cheap enough to calculate..
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
shouldn't there be a previously disabled test case enabled after this? |
|
@arnetheduck see #712 which adds the |
Should address #703 along with in general finishing a long-standing TODO.