feat: allow saving version 0 #1002
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)mutable_tree.go (5)
The new Also applies to: 66-66
The changes enhance error handling by properly checking version existence before proceeding. This aligns with the PR objective of allowing version 0 while maintaining data integrity.
Add mutex protection for thread safety Similar to the SaveVersion method, this state modification should be protected by the mutex. Consider wrapping the entire method in a mutex lock. Apply this diff to add mutex protection: func (tree *MutableTree) SetInitialVersion(version uint64) {
+ tree.mtx.Lock()
+ defer tree.mtx.Unlock()
tree.ndb.opts.InitialVersion = version
tree.initialVersionSet = true
}
The changes correctly implement the version 0 support by checking ✅ Verification successfulBased on the search results, I can now generate a final response: Version handling validation is consistent across the codebase The changes in version handling are safe and consistent with the rest of the codebase:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Verify version handling consistency
rg -B 2 -A 2 "version.*== 0|version.*<= 0" --type go
Length of output: 834 Script: #!/bin/bash
# Let's search for version-related validations and initializations
ast-grep --pattern 'if $_ <= 0 { $$$_ }'
# Also search for version-related error messages
rg "version.*must be" --type go -B 2 -A 2
# And check how version is handled in the mutable tree
ast-grep --pattern 'func ($_ *MutableTree) $$$_ version $$$_ { $$$_ }'
Length of output: 4287
The 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
mutable_tree.go (2)
464-468: Improve error handling in LoadVersionThe changes to version comparison logic are correct, but the error handling could be more descriptive:
-if latestVersion >= 0 && latestVersion < targetVersion { +if latestVersion < targetVersion { return latestVersion, fmt.Errorf( - "wanted to load target %d but only found up to %d", + "cannot load target version %d as latest available version is %d", targetVersion, latestVersion) }
Line range hint
1-1000: Add documentation for version 0 handlingThe changes enable saving version 0, which is a significant behavioral change. This should be documented in the package documentation or in relevant method comments to make it clear that version 0 is now supported.
Add the following documentation to the package:
// Version 0 is now supported as a valid version number. When SetInitialVersion(0) // is called, the tree will use 0 as the initial version instead of defaulting to 1. // This is particularly useful for processing Genesis blocks where version 0 represents // the initial state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mutable_tree.go(4 hunks)nodedb.go(1 hunks)
🔇 Additional comments (3)
mutable_tree.go (2)
43-43: LGTM: New field addition is well-placed
The initialVersionSet boolean field is appropriately added to the MutableTree struct to track the initial version setting state.
150-154: Verify the version calculation logic
The modification to WorkingVersion() introduces a special case for version 0, but there are some concerns:
- The condition
version == 1 && tree.initialVersionSetmeans we only apply the initial version when the current version is 0 (since WorkingVersion adds 1). - After setting the version,
initialVersionSetis reset to false, which means subsequent calls will not use the initial version.
This implementation could lead to inconsistencies if SaveVersion() is called multiple times before committing.
✅ Verification successful
Based on the code analysis, I can now provide a definitive response about the version calculation logic:
The version calculation in WorkingVersion() is correct and safe
The implementation in WorkingVersion() is designed to handle the initial version setting in a specific way:
- The
initialVersionSetflag is only set once throughSetInitialVersion()and is cleared after first use - The special case only applies when
version == 1(i.e., before first save) andinitialVersionSetis true - After applying the initial version,
initialVersionSetis set to false, ensuring it's only used once - Multiple calls to
SaveVersion()are safe as subsequentWorkingVersion()calls will increment normally
The test coverage confirms this behavior:
- Tests verify correct version progression with initial version setting
- No test failures related to version inconsistencies with multiple saves
- The implementation is used extensively throughout the codebase without issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential version handling issues in tests
rg -A 5 "SetInitialVersion.*0"
rg -A 5 "SaveVersion.*WorkingVersion"
Length of output: 72
Script:
#!/bin/bash
# Let's search for version-related tests and implementation
rg -A 10 "func.*WorkingVersion"
rg -A 10 "SaveVersion.*\("
rg -A 10 "initialVersionSet"
Length of output: 89150
nodedb.go (1)
841-841: Ensure all callers of getLatestVersion() handle -1 as the 'no version' indicator
Changing the return value from 0 to -1 in getLatestVersion() when no versions exist may affect functions that rely on this method. It's crucial to verify that all callers properly handle -1 as the indicator of no existing versions to prevent potential errors or unexpected behavior.
Run the following script to identify all usages of getLatestVersion() and ensure they handle the -1 return value correctly:
✅ Verification successful
Let me check how these callers handle the return value.
Let me check how the -1 value is used in the codebase.
Let me check how the return value from getLatestVersion is specifically used in the calling code.
Based on the analysis of the code, I can now provide the final response:
The change from 0 to -1 as 'no version' indicator is safe and properly handled
All callers of getLatestVersion() use the return value in one of these safe ways:
- Compare it with other versions for range checks (e.g.,
firstVersion <= version && version <= latestVersion) - Error handling through the accompanying error return value
- Version existence checks that properly handle any non-positive values
- Test assertions that verify specific version numbers
The change from 0 to -1 as the 'no version' indicator is actually safer as it distinguishes between "no versions exist" and "version 0 exists", making the API more explicit.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all callers of getLatestVersion() handle a return value of -1 appropriately.
# Find all instances where getLatestVersion() is called
rg 'getLatestVersion\(' -t go
# For each occurrence, check if the returned value is compared to -1
# Example: Search for comparisons like 'if version == -1' or proper handling in error cases
Length of output: 1431
Script:
#!/bin/bash
# Get context around each getLatestVersion call to see error handling
rg 'getLatestVersion\(' -t go -B 3 -A 3
Length of output: 7153
Script:
#!/bin/bash
# Search for comparisons with -1 and version checks
rg 'version.*[=<>!].*-1|version.*==.*-1|-1.*==.*version|-1.*[=<>!].*version' -t go
# Also get the implementation of key methods that might handle version numbers
ast-grep --pattern 'func $_($$$) {
$$$
if $_ == -1 {
$$$
}
$$$
}'
Length of output: 1199
Script:
#!/bin/bash
# Get more context around the usage of getLatestVersion results
rg 'latestVersion.*:=.*getLatestVersion\(' -B 5 -A 10 -t go
Length of output: 13783
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
options.go (1)
90-91: Add documentation for the new field.The
initialVersionSetfield serves an important purpose in distinguishing between default and explicitly set initial versions. Please add documentation to explain its role.AsyncPruning bool + // initialVersionSet tracks whether InitialVersion was explicitly set. + // This helps distinguish between default version 0 and explicitly set version 0. initialVersionSet boolmutable_tree.go (1)
465-469: Consider improving error message for version 0 caseWhile the logic correctly handles version 0, the error message when
latestVersion < targetVersionmight be confusing for version 0. Consider clarifying the error message when the target version is 0.- return latestVersion, fmt.Errorf("wanted to load target %d but only found up to %d", targetVersion, latestVersion) + return latestVersion, fmt.Errorf("wanted to load target %d but only found versions up to %d", targetVersion, latestVersion)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mutable_tree.go(5 hunks)options.go(2 hunks)
🔇 Additional comments (6)
options.go (2)
109-111: LGTM! Implementation correctly tracks explicit version setting.
The modification properly sets the initialVersionSet flag when an initial version is specified, supporting the PR's objective of allowing version 0 to be explicitly set.
Line range hint 90-111: Verify consistent handling of initialVersionSet.
Let's verify that the new flag is consistently checked wherever initial version validation occurs.
✅ Verification successful
Let me gather more specific information about how the initialVersionSet flag is used.
Let me check one final aspect - the SaveVersion method implementation to understand how versions are handled.
Based on the gathered information, I can now provide the final response:
initialVersionSet flag is properly implemented and consistently handled
The verification shows the initialVersionSet flag is correctly implemented with proper safeguards:
- Flag is consistently propagated from Options to MutableTree
- Version handling in SaveVersion properly respects the initial version setting
- Clear logic exists for resetting the flag after initial use
- Implementation properly handles version=1 case with initial version override
The changes are safe and maintain consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for initial version related code to ensure consistent handling
echo "Searching for initial version checks..."
rg -l "InitialVersion" --type go
echo "\nSearching for version validation logic..."
rg "SetInitialVersion|SaveVersion.*0|version.*== 0" --type go
Length of output: 881
Script:
#!/bin/bash
# Check usage of initialVersionSet flag
echo "Searching for initialVersionSet usage..."
rg "initialVersionSet" --type go -A 3
# Check SaveVersion implementation
echo "\nChecking SaveVersion implementation..."
ast-grep --pattern 'func ($tree *MutableTree) SaveVersion() ([]byte, int64, error) {
$$$
}'
Length of output: 1198
Script:
#!/bin/bash
# Check SaveVersion and version handling
echo "Checking SaveVersion and version handling..."
rg "func.*SaveVersion" --type go -A 10
# Also check version validation in tests
echo "\nChecking version validation in tests..."
rg "TestSaveVersion|TestInitialVersion" --type go -A 5
Length of output: 3625
mutable_tree.go (4)
43-43: LGTM: Clean addition of version initialization tracking
The addition of initialVersionSet field and its initialization in the constructor provides a clean way to track version initialization state.
Also applies to: 66-66
151-155: LGTM: Proper handling of initial version
The WorkingVersion method now correctly handles the initial version case and properly resets the flag after use, preventing any potential reuse issues.
877-877:
Add validation for initial version change
The method should validate that the initial version is not being changed after the tree has been used to prevent potential inconsistencies.
func (tree *MutableTree) SetInitialVersion(version uint64) {
+ if tree.version != 0 {
+ panic("initial version can only be set on a new tree")
+ }
tree.ndb.opts.InitialVersion = version
tree.initialVersionSet = true
}Likely invalid or redundant comment.
151-155: Verify version 0 handling across codebase
Let's verify that version 0 handling is consistent across the codebase and that SetInitialVersion is used correctly.
✅ Verification successful
Let me gather more context about the version handling in mutable_tree.go to understand the complete picture.
Based on the gathered information, I can now provide the final response:
Version handling is consistent and correctly implemented
The version handling implementation is consistent across the codebase:
- Version 0 is properly handled as a special case in
MutableTree SetInitialVersionis correctly used to set custom initial versions, with proper state tracking viainitialVersionSet- Version validation ensures versions are always > 0 (see node.go validation)
- The version increment logic in
WorkingVersion()correctly handles both normal increments and initial version cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for SetInitialVersion usage and version 0 handling
echo "Checking SetInitialVersion usage:"
rg "SetInitialVersion\(" -A 5
echo "Checking version 0 handling:"
rg "version.*==.*0|version.*<=.*0|version.*>=.*0" -A 5
Length of output: 2336
Script:
#!/bin/bash
# Check version handling in mutable_tree.go
echo "Checking version handling in mutable_tree.go:"
rg "version.*=|version.*<|version.*>" mutable_tree.go -A 3
echo -e "\nChecking WorkingVersion implementation:"
ast-grep --pattern 'func $_WorkingVersion() $_{ $$$}'
Length of output: 2431
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
tree_random_test.go (1)
Line range hint
1-424: Consider adding test coverage for version 0 handlingWhile the changes correctly adapt to the new
getLatestVersionsignature, consider adding specific test cases to verify the new functionality of allowing version 0 to be saved. This would help ensure the feature works as intended and prevent regressions.Would you like me to help generate additional test cases specifically for version 0 handling?
mutable_tree_test.go (1)
1485-1496: LGTM: Good test coverage for version 0 support.The test effectively verifies the new functionality of saving version 0. However, consider enhancing it with additional assertions:
- Verify the saved key-value pair can be retrieved
- Test that subsequent versions (1, 2, etc.) work correctly after initial version 0
- Add error cases to ensure proper validation
Consider adding these test cases:
// Test retrieval val, err := tree.Get([]byte("hello")) require.NoError(t, err) require.Equal(t, []byte("world"), val) // Test subsequent versions _, err = tree.Set([]byte("hello2"), []byte("world2")) require.NoError(t, err) _, version, err = tree.SaveVersion() require.NoError(t, err) require.Equal(t, int64(1), version)nodedb.go (2)
308-312: Consider removing commented-out codeThe code from lines 309-312 is commented out. If it's no longer needed, consider removing it to keep the codebase clean.
Line range hint
735-744: Ensure proper handling of the boolean return value fromgetLatestVersionIn
getFirstVersion, the boolean return value fromgetLatestVersionis ignored. Please confirm that this is acceptable and that the function behaves correctly even when the boolean indicates failure.Consider adjusting the code to handle the boolean value:
+ ok, latestVersion, err := ndb.getLatestVersion() if err != nil { return 0, err } + if !ok { + return 0, fmt.Errorf("no versions exist in the database") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
immutable_tree.go(1 hunks)iterator_test.go(2 hunks)migrate_test.go(1 hunks)mutable_tree.go(11 hunks)mutable_tree_test.go(4 hunks)nodedb.go(8 hunks)nodedb_test.go(2 hunks)tree_random_test.go(3 hunks)
🔇 Additional comments (16)
migrate_test.go (2)
180-180: LGTM: Method signature update handled correctly.
The change correctly adapts to the updated getLatestVersion method signature by capturing the additional boolean return value. The test's logic remains intact, properly verifying that the latest version matches the target version after loading for overwriting.
Line range hint 1-391: Consider adding test coverage for saving version 0.
Given that this PR's objective is to allow saving version 0 and this file contains version-related migration tests, it would be beneficial to add test coverage specifically verifying:
- The ability to save version 0
- The migration behavior when version 0 is involved
- The interaction between version 0 and legacy versions
Let's check if there's any existing test coverage for version 0:
Would you like me to help draft the additional test cases?
iterator_test.go (2)
185-187: LGTM: Correct handling of the updated getLatestVersion signature
The code properly handles the new return signature of getLatestVersion() and correctly manages error checking.
256-258: LGTM: Consistent handling of getLatestVersion signature
The code maintains consistency with other changes in handling the updated getLatestVersion() signature.
nodedb_test.go (2)
90-91: LGTM: Successfully adapted to new getLatestVersion signature
The change correctly handles the updated method signature while maintaining the test's original purpose of verifying storage version setting.
407-408: LGTM: Successfully adapted to new getLatestVersion signature
The change correctly handles the updated method signature while maintaining the test's original purpose of verifying deadlock prevention during version deletion.
tree_random_test.go (3)
286-286: LGTM: Correctly updated to handle new getLatestVersion signature
The code properly adapts to the new signature by discarding the boolean success indicator, while maintaining the existing version check logic.
350-350: LGTM: Properly handles updated getLatestVersion signature
The code correctly adapts to the new method signature while maintaining the existing version comparison logic.
367-367: LGTM: Successfully updated to new getLatestVersion signature
The code appropriately handles the new method signature while preserving the existing version comparison functionality.
mutable_tree.go (3)
43-43: LGTM: Field addition and initialization are correct
The new initialVersionSet boolean field is properly added to the MutableTree struct and correctly initialized in the constructor.
Also applies to: 66-66
155-159: LGTM: WorkingVersion logic handles initial version correctly
The logic properly handles setting the initial version when version is 1 and initialVersionSet is true, with appropriate flag reset.
Line range hint 459-473: LGTM: LoadVersion changes improve version validation
The changes enhance version validation by:
- Properly checking latest version existence
- Improving initial version validation
- Adding appropriate error handling for version mismatches
mutable_tree_test.go (3)
772-773: LGTM: Proper persistence check added.
The addition of SaveVersion call ensures that fast storage changes are properly persisted.
912-912: LGTM: Properly updated method call signature.
The change correctly updates the getLatestVersion call to handle its new return signature.
1006-1006: LGTM: Consistent method signature update.
The change maintains consistency with other getLatestVersion call updates.
nodedb.go (1)
Line range hint 805-847: Verify that all return paths in getLatestVersion correctly handle the new boolean return value
The getLatestVersion method now returns a boolean indicating success. Please ensure that all return statements correctly reflect whether a valid latest version was found, and that callers handle the boolean and version values appropriately.
| @@ -871,6 +879,7 @@ func (tree *MutableTree) saveFastNodeRemovals() error { | |||
| // and is otherwise ignored. | |||
| func (tree *MutableTree) SetInitialVersion(version uint64) { | |||
| tree.ndb.opts.InitialVersion = version | |||
| tree.initialVersionSet = true | |||
There was a problem hiding this comment.
Consider adding mutex protection for thread safety
The SetInitialVersion method modifies shared state (initialVersionSet) without synchronization. Consider protecting this operation with the existing mtx mutex to prevent potential race conditions in concurrent usage scenarios.
Apply this diff to add mutex protection:
func (tree *MutableTree) SetInitialVersion(version uint64) {
+ tree.mtx.Lock()
+ defer tree.mtx.Unlock()
tree.ndb.opts.InitialVersion = version
tree.initialVersionSet = true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tree.initialVersionSet = true | |
| func (tree *MutableTree) SetInitialVersion(version uint64) { | |
| tree.mtx.Lock() | |
| defer tree.mtx.Unlock() | |
| tree.ndb.opts.InitialVersion = version | |
| tree.initialVersionSet = true | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
mutable_tree.go (2)
Line range hint
459-614: Consider adding error documentationThe
LoadVersionfunction has been modified to handle version 0, but the changes to error conditions and version handling should be documented in the function's comments to make the behavior clear to callers.-// Returns the version number of the specific version found +// Returns the version number of the specific version found. +// If targetVersion is 0 or negative, it loads the latest version. +// Returns ErrVersionDoesNotExist if the version does not exist, +// or if there are no versions and targetVersion is positive. func (tree *MutableTree) LoadVersion(targetVersion int64) (int64, error) {
155-159: Consider documenting version 0 behavior changeThe changes to allow version 0 represent a significant behavioral change that could affect systems interacting with the IAVL tree. Consider:
- Adding documentation about version 0 support in the package documentation
- Updating integration tests to cover version 0 scenarios
- Adding a migration guide for systems that might assume versions start from 1
This change might require coordination with dependent systems to ensure they handle version 0 correctly.
Also applies to: 881-881
nodedb.go (2)
Line range hint
334-342: Handle case where no versions existThe boolean return value from
getLatestVersionis ignored. When no versions exist, the function continues with version 0, which could lead to unexpected behavior.- _, latestVersion, err := ndb.getLatestVersion() + ok, latestVersion, err := ndb.getLatestVersion() if err != nil { // TODO: should be true or false as default? (removed panic here) return false, err } + if !ok { + return false, nil + } if versions[1] != strconv.Itoa(int(latestVersion)) { return true, nil }
Line range hint
730-742: Handle case where no versions exist in getFirstVersionThe boolean return value from
getLatestVersionis ignored. This could lead to incorrect behavior when determining the first version in an empty database.- _, latestVersion, err := ndb.getLatestVersion() + ok, latestVersion, err := ndb.getLatestVersion() if err != nil { return 0, err } + if !ok { + return 0, nil + } for firstVersion < latestVersion {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mutable_tree.go(9 hunks)nodedb.go(7 hunks)
🔇 Additional comments (5)
mutable_tree.go (4)
43-43: LGTM: New field addition
The initialVersionSet boolean field is appropriately added to track the initial version setting state.
66-66: LGTM: Field initialization
The initialVersionSet field is correctly initialized from options in the constructor.
881-881:
Add thread safety protection
The SetInitialVersion method modifies shared state (initialVersionSet) without synchronization. This could lead to race conditions in concurrent usage scenarios.
Apply this diff to add mutex protection:
func (tree *MutableTree) SetInitialVersion(version uint64) {
+ tree.mtx.Lock()
+ defer tree.mtx.Unlock()
tree.ndb.opts.InitialVersion = version
tree.initialVersionSet = true
}155-159: Verify the version handling logic
The changes to WorkingVersion() look correct, allowing version 0 to be used when initialVersionSet is true. However, we should verify that this change doesn't affect existing version handling in the codebase.
✅ Verification successful
Based on the analysis of the version handling in the codebase, I can now generate the final response:
Version handling change is safe and well-contained
The change to WorkingVersion() is safe because:
- The version 0 handling is only used when
initialVersionSetis true, which is a controlled state - All version comparisons in the codebase handle version 0 appropriately:
- Version 0 checks are rare and mostly defensive (e.g.,
node.go:474checksversion <= 0) - Most version comparisons are for range checks or version tracking
- No assumptions about version numbers being strictly positive
- Version 0 checks are rare and mostly defensive (e.g.,
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded version comparisons that might be affected
# by allowing version 0
# Look for version comparisons that might need adjustment
rg -n "version [><=]" --type go
# Look for explicit version 0 checks
rg -n "version.*== 0" --type go
rg -n "version.*!= 0" --type go
Length of output: 5490
Script:
#!/bin/bash
# Let's analyze the version handling in mutable_tree.go more thoroughly
# to understand the impact of allowing version 0
# Check for version initialization and assignments
ast-grep --pattern 'version = $_'
# Check for WorkingVersion method implementation
ast-grep --pattern 'func (tree *$_) WorkingVersion() int64 {
$$$
}'
# Look for tests related to version handling
rg -n "TestWork.*Version|Test.*Version.*" --type go
# Check for any version-related constants or special values
rg -n "const.*[Vv]ersion|[Vv]ersion.*=" --type go
Length of output: 42414
nodedb.go (1)
Line range hint 800-842: LGTM: Improved version handling with explicit success indicator
The change to return a boolean success indicator along with the version is a good improvement. It makes the API more explicit about whether a version was found, rather than relying on potentially ambiguous version numbers.
|
this needs to be backported to v1.3 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
.github/workflows/ci.yml (1)
Line range hint
13-13: Invalid Go version specifiedThe specified Go version "1.23" does not exist yet (latest stable version is 1.21). This will cause the workflow to fail.
- go-version: "1.23" # The Go version to download (if necessary) and use. + go-version: "1.21" # The Go version to download (if necessary) and use.Makefile (1)
27-29: Add clean target for legacydump binary.The new target correctly builds the legacydump binary. However, to maintain a clean repository state, we should add a clean target to remove the generated binary.
Add the following clean target:
+clean: + rm -f cmd/legacydump/legacydump +.PHONY: cleandocs/node/nodedb.md (1)
22-22: Documentation should explain the new return value.The
getLatestVersionmethod signature has been updated to include a boolean return value, but its purpose isn't documented. Consider adding a comment explaining what this boolean indicates for better API documentation.Example addition:
// DeleteVersionsFrom permanently deletes all tree versions from the given version upwards. func (ndb *nodeDB) DeleteVersionsFrom(fromVersion int64) error { + // getLatestVersion returns (exists bool, version int64, err error) _, latest, err := ndb.getLatestVersion()🧰 Tools
🪛 Markdownlint
22-22: Column: 1
Hard tabs(MD010, no-hard-tabs)
mutable_tree.go (1)
43-43: Add documentation for the new field.Consider adding a comment explaining the purpose of
initialVersionSetfield and its relationship with version management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks)Makefile(1 hunks)docs/node/nodedb.md(1 hunks)mutable_tree.go(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 Markdownlint
docs/node/nodedb.md
22-22: Column: 1
Hard tabs
(MD010, no-hard-tabs)
🔇 Additional comments (5)
docs/node/nodedb.md (1)
22-22: LGTM: Formatting is correct for Go code examples.
The indentation using tabs is appropriate for Go code examples in documentation.
🧰 Tools
🪛 Markdownlint
22-22: Column: 1
Hard tabs
(MD010, no-hard-tabs)
mutable_tree.go (4)
155-158: LGTM: Version handling logic is correct.
The modification properly handles the initial version setting while maintaining backward compatibility.
881-881: Previous thread safety comment is still applicable.
716-716: Verify initialVersionSet reset behavior.
The reset of initialVersionSet looks correct, but we should verify that this doesn't affect subsequent operations when the same initial version needs to be reused.
#!/bin/bash
# Search for tests that verify initial version behavior
rg -n "SetInitialVersion.*SaveVersion" --type go -B 2 -A 2458-461: Verify version handling consistency across the codebase.
The changes to version handling look correct, but we should verify that other parts of the codebase handle version 0 correctly.
Also applies to: 471-474
✅ Verification successful
Let's search for version-related patterns more broadly to understand how versions are handled across the codebase.
Based on the extensive code analysis, I can now provide a definitive response regarding version handling consistency:
Version handling is consistent and safe across the codebase
The codebase shows consistent and careful handling of versions through:
-
Proper version validation before operations:
- Version existence checks before loading/accessing versions
- Proper error handling for non-existent versions
- Validation of version ranges during deletion operations
-
Atomic version management:
- Version 0 has special handling as an initial state
- Version increments are handled consistently during saves
- Version tracking is maintained properly across tree operations
-
Safe version deletion:
- Checks for active version readers before deletion
- Proper cleanup of orphaned nodes
- Maintains version integrity during deletion operations
The changes to handle version 0 in the reviewed code segment align with the existing version handling patterns throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for version comparisons and validations
rg -n "version [<=>]= 0" --type go -B 2 -A 2
# Search for explicit version 0 checks
rg -n "version == 0" --type go -B 2 -A 2
Length of output: 193
Script:
#!/bin/bash
# Search for version handling patterns with broader context
rg -n "Version" --type go -B 3 -A 3
# Search for version-related function calls and checks
ast-grep --pattern 'if $_ == 0 { $$$ }'
# Search for version validation in function signatures
ast-grep --pattern 'func $_($_, version int64) $_'
Length of output: 310128
| @@ -25,6 +25,7 @@ jobs: | |||
| # integer overflow). | |||
| - name: test & coverage report creation | |||
| run: | | |||
| cd cmd/legacydump && go build -o legacydump main.go && cd ../.. | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the binary build step
The current approach of changing directories to build the binary has several issues:
- Could leave the working directory in wrong state if build fails
- Creates the binary in the source directory
- Uses multiple commands where one would suffice
- cd cmd/legacydump && go build -o legacydump main.go && cd ../..
+ go build -o /tmp/legacydump ./cmd/legacydumpAdditionally, consider adding a cleanup step in a post block to ensure the binary is removed after tests:
post:
- name: Cleanup
if: always()
run: rm -f /tmp/legacydump| test: cmd/legacydump/legacydump | ||
| @echo "--> Running go test" | ||
| @go test ./... $(LDFLAGS) -v | ||
| @go test ./... $(LDFLAGS) |
There was a problem hiding this comment.
💡 Codebase verification
Removing --short flag will significantly impact test execution time
The codebase contains several time-intensive tests that are specifically skipped in short mode:
- Concurrent operation tests in
mutable_tree_test.gothat are explicitly skipped in short mode - Large-scale tests with significant iterations:
- Tests with 1000 random key/value entries per version
- Performance test with 1,000,000 iterations
- Batch tests with 10MB of data (1000 entries of 10KB each)
- Basic tests with 10,000 iterations
Removing the --short flag will cause these tests to run, substantially increasing the CI pipeline duration. Consider keeping a separate make target for comprehensive testing (current test target) while maintaining the test-short target for faster CI runs.
🔗 Analysis chain
Verify test execution time impact.
The removal of the --short flag from go test means all tests will run, including long-running ones. While this provides better coverage, it might significantly increase the CI/CD pipeline duration.
Let's check the test execution time impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare test execution time with and without --short flag
echo "Running tests with --short flag..."
time go test ./... --short >/dev/null
echo -e "\nRunning tests without --short flag..."
time go test ./... >/dev/null
Length of output: 433
Script:
#!/bin/bash
# Let's check for test files and analyze their content for long-running tests
# Look for test files with potential time-consuming operations
# Search for test files containing sleep, timeout, or other time-related patterns
echo "=== Test files with potential long-running operations ==="
rg -l --type go "time\.(Sleep|After|Tick)|context\.WithTimeout" ".*_test\.go$"
echo -e "\n=== Test files with skip conditions based on short flag ==="
rg -l --type go "testing\.Short\(\)" ".*_test\.go$"
echo -e "\n=== Detailed context of tests using Short() flag ==="
rg -B2 -A2 --type go "testing\.Short\(\)" ".*_test\.go$"
Length of output: 733
Script:
#!/bin/bash
# Let's try a different approach to find test files
echo "=== Finding Go test files ==="
fd "_test\.go$"
echo -e "\n=== Checking Makefile history for test-related changes ==="
git log -p -- Makefile | grep -A 5 -B 5 "go test"
Length of output: 3363
Script:
#!/bin/bash
# Let's analyze the test files for time-consuming operations and short flag usage
echo "=== Checking test files for time-consuming operations ==="
rg -C3 "time\.(Sleep|After|Tick)|context\.WithTimeout" basic_test.go batch_test.go tree_test.go mutable_tree_test.go
echo -e "\n=== Checking test files for testing.Short() usage ==="
rg -C3 "testing\.Short\(\)" basic_test.go batch_test.go tree_test.go mutable_tree_test.go
echo -e "\n=== Checking for large test data or iterations ==="
rg -C3 "for.+range|for.+:=.+;.+;" basic_test.go batch_test.go tree_test.go mutable_tree_test.go | rg "1000|10000"
Length of output: 2610
|
@Mergifyio backport release/v1.3.x |
✅ Backports have been created
|
(cherry picked from commit 256952e)
This patch makes
SetInitialVersion(0)calls valid, which previously would error duringSaveVersion. This is particularly useful from server/v2 which processes and saves a Genesis block at versioninitialHeight - 1, which is 0 in the default case.Unrelated to above is some clean up;
legacydumpis a compiled binary file and shouldn't be committed. An amd64 binary is presently in master, which doesn't run on e.g. arm64. Instead of this, removed the binary and updated theMakefileand Github action to build where required (by tests).Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation