Skip to content
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

chore: backport the pruning fix PR to v1.1.x #910

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

cool-develope
Copy link
Collaborator

ref: #909

Copy link
Contributor

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the test failure in CI, I have tested this on Osmosis mainnet and have had positive results.

return err
}
if err := b.batch.Close(); err != nil {
b.mtx.Unlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come there is a need for the extra mutexs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is related to the legacy node pruning, it runs in background

@tac0turtle
Copy link
Member

can you add a changelog since its changing things

@@ -71,6 +71,7 @@ type Node struct {
leftNode *Node
rightNode *Node
subtreeHeight int8
isLegacy bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
isLegacy bool
isV0Node bool

or similar. legacy is ambiguous?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are using the legacy term from design stage, just worried if there are many things have to change

// 1.1.0-<version of the current live state>. Returns error if storage version is incorrect or on
// db error, nil otherwise. Requires changes to be committed after to be persisted.
func (ndb *nodeDB) setFastStorageVersionToBatch(latestVersion int64) error {
func (ndb *nodeDB) SetFastStorageVersionToBatch(latestVersion int64) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this now exported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all APIs which are used outside of nodeDB is exported, the function export rules are messy in the current imp, but just following the current imp

@czarcas7ic
Copy link
Contributor

czarcas7ic commented Mar 14, 2024

I wanted to point out that when I implemented this IAVL branch on cosmos sdk, some tests started failing (one test that I checked is exactly the same as the test in v0.50, so it seems to not be due to me being on v0.47). These tests are:

TestMultistoreLoadWithUpgrade

TestQueryABCIHeight

TestSetLoader

They seem to be non deterministic as well, so if I rerun there are likely more than just these.

@tac0turtle
Copy link
Member

I wanted to point out that when I implemented this IAVL branch on cosmos sdk, some tests started failing (one test that I checked is exactly the same as the test in v0.50, so it seems to not be due to me being on v0.47). These tests are:

TestMultistoreLoadWithUpgrade

TestQueryABCIHeight

TestSetLoader

They seem to be non deterministic as well, so if I rerun there are likely more than just these.

we will look into this prior to tag, thanks for the heads up

@cool-develope
Copy link
Collaborator Author

I wanted to point out that when I implemented this IAVL branch on cosmos sdk, some tests started failing (one test that I checked is exactly the same as the test in v0.50, so it seems to not be due to me being on v0.47). These tests are:

TestMultistoreLoadWithUpgrade

TestQueryABCIHeight

TestSetLoader

They seem to be non deterministic as well, so if I rerun there are likely more than just these.

@czarcas7ic please check the new commit

@czarcas7ic
Copy link
Contributor

Tests now pass @tac0turtle @cool-develope

@tac0turtle tac0turtle merged commit ca1d22d into release/v1.1.x Mar 14, 2024
5 checks passed
@tac0turtle tac0turtle deleted the backport/v1.1.x_pruning_ref branch March 14, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants