Snap sync: use zero-element proof for checking validity of final, empty range result #3047
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
scorbajio
added
PR state: needs review
package: mpt
type: feature
and removed
PR state: WIP
labels
|
Updated this via UI Can this get a review at some point? 🙏 |
…s used in proof verification
…passed in TrieOpts
|
Will put this back in WIP since it has failing tests. |
|
@scorbajio What is the status of this PR? |
There was a problem hiding this comment.
Two comments.
The implementation looks correct to me, but it seems that it lacks tests of the "unhappy" case that a peer gives us a zero element proof when there are clearly elements left in the trie (for both account + storage fetcher). Plus a case of the all-elements proof which is not covered 😄 👍
packages/trie/src/proof/range.ts
Outdated
| useKeyHashingFunction | ||
| ) | ||
|
|
||
| if (value !== null || (await hasRightElement(trie, firstKey))) { | ||
| if (value !== null || (await hasRightElement(trie, firstKey as any))) { |
There was a problem hiding this comment.
Why does the as any has to be added here? This was not necessary before (also in other occurrences of this file)
There was a problem hiding this comment.
See my other comment bellow (#3047 (comment)). It seems like the implementation wasn't allowing the zero-element parameter combination of an existing firstKey, a null lastKey, and an existing proof to continue to the zero-element proof verification section of the verifyRangeProof implementation because it assumed if firstKey, lastKey, and proof aren't all non-null values after moving past the all-elements proof verification section of the code, that there is an input error, when this is not the case for a zero-element proof. I ended up moving the conditional that checks firstKey, lastKey, or proof aren't null to right after the zero-element proof check, so those three variables aren't being checked to be non-null in that section.
There was a problem hiding this comment.
I've added a check to make sure the parameters are as expected when running a zero-element proof, so the casting to any has been removed.
| throw new Error('invalid zero element proof: value mismatch') | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| if (proof === null || firstKey === null || lastKey === null) { | ||
| throw new Error( | ||
| 'invalid all elements proof: proof, firstKey, lastKey must be null at the same time' |
There was a problem hiding this comment.
This is new, but has to do with an all elements proof 🤔 (looks correct to me, but maybe add a test 😄 )
There was a problem hiding this comment.
This check is actually not new, I've only moved it bellow where zero-element proof checking happens. It was previously like this:
if (proof === null || firstKey === null || lastKey === null) {
throw new Error(
'invalid all elements proof: proof, firstKey, lastKey must be null at the same time'
)
}
// Zero element proof
if (keys.length === 0) {
const { trie, value } = await verifyProof(
rootHash,
nibblestoBytes(firstKey),
proof,
useKeyHashingFunction
)
if (value !== null || (await hasRightElement(trie, firstKey))) {
throw new Error('invalid zero element proof: value mismatch')
}
return false
}
The reason I did that was because zero-element proof checking was failing when I was passing in just the root, firstKey, and proof, with null for lastKey and empty arrays for keys and values, as such:
const isMissingRightRange = await Trie.verifyRangeProof(
this.root,
origin,
null,
[],
[],
<any>rangeResult.proof,
{ useKeyHashingFunction: keccak256 }
)
The error resulting from this was confusing a zero-element request to be a failed all-elements request:
2024-02-16T00:22:13.531Z client:AccountFetcher Error: invalid all elements proof: proof, firstKey, lastKey must be null at the same time
at Module.verifyRangeProof (/Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/packages/trie/src/proof/range.ts:450:11)
at Function.verifyRangeProof (/Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/packages/trie/src/trie.ts:208:12)
at AccountFetcher.request (/Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/packages/client/src/sync/fetcher/accountfetcher.ts:400:50)
at /Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/packages/client/test/sync/fetcher/accountfetcher.spec.ts:313:17
at runTest (file:///Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/node_modules/@vitest/runner/dist/index.js:486:11)
at runSuite (file:///Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/node_modules/@vitest/runner/dist/index.js:594:15)
at runSuite (file:///Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/node_modules/@vitest/runner/dist/index.js:594:15)
at runFiles (file:///Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/node_modules/@vitest/runner/dist/index.js:645:5)
at startTests (file:///Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/node_modules/@vitest/runner/dist/index.js:654:3)
at file:///Users/indigoalpha/code/ethjs/0/ethereumjs-monorepo/node_modules/vitest/dist/entry.js:278:7
|
@scorbajio This is a somewhat strange order of things with some likelihood to cause confusion with Jochem doing a review and leave some comments and then this PR being set to "Needs review" without the things mentioned actually being adressed. 🤔 Maybe as some general suggestion always good to add some short comment when one is changing the PR state, this always helps to avoid misunderstandings and additionaly avoid of PR state changes getting lost since GitHub does not notify. Thanks 🙏 |
That's a good point 🙂. I've added a basic test for the storage fetcher and account fetcher to test if they will reject a zero-element proof if an element to the right exists, will be able to write more range proof tests once the flat db work is further along and we can create our own range proofs, but the all-elements proofs are already being tested in the trie package (e.g. ). |
|
Updated this via UI @jochem-brouwer can this get a review update? |
|
Updated this via UI |
…ty range result (#3047) * Use no-elements proof for final check in account fetcher * Use no-elements proof for final check in storage fetcher * Check inputs after zero element proof code section * Cleanup * Reject peer if zero-element proof fails * Add tests for zero-element proof * Remove proofTrie usage * Use hashing function in static version of verifyRangeProof * Include useKeyHashing in call to fromProof so that hashing function is used in proof verification * Use appliedKey for hashing function instead of the function directly passed in TrieOpts * Pass in hashing function for use in static proof verification calls * Fix static range proof verification errors * Use custom hashing function from opts if available * Add test to check if zero element range proof verification fails with remaining elements to the right * Check if parameters are as expected for zero-element proof * Fix linting issues --------- Co-authored-by: Scotty <[email protected]> Co-authored-by: Holger Drewes <[email protected]> Co-authored-by: Indigo Alpha <[email protected]>
* client: PreimageManager and Preimage storage in MetaDB * common: add getAppliedKey method to StateManagerInterface * evm: add preimage reporting * statemanager: add getAppliedKey methods to the statemanager implementations * vm: improve some types * vm: implement preimage reporting * client: add preimage saving * evm: update to unprefixedHexToBytes * vm: add reportPreimages test to runTx tests * vm: pass down reportPreimages arg from block to tx * client: pass down reportPreimages from client to vm through runBlock * client: unify config options naming * client: preimageManager test * client: test preimage, wip * common: add todo to gasPrices from 6800 * statemanager: fix type issues * dev accessWitness class on the lines of geth impl * integrate accesswitness in evm/vm/runtx flow * plugin the gas schedule for tx origin and destination accesses * complete, debug and fix the call and create access charges * plug the access gas usage on the evm opcode runs * debug and fix the code chunk accesses and the poststate setting * implement access merging and accessed state tracking and traversing * also provide chunkKey for easy reference * decode raw accesses to structured ones and debug log them * debug and add the missing accesses for coinbase, withdrawals * stateManager: implement cache handling in the context of statelessness * modify poststate to use accesses from the accesswitness to compare and fix mising chunkKey in returned accesses * stateManager: getComputedValue * fixes for the getcomputedval fn helper for the code * correctly implement the getcontractcode with partial accessed segments available and corresponding error handling in evm run * statemanager: tentative fixes & improvements to code cache handling * statemanager: checkpoint code cache * fix the code chunk comparision in post state verification * rename kaustinen2 local testnet data for * fix pre and post state null chunks handling and corresponding get computed fixes * setup the client to statelessly execute the verkle blocks randomly from anywhere the chain * setup to statelessly run block13 of kaustinen2 in client test spec for easy debugging * setup the client kaustinen2 stateless test to test and debug block 16 * improve the post state witness mismatch logging to be more comprehensible for debugging * improve chunk verification tracking logging and fix post/prestate key coding from the witnesses * debug and fix code accesses and overhaul/fix accesscharges on create/call * add a more complete matching of generated and provided executed witnesses * fix return * debug, discuss and remove proof of absence charges for contract create for fixing the extra witnesses * only charge code accesses if accessed from state and when code is written on contract creation * handle bigint treeIndex for the bigint slot and debug/match the predersenhash with kauntinen2 usage * client: fix kaustinen tests and migrate to new testing framework * shift kaustinen 2 test to block 13 * client: remove stale kaustinen2 test data * vm: adjust rewardAccount new common arg ordering and make it optional * add block12 to the spec list * move invalid opcode check outside jump analysis * small cleanup in runtx * add preimages spec * build various block scenarios for preimages gen * client: revert vmexecution preimages test * client: remove unnecessary boolean explicit comparison * client: remove unused import * client: revert re-ordering * stateManager: support non keccak256 applied keys * debug and persist preimages on runWithoutSetHead as well * add preimages for coinbase,withdrawals to the test and fix the spec to validate them * client: fix preimage tests ts errors * client: minor adjustments to preimage test * client: save preimages helper * vm: save preimages at the block level, new applyBlockResult type * common: make getAppliedKey optional * evm/vm: handle optional getAppliedKey * client: add preimage doc * trie: export "Path" interface (#3292) * trie: export "Path" interface * Move over Path interface export to types for consistency --------- Co-authored-by: Holger Drewes <[email protected]> * Snap sync: use zero-element proof for checking validity of final, empty range result (#3047) * Use no-elements proof for final check in account fetcher * Use no-elements proof for final check in storage fetcher * Check inputs after zero element proof code section * Cleanup * Reject peer if zero-element proof fails * Add tests for zero-element proof * Remove proofTrie usage * Use hashing function in static version of verifyRangeProof * Include useKeyHashing in call to fromProof so that hashing function is used in proof verification * Use appliedKey for hashing function instead of the function directly passed in TrieOpts * Pass in hashing function for use in static proof verification calls * Fix static range proof verification errors * Use custom hashing function from opts if available * Add test to check if zero element range proof verification fails with remaining elements to the right * Check if parameters are as expected for zero-element proof * Fix linting issues --------- Co-authored-by: Scotty <[email protected]> Co-authored-by: Holger Drewes <[email protected]> Co-authored-by: Indigo Alpha <[email protected]> * Integrate `kzg-wasm` into monorepo (#3294) * Update to official trusted setup * Remove devnet6 * Add kzg-wasm * Update block tests to use kzg-wasm * Update tests * Add more 4844 tests to browser run * Initial integration of kzg-wasm on git * Update kzg-wasm build * Fix linter weirdness * Move initKzg to `runTests` * Fix tests * More cleanup * Goodbye c-kzg * fix kzg references * Replace c-kzg with kzg-wasm in package.json * Update kzg wasm commit and vm tester config * Update initKzg to createKZG * fix copy pasta * Fix more copy pasta * update kzg-wasm to npm release * One last bit of copy pasta * Address feedback * client: remove try/catch blocks createKZG() and remove the initKZG stale comment --------- Co-authored-by: Jochem Brouwer <[email protected]> * Make trustedSetupPath in Util kzg module optional (#3296) * client: add tx preimages to preimage test cases --------- Co-authored-by: acolytec3 <[email protected]> Co-authored-by: harkamal <[email protected]> Co-authored-by: Jochem Brouwer <[email protected]> Co-authored-by: Scotty <[email protected]> Co-authored-by: Holger Drewes <[email protected]> Co-authored-by: Scorbajio <[email protected]> Co-authored-by: Indigo Alpha <[email protected]>
During snap sync, if the client requests a range that is both empty and has no elements remaining to the right of it, the peer responds with just a proof. This proof can be checked as part of a zero-element proof to validate that there really are no remaining elements to be fetched past the range. This helps avoid premature halting, corrupt peers, and DoS attacks on snap sync.