-
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: update store module for new iavl #15568
Conversation
This comment has been minimized.
This comment has been minimized.
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.
before merging this we need an issue on how to do migrations
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.
LGTM apart from nits.
build doesnt pass |
waiting to merge this until we have migrations tested as this merge would signify the adoption of this in the Eden release |
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.
Thank you for this change @cool-develope, I've added my first round of review, please take a look.
a187b06
to
f562b91
Compare
|
||
// "restart" | ||
ms = newMultiStoreWithMounts(db, pruningtypes.NewCustomPruningOptions(2, 11)) | ||
ms.SetSnapshotInterval(3) |
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.
i think we should still keep this so we dont break pruning dependence on snapshot intervals
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.
for now, the snapshot interval doesn't affect the pruning strategy if we don't call HandleHeightSnapshot
.
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.
Looks good. Been running this on juno for a week now
lets resolve the conflicts then we can merge this |
store/CHANGELOG.md
Outdated
- Remov `DeleteVersion`, `DeleteVersions`, `LazyLoadVersionForOverwriting` from `iavl` tree API. | ||
- Add `DeleteVersionsTo` and `SaveChangeSet`, since it will keep versions sequentially like `fromVersion` to `toVersion`. | ||
- Refactor the pruning manager to use `DeleteVersionsTo`. | ||
- Remove `IAVLLazyLoading` option from baseapp. |
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.
This one should be in the main changelog ./CHANGELOG.md instead of under store/CHANGELOG.md
store/iavl/tree.go
Outdated
TraverseStateChanges(startVersion, endVersion int64, fn func(version int64, changeSet *iavl.ChangeSet) error) error | ||
SaveChangeSet(changeSet *iavl.ChangeSet) (int64, error) |
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.
This comment is still unimplemented.
if err != nil { | ||
return err | ||
// Consider the snapshot height | ||
pruneHeight := height - 1 - int64(m.opts.KeepRecent) // we should keep the current height at least |
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.
What if pruneHeight
gets negative?
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.
this will be checked in PruneStores
when pruning in rootmulti
store/go.mod
Outdated
@@ -10,7 +10,7 @@ require ( | |||
github.com/cometbft/cometbft v0.38.0-alpha.2 | |||
github.com/cosmos/cosmos-db v1.0.0 | |||
github.com/cosmos/gogoproto v1.4.10 | |||
github.com/cosmos/iavl v0.21.0 | |||
github.com/cosmos/iavl v1.0.0-beta.1 |
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.
nit, use v1.0.0-beta.2
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.
utACK, few nits
Let's get @julienrbrt's suggestions in and we can merge. The conflicts should be trivial to resolve. |
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change