Adds an unoptimized Merkle trie contract (MerkleTrieLib.sol)#154
Adds an unoptimized Merkle trie contract (MerkleTrieLib.sol)#154smartcontracts wants to merge 6 commits intomasterfrom
Conversation
willmeister
left a comment
willmeister
left a comment
There was a problem hiding this comment.
Looks good! 🔥
I left a few suggestions, but I think all the logic should work, with the only caveat I see being guarantees about the length of the value of the first node.
| /** | ||
| * @notice Library for dealing with Merkle tries. | ||
| */ | ||
| contract MerkleTrieLib { |
There was a problem hiding this comment.
Sort of nitpicky: Might suggest either making this a library instead of a contract or dropping/replacing Lib in the name.
There was a problem hiding this comment.
Ah yeah, whoops. I meant to change the name. It's annoying to test libraries and @karlfloersch mentioned that Solidity is considered deprecating libraries anyway.
| * @param _key Key of the node to verify. | ||
| * @param _value Value of the node to verify. | ||
| * @param _root Root of the trie. | ||
| * @param _proof Encoded proof. |
There was a problem hiding this comment.
Would add more info about the format & contents of _proof
| if (keyIndex == 0) { | ||
| // First proof element is always the root node. | ||
| require( | ||
| keccak256(node.encoded) == currentHash, | ||
| "Invalid root hash" | ||
| ); |
There was a problem hiding this comment.
This gets checked every iteration but is only applicable for the first. 2 Questions:
- Can it be added as a check inside of the current
else ifandelseonly to be done if the node is invalid? - Is it possible for the first node to be < 32 bytes? (this doesn't matter if the answer to 1 is yes)
There was a problem hiding this comment.
So re: 2., the first node can be < 32 bytes in certain cases. Given that, I think the extra logic to move this into the else if or else statements would probably make this messier overall.
There was a problem hiding this comment.
Yeah, I agree that we'll want to keep it DRY. Does it make sense to just completely remove this if? If I'm understanding correctly, we'll want to treat the root the same as other nodes in that currentHash should equal keccak256(node.encoded) IFF node.encoded is 32 bytes and node.encoded (not hashing it) otherwise. Sorry if it wasn't clear -- 2 above was meant to point out that in the case that the root node is < 32 bytes, I think the require(...) will fail when it shouldn't. I could definitely be misunderstanding something though!
| // Nodes with `TREE_RADIX + 1` elements are branches. | ||
| if (node.decoded.length == TREE_RADIX + 1) { |
There was a problem hiding this comment.
Is it worthwhile to define a BRANCH_NODE_LENGTH const above to be TREE_RADIX + 1? That would alleviate the need for the comment and addition in every iteration (though this is likely optimized by the compiler).
There was a problem hiding this comment.
Yep, will do.
| } else { | ||
| // Find the next node within the branch node and repeat. | ||
| RLPReader.RLPItem memory next = node.decoded[uint8(key[keyIndex])]; | ||
| currentHash = getCorrectBytes(next).toBytes32(); |
There was a problem hiding this comment.
I might suggest renaming getCorrectBytes(...) to getNodeHash(...). If we're going to call it hash everywhere else even though it may not technically be a hash, we should lean into it. If not, maybe getNodeIDBytes(...) or something? 🤷
There was a problem hiding this comment.
Yeah, this one is a little weird. I think ID works better to be honest. I'll change that up.
| if (node.decoded.length == 2) { | ||
| // Throw this step into a new function to avoid `STACK_TOO_DEEP`. | ||
| bool done; | ||
| (done, currentHash, keyIndex) = checkNonBranchNode( |
There was a problem hiding this comment.
Looks like keyIndex will only ever increase if !done, and always be 0 when done. Does it make sense to remove done and convey done that way, (maybe with a DONE_KEY_INDEX const?) or is that too cryptic?
There was a problem hiding this comment.
Hmmmm. Returning 0 for keyIndex was mainly a way to deal with the fact that checkNonBranchNode must return all three values. It might be a little confusing to make this change, could make it seem like keyIndex == 0 is a necessary requirement for being done.
There was a problem hiding this comment.
Sounds good! I thought it may be a stretch but thought I'd mention. I support leaving it in 👍
|
@kfichter I want to merge this once it passes CI as well! Lol |
|
Seems this has been superseded by #160 |
* Separates storage from SCC and CTC (#151) * First pass version * More minor tweaks for tests to pass * Add authentication * Minor config updates * Fix lint error * Fix naming changes per review * Enable Deployer Whitelist (#119) * first pass, test runner updated * add ability to only validate flag, test passes * all tests passing * clean up console.logs * enforce gas refund preservation * more cleanup/import removal * whitelisted -> allowed * first pass, test runner updated * add ability to only validate flag, test passes * all tests passing * clean up console.logs * enforce gas refund preservation * more cleanup/import removal * whitelisted -> allowed * remove whitespace * Restrict StateTransitionerFactory (#140) * added msg sender check * add create test * cleanup * add param * add addressmanager.address param * CTC Chain Monotonicity Fixes (#93) * [wip] Fix block time logic * some sad path and happy tests passing * more progress * first pass sad cases tested * cleanup, adding empty tests * more reversion tests * rename shouldstartat} * add final couple tests * enable more tests * cleanup * remove .only * textual cleanup * make queue length public * improve structure, comments * update deploy config * address nits Co-authored-by: Karl Floersch <karl@karlfloersch.com> * fix declarations, lint (#152) * Adds river's new Merkle tree implementation, with some cleanup (#148) * Reverts an accidental breaking merge * Added new merkle tree impl * add comments * Final cleanups and merge Co-authored-by: Ben Jones <ben@pseudonym.party> * Fix run gas Lower Bound (#94) * added the check * add test * lower OVM TX size for Kovan * re-remove gas check * update gas vals slightly * lint * lint * Merge master into freeze integration branch (#153) * update solidity version to ^0.7.0 (#122) * update solc version to ^0.7.0 * interfaces back to solidity >0.6.0 <0.8.0 * update solc to 0.7.6 * back to 0.7.4 * upgrade to 0.7.6, fix EXTCODESIZE check * versions >0.5.0 <0.8.0 for xdomain msgers * ctc: disable appendQueueBatch (#150) * ctc: disable appendSequencerBatch * typo: fix * re-enable verifyQueueTransaction test: * add explicit test for verifying queue elements against either append Co-authored-by: Ben Jones <ben@pseudonym.party> * fix up test * remove .only Co-authored-by: Alina <alina@optimism.io> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> * add check, simple test, update deploy (#154) * go back to first name (#155) * lint * fix js number error * add error logging to help debug deploy * [code freeze] Fix deploy script (#156) * fix deploy script * add block time config * ensure value is integer * lint * remove console logs from deploy * Moves gas check to applyTransaction (#161) * move to OVM_ST, pass test * remove old test because functionality moved * linting * remove leaf hasing * use safe EXEMRG wrapper (#162) * use safeREQUIRE * add owner getter * relayer: add to config (#160) * relayer: add to config * lint: fix * Fix minor error in test config Co-authored-by: Kelvin Fichter <kelvinfichter@gmail.com> Co-authored-by: ben-chain <ben@pseudonym.party> Co-authored-by: Alina <alina@optimism.io> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> Co-authored-by: Kevin Ho <kevinjho1996@gmail.com>
…e_test Adds an end to end system test that runs an L1 node, a rollup node, and a L2 node. It then submits a deposit and then verifies that the result of the deposit shows up on Layer 2.
526787acd popm/wasm: improve js.Value creation, add JSMarshaler interface (ethereum-optimism#160) be5de3b9a popm/wasm: add error codes (ethereum-optimism#154) git-subtree-dir: heminetwork git-subtree-split: 526787acd0454b26bd4f44927aadcdda4bbdac58
526787acd popm/wasm: improve js.Value creation, add JSMarshaler interface (ethereum-optimism#160) be5de3b9a popm/wasm: add error codes (ethereum-optimism#154) git-subtree-dir: heminetwork git-subtree-split: 526787acd0454b26bd4f44927aadcdda4bbdac58
526787acd popm/wasm: improve js.Value creation, add JSMarshaler interface (ethereum-optimism#160) be5de3b9a popm/wasm: add error codes (ethereum-optimism#154) git-subtree-dir: heminetwork git-subtree-split: 526787acd0454b26bd4f44927aadcdda4bbdac58
### Description Small PR to add an `is_system_transaction` method to the `OpTxEnvelope` that applies to deposit transactions. Upstreams [this bit of logic in kona](https://github.com/anton-rs/kona/blob/main/crates/executor/src/util.rs#L73-L79).
3 participants
Description
Hello! This PR introduces an unoptimized contract for dealing with Merkle tries. Currently supports a single method
verifyInclusionProof(bytes _key, bytes _value, bytes32 _root, bytes memory _proof) returns (bool).Contract is designed to handle both binary and hexary tries (modified via
TREE_RADIX). Currently only tested with hexary tries until we're sure about the format of the binary trie proofs.Current gas usage for 2048 nodes is ~300k gas, where ~100k of that is
calldataand can't be reduced w/o modifying the underlying tree structure. Generally speaking,calldatasize approximately follows the formulalog_radix(n) * [(radix + 1) + (2 / n)] * 32, plus extra when accounting for RLP encoding. Gas cost is thencalldata_bytes * 16(calldatais 16 gas per byte). For a trie of height 40, this comes out to ~100-200k gas. Most of the excess gas stems from RLP encoding within the proof.Metadata
Fixes
Contributing Agreement