-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(store/v2): add support for iavl/v2 #22424
Open
kocubinski
wants to merge
11
commits into
main
Choose a base branch
from
kocu/iavl-v2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
7b89c8f
feat(store/v2): iavl/v2 wrapper
kocubinski a837b9b
go mod tidy all
kocubinski 906144b
rm iavl v1 replace
kocubinski ee21c4a
go mod tidy all
kocubinski 063a18f
fix test and potential faulty uint64 conversion
kocubinski c3415de
fix iavl/v2 version and method receivers
kocubinski 56ea0e5
some clean up
kocubinski 200f7dd
clean up error handling in Batch.Write()
kocubinski e1f0574
reuse uint64 check
kocubinski bb2eeec
add integer overflow guards in iavl v2 wrapper
kocubinski 2f7ef1e
fix prune rollback
kocubinski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
package iavlv2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please add the tree test like iavl v1? |
||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
|
||
"github.com/cosmos/iavl/v2" | ||
ics23 "github.com/cosmos/ics23/go" | ||
|
||
"cosmossdk.io/store/v2" | ||
"cosmossdk.io/store/v2/commitment" | ||
) | ||
|
||
var ( | ||
_ commitment.Tree = (*Tree)(nil) | ||
_ store.PausablePruner = (*Tree)(nil) | ||
) | ||
|
||
type Tree struct { | ||
tree *iavl.Tree | ||
} | ||
|
||
func NewTree(treeOptions iavl.TreeOptions, dbOptions iavl.SqliteDbOptions, pool *iavl.NodePool) (*Tree, error) { | ||
sql, err := iavl.NewSqliteDb(pool, dbOptions) | ||
if err != nil { | ||
return nil, err | ||
} | ||
tree := iavl.NewTree(sql, pool, treeOptions) | ||
return &Tree{tree: tree}, nil | ||
} | ||
|
||
func (t Tree) Set(key, value []byte) error { | ||
_, err := t.tree.Set(key, value) | ||
return err | ||
} | ||
kocubinski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
func (t Tree) Remove(key []byte) error { | ||
_, _, err := t.tree.Remove(key) | ||
return err | ||
} | ||
|
||
func (t Tree) GetLatestVersion() (uint64, error) { | ||
return uint64(t.tree.Version()), nil | ||
} | ||
|
||
func (t Tree) Hash() []byte { | ||
return t.tree.Hash() | ||
} | ||
|
||
func (t Tree) Version() uint64 { | ||
return uint64(t.tree.Version()) | ||
} | ||
|
||
func (t Tree) LoadVersion(version uint64) error { | ||
// TODO fix this in iavl v2 | ||
kocubinski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if version == 0 { | ||
return nil | ||
} | ||
return t.tree.LoadVersion(int64(version)) | ||
} | ||
|
||
func (t Tree) Commit() ([]byte, uint64, error) { | ||
h, v, err := t.tree.SaveVersion() | ||
return h, uint64(v), err | ||
} | ||
|
||
func (t Tree) SetInitialVersion(version uint64) error { | ||
t.tree.SetShouldCheckpoint() | ||
return t.tree.SetInitialVersion(int64(version)) | ||
} | ||
|
||
func (t Tree) GetProof(version uint64, key []byte) (*ics23.CommitmentProof, error) { | ||
return t.tree.GetProof(int64(version), key) | ||
} | ||
|
||
func (t Tree) Get(version uint64, key []byte) ([]byte, error) { | ||
if int64(version) != t.tree.Version() { | ||
return nil, fmt.Errorf("loading past version not yet supported") | ||
} | ||
return t.tree.Get(key) | ||
} | ||
kocubinski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
func (t Tree) Export(version uint64) (commitment.Exporter, error) { | ||
return nil, errors.New("snapshot import/export not yet supported") | ||
} | ||
|
||
func (t Tree) Import(version uint64) (commitment.Importer, error) { | ||
return nil, errors.New("snapshot import/export not yet supported") | ||
} | ||
|
||
func (t Tree) Close() error { | ||
return t.tree.Close() | ||
} | ||
|
||
func (t Tree) Prune(version uint64) error { | ||
return t.tree.DeleteVersionsTo(int64(version)) | ||
} | ||
|
||
// PausePruning is unnecessary in IAVL v2 due to the advanced pruning mechanism | ||
func (t Tree) PausePruning(bool) {} | ||
kocubinski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
func (t *Tree) WorkingHash() []byte { | ||
return t.tree.Hash() | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Consider using a more actively maintained SQLite library
The current choice of
github.com/bvinc/go-sqlite-lite v0.6.1
raises several concerns:Consider evaluating alternative SQLite libraries with explicit locking mechanisms that are actively maintained, such as:
modernc.org/sqlite
github.com/glebarez/go-sqlite
🔗 Analysis chain
Verify the SQLite library migration.
The addition of
github.com/bvinc/go-sqlite-lite v0.6.1
aligns with the PR objective to migrate frommattn/go-sqlite3
. However, let's verify if v0.6.1 is the latest stable version and if there are any known issues.Let me gather more information about the issues and the repository activity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 355
Script:
Length of output: 2015
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fair point, but I personally favor how
bvinc/go-sqlite-lite
is a very minimal wrapper around the original libc API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kocubinski, understood. Thank you for explaining your preference for
bvinc/go-sqlite-lite
due to its minimal wrapper around the original libc API. I'll keep this in mind for future reviews.✏️ Learnings added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modernc.org/sqlite could be a good fit since it's a pure go implementation (won't conflict with iavl/v2) and maybe even more performant than the C one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kocubinski, that's a great point. Modernc.org/sqlite, being a pure Go implementation, could indeed avoid conflicts with iavl/v2 and potentially offer better performance.
From the information gathered:
It seems like a promising alternative worth evaluating. Let me know if you need any assistance with this.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 2119
✏️ Learnings added