fix(trie): fix ZkMerkleStateTrie returns invalid proofs #63
Conversation
|
Warning Rate Limit Exceeded@jyc228 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 26 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes involve enhancing the handling and processing of data within a blockchain context, specifically focusing on the optimization and security of Merkle Trie operations. A new function for reversing byte slices has been introduced, and various components have been adjusted to improve the creation and verification of zero-knowledge (ZK) state tries. This includes modifications to key handling, state trie creation, and proof generation, emphasizing efficiency and privacy enhancements in blockchain data structures. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (8)
- common/bytes.go (1 hunks)
- internal/ethapi/api.go (1 hunks)
- trie/iterator_test.go (1 hunks)
- trie/zk/merkle_tree_node.go (1 hunks)
- trie/zk/util.go (2 hunks)
- trie/zk_merkle_stack_trie.go (1 hunks)
- trie/zk_merkle_state_trie.go (1 hunks)
- trie/zk_merkle_trie.go (2 hunks)
Additional comments: 10
trie/zk_merkle_stack_trie.go (1)
- 19-19: The modification to reverse the
hashparameter in theUpdatemethod before passing it to theMerkleTree.Updatemethod aligns with the PR's objective to ensure keys are correctly reversed during probe operations. This change is crucial for addressing the issue with invalid proofs in zkStateTries.trie/zk/util.go (1)
- 74-94: The changes in the
computeNodeHashfunction, including the removal of theValueHashfield logic from theLeafNodestruct and adjustments in the node hash computation logic, are consistent with the PR's objective to enhance the integrity and reliability of the proof generation process. The reorganization and error handling modifications improve the clarity and maintainability of the code.trie/zk_merkle_trie.go (1)
- 15-44: The introduction of the
mutateKeyfunction and its usage in methods such asMustGet,Get,GetLeafNode,MustUpdate,Update,MustDelete, andDeletefor key transformation is a significant improvement. This change ensures that keys are correctly reversed during probe operations, aligning with the PR's objective to fix invalid proofs in zkStateTries. The implementation is consistent, enhancing the code's maintainability and readability.common/bytes.go (1)
- 153-159: The addition of the
ReverseBytesfunction is a valuable utility that supports the PR's objective by providing a standardized way to reverse byte slices. This functionality is essential for ensuring that keys are correctly reversed during probe operations in zkStateTries, contributing to the integrity and reliability of the proof generation process.trie/zk_merkle_state_trie.go (1)
- 119-122: The modification to the
Provemethod to use thecommon.ReverseBytesfunction for reversing the key aligns with the PR's objective to ensure keys are correctly reversed during probe operations. This change is crucial for addressing the issue with invalid proofs in zkStateTries, enhancing the integrity and reliability of the proof generation process.trie/iterator_test.go (1)
- 645-645: The modification in the
TestMerkleTreeIteratorfunction to usezk.MustNewSecureHash(common.LeftPadBytes([]byte(val.k), 32)).Bytes()for thetree.Updatemethod call is a minor but important change. It ensures consistency in how byte slices are handled and passed to theUpdatemethod, potentially affecting the test's behavior and its interaction with thezkpackage. This change aligns with the overall codebase's approach to handling byte slices and hash computations.internal/ethapi/api.go (4)
- 759-762: The logic for determining the trie type based on the database's properties (
IsZkStateTrieandIsZk) is clear and straightforward. However, ensure that thetrie.NewZkMerkleStateTrie,trie.NewZkTrie, andtrie.NewStateTriefunctions handle errors appropriately and do not return a valid trie object in case of initialization failures.- 756-765: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2592-2595]
The
SendTransactionmethod correctly assembles and signs a transaction based on the provided arguments. However, it's crucial to ensure that thesetDefaultsmethod, which fills in missing transaction fields, properly handles all edge cases and inputs. This includes setting reasonable defaults for gas price in different network conditions (e.g., EIP-1559 networks) and ensuring nonce management is accurate to prevent transaction replacement issues.
- 756-765: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3285-3290]
The
GetRawHeadermethod inDebugAPIprovides a way to retrieve the RLP encoding of a block header. It's important to verify that the RLP encoding and decoding operations are symmetric and that no data is lost or incorrectly encoded during the process. This ensures that the raw header data can be reliably used for debugging or other purposes.
- 756-765: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3364-3369]
In the
NetAPIstruct, methods likeListening,PeerCount, andVersionprovide network-related information. Ensure that thePeerCountmethod accurately reflects the number of connected peers, including any potential edge cases where peers are momentarily disconnected or in the process of connecting/disconnecting. This accuracy is crucial for network monitoring and diagnostics.
Pangssu
changed the title
trie/zk_merkle_trie.go
Outdated
| @@ -12,11 +12,36 @@ import ( | |||
|
|
|||
| type ZkMerkleTrie struct { | |||
| *zk.MerkleTree | |||
| db *Database | |||
| db *Database | |||
| mutateKey func(key []byte) []byte | |||
There was a problem hiding this comment.
Why don't we rename this? Such as transformKey, preProcessKey, or reverseKey.
There was a problem hiding this comment.
i'll change it to a transformKey.
| @@ -116,10 +116,10 @@ func (z *ZkMerkleStateTrie) delete(key []byte) error { | |||
| } | |||
|
|
|||
| func (z *ZkMerkleStateTrie) Prove(key []byte, proofDb ethdb.KeyValueWriter) error { | |||
| return z.MerkleTree.Prove(key, func(node zk.TreeNode) error { | |||
| return z.prove(common.ReverseBytes(key), proofDb, func(node zk.TreeNode) error { | |||
There was a problem hiding this comment.
| return z.prove(common.ReverseBytes(key), proofDb, func(node zk.TreeNode) error { | |
| return z.prove(z.mutateKey(key), proofDb, func(node zk.TreeNode) error { |
| value := node.CanonicalValue() | ||
| if leaf, ok := node.(*zk.LeafNode); ok { | ||
| if preImage := z.GetKey(zkt.ReverseByteOrder(leaf.Key)); len(preImage) > 0 { | ||
| if preImage := z.GetKey(common.ReverseBytes(leaf.Key)); len(preImage) > 0 { |
There was a problem hiding this comment.
| if preImage := z.GetKey(common.ReverseBytes(leaf.Key)); len(preImage) > 0 { | |
| if preImage := z.GetKey(z.mutateKey(leaf.Key)); len(preImage) > 0 { |
jyc228
force-pushed
the
fix/zkstatetrie-invalid-proof
branch
from
4ac11a6 to
539dff7
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- common/bytes.go (1 hunks)
- internal/ethapi/api.go (1 hunks)
- trie/iterator_test.go (1 hunks)
- trie/zk/merkle_tree_node.go (1 hunks)
- trie/zk/util.go (2 hunks)
- trie/zk_merkle_stack_trie.go (1 hunks)
- trie/zk_merkle_state_trie.go (3 hunks)
- trie/zk_merkle_trie.go (4 hunks)
Files skipped from review as they are similar to previous changes (8)
- common/bytes.go
- internal/ethapi/api.go
- trie/iterator_test.go
- trie/zk/merkle_tree_node.go
- trie/zk/util.go
- trie/zk_merkle_stack_trie.go
- trie/zk_merkle_state_trie.go
- trie/zk_merkle_trie.go
jyc228
force-pushed
the
fix/zkstatetrie-invalid-proof
branch
2 times, most recently
from
49f752c to
940f093
Compare
trie/zk_merkle_trie.go
Outdated
| @@ -44,7 +104,8 @@ func (z *ZkMerkleTrie) Commit(_ bool) (common.Hash, *trienode.NodeSet, error) { | |||
| } | |||
| err := z.ComputeAllNodeHash(func(node zk.TreeNode) error { return z.db.Put(node.Hash()[:], node.CanonicalValue()) }) | |||
| if err != nil { | |||
| log.Error("Failed to commit zk merkle trie", "err", err) | |||
| z.logger.Error("failed to Commit", "error", err) | |||
| return common.Hash{}, nil, nil | |||
There was a problem hiding this comment.
| return common.Hash{}, nil, nil | |
| return common.Hash{}, nil, err |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- common/bytes.go (1 hunks)
- internal/ethapi/api.go (1 hunks)
- trie/iterator_test.go (1 hunks)
- trie/zk/merkle_tree_node.go (1 hunks)
- trie/zk/util.go (2 hunks)
- trie/zk_merkle_stack_trie.go (1 hunks)
- trie/zk_merkle_state_trie.go (3 hunks)
- trie/zk_merkle_trie.go (4 hunks)
Files skipped from review as they are similar to previous changes (8)
- common/bytes.go
- internal/ethapi/api.go
- trie/iterator_test.go
- trie/zk/merkle_tree_node.go
- trie/zk/util.go
- trie/zk_merkle_stack_trie.go
- trie/zk_merkle_state_trie.go
- trie/zk_merkle_trie.go
jyc228
force-pushed
the
fix/zkstatetrie-invalid-proof
branch
from
940f093 to
f648812
Compare
Description
Get proofapi is responding to the wrong proof because it did not reverse the key when zktrie probe.
Summary by CodeRabbit
New Features
Enhancements
Refactor