feat: bring ZkTrie in#1076
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 38
🧹 Outside diff range and nitpick comments (64)
trie/byte32.go (1)
37-42: LGTM: NewByte32FromBytesPaddingZero function is correctly implemented. Minor comment improvement suggested.The function correctly implements the described behavior.
Consider updating the comment to more explicitly state the difference in padding behavior compared to
NewByte32FromBytes:-// create bytes32 with zeropadding to shorter bytes, or truncate it +// create bytes32 with zero-padding at the end for shorter bytes, or truncate longer bytestrie/byte32_test.go (3)
10-30: LGTM: Well-structured table-driven tests.The test function is well-structured using a table-driven approach, which is efficient for testing multiple scenarios. The test cases cover different input sizes, which is good for edge case testing.
Consider adding a test case for an empty input ([]byte{}) to ensure the methods handle this edge case correctly.
32-43: LGTM: Comprehensive test execution and assertions.The test execution loop is well-structured, covering all necessary assertions to validate the functionality of both methods and their
Hashoperations.Consider adding descriptive messages to the assertions to improve test failure diagnostics. For example:
assert.Equal(t, tt.expected, byte32Result.Bytes(), "NewByte32FromBytes result mismatch")
1-44: Overall: Well-structured and comprehensive test file.This test file provides good coverage for the
Byte32functionality, including both byte array results and their hash representations. The use of table-driven tests with hard-coded expected values ensures robustness against implementation changes.To improve maintainability, consider extracting the test cases into a separate variable or even a separate function. This would make it easier to add or modify test cases in the future without cluttering the main test function. For example:
var byte32TestCases = []struct { input []byte expected []byte expectedPaddingZero []byte expectedHash string expectedHashPadding string }{ // ... (existing test cases) ... } func TestNewByte32(t *testing.T) { for _, tt := range byte32TestCases { // ... (existing test logic) ... } }trie/hash_test.go (5)
14-23: LGTM: TestCheckBigIntInField covers key edge cases.The test function effectively checks the
CheckBigIntInFieldfunction for important edge cases: zero, the largest valid value (Q-1), and the first invalid value (Q). The use ofassertfrom the testify package is appropriate for these unit tests.Consider adding a few more test cases:
- A small positive number (e.g., 1 or 10) to test a typical valid input.
- A negative number to ensure it's handled correctly (likely should return false).
- A very large number well beyond Q to test upper bound handling.
25-62: LGTM: Comprehensive testing of hash and big integer conversions.This test function covers a wide range of scenarios for hash and big integer conversions, including normal cases and error handling. The use of both
assertandrequireis appropriate for different test cases.To improve readability and maintainability, consider breaking this large test function into smaller, more focused test functions. For example:
TestNewHashFromBytesTestNewHashFromCheckedBytesTestNewBigIntFromHashBytesTestHashMarshalUnmarshalTestNewHashFromBytesErrorsTestNewBigIntFromHashBytesErrorsThis separation would make it easier to understand and maintain each specific test case.
64-75: LGTM: Good coverage of hash conversions from big integers and strings.The test function effectively checks the conversion of both small integers and large string representations to hash objects. The assertions for both hexadecimal and string representations are appropriate.
Consider adding the following test cases to improve coverage:
- Test with a negative big integer to ensure proper handling.
- Test with a string that's not a valid number to check error handling in
NewHashFromString.- Test with a big integer that's out of the valid field range to ensure proper error handling in
NewHashFromBigInt.
77-83: LGTM: Good test for NewHashFromBytes with random input.The test function effectively checks the
NewHashFromBytesfunction using random input, ensuring the correct number of bytes are read and that the resulting hash matches the input.Consider adding an error check for the
NewHashFromBytesfunction call:h2 := NewHashFromBytes(h.Bytes()) require.NotNil(t, h2, "NewHashFromBytes should not return nil") require.Equal(t, h, *h2)This addition would ensure that
NewHashFromBytesdoesn't return an unexpected nil value.
1-83: Overall: Excellent test coverage with minor improvement suggestions.This new test file provides comprehensive coverage for hash-related functions in the trie package. The tests are well-structured, follow Go conventions, and effectively use the testify package for assertions. They cover both normal cases and error scenarios, which is crucial for robust testing.
To further improve this already solid test file:
- Consider breaking down larger test functions (e.g.,
TestNewHashAndBigIntFromBytes) into smaller, more focused tests for better maintainability.- Add a few more edge cases to some test functions as suggested in previous comments.
- Ensure consistent error handling across all test functions.
These minor enhancements will make the test suite even more robust and easier to maintain in the future.
trie/util_test.go (7)
23-47: LGTM: TestBitManipulations is comprehensive.The test provides excellent coverage for both
TestBitandTestBitBigEndianfunctions, correctly handling the difference between standard and big-endian bit ordering.Consider using a table-driven test to reduce code duplication and make it easier to add more test cases in the future. For example:
func TestBitManipulations(t *testing.T) { bitmap := []byte{0b10101010, 0b01010101} testCases := []struct { name string testFunc func([]byte, uint) bool expected []bool }{ { name: "TestBit", testFunc: TestBit, expected: []bool{false, true, false, true, false, true, false, true, true, false, true, false, true, false, true, false}, }, { name: "TestBitBigEndian", testFunc: TestBitBigEndian, expected: []bool{true, false, true, false, true, false, true, false, false, true, false, true, false, true, false, true}, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { results := make([]bool, 16) for i := uint(0); i < 16; i++ { results[i] = tc.testFunc(bitmap, i) } assert.Equal(t, tc.expected, results) }) } }This approach makes the test more maintainable and easier to extend.
49-54: LGTM: TestBigEndianBitsToBigInt is correct.The test effectively verifies the conversion from big-endian bit representation to integer.
Consider adding an additional test case with a different bit pattern to increase coverage. For example:
func TestBigEndianBitsToBigInt(t *testing.T) { testCases := []struct { name string bits []bool expected *big.Int }{ { name: "Alternating bits", bits: []bool{true, false, true, false, true, false, true, false}, expected: big.NewInt(170), }, { name: "Random pattern", bits: []bool{true, true, false, true, false, false, true, true}, expected: big.NewInt(211), }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { result := BigEndianBitsToBigInt(tc.bits) assert.Equal(t, tc.expected, result) }) } }This approach provides better coverage and makes it easier to add more test cases in the future.
56-60: LGTM: TestToSecureKey verifies basic functionality.The test correctly checks for the absence of errors and the expected output for a given input.
Consider adding the following improvements:
- Test with multiple input values to ensure consistent behavior.
- Include a test case with an empty input to verify edge case handling.
- Add a negative test case to check error handling for invalid inputs.
Example:
func TestToSecureKey(t *testing.T) { testCases := []struct { name string input []byte expected string expectError bool }{ { name: "Valid input", input: []byte("testKey"), expected: "10380846131134096261855654117842104248915214759620570252072028416245925344412", }, { name: "Empty input", input: []byte{}, expected: "0", // Adjust this based on the expected behavior for empty input }, { name: "Invalid input", input: []byte{0xFF, 0xFF, 0xFF}, // Adjust based on what constitutes an invalid input expectError: true, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { secureKey, err := ToSecureKey(tc.input) if tc.expectError { assert.Error(t, err) } else { assert.NoError(t, err) assert.Equal(t, tc.expected, secureKey.String()) } }) } }These additional test cases will provide more comprehensive coverage of the
ToSecureKeyfunction.
62-66: LGTM: TestToSecureKeyBytes verifies basic functionality.The test correctly checks for the absence of errors and the expected output for a given input.
Consider applying similar improvements as suggested for
TestToSecureKey:
- Test with multiple input values.
- Include a test case with an empty input.
- Add a negative test case for invalid inputs.
Example:
func TestToSecureKeyBytes(t *testing.T) { testCases := []struct { name string input []byte expected []byte expectError bool }{ { name: "Valid input", input: []byte("testKey"), expected: []byte{0x16, 0xf3, 0x59, 0xc7, 0x30, 0x7e, 0x8, 0x97, 0xdc, 0x7c, 0x6b, 0x99, 0x53, 0xd1, 0xe1, 0xd8, 0x3, 0x6d, 0xc3, 0x83, 0xd4, 0xa, 0x0, 0x19, 0x9e, 0xda, 0xf0, 0x65, 0x27, 0xda, 0xf4, 0x9c}, }, { name: "Empty input", input: []byte{}, expected: make([]byte, 32), // Adjust based on expected behavior for empty input }, { name: "Invalid input", input: []byte{0xFF, 0xFF, 0xFF}, // Adjust based on what constitutes an invalid input expectError: true, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { secureKeyBytes, err := ToSecureKeyBytes(tc.input) if tc.expectError { assert.Error(t, err) } else { assert.NoError(t, err) assert.Equal(t, tc.expected, secureKeyBytes.Bytes()) } }) } }These additional test cases will provide more comprehensive coverage of the
ToSecureKeyBytesfunction.
68-79: LGTM: TestHashElems provides a good basic check.The test effectively verifies the
HashElemsfunction with a mix of fixed and generated input values.Consider the following improvements to enhance test coverage:
- Add test cases with different numbers of elements (e.g., 0, 1, 31, 33) to verify edge cases.
- Include a test case with very large
big.Intvalues to ensure proper handling.- Add a negative test case to check error handling for invalid inputs.
Example:
func TestHashElems(t *testing.T) { testCases := []struct { name string fst *big.Int snd *big.Int elems []*big.Int expected string expectError bool }{ { name: "32 elements", fst: big.NewInt(5), snd: big.NewInt(3), elems: generateBigIntSlice(32, 1), expected: "0746c424799ef6ad7916511016a5b8e30688fa6d62664eeb97d9f2ba07685ed8", }, { name: "No elements", fst: big.NewInt(5), snd: big.NewInt(3), elems: []*big.Int{}, expected: "expectedHashForNoElements", // Replace with actual expected hash }, { name: "Large values", fst: new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil), snd: new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil), elems: []*big.Int{new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil)}, expected: "expectedHashForLargeValues", // Replace with actual expected hash }, { name: "Invalid input", fst: nil, snd: big.NewInt(3), elems: []*big.Int{big.NewInt(1)}, expectError: true, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { result, err := HashElems(tc.fst, tc.snd, tc.elems...) if tc.expectError { assert.Error(t, err) } else { assert.NoError(t, err) assert.Equal(t, tc.expected, result.Hex()) } }) } } func generateBigIntSlice(length int, start int64) []*big.Int { slice := make([]*big.Int, length) for i := range slice { slice[i] = big.NewInt(start + int64(i)) } return slice }These additional test cases will provide more comprehensive coverage of the
HashElemsfunction, including edge cases and error handling.
81-96: LGTM: TestPreHandlingElems provides basic coverage.The test effectively checks the
HandlingElemsAndByte32function with both full and partial input arrays.Consider the following improvements to enhance test coverage and clarity:
- Use a table-driven test to make it easier to add more test cases.
- Add test cases with different flag arrays to verify the function's behavior under various conditions.
- Include a test case with an empty input array to check edge case handling.
- Add a negative test case to verify error handling for invalid inputs.
- Use more descriptive names for test cases to improve readability.
Example:
func TestHandlingElemsAndByte32(t *testing.T) { testCases := []struct { name string flagArray uint32 elems []Byte32 expected string expectError bool }{ { name: "Full array with alternating flags", flagArray: 0b10101010101010101010101010101010, elems: generateByte32Array(32), expected: "0c36a6406e35e1fec2a5602fcd80ab04ec24d727676a953673c0850d205f9378", }, { name: "Single element with all flags set", flagArray: 0xFFFFFFFF, elems: generateByte32Array(1), expected: "0000000000000000000000000000000000000000000000000000007465737431", }, { name: "Empty array with flags set", flagArray: 0xFFFFFFFF, elems: []Byte32{}, expected: "expectedHashForEmptyArray", // Replace with actual expected hash }, { name: "Invalid input: mismatched flag and elem count", flagArray: 0b1111, elems: generateByte32Array(32), expectError: true, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { result, err := HandlingElemsAndByte32(tc.flagArray, tc.elems) if tc.expectError { assert.Error(t, err) } else { assert.NoError(t, err) assert.Equal(t, tc.expected, result.Hex()) } }) } } func generateByte32Array(length int) []Byte32 { elems := make([]Byte32, length) for i := range elems { elems[i] = *NewByte32FromBytes([]byte("test" + strconv.Itoa(i+1))) } return elems }These improvements will provide more comprehensive coverage of the
HandlingElemsAndByte32function, including edge cases and error handling, while also improving the test's readability and maintainability.
1-96: Overall, trie/util_test.go provides good test coverage with room for improvement.The test file contains well-structured tests for various utility functions in the trie package. Each test function provides good basic coverage and uses appropriate assertions. The use of predetermined expected values ensures deterministic behavior of the utility functions.
To further enhance the quality and coverage of the tests, consider implementing the following improvements across all test functions:
- Use table-driven tests to improve readability and make it easier to add more test cases.
- Add edge cases (e.g., empty inputs, maximum values) to ensure robust function behavior.
- Include negative test cases to verify error handling for invalid inputs.
- Use more descriptive names for test cases to improve readability.
- Consider testing with a wider range of input values to ensure consistent behavior across different scenarios.
Implementing these suggestions will result in more comprehensive test coverage, improved maintainability, and increased confidence in the correctness of the trie package's utility functions.
core/vm/gas_table_test.go (2)
140-172: Test logic adapted to new structure.The core test logic has been successfully adapted to the new sub-test structure. The conditional setting of
configbased ontt.eip3860is a good approach for testing both scenarios.For improved readability, consider extracting the
configinitialization into a separate function:func getConfig(eip3860 bool) Config { config := Config{} if eip3860 { config.ExtraEips = []int{3860} } return config }Then use it as:
config := getConfig(tt.eip3860)
178-182: Improved gas usage check for successful deployments.The addition of a condition to check gas usage only for successful deployments is a good improvement. It prevents unnecessary checks and aligns the test behavior more closely with real-world scenarios.
For added clarity, consider adding a comment explaining the significance of the 100_000 value:
// Only check gas usage if deployment succeeded (minGas < 100_000) // 100_000 is the maximum gas allowed for these test cases if minGas < 100_000 { if gasUsed != tt.gasUsed { t.Errorf("test %d: gas used mismatch: have %v, want %v", i, gasUsed, tt.gasUsed) } }core/chain_makers_test.go (1)
Line range hint
201-279: Consider updating ExampleGenerateChain to reflect current consensus mechanismWhile
ExampleGenerateChainremains unchanged, it's worth noting that it still uses the legacy Proof of Work (PoW) consensus mechanism (ethash.NewFaker()). If the project is moving away from PoW, consider updating this example to reflect the current consensus mechanism used in the project.If applicable, update the example to use the current consensus mechanism or add a comment explaining why it still uses PoW for demonstration purposes.
🧰 Tools
🪛 Gitleaks
38-38: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
core/state_processor.go (1)
Line range hint
147-151: Improved error handling for EVM tracers.The changes enhance error reporting by including errors from all tracers, not just those in debug mode. This is a good improvement that aligns with best practices for comprehensive error handling.
Consider adding a brief comment explaining the purpose of this error handling block, e.g.:
// Incorporate any errors from the EVM tracer into the result if evm.Config.Tracer != nil { // ... (existing code) }This would help future maintainers understand the intent behind this error handling logic.
core/state/state_test.go (1)
Line range hint
1-324: Summary: Address test coverage gap introduced by ZkTrie integrationThe changes in this file involve skipping two tests (
TestDumpandTestIterativeDump) due to ZkTrie not supporting iterators. While this addresses the immediate compatibility issue, it introduces a gap in test coverage for ZkTrie's Dump and IterativeDump functionalities.To maintain the integrity of the test suite and ensure proper functionality of ZkTrie, consider the following actions:
- Create technical debt tickets to track these skipped tests.
- Develop alternative testing strategies for ZkTrie that work within its current limitations.
- Update the existing tests or create new ones specifically for ZkTrie to ensure its Dump and IterativeDump functionalities are adequately tested.
- Consolidate the skip logic to improve maintainability.
By addressing these points, you can ensure that the integration of ZkTrie doesn't compromise the overall test coverage and quality of the codebase.
core/blockchain_snapshot_test.go (1)
Line range hint
3-1010: Comprehensive snapshot test suite implementedThe test suite covers a wide range of scenarios for blockchain snapshot functionality, including:
- Normal restarts with valid snapshots
- Crashes with various commit points
- Gapped snapshots
- SetHead operations
- Wiping crashes
Each test case is well-documented with clear expectations and covers both the HashScheme and PathScheme.
The test suite appears to be thorough and well-implemented. It should provide good coverage for the snapshot functionality.
Some suggestions for potential improvements:
- Consider adding more edge cases, such as extremely long chains or chains with complex state.
- It might be beneficial to add some performance benchmarks for snapshot operations.
- Consider adding tests for concurrent operations on snapshots, if applicable.
core/vm/runtime/runtime_test.go (1)
Line range hint
273-702: Evaluate the overall impact of Scroll-specific modifications.The changes in this file consistently relate to Scroll's lack of support for SELFDESTRUCT and its different implementation of blockhash. While these modifications appear intentional, they represent significant deviations from standard Ethereum behavior.
Consider documenting these Scroll-specific differences prominently in the project documentation. It would be beneficial to provide a comprehensive list of all deviations from standard Ethereum behavior, their rationales, and any potential implications for developers and users migrating from Ethereum to Scroll.
trie/util.go (7)
9-11: Correct typos in function documentation commentsThe documentation for
HashElemsWithDomaincontains typographical errors. The word "fieds" should be "fields," and "recursiving" should be "recursive."Apply this diff to fix the typos:
-// reduce 2 fieds into one, with a specified domain field which would be used in -// every recursiving call +// reduces 2 fields into one, with a specified domain field which is used in +// every recursive call
41-46: Add documentation comment forHashElemsfunctionThe
HashElemsfunction is exported but lacks a documentation comment. Including a comment enhances code readability and adheres to Go documentation standards.Apply this diff to add the documentation comment:
+// HashElems calls HashElemsWithDomain with a calculated domain based on the number of elements. func HashElems(fst, snd *big.Int, elems ...*big.Int) (*Hash, error) {
74-77: Clarify byte order calculation inSetBitBigEndianConsider adding an explanatory comment or adjusting the variable names to make the calculation
uint(len(bitmap))-n/8-1clearer. This will help future maintainers understand the big-endian bit-setting logic.
79-82: Add documentation comment forTestBitfunctionThe
TestBitfunction is exported but lacks a documentation comment. Adding a comment improves readability and follows Go best practices.Apply this diff:
+// TestBit checks whether the bit at position n in the bitmap is set to 1. func TestBit(bitmap []byte, n uint) bool {
84-87: Add documentation comment forTestBitBigEndianfunctionSimilarly,
TestBitBigEndianlacks a documentation comment. Including one enhances readability and maintains consistency.Apply this diff:
+// TestBitBigEndian checks whether the bit at position n in the bitmap is set to 1, using big-endian order. func TestBitBigEndian(bitmap []byte, n uint) bool {
103-107: Add documentation comment forToSecureKeyfunctionThe
ToSecureKeyfunction is exported but lacks a documentation comment. Documenting it enhances clarity and adheres to Go standards.Apply this diff:
+// ToSecureKey converts a byte slice key into its "secured" big.Int representation. func ToSecureKey(key []byte) (*big.Int, error) {
109-117: Add documentation comment forToSecureKeyBytesfunctionIncluding a documentation comment for
ToSecureKeyBytesimproves readability and follows best practices.Apply this diff:
+// ToSecureKeyBytes converts a byte slice key into a 32-byte "secured" key represented as a big-endian integer. func ToSecureKeyBytes(key []byte) (*Byte32, error) {rollup/tracing/proof_test.go (2)
70-71: Improve comment clarity and professionalismThe comment uses informal language ("a hacking"), which may not be appropriate. Consider rephrasing it for clarity and professionalism.
Suggested rephrased comment:
-// a hacking to grep the value part directly from the encoded leaf node, -// notice the sibling of key `k*32`` is just the leaf of key `m*32` +// Extract the value part directly from the encoded leaf node. +// Note that the sibling of key `k*32` is just the leaf of key `m*32`.
72-72: Avoid magic numbers in slice indexingThe slice indices
l-33:l-1use magic numbers, which can reduce code readability and maintainability. Define constants or add comments to clarify the meaning of these values.For example:
const valueSize = 32 // Size of the value in bytes startIndex := l - valueSize - 1 endIndex := l - 1 assert.Equal(t, siblings[0][startIndex:endIndex], nd)trie/hash.go (2)
104-104: Correct typos in commentsThere are typographical errors in the comments:
- Line 104: "has ben generated" should be "has been generated".
- Line 133: "has ben generated" should be "has been generated".
Also applies to: 133-133
91-91: Fix spacing in commentsIn the comments for the
SetandClonemethods, "in to" should be "into":
- Line 91: "Set copies the given hash into this".
- Line 95: "Clone copies the given hash into this".
Also applies to: 95-95
rollup/tracing/proof.go (3)
29-30: Enhance the function comment for clarityThe function comment contains grammatical errors and can be improved for better readability.
Apply this diff to improve the function comment:
-// Merge merge the input tracer into current and return current tracer +// Merge merges the input tracer into the current one and returns the current tracer
Line range hint
146-148: Avoid usingpanicin library code; return an error insteadPanicking on unexpected node types can be replaced with returning an error to handle it gracefully.
Modify the code to return an error:
default: - panic(fmt.Errorf("unexpected node type %d", n.Type)) + return fmt.Errorf("unexpected node type %d", n.Type)Ensure that the anonymous function signature allows for error return.
Line range hint
126-155: UpdateProvemethod to handle errors from callbacksSince the callback functions may now return errors, the
Provemethod should handle these errors appropriately.Modify the code to capture and return the error:
-func (t *ProofTracer) Prove(key []byte, proofDb ethdb.KeyValueWriter) error { +func (t *ProofTracer) Prove(key []byte, proofDb ethdb.KeyValueWriter) error { fromLevel := uint(0) var mptPath []*trie.Node -return t.trie.ProveWithDeletion(key, fromLevel, +err := t.trie.ProveWithDeletion(key, fromLevel, func(n *trie.Node) error { // Callback logic... }, func(n *trie.Node, _ *trie.Node) { // Callback logic... }, ) +if err != nil { + return err +} +return nil }trie/secure_trie.go (3)
64-72: Consider exportingnewStateTrieif intended for public useThe function
newStateTrieis unexported due to the lowercase initial letter. If this constructor is meant to replaceNewSecurefor external packages, consider renaming it toNewStateTrieto make it accessible outside the package.
Line range hint
166-173: Avoid shadowing theerrvariable inUpdateAccountIn the
ifstatement,erris redeclared, which shadows the outererr. This can lead to confusion and potential bugs.Consider renaming the inner
errvariable:func (t *stateTrie) UpdateAccount(address common.Address, acc *types.StateAccount) error { hk := t.hashKey(address.Bytes()) data, err := rlp.EncodeToBytes(acc) if err != nil { return err } - if err := t.trie.Update(hk, data); err != nil { - return err + if updateErr := t.trie.Update(hk, data); updateErr != nil { + return updateErr } t.getSecKeyCache()[string(hk)] = address.Bytes() return nil }
Line range hint
226-236: Ensure safe conversion betweenstringand[]byteinCommitConverting
hkfromstringback to[]byteusing[]byte(hk)may lead to issues if the original[]bytecontains non-UTF8 data. Since keys are hashed and may contain arbitrary bytes, this conversion can corrupt data.Consider storing
hkas[]bytein thesecKeyCacheto avoid conversions:-t.secKeyCache map[string][]byte +secKeyCache map[[common.HashLength]byte][]byte func (t *stateTrie) getSecKeyCache() map[[common.HashLength]byte][]byte { ... - t.secKeyCache = make(map[string][]byte) + t.secKeyCache = make(map[[common.HashLength]byte][]byte) } func (t *stateTrie) MustUpdate(key, value []byte) { hk := t.hashKey(key) t.trie.MustUpdate(hk, value) - t.getSecKeyCache()[string(hk)] = common.CopyBytes(key) + t.getSecKeyCache()[hk] = common.CopyBytes(key) } // Similar changes for other methods using `secKeyCache`This change avoids the need to cast between
stringand[]byte, ensuring data integrity.trie/zk_trie_node.go (1)
42-42: Fix typo in function comment: "DeduceUploadType" should be "DeduceUpgradeType"The function comment for
DeduceUpgradeTypeincorrectly spells "DeduceUploadType" instead of "DeduceUpgradeType". Correcting this enhances code readability and maintains consistency.rollup/tracing/tracing.go (1)
488-491: Consider consolidating deletion marking to avoid redundancyThe code marks deletions on both
zktrieTracerandenv.ZkTrieTracer[addrStr], then mergeszktrieTracerintoenv.ZkTrieTracer[addrStr]. To avoid redundancy, consider marking deletions directly onenv.ZkTrieTracer[addrStr], or ensure that both steps are necessary.trie/stacktrie_test.go (2)
429-431: Ensure the 'cmp' method handles nil slices appropriatelyWhile
bytes.Comparehandlesnilslices correctly, consider adding a comment or check to clarify thatk.kandother.kare expected to be non-nilto prevent unexpected behavior.Optionally, add a comment:
func (k *kv) cmp(other *kv) int { + // Assume k.k and other.k are non-nil return bytes.Compare(k.k, other.k) }
Line range hint
433-473: Set a fixed seed for the random number generator to ensure test reproducibilityIn
TestPartialStackTrie, usingrand.Intnwithout setting a seed can lead to non-deterministic test results. Setting a fixed seed ensures that tests are reproducible and failures can be consistently investigated.Apply the following diff to set a fixed seed:
func TestPartialStackTrie(t *testing.T) { + rand.Seed(1) // Set a fixed seed for reproducibility for round := 0; round < 100; round++ { var (Alternatively, if you prefer true randomness but want to reproduce failures, you can capture and log the seed used in each test run.
trie/sync_test.go (1)
145-145: Remove unnecessary commented-out codeThe line
// emptyD, _ := New(TrieID(types.EmptyRootHash), dbD)is commented out. If this code is no longer needed, consider removing it to enhance code readability and maintainability.core/txpool/blobpool/blobpool_test.go (6)
Line range hint
262-262: Typo in comment: "veirfy" should be "verify"There's a typo in the comment on line 262. The word "veirfy" should be corrected to "verify".
Apply this diff to fix the typo:
-// Insert a sequence of transactions with already passed nonces to veirfy that the entire set will get dropped. +// Insert a sequence of transactions with already passed nonces to verify that the entire set will get dropped.
Line range hint
307-307: Typo in comment: "veirfy" should be "verify"There's a typo in the comment on line 307. The word "veirfy" should be corrected to "verify".
Apply this diff to fix the typo:
-// Insert a sequence of transactions fully overdrafted to veirfy that the entire set will get invalidated. +// Insert a sequence of transactions fully overdrafted to verify that the entire set will get invalidated.
Line range hint
391-391: Missing error handling forstatedb.CommitThe call to
statedb.Commit(0, true)does not handle the returned error. It's important to check and handle errors to ensure that state commits are successful and to prevent potential issues during testing.Apply this diff to handle the error:
-statedb.Commit(0, true) +if _, err := statedb.Commit(0, true); err != nil { + t.Fatalf("failed to commit state: %v", err) +}
Line range hint
483-483: Missing error handling forstatedb.CommitThe call to
statedb.Commit(0, true)does not handle the returned error. It's important to check and handle errors to ensure that state commits are successful and to prevent potential issues during testing.Apply this diff to handle the error:
-statedb.Commit(0, true) +if _, err := statedb.Commit(0, true); err != nil { + t.Fatalf("failed to commit state: %v", err) +}
Line range hint
945-945: Typo in comment: "tumpe" should be "tuple"There's a typo in the comment on line 945. The word "tumpe" should be corrected to "tuple".
Apply this diff to fix the typo:
-// seed is a helper tumpe to seed an initial state db and pool +// seed is a helper tuple to seed an initial state db and pool
Line range hint
220-452: Consider refactoringTestOpenDropsinto smaller functionsThe
TestOpenDropsfunction spans over 230 lines, making it difficult to read and maintain. Breaking it down into smaller, focused helper functions can improve readability and maintainability.trie/zk_trie.go (2)
508-530: Simplify error handling inTryGetIn the
TryGetmethod, when the key is not found, it returnsnil:if err == ErrKeyNotFound { // according to https://github.com/ethereum/go-ethereum/blob/... return nil, nil } else if err != nil { return nil, err }Consider simplifying the error handling by returning early when the key is not found:
if err == ErrKeyNotFound { return nil, nil } if err != nil { return nil, err }This improves readability by reducing the need for
elseafter a return.
1318-1319: Implement or properly handleGetAccountByHashThe
GetAccountByHashmethod is unimplemented:func (mt *ZkTrie) GetAccountByHash(addrHash common.Hash) (*types.StateAccount, error) { return nil, errors.New("not implemented") }If this method is intended for future implementation, consider returning
ErrNotImplementedor documenting its unavailability to inform users appropriately.core/state/snapshot/generate_test.go (8)
Line range hint
65-67: Handle errors returned bymakeStorageTrie.In the
testGenerationfunction, the error returned bymakeStorageTrieis ignored. Ignoring errors can lead to unexpected behavior or panics if the function fails.Modify the code to handle the error:
- stRoot := helper.makeStorageTrie(common.Hash{}, []string{"key-1", "key-2", "key-3"}, []string{"val-1", "val-2", "val-3"}, false) + stRoot, err := helper.makeStorageTrie(common.Hash{}, []string{"key-1", "key-2", "key-3"}, []string{"val-1", "val-2", "val-3"}, false) + if err != nil { + t.Fatalf("Failed to create storage trie: %v", err) + }
Line range hint
101-103: Check for errors when creating a new state trie.In the
newHelperfunction, the error returned bytrie.NewStateTrieis ignored. Proper error handling ensures that any issues during trie initialization are caught early.Update the code to handle the error:
- accTrie, _ := trie.NewStateTrie(trie.StateTrieID(types.EmptyRootHash), triedb) + accTrie, err := trie.NewStateTrie(trie.StateTrieID(types.EmptyRootHash), triedb) + if err != nil { + log.Fatalf("Failed to create new state trie: %v", err) + }
Line range hint
153-155: Handle errors fromCommitoperations.In the
Commitmethod, the error returned byt.accTrie.Commit(true)is ignored. This could suppress critical errors that need attention.Modify the code to handle the error:
- root, nodes, _ := t.accTrie.Commit(true) + root, nodes, err := t.accTrie.Commit(true) + if err != nil { + log.Fatalf("Failed to commit account trie: %v", err) + }
Line range hint
72-74: Avoid hardcoding expected hash values in tests.Comparing the computed root hash to a hardcoded value can cause tests to fail if the underlying data or hashing algorithm changes.
Consider computing the expected root dynamically or removing the hardcoded hash comparison to improve test resilience.
Line range hint
220-225: Uset.Errorinstead oft.Fatalwithin helper functions.In the
checkSnapRootfunction, usingt.Fatalmay not behave as expected because it's called from within a helper function, not the main test function.Replace
t.Fatalwitht.Errorto log the error without stopping the test abruptly. Ensure the error is properly handled in the test flow.- t.Fatal(err) + t.Error(err)
Line range hint
421-423: Increase test timeout durations for reliability.Hardcoded timeouts like
time.After(3 * time.Second)may cause tests to fail on slower systems or under heavy load.Consider increasing the timeout durations or making them configurable to enhance test reliability across different environments.
Line range hint
375-378: Avoid modifying global variables concurrently in tests.Modifying
accountCheckRangein concurrent tests can lead to race conditions and unpredictable behavior.Use local variables within the test functions or synchronize access to shared variables to prevent data races.
Line range hint
1070-1075: Handle errors when creating new state tries in helper functions.In several helper functions, errors returned by
trie.NewStateTrieare ignored. Proper error handling is crucial for debugging and maintaining code quality.Update the helper functions to handle errors appropriately.
core/state/statedb.go (3)
Line range hint
750-755: Inconsistent method naming: 'Selfdestruct6780' should be 'SelfDestruct6780'The method
Selfdestruct6780does not follow Go's naming conventions for exported methods. For consistency and readability, please rename it toSelfDestruct6780.Apply this diff to correct the method name:
-func (s *StateDB) Selfdestruct6780(addr common.Address) { +func (s *StateDB) SelfDestruct6780(addr common.Address) {
Line range hint
1967-2139: Consider refactoring the 'Commit' method to improve maintainabilityThe
Commitmethod spans over 170 lines, making it lengthy and potentially difficult to maintain. Refactoring it into smaller, modular functions would enhance readability and maintainability. Breaking down complex methods helps isolate functionality and makes the codebase easier to understand and test.
Line range hint
1914-1917: Typographical error in commentThe comment contains a grammatical error:
-// The fast approach can be failed if the snapshot is not fully +// The fast approach can fail if the snapshot is not fully
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (82)
- cmd/evm/runner.go (0 hunks)
- cmd/utils/flags.go (1 hunks)
- core/blockchain.go (3 hunks)
- core/blockchain_repair_test.go (2 hunks)
- core/blockchain_sethead_test.go (1 hunks)
- core/blockchain_snapshot_test.go (1 hunks)
- core/blockchain_test.go (44 hunks)
- core/chain_makers.go (2 hunks)
- core/chain_makers_test.go (1 hunks)
- core/forkid/forkid_test.go (1 hunks)
- core/genesis.go (1 hunks)
- core/genesis_test.go (2 hunks)
- core/state/database.go (0 hunks)
- core/state/iterator_test.go (1 hunks)
- core/state/snapshot/difflayer_test.go (1 hunks)
- core/state/snapshot/disklayer_test.go (1 hunks)
- core/state/snapshot/generate_test.go (1 hunks)
- core/state/snapshot/holdable_iterator_test.go (1 hunks)
- core/state/snapshot/iterator_test.go (1 hunks)
- core/state/snapshot/snapshot_test.go (1 hunks)
- core/state/state_object.go (4 hunks)
- core/state/state_prove.go (0 hunks)
- core/state/state_test.go (2 hunks)
- core/state/statedb.go (1 hunks)
- core/state/statedb_fuzz_test.go (1 hunks)
- core/state/statedb_test.go (5 hunks)
- core/state/sync_test.go (1 hunks)
- core/state_processor.go (1 hunks)
- core/state_processor_test.go (2 hunks)
- core/txpool/blobpool/blobpool_test.go (2 hunks)
- core/txpool/legacypool/legacypool_test.go (1 hunks)
- core/types/hashes.go (2 hunks)
- core/types/hashing_test.go (1 hunks)
- core/types/state_account_marshalling.go (1 hunks)
- core/types/state_account_marshalling_test.go (4 hunks)
- core/vm/gas_table_test.go (1 hunks)
- core/vm/logger.go (0 hunks)
- core/vm/runtime/runtime_test.go (3 hunks)
- eth/filters/filter_system_test.go (1 hunks)
- eth/filters/filter_test.go (1 hunks)
- eth/tracers/js/goja.go (0 hunks)
- eth/tracers/logger/access_list_tracer.go (0 hunks)
- eth/tracers/logger/logger.go (0 hunks)
- eth/tracers/logger/logger_json.go (0 hunks)
- eth/tracers/native/4byte.go (0 hunks)
- eth/tracers/native/call.go (0 hunks)
- eth/tracers/native/call_flat.go (0 hunks)
- eth/tracers/native/mux.go (0 hunks)
- eth/tracers/native/noop.go (0 hunks)
- go.mod (0 hunks)
- rollup/ccc/async_checker_test.go (1 hunks)
- rollup/ccc/logger.go (0 hunks)
- rollup/tracing/proof.go (5 hunks)
- rollup/tracing/proof_test.go (1 hunks)
- rollup/tracing/tracing.go (6 hunks)
- trie/byte32.go (1 hunks)
- trie/byte32_test.go (1 hunks)
- trie/database.go (1 hunks)
- trie/database_supplement.go (0 hunks)
- trie/hash.go (1 hunks)
- trie/hash_test.go (1 hunks)
- trie/iterator_test.go (9 hunks)
- trie/node_test.go (1 hunks)
- trie/proof.go (1 hunks)
- trie/proof_test.go (1 hunks)
- trie/secure_trie.go (13 hunks)
- trie/secure_trie_test.go (3 hunks)
- trie/stacktrie.go (1 hunks)
- trie/stacktrie_test.go (1 hunks)
- trie/sync_test.go (6 hunks)
- trie/tracer_test.go (5 hunks)
- trie/trie_test.go (14 hunks)
- trie/triedb/hashdb/database.go (2 hunks)
- trie/triedb/hashdb/database_supplement.go (0 hunks)
- trie/util.go (1 hunks)
- trie/util_test.go (1 hunks)
- trie/zk_trie.go (3 hunks)
- trie/zk_trie_database.go (0 hunks)
- trie/zk_trie_database_test.go (0 hunks)
- trie/zk_trie_impl_test.go (0 hunks)
- trie/zk_trie_node.go (1 hunks)
- trie/zk_trie_node_test.go (1 hunks)
⛔ Files not processed due to max files limit (2)
- trie/zk_trie_proof_test.go
- trie/zk_trie_test.go
💤 Files with no reviewable changes (20)
- cmd/evm/runner.go
- core/state/database.go
- core/state/state_prove.go
- core/vm/logger.go
- eth/tracers/js/goja.go
- eth/tracers/logger/access_list_tracer.go
- eth/tracers/logger/logger.go
- eth/tracers/logger/logger_json.go
- eth/tracers/native/4byte.go
- eth/tracers/native/call.go
- eth/tracers/native/call_flat.go
- eth/tracers/native/mux.go
- eth/tracers/native/noop.go
- go.mod
- rollup/ccc/logger.go
- trie/database_supplement.go
- trie/triedb/hashdb/database_supplement.go
- trie/zk_trie_database.go
- trie/zk_trie_database_test.go
- trie/zk_trie_impl_test.go
✅ Files skipped from review due to trivial changes (7)
- core/blockchain_test.go
- core/chain_makers.go
- core/state/iterator_test.go
- core/state/snapshot/iterator_test.go
- core/state/statedb_fuzz_test.go
- trie/node_test.go
- trie/proof_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/types/hashes.go
- trie/iterator_test.go
- trie/secure_trie_test.go
- trie/stacktrie.go
- trie/tracer_test.go
- trie/trie_test.go
- trie/triedb/hashdb/database.go
🧰 Additional context used
🔇 Additional comments (103)
trie/byte32.go (5)
1-7: LGTM: Package declaration and imports are appropriate.The package declaration and imports are correct and align with the functionality provided in this file.
9-9: LGTM: Byte32 type definition is clear and efficient.The
Byte32type is well-defined as a fixed-size 32-byte array, which is an efficient representation for the intended purpose.
21-35: LGTM: Bytes method and NewByte32FromBytes function are well-implemented.The
Bytesmethod efficiently returns the underlying byte array. TheNewByte32FromBytesfunction correctly implements the described behavior, handling both longer and shorter input slices appropriately.
1-42: Overall, the implementation in byte32.go is well-structured and efficient.This new file introduces a solid foundation for handling 32-byte arrays in the trie package. The
Byte32type and its associated methods provide necessary functionality for hashing and byte manipulation. The implementations are correct, with appropriate error handling and clear comments. Minor suggestions have been made for improvement, and one verification task has been requested regarding a constant definition.
11-19: LGTM: Hash method implementation looks correct. Verify constant definition.The
Hashmethod correctly implements a custom hashing scheme using the Poseidon algorithm, with appropriate error handling.Please verify the definition and value of the
HASH_DOMAIN_BYTE32constant, as it's not defined in this file.✅ Verification successful
Further Verification Needed: Check definition of
HASH_DOMAIN_ELEMS_BASE.Please verify the definition and value of the
HASH_DOMAIN_ELEMS_BASEconstant to ensure it's correctly implemented.
Verified:
HASH_DOMAIN_BYTE32andHASH_DOMAIN_ELEMS_BASEare correctly defined.The
Hashmethod correctly utilizes the defined constants, ensuring proper implementation of the hashing scheme.
HASH_DOMAIN_BYTE32is set to512(2 * 256).HASH_DOMAIN_ELEMS_BASEis correctly defined as256intrie/hash.go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the definition of HASH_DOMAIN_BYTE32 rg -n "HASH_DOMAIN_BYTE32\s*=\s*" --type goLength of output: 114
Script:
# #!/bin/bash # Search for the definition of HASH_DOMAIN_ELEMS_BASE rg -n "HASH_DOMAIN_ELEMS_BASE\s*=\s*" --type goLength of output: 95
trie/byte32_test.go (1)
1-8: LGTM: Appropriate package and imports.The package declaration and imports are correct and suitable for this test file. The use of
testify/assertis a good practice for writing clear and concise tests.rollup/ccc/async_checker_test.go (1)
33-33: LGTM! Verify consistency across the codebase.The change from
trie.HashDefaultsWithZktrietotrie.HashDefaultsaligns with the broader refactoring effort to standardize trie handling across the project. This modification ensures that the test suite accurately reflects the updates made in the main codebase.To ensure consistency, let's verify that similar changes have been made across the codebase:
This will help confirm that the refactoring has been applied consistently throughout the project.
✅ Verification successful
Refactoring Verified Successfully.
All instances of
HashDefaultsWithZktriehave been removed and replaced withHashDefaultsconsistently across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of HashDefaultsWithZktrie and compare with HashDefaults usage # Search for any remaining instances of HashDefaultsWithZktrie echo "Searching for remaining instances of HashDefaultsWithZktrie:" rg "HashDefaultsWithZktrie" echo "---" # Search for instances of HashDefaults echo "Searching for instances of HashDefaults:" rg "HashDefaults"Length of output: 4466
trie/hash_test.go (1)
3-12: LGTM: Import statements are well-organized and appropriate.The import statements are correctly structured, with standard library imports followed by third-party packages. All imported packages are utilized in the test functions.
core/types/state_account_marshalling.go (1)
45-46: LGTM. Consider verifying the impact of type change.The change from
[]zkt.Byte32to[][32]byteis a good move towards using standard Go types. This should improve compatibility and reduce dependencies.To ensure this change doesn't introduce any issues, please run the following script to check for any remaining uses of
zkt.Byte32:trie/util_test.go (1)
11-21: LGTM: TestSetBitBigEndian is well-implemented.The test covers various bit positions across the entire byte array, providing good coverage for the
SetBitBigEndianfunction. The expected result correctly represents the set bits in big-endian format.core/state/snapshot/holdable_iterator_test.go (2)
1-2: LGTM: Build constraint additionThe addition of the build constraint
//go:build all_testsalong with its legacy format is a good practice. It allows for better control over which tests are built and run in different scenarios.
1-2: Summary: Build constraint addition enhances test organizationThe only change in this file is the addition of the
//go:build all_testsconstraint. This modification improves the organization of tests by ensuring that these specific tests are only built and run when theall_teststag is specified. The existing test functions remain unchanged and continue to provide good coverage for the holdable iterator functionality.core/types/state_account_marshalling_test.go (6)
41-45: Improved byte slice access in assertions.The changes from
Bytes()to direct slice access[:]are consistent and correct. This modification potentially improves efficiency by eliminating unnecessary method calls while maintaining the same functionality.
52-52: Consistent byte slice access in flattening loop.The change from
Bytes()to[:]is consistent with earlier modifications and maintains the same functionality while potentially improving efficiency.
78-82: Consistent byte slice access in zero account test.The changes from
Bytes()to[:]are consistent with earlier modifications and across all assertions in this test function. They maintain the same functionality while potentially improving efficiency.
102-106: Consistent byte slice access in non-empty account test.The changes from
Bytes()to[:]are consistent with earlier modifications and across all assertions in this test function. They maintain the same functionality while potentially improving efficiency.
113-113: Consistent byte slice access in max fields account test.The changes from
Bytes()to[:]are consistent with earlier modifications, both in assertions and the flattening loop. They maintain the same functionality while potentially improving efficiency.Also applies to: 141-145, 152-152
Line range hint
1-190: Summary of changes in state_account_marshalling_test.goThe modifications in this file consistently replace
Bytes()method calls with direct slice access[:]across all test functions. These changes:
- Maintain existing test logic and functionality.
- Potentially improve efficiency by eliminating unnecessary method calls.
- Align with a broader refactoring effort to streamline byte slice handling.
The consistency of these changes across the file suggests a systematic approach to the refactoring process.
core/vm/gas_table_test.go (3)
136-137: Improved test structure with sub-tests.The introduction of
t.Runfor each test case is a positive change. This approach enhances test organization, provides clearer output for failures, and allows for better isolation of test cases.Also applies to: 183-183
137-139: Clarification needed on skipped tests.The addition of the skip condition for non-EIP-3860 tests is noted. This change aligns with Scroll's default behavior. However, could you please clarify:
- Are there any implications of not running these skipped tests?
- How is backwards compatibility ensured if EIP-3860 is always enabled?
136-183: Overall improvements to TestCreateGas function.The changes to the
TestCreateGasfunction represent a significant improvement in test structure, clarity, and alignment with Scroll's implementation. The introduction of sub-tests, conditional execution based on EIP-3860, and refined gas usage checks all contribute to a more robust and maintainable test suite.While there are a few minor suggestions for further enhancements, the overall direction of these changes is positive and aligns well with best practices in Go testing.
core/genesis_test.go (5)
47-47: Verify the intention behind commenting out the PathScheme test.The PathScheme test case has been commented out. This could potentially reduce test coverage for different database schemes. Can you confirm if this is intentional and if there's an alternative way to ensure PathScheme is still being tested?
52-52: Verify the correctness of the new customghash value.The
customghashvalue has been updated. Please confirm that this new hash (0xc96ed5df64e683d5af1b14ec67126e31b914ca828021c330efa00572a61ede8f) correctly corresponds to the current custom genesis block configuration. It's important to ensure this value is accurate to maintain the integrity of the tests.
Line range hint
77-95: Verify the intention behind commenting out multiple test cases.Several test cases have been commented out, including scenarios for nil genesis blocks and mainnet comparisons. This significantly reduces the test coverage for genesis block setups. Can you explain the rationale behind this change? If these scenarios are no longer relevant, consider adding new test cases to maintain comprehensive coverage of genesis block configurations.
192-192: Simplification of trie configuration looks good.The change to consistently use
trie.HashDefaultsfor the trie configuration simplifies the code and aligns with the broader refactoring of trie configurations. This is a good improvement.However, please verify that this change doesn't negatively impact any scenarios that previously relied on the
ZktrieEnabledcondition. Ensure that all relevant test cases still pass with this new configuration.
Line range hint
1-268: Overall changes simplify testing, but warrant a review of test coverage.The changes in this file align with the broader refactoring of trie configurations in the codebase. While they simplify the testing process, they also reduce test coverage in some areas, particularly for different genesis block scenarios.
To ensure the continued robustness of the testing suite:
- Review the overall test coverage for genesis block configurations and setups.
- Consider adding new test cases to cover any scenarios that might have been lost due to the commented-out tests.
- Verify that the simplification of trie configuration doesn't overlook any edge cases previously covered by the more complex setup.
These steps will help maintain the integrity and comprehensiveness of the test suite while adapting to the new codebase structure.
trie/database.go (1)
116-116: LGTM. Verify impact on trie database behavior.The change from
mptResolver{}toZkChildResolver{}aligns with the PR objective of integrating the ZkTrie implementation. This modification is part of the broader refactoring effort to remove legacy trie references.To ensure this change doesn't introduce unexpected behavior:
- Verify that
ZkChildResolverimplements all necessary methods of the previousmptResolver.- Check if this change affects the performance or functionality of the trie database.
- Update any relevant documentation or comments that might reference the old
mptResolver.Run the following script to verify the implementation of
ZkChildResolver:✅ Verification successful
Verified:
ZkChildResolvercorrectly implements all required methods.The
ZkChildResolverstruct intrie/zk_trie_node.goimplements theForEachmethod, matching themptResolver's implementation. This ensures that the replacement aligns with the expected interface, maintaining consistent behavior for the trie database.
- Confirmed that
ZkChildResolverprovides the necessaryForEachmethod asmptResolverdid.- No additional methods are required for the resolver interface based on the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of ZkChildResolver # Test: Check for the definition of ZkChildResolver rg -A 10 'type ZkChildResolver struct' # Test: Compare methods of ZkChildResolver with mptResolver rg -A 5 'func \((\w+) (ZkChildResolver|mptResolver)\)'Length of output: 1544
core/state/snapshot/difflayer_test.go (2)
1-2: LGTM: Build constraints added correctly.The addition of build constraints for
all_testsis appropriate. This allows for selective test execution, potentially improving build times for regular development cycles while ensuring comprehensive testing when needed.
Line range hint
1-479: Summary: Minor change to test file configuration.The only change to this file is the addition of build constraints for
all_tests. The existing test functions (TestMergeBasics,TestMergeDelete,TestInsertAndMerge, and various benchmarks) remain unchanged and appear to provide good coverage of thediffLayerfunctionality, including merging, deletion, and insertion operations.core/state/state_object.go (4)
146-146: Improved empty root hash checkThe update to use
types.EmptyRootHashinstead ofs.db.db.TrieDB().EmptyRoot()is a good change. It standardizes the empty root hash check across the codebase, potentially improves performance by avoiding a database call, and enhances code readability.
256-256: Consistent use of EmptyRootHash in finalise methodThe update to use
types.EmptyRootHashin the prefetching condition of thefinalisemethod is a good change. It maintains consistency with the earlier updates in the file and aligns with the broader changes in the codebase for standardizing empty root hash checks.
Line range hint
1-577: Overall assessment of changes in state_object.goThe modifications in this file consistently update the handling of empty root hashes and simplify some methods. These changes align with the broader refactoring effort mentioned in the PR summary, aiming to streamline the trie implementation. The updates improve code consistency and potentially enhance performance by reducing unnecessary database calls.
Key improvements:
- Standardized use of
types.EmptyRootHash- Simplified
GetCommittedStatemethod- Consistent empty root hash checks in
getTrieandfinalisemethodsThese changes contribute positively to the codebase's maintainability and readability.
202-202: Simplified GetCommittedState methodThe simplification of this method by directly assigning the snapshot value to
common.BytesToHash(enc)reduces complexity and improves readability. This change aligns with the removal of ZK trie specific logic.Please verify that the snapshot always returns data in the correct format for this direct assignment. Run the following script to check the usage of snapshots in the codebase:
✅ Verification successful
Verified: GetCommittedState Method Simplification
The direct assignment using
common.BytesToHash(enc)has been verified across the codebase, ensuring that snapshots return data in the correct format.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check snapshot usage and data format handling # Search for snapshot data retrieval and usage rg -n "snap\.Storage\(" -A 5 rg -n "common\.BytesToHash\(" -A 2 -B 2Length of output: 70104
core/state_processor_test.go (4)
256-256: Base fee update approvedThe base fee in the error message has been updated from 38100000 to 39370000, consistent with the change in the ErrFeeCapTooLow test case.
To ensure the consistency of this change across the codebase, please run the following verification:
#!/bin/bash # Search for other occurrences of the old and new base fee values echo "Searching for old base fee value (38100000):" rg "38100000" --type go echo "Searching for new base fee value (39370000):" rg "39370000" --type go
203-203: Verify base fee consistencyThe base fee in the error message has been updated from 38100000 to 39370000. Please ensure this change is consistent with any modifications in the fee calculation logic elsewhere in the codebase.
#!/bin/bash # Search for other occurrences of the old and new base fee values echo "Searching for old base fee value (38100000):" rg "38100000" --type go echo "Searching for new base fee value (39370000):" rg "39370000" --type go
250-250: Transaction hash update approvedThe transaction hash in the error message has been updated. This change is consistent with the previous update and is likely due to modifications in the transaction creation or hashing process.
To ensure the consistency of this change, please verify the transaction creation process in the test setup:
✅ Verification successful
Transaction hash update verified
The change in the transaction hash corresponds directly to the updated parameters in the transaction creation process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the mkDynamicCreationTx function usage rg "mkDynamicCreationTx\(" --type goLength of output: 839
Script:
#!/bin/bash # Search for the mkDynamicCreationTx function usage in Go files rg "mkDynamicCreationTx\(" --type goLength of output: 303
244-244: Transaction hash update approvedThe transaction hash in the error message has been updated. This change is likely due to modifications in the transaction creation or hashing process.
To ensure the consistency of this change, please verify the transaction creation process in the test setup:
✅ Verification successful
Transaction hash update verified
The updated base fee values confirm that the transaction creation process has been modified, resulting in the updated transaction hash in the error message.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the mkDynamicCreationTx function definition rg "func mkDynamicCreationTx" --type goLength of output: 839
core/blockchain_snapshot_test.go (2)
1-2: New build constraints addedThe new build constraints
//go:build all_testsand// +build all_testshave been added to the file. This ensures that these tests are only built and run when theall_testsbuild tag is specified.These constraints are appropriate for snapshot tests, which may be more resource-intensive and not necessary for every build.
Line range hint
1-1010: Overall assessment: Well-implemented and comprehensive test suiteThe changes to this file, including the new build constraints and the comprehensive test suite for blockchain snapshot functionality, are well-implemented and thorough. The test cases cover a wide range of scenarios and should provide good coverage for the snapshot feature.
Key points:
- Appropriate build constraints added
- Comprehensive test suite covering various snapshot scenarios
- Clear documentation and expectations for each test case
- Support for both HashScheme and PathScheme
No significant issues were found, and the implementation appears to be of high quality.
core/state/snapshot/disklayer_test.go (1)
1-2: LGTM: Build constraint added for test isolation.The addition of the build constraint
all_testsis a good practice. It allows for better control over which tests are run in different scenarios, potentially speeding up the regular test suite by excluding these tests unless explicitly requested.core/state/sync_test.go (8)
1-2: LGTM: Build tags added for test isolation.The addition of build tags is a good practice. It ensures that these potentially resource-intensive tests are only compiled and run when specifically requested, improving build times for regular development.
Line range hint
32-70: LGTM: Comprehensive test state creation.The
makeTestStatefunction creates a diverse set of test accounts, covering various scenarios including different balances, nonces, code, and storage. This provides a robust foundation for testing state synchronization.
Line range hint
74-97: LGTM: Thorough state verification.The
checkStateAccountsfunction provides a comprehensive check of the reconstructed state, verifying balances, nonces, and code for each account. It also correctly uses the provided scheme for configuration, ensuring flexibility in testing different trie implementations.
Line range hint
121-132: LGTM: Comprehensive empty state sync test.The
TestEmptyStateSyncfunction effectively tests the edge case of empty state synchronization. It's particularly good that it checks both HashScheme and PathScheme, ensuring consistency across different trie implementations.
Line range hint
137-258: LGTM: Comprehensive iterative state sync tests.The
testIterativeStateSyncfunction and its variants provide thorough coverage of different synchronization scenarios. The tests cover individual and batched syncing, syncing from memory and disk, and syncing by path. This comprehensive approach ensures robustness of the state synchronization process across various conditions.
Line range hint
401-484: LGTM: Thorough random order state sync tests.The
TestIterativeRandomStateSyncfunction and its variants effectively test the robustness of the synchronization process when nodes are synced in a random order. This is an important test case as it simulates real-world scenarios where data might not always arrive sequentially. The tests cover both individual and batched syncing, providing good coverage of different sync strategies.
Line range hint
570-664: LGTM: Crucial incomplete state sync test.The
TestIncompleteStateSyncfunction is a critical test that ensures data integrity during the synchronization process. By verifying that only complete sub-tries are present in the database at any point during the sync, it guards against potential inconsistencies that could arise from partial or interrupted syncs. The separate checks for code and trie nodes provide comprehensive coverage of all sync scenarios.
Line range hint
1-670: Overall: Excellent improvements to state sync testing.The changes to this file significantly enhance the test coverage for state synchronization. The new tests cover a wide range of scenarios including empty states, iterative syncing, random order syncing, and incomplete syncs. These improvements will help ensure the robustness and reliability of the state synchronization process across different conditions and edge cases.
core/forkid/forkid_test.go (1)
1-2: Build tags added for test file inclusion.The addition of build tags
//go:build all_testsand// +build all_testsensures that this test file is only included when running all tests. This is a good practice for organizing and optimizing test execution.core/vm/runtime/runtime_test.go (3)
273-273: Verify the implications of skipping the TestBlockhash test.Skipping this test due to Scroll's different implementation of blockhash may have implications for compatibility with standard Ethereum clients.
Could you provide more details on how Scroll's blockhash implementation differs and how it maintains compatibility with the Ethereum ecosystem?
702-702: Consider the implications of not supporting SELFDESTRUCT.Skipping this test due to lack of SELFDESTRUCT support in Scroll is a significant deviation from standard Ethereum behavior. This could have far-reaching implications for contract compatibility and gas refunds.
Please clarify the rationale behind not supporting SELFDESTRUCT and how Scroll plans to handle existing contracts that use this opcode.
676-683: Address the removal of SELFDESTRUCT test case.The removal of the SELFDESTRUCT test case is consistent with Scroll's lack of support for this opcode. However, this might affect the coverage of cold account access cost testing.
How does Scroll plan to ensure comprehensive testing of cold account access costs without the SELFDESTRUCT case? Are there alternative test cases that could be added to maintain coverage?
eth/filters/filter_system_test.go (1)
471-471: LGTM. Verify test results after ZkTrie removal.The change aligns with the broader goal of phasing out ZkTrie configurations. This simplification is good for maintainability.
Please ensure that this change doesn't negatively impact the test results. Run the following command to verify:
core/state/statedb_test.go (4)
33-33: LGTM: Import statement reorganized.The import for "github.com/holiman/uint256" has been moved to the top of the import block, improving code organization.
799-799: Clarify the reason for commenting out the PathScheme test.The test case using
rawdb.PathSchemehas been commented out. This aligns with similar changes in other files, but it's important to understand the reasoning behind this modification.Could you please explain why this test case has been disabled? Is this a temporary change or part of a larger refactoring effort?
1054-1054: Verify the impact of reduced cache size.The cache size in the trie database configuration has been reduced from 1024 to 128. This change might affect test performance or memory usage.
Could you please explain the reasoning behind this cache size reduction? Have you observed any impact on test performance or memory usage?
1109-1109: Address skipped tests due to ZkTrie incompatibility.The
TestResetObjectandTestDeleteStoragetests have been skipped with the message "Snapshot doesn't support ZkTrie". This change is consistent with the removal of ZkTrie-related functionality in other files.Could you please provide information on the plans to address this incompatibility? Are there ongoing efforts to make these tests compatible with ZkTrie, or will they be replaced with alternative tests?
Also applies to: 1144-1144
core/blockchain_sethead_test.go (1)
1957-1957: Verify the impact of removing PathScheme from testsThe
rawdb.PathSchemehas been commented out, reducing the number of schemes tested. While this simplifies the testing process, it might lead to missing issues specific to thePathScheme.To ensure this change doesn't introduce unintended consequences, please run the following verification:
If
PathSchemeis still used elsewhere in the codebase, consider keeping the tests for both schemes to maintain comprehensive coverage.✅ Verification successful
PathScheme usage remains widespread across the codebase. Removing it from
core/blockchain_sethead_test.godoes not impact overall coverage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to PathScheme in test files # and verify if PathScheme is still used elsewhere in the codebase. echo "Checking for PathScheme references in test files:" rg "PathScheme" --type go --glob "*_test.go" echo "\nChecking for PathScheme usage in non-test files:" rg "PathScheme" --type go --glob "!*_test.go"Length of output: 14708
core/blockchain.go (4)
Line range hint
156-171: Simplified triedbConfig methodThe
triedbConfigmethod has been refactored to remove theisUsingZktrieparameter, simplifying its implementation. This change aligns with the removal of explicit zkTrie handling throughout the codebase.
179-180: Updated default cache configuration for zkTrie compatibilityThe
defaultCacheConfighas been modified to disable snapshots (SnapshotLimitset to 0) and prevent waiting for snapshot construction on startup (SnapshotWaitset to false). These changes are due to the current incompatibility between snapshots and zkTrie.
275-275: Simplified blockchain initialization processThe
NewBlockChainfunction has been updated to reflect the changes in thetriedbConfigmethod. The removal of the zkTrie-specific conditional check simplifies the trie database initialization process, moving towards a more unified approach.
Line range hint
1-1890: Overall simplification and removal of zkTrie-specific configurationsThe changes in this file are part of a broader effort to streamline the codebase by removing explicit zkTrie handling. The modifications to the
triedbConfigmethod,defaultCacheConfig, andNewBlockChainfunction all contribute to this goal. These changes simplify the code and make it more consistent, while also addressing the current incompatibility between snapshots and zkTrie.trie/util.go (1)
12-39: Ensure proper handling of base cases inHashElemsWithDomainIn the
HashElemsWithDomainfunction, whenlen(elems) == 0, the function returnsNewHashFromBigInt(baseH)(line 20). Please verify that this behavior correctly handles scenarios where onlyfstandsndare provided without additional elements.rollup/tracing/proof_test.go (1)
81-86:⚠️ Potential issue
key3andkey4have the same value, potential duplicationBoth
key3andkey4are assigned the same value (bytes.Repeat([]byte("x"), 32)), which may not be intentional and could lead to duplicate keys in your tests. Verify ifkey3should have a different value.If
key3should be unique, consider updating its value:-key3 := bytes.Repeat([]byte("x"), 32) +key3 := bytes.Repeat([]byte("y"), 32)Likely invalid or redundant comment.
trie/zk_trie_node_test.go (7)
12-23: LGTM:TestNewEmptyNodeis correctly implemented.The test for
NewEmptyNodecorrectly asserts the node type and verifies both the node and value hashes.
25-42: LGTM:TestNewLeafNodeis correctly implemented.The test appropriately verifies the node type, compressed flags, value preimage, and checks the correctness of node and value hashes.
44-58: LGTM:TestNewParentNodeis correctly implemented.The test checks the node type, child nodes, and validates node and value hashes accurately.
60-77: LGTM:TestNewParentNodeWithEmptyChildis correctly implemented.The test ensures that parent nodes with empty children are handled properly and verifies hashes accordingly.
88-197: LGTM:TestNewNodeFromBytesis comprehensive.The test covers deserialization of parent, leaf, and empty nodes from bytes, including handling of invalid byte arrays and node types.
199-223: LGTM:TestNodeValueAndDatais well-implemented.The test correctly verifies the canonical value and data methods for leaf, parent, empty, and invalid nodes.
225-240: LGTM:TestNodeStringcorrectly tests string representations.The test appropriately checks the string outputs for leaf, parent, empty, and invalid nodes.
trie/secure_trie.go (19)
51-56: StructstateTrieis well-definedThe
stateTriestruct fields are correctly defined, and the addition ofsecKeyCacheOwnerfor managing the key cache consistency is appropriate.
80-81: MethodMustGetcorrectly retrieves valuesThe
MustGetmethod properly wraps the underlying trie'sMustGet, handling key hashing internally.
Line range hint
88-92: Verify error handling inGetStorageEnsure that the error returned by
rlp.Split(enc)is appropriately handled. Since the function returnscontent, err, any error fromrlp.Splitwill be propagated, but it's crucial to confirm that this aligns with the intended error handling strategy.
Line range hint
100-106:GetAccountmethod functions as expectedThe method successfully retrieves and decodes the account data associated with the given address.
Line range hint
113-119:GetAccountByHashcorrectly retrieves accounts by hashThe method handles address hashes appropriately, providing flexibility in account retrieval.
127-129:GetNodemethod delegates correctlyThe implementation correctly calls
t.trie.GetNode, ensuring node retrieval functionality is maintained.
Line range hint
140-145:MustUpdateupdates trie and caches preimagesThe method correctly updates the trie with the hashed key and caches the preimage for future retrieval.
Line range hint
154-161: Ensure RLP encoding errors are handled inUpdateStorageWhile encoding
valueusing RLP, any potential errors fromrlp.EncodeToBytesshould be checked and handled to prevent unintended behavior.Apply this diff to handle possible encoding errors:
func (t *stateTrie) UpdateStorage(_ common.Address, key, value []byte) error { hk := t.hashKey(key) - v, _ := rlp.EncodeToBytes(value) + v, err := rlp.EncodeToBytes(value) + if err != nil { + return err + } err := t.trie.Update(hk, v) if err != nil { return err }
179-181: Confirm intentional no-op inUpdateContractCodeThe
UpdateContractCodemethod returnsnilwithout performing any action. If this is intended (e.g., not applicable forstateTrie), consider adding a comment explaining why it's a no-op to improve code clarity.
Line range hint
185-189:MustDeleteappropriately removes keysThe method correctly deletes the hashed key from both the cache and the trie, ensuring data consistency.
194-198:DeleteStoragemethod functions correctlyThe method properly handles the deletion of storage entries, maintaining the integrity of the trie.
201-205:DeleteAccountcorrectly removes account entriesAccount deletion is handled correctly by hashing the address and updating the trie and key cache.
Line range hint
209-215:GetKeyretrieves preimages effectivelyThe method first checks the local cache before querying the preimage store, optimizing retrieval.
244-246:Hashmethod correctly retrieves trie root hashThe method delegates to the underlying trie, ensuring consistency in hash computation.
249-253:Copymethod maintains trie state accuratelyThe
Copymethod creates a newstateTrieinstance with shared references where appropriate, which is acceptable.
259-261:NodeIteratorreturns iterator as expectedThe method provides access to trie nodes starting from a given key, facilitating traversal.
265-267:MustNodeIteratoraligns with conventionSuppressing errors in
MustNodeIteratoris consistent with otherMustmethods.
Line range hint
272-278: Concurrency considerations inhashKeyThe
hashKeymethod uses a buffer that's not thread-safe. SincestateTrieis documented as not safe for concurrent use, this is acceptable. Ensure that any future modifications maintain this constraint or add synchronization.
Line range hint
284-289:getSecKeyCacheensures key cache consistencyThe method correctly resets the cache when ownership changes, preventing stale data usage.
core/state/snapshot/snapshot_test.go (1)
1-2: Build constraint added to include test in specific buildsThe
all_testsbuild constraint has been correctly added at the top of the file to include this test in builds that specify theall_teststag.rollup/tracing/tracing.go (8)
30-31: Appropriate imports added for ZkTrie functionalityThe added imports for
trieandzkproofare necessary for the new ZkTrie features implemented in the code.
83-83: Addition ofZkTrieTracerfield toTraceEnvstructureThe
ZkTrieTracerfield is correctly added to theTraceEnvstructure to track ZkTrie proofs per address.
122-122: InitializeZkTrieTracerinCreateTraceEnvHelperThe
ZkTrieTracermap is properly initialized in theCreateTraceEnvHelperfunction.
440-442: Ensure safe handling when opening storage trie and type assertionThe code safely opens the storage trie and checks whether it is a ZkTrie, handling errors appropriately.
448-448: InstantiatezktrieTracerwith the ZkStorageTrieThe
zktrieTraceris correctly instantiated using thezkStorageTrie.
464-465: CachezktrieTracerinenv.ZkTrieTracerThe code checks for existing tracers and caches the
zktrieTracerinenv.ZkTrieTracerappropriately.
471-472: Mark deletions inenv.ZkTrieTracerwhen necessaryThe code correctly marks deletions in the ZkTrie tracer when a key is being deleted.
479-480: Generate storage proofs usingzkStorageTrie.ProvemethodThe code generates storage proofs with
zkStorageTrie.Proveand handles errors properly.trie/sync_test.go (2)
817-817: Consistent use oftypes.EmptyRootHashThe initialization of
srcTriewithtypes.EmptyRootHashis correct and consistent with the rest of the codebase.
851-851: Correct usage oftypes.EmptyRootHashin database updateThe
Updatefunction appropriately usestypes.EmptyRootHash, ensuring consistency across updates.core/state/statedb.go (1)
360-360: Simplification of 'GetProof' method improves clarityThe removal of the
IsUsingZktriecheck in theGetProofmethod simplifies the code and enhances readability. By directly callingGetProofByHash, the method becomes more straightforward and aligns with the updated trie implementation.cmd/utils/flags.go (1)
286-286:⚠️ Potential issueConsider the impact of changing the default GC mode to 'full'
Changing the default value of
GCModeFlagfromGCModeArchivetoGCModeFullwill cause nodes to prune old state data by default. This could affect users who require access to historical state data that is only available in archive nodes. Please ensure that this change is intentional and that it is communicated to users, possibly by updating documentation or release notes.Run the following script to verify that there are no hard-coded dependencies on
GCModeArchiveelsewhere in the codebase:✅ Verification successful
No hard-coded dependencies on
GCModeArchivefound outsideflags.go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for hard-coded references to `GCModeArchive` in the codebase. # Expected result: No occurrences of `GCModeArchive` being used as a default value elsewhere. rg 'GCModeArchive'Length of output: 559
|
|
colinlyguo
dismissed stale reviews from 0xmountaintop, georgehao, and themself
via
a2092c2
da-codec breaks it. fixed. |
4 participants
1. Purpose or design rationale of this PR
This PR moves in the ZkTrie implementation from its standalone repo and implements the
Commit()function to behave as the Geth expects it to behave.2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Release Notes
New Features
Byte32type for managing 32-byte data structures, including hashing and conversion methods.ZkTriefunctionality with improved error handling and state management.Bug Fixes
Tests
Byte32, trie nodes, andZkTrieoperations.Chores
ZkTrieimplementation.go.modto reflect current library usage.