feat: enable KromaZKTrie by default #76
Conversation
WalkthroughThe recent updates predominantly involve renaming the 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 (
|
jyc228
changed the title
There was a problem hiding this comment.
If you don't mind, let's use upper case for acronyms. It's preferred way in golang.
So, let's change KromaZkTrie to KromaZKTrie, all capitals.
FYI, let's see Initialisms in https://go.dev/wiki/CodeReviewComments.
|
@0xHansLee |
jyc228
force-pushed
the
refac/rename-experimentalzktrie-to-kromazktrie
branch
from
7d34a63 to
0678bc2
Compare
jyc228
changed the title
![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 (10)
- cmd/geth/config.go (1 hunks)
- cmd/geth/main.go (1 hunks)
- cmd/utils/flags.go (1 hunks)
- core/blockchain.go (1 hunks)
- core/state/database.go (2 hunks)
- eth/backend.go (1 hunks)
- eth/ethconfig/config.go (1 hunks)
- internal/ethapi/api.go (1 hunks)
- trie/database.go (2 hunks)
- trie/merkle_trie.go (1 hunks)
Additional comments: 13
trie/merkle_trie.go (1)
- 83-83: The update from
db.IsZkStateTrie()todb.IsKromaZK()aligns with the PR's objective to makeKromaZKTriethe default trie implementation. Ensure that theDatabaseinterface and its implementations are correctly updated to include theIsKromaZKmethod for this condition to function as expected.eth/ethconfig/config.go (1)
- 193-193: Renaming
ExperimentalZkTreetoKromaZKTriewithin theConfigstruct aligns with the PR's objective. Ensure that all references to the old field name are updated across the codebase to maintain consistency.core/state/database.go (2)
- 179-179: The update in the
OpenTriemethod to check forKromaZKaligns with the PR's objective. Ensure that thetrie.Databaseinterface and its implementations are correctly updated to include theIsKromaZKmethod for this condition to function as expected.- 200-200: The update in the
OpenStorageTriemethod to check forKromaZKaligns with the PR's objective. Ensure that thetrie.Databaseinterface and its implementations are correctly updated to include theIsKromaZKmethod for this condition to function as expected.cmd/geth/config.go (1)
- 193-193: The change to use
KromaZKTrieas the default trie implementation is noted. Ensure that all references to the previousExperimentalZkTreeacross the entire codebase have been updated accordingly to maintain consistency.Verification successful
The search for any remaining references to
ExperimentalZkTreedid not yield any results, indicating that all necessary updates have likely been made to replace or remove references toExperimentalZkTreein favor ofKromaZKTrie. This supports the conclusion that the change has been properly integrated across the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to `ExperimentalZkTree` which should have been replaced. rg --type go 'ExperimentalZkTree'Length of output: 33
trie/database.go (3)
- 40-40: Renaming
ExperimentalZkTrietoKromaZKTriein theConfigstruct aligns with the new naming convention. Ensure this change is consistently applied across all relevant parts of the codebase.- 381-382: Replacing
IsZkStateTriewithIsKromaZKreflects the updated naming convention. Verify that this new method is correctly used wherever trie type checks are performed.- 389-393: The
SetBackendmethod correctly updates theKromaZKTrieconfiguration. Ensure that the logic here accurately reflects the intended behavior when switching trie backends, especially in the context of enabling or disabling theKromaZKTrie.cmd/geth/main.go (1)
- 160-160: Renaming
utils.ExperimentalZkTrietoutils.KromaZKTriealigns with the PR's objective to makeKromaZKTriethe default trie implementation. This change is straightforward and seems correctly implemented in the context of the provided code. However, it's crucial to ensure that all references to the oldExperimentalZkTrieacross the entire codebase are updated toKromaZKTrieto maintain consistency and avoid potential configuration errors or runtime issues.Verification successful
The verification process did not find any remaining references to
ExperimentalZkTriein the Go files of the codebase, indicating that the renaming toKromaZKTriehas been successfully applied across the entire codebase. This aligns with the initial review comment's concern and suggests that the change has been implemented correctly and thoroughly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to ExperimentalZkTrie in the codebase. rg --type go 'ExperimentalZkTrie' # Expecting no results, indicating all references have been updated.Length of output: 33
eth/backend.go (1)
- 211-211: The change from
ExperimentalZkTrietoKromaZKTriein thecacheConfigstruct reflects the PR's objective to makeKromaZKTriethe default state trie implementation. This update is consistent with the broader system updates and aligns with the PR's goals. However, ensure that all references to the oldExperimentalZkTriefield across the codebase are updated toKromaZKTrieto maintain consistency and avoid potential configuration errors.cmd/utils/flags.go (1)
- 1039-1043: The renaming of the flag from
ExperimentalZkTrietoKromaZKTrieand the addition of a default value oftruealign with the PR's objective to makeKromaZKTriethe default trie implementation. Ensure that all references to this flag in documentation, tests, and other parts of the codebase are updated accordingly to reflect this change. Additionally, verify that setting this flag totrueby default does not introduce any unexpected behavior or conflicts with other configurations.internal/ethapi/api.go (1)
- 790-790: The change from checking
IsZk()toIsKromaZK()in thenewStateTriefunction aligns with the PR's objective to transition toKromaZKTrie. However, ensure that all references and dependencies throughout the codebase are updated accordingly to prevent any inconsistencies or errors.core/blockchain.go (1)
- 151-162: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [24-25]
LGTM!
| SnapshotNoBuild bool // Whether the background generation is allowed | ||
| SnapshotWait bool // Wait for snapshot construction on startup. TODO(karalabe): This is a dirty hack for testing, nuke it | ||
|
|
||
| ExperimentalZkTrie bool // use ZkMerkleStateTrie instead of ZkTrie | ||
| KromaZKTrie bool // use ZkMerkleStateTrie instead of ZkTrie | ||
| } | ||
|
|
||
| // triedbConfig derives the configures for trie database. | ||
| func (c *CacheConfig) triedbConfig() *trie.Config { | ||
| config := &trie.Config{Preimages: c.Preimages, ExperimentalZkTrie: c.ExperimentalZkTrie} | ||
| config := &trie.Config{Preimages: c.Preimages, KromaZKTrie: c.KromaZKTrie} | ||
| if c.StateScheme == rawdb.HashScheme { | ||
| config.HashDB = &hashdb.Config{ | ||
| CleanCacheSize: c.TrieCleanLimit * 1024 * 1024, |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
There is a TODO comment regarding missing tests.
Do you want me to generate the tests or open a GitHub issue to track this task?
There was a problem hiding this comment.
@0xHansLee
For now, I'll only reflect it in KromaZKTrie and not the rest.
Got it, thanks!
Summary by CodeRabbit
ExperimentalZkTreetoKromaZKTrieacross various configurations and methods to reflect the new naming convention for ZK state trie usage.KromaZKTriewith a default value oftrue, enhancing user control over this feature.