-
Notifications
You must be signed in to change notification settings - Fork 772
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.
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
client: save preimages feature #3143
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. |
I would actually add this flag into Trie. Then, before you hash the key, save it into another DB (as preimage). This would only require an extra option for the DB to write too, though. But adding it into Trie means that we capture all trie items which are ever put, and we can be 100% (?) sure that we capture all preimages. |
So I haven't pushed the whole WIP, as I was still playing around with some lower-level (vm/statemanager/trie) handling. I don't think we can strictly have that at the trie level, as it needs to be at least one-level higher up to handle the transition logic from trie->verkle-trie. Also, after discussing with gajinder, we though that the metaDB from the client seemed like a good place to put this. Nothing set in stone though, happy to hear your thoughts/ideas My current plan is to have the following structure:
|
Yes, upon first thoughts I think this is right. What about the idea of Verkle where touched-but-deleted is different than in Merkle where touched-but-deleted does not change the trie? I think I recall this is dropped? To test if all preimages are indeed stored in the StateManager I think we can add an extra test to the blockchain/state tests, what we do is we indeed copy all the MetaDB pre-image items, then after |
Updated this via UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two final comments :) Thanks for the update!
'0x4600000000000000000000000000000000000000', | ||
'0x4700000000000000000000000000000000000000', | ||
], | ||
// no txs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there no txs in this block? It seems that there are?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there also be a test where a contract is invoked and a storage slot is written to? This is tested into client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@g11tech would know more about those cases than me, would you mind clarifying gajinder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes there are, we need to run the test with debug enabled for evm and add the list of things accessed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I logged the accesses and added some tx preimages to the two blocks that have transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* trie: export "Path" interface * Move over Path interface export to types for consistency --------- Co-authored-by: Holger Drewes <[email protected]>
…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]>
* 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]>
14ffd9e
to
1903314
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Great work have removed my preimage -> preImage stuff :)
Seems browser tests are not going through here? |
ee09f29
to
1408e29
Compare
addAlwaysWarmAddress(address: string, addToAccessList?: boolean): void | ||
addAlwaysWarmSlot(address: string, slot: string, addToAccessList?: boolean): void | ||
startReportingAccessList(): void | ||
startReportingPreimages(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, just seeing on release notes writing: this needs to be optional for backwards compatibility.
* Add 4844-browser-readiness and KZG WASM setup instructions to tx CHANGELOG and README * Add all CHANGELOG files * Bump versions for block, blockchain, client and common, give internal verkle dependencies a caret range * Bump versions for devp2p, ethash, evm and genesis * Bump versions for statemanager, trie, tx and util * Bump versions for verkle, vm and wallet * Add changes from Preimage PR #3143 * Added monorepo-wide docs:build command to root package.json * Rebuild docs * Update README and example files * Rebuild package-lock.json * Add various continued work PRs to the CHANGELOG entries * Bump EVM to v3.0.0 and VM to v8.0.0 (in-between breaking releases due to new EVM async create() constructor) * Rebuild package-lock.json * Add additional EVM/VM breaking release CHANGELOG notes, slight simplification of EVM.create() constructor calling * Add additional EVM/VM breaking release README notes * Rebuild docs * Some TypeScript test fixes * More CHANGELOG additions * Update CHANGELOG files (small PRs) * Update KZG related CHANGELOG and README entries and example code * EVM: make main constructor protected
This PR will add a savePreimages option to the client, which will save a key-value map of keyHashes to their preimages (pre-hashed keys). This is a required preliminary step to eventually implement the merkle->verkle transition logic.
Could also be somewhat related to #614