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

feat: implement the lazy_set feature #750

Merged
merged 15 commits into from
May 19, 2023
Merged

feat: implement the lazy_set feature #750

merged 15 commits into from
May 19, 2023

Conversation

cool-develope
Copy link
Collaborator

Closes: #747

  • It will allow loading the tree from the legacy version (old format tree).
  • all new nodes will be written in a new format (node key format) after loading it.

@cool-develope cool-develope requested a review from a team as a code owner April 26, 2023 16:52
@cool-develope cool-develope requested a review from yihuang April 26, 2023 18:51
nodedb.go Show resolved Hide resolved
migrate_test.go Outdated Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

overall changes look good to me, would like a few more approvals before merging

nodedb.go Outdated Show resolved Hide resolved
## Usage

It takes 4 arguments:
- dbtype: the type of database to use.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can dbtype be inferred from the database?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe, the purpose of this tool is to generate the legacy tree, it can be used in benchmarking and dbtype is useful to compare different db performance

cmd/dbgenerator/README.md Outdated Show resolved Hide resolved
cmd/dbgenerator/README.md Outdated Show resolved Hide resolved
cmd/dbgenerator/README.md Outdated Show resolved Hide resolved
cmd/dbgenerator/go.mod Outdated Show resolved Hide resolved
node.go Outdated Show resolved Hide resolved
node.go Outdated Show resolved Hide resolved
node.go Outdated Show resolved Hide resolved
node.go Show resolved Hide resolved
nodedb.go Outdated Show resolved Hide resolved
@tac0turtle
Copy link
Member

@cool-develope seems some tests are still failing

@cool-develope
Copy link
Collaborator Author

@cool-develope seems some tests are still failing

it seems like the dbdir removing is not working properly in GA

@@ -21,7 +21,7 @@ type KVPairReceiver func(pair *KVPair) error
//
// The algorithm don't run in constant memory strictly, but it tried the best the only
// keep minimal intermediate states in memory.
func (ndb *nodeDB) extractStateChanges(prevVersion int64, prevRoot *NodeKey, root *NodeKey, receiver KVPairReceiver) error {
func (ndb *nodeDB) extractStateChanges(prevVersion int64, prevRoot, root []byte, receiver KVPairReceiver) error {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a big fan of this change in particular. Actually it's what I originally recommended. #676 (review)

leftNodeKey *NodeKey
rightNodeKey *NodeKey
leftNodeKey []byte
rightNodeKey []byte
Copy link
Member

Choose a reason for hiding this comment

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

🤩

Copy link
Member

@kocubinski kocubinski May 15, 2023

Choose a reason for hiding this comment

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

I propose taking this further. Why not define nodeKey []byte and store version and nonce as private, top-level fields on Node?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[]byte nodeKey is meaningless, leftNodeKey and rightNodeKey is used in getting children
The only thing we need is nonce and version

Copy link
Member

Choose a reason for hiding this comment

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

NodeKey seems also without meaning, just makes the code harder to read. if we need nonce and version why not put it on Node directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good question, the nonce and version are used together in several encodings, so I created the specific struct NodeKey
and also it ensures version+nonce as a node key

nodedb.go Show resolved Hide resolved
@kocubinski
Copy link
Member

@cool-develope seems some tests are still failing

it seems like the dbdir removing is not working properly in GA

we should probably use t.TmpDir() #750 (comment)

migrate_test.go Outdated
}
}

cmd := exec.Command("sh", "-c", fmt.Sprintf("cd cmd/dbgenerator && go run . %s %s random %d %d", dbType, dbDir, version, version/2)) //nolint:gosec
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we import it as library, since it's all golang code here

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 will cause circular dependency since we are in v1, go.mod doesn't analyze the iavl imports

Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

LGTM, the only concern is the uint32 nonce overflow in genesis, but there's still a long way to go before production data reach that limit, and we can probably find some workarounds for genesis version in the future.

@tac0turtle tac0turtle enabled auto-merge (squash) May 19, 2023 19:32
@cool-develope
Copy link
Collaborator Author

cool-develope commented May 19, 2023

@tac0turtle , it seems like 5mins is not enough for test since I added migrate_test.go which requires long running

@tac0turtle tac0turtle merged commit f706bde into master May 19, 2023
@tac0turtle tac0turtle deleted the lazy_set branch May 19, 2023 20:12
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.

Implement a lazy set for the migration to the new node key format
6 participants