fix: iavl live migration of orphan nodes #866
Conversation
This reverts commit 6444ca0.
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-go@v5 | ||
| - name: Check out repository code | ||
| uses: actions/checkout@v4 | ||
| - name: 🐿 Setup Golang | ||
| uses: actions/setup-go@v4 | ||
| with: | ||
| go-version: '^1.20.0' | ||
| go-version: 1.21 | ||
| - name: golangci-lint | ||
| uses: golangci/golangci-lint-action@v3 | ||
| with: | ||
| version: v1.51.2 |
There was a problem hiding this comment.
This is a drive by change, the linter was not working on any open PR
czarcas7ic
changed the title
WalkthroughThe project has undergone a significant update focused on improving performance, code organization, and maintainability. The Golang version was upgraded, and a custom linting command was introduced. Thread safety in Changes
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? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // cloneNoChildFetch clones a node without fetching its children. | ||
| func (node *Node) cloneNoChildFetch() (*Node, error) { | ||
| if node.isLeaf() { | ||
| return nil, ErrCloneLeafNode | ||
| } | ||
| // match compatibility with old by clearing original node's pointer ref's. I don't really get why this is needed. | ||
| var leftNode, rightNode *Node | ||
| if node.nodeKey != nil { | ||
| leftNode, rightNode = node.leftNode, node.rightNode | ||
| node.leftNode, node.rightNode = nil, nil | ||
| } | ||
| return &Node{ | ||
| key: node.key, | ||
| subtreeHeight: node.subtreeHeight, | ||
| size: node.size, | ||
| hash: nil, | ||
| nodeKey: nil, | ||
| leftNode: leftNode, | ||
| rightNode: rightNode, | ||
| leftNodeKey: node.leftNodeKey, | ||
| rightNodeKey: node.rightNodeKey, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The cloneNoChildFetch method correctly clones a node without fetching its children, which can be useful for performance optimization. However, the comment on line 331 about clearing the original node's pointer references is unclear and should be explained or removed if it's not relevant.
- // match compatibility with old by clearing original node's pointer ref's. I don't really get why this is needed.
+ // Clearing the original node's pointer references to avoid unintended side effects during cloning.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.
| // cloneNoChildFetch clones a node without fetching its children. | |
| func (node *Node) cloneNoChildFetch() (*Node, error) { | |
| if node.isLeaf() { | |
| return nil, ErrCloneLeafNode | |
| } | |
| // match compatibility with old by clearing original node's pointer ref's. I don't really get why this is needed. | |
| var leftNode, rightNode *Node | |
| if node.nodeKey != nil { | |
| leftNode, rightNode = node.leftNode, node.rightNode | |
| node.leftNode, node.rightNode = nil, nil | |
| } | |
| return &Node{ | |
| key: node.key, | |
| subtreeHeight: node.subtreeHeight, | |
| size: node.size, | |
| hash: nil, | |
| nodeKey: nil, | |
| leftNode: leftNode, | |
| rightNode: rightNode, | |
| leftNodeKey: node.leftNodeKey, | |
| rightNodeKey: node.rightNodeKey, | |
| }, nil | |
| } | |
| // cloneNoChildFetch clones a node without fetching its children. | |
| func (node *Node) cloneNoChildFetch() (*Node, error) { | |
| if node.isLeaf() { | |
| return nil, ErrCloneLeafNode | |
| } | |
| // Clearing the original node's pointer references to avoid unintended side effects during cloning. | |
| var leftNode, rightNode *Node | |
| if node.nodeKey != nil { | |
| leftNode, rightNode = node.leftNode, node.rightNode | |
| node.leftNode, node.rightNode = nil, nil | |
| } | |
| return &Node{ | |
| key: node.key, | |
| subtreeHeight: node.subtreeHeight, | |
| size: node.size, | |
| hash: nil, | |
| nodeKey: nil, | |
| leftNode: leftNode, | |
| rightNode: rightNode, | |
| leftNodeKey: node.leftNodeKey, | |
| rightNodeKey: node.rightNodeKey, | |
| }, nil | |
| } |
| return fmt.Errorf("writing mode, %w", err) | ||
| } | ||
| if mode&ModeLegacyLeftNode != 0 { // legacy leftNodeKey | ||
| err = encoding.EncodeBytes(w, node.leftNodeKey) | ||
| err = encoding.Encode32BytesHash(w, node.leftNodeKey) | ||
| if err != nil { | ||
| return fmt.Errorf("writing the legacy left node key, %w", err) | ||
| } |
There was a problem hiding this comment.
The update to the writeBytes method to encode the leftNodeKey when in legacy mode is correct. However, the error handling should be consistent with the rest of the codebase. Ensure that the error messages are informative and consistent with the project's error handling conventions.
- return fmt.Errorf("writing the legacy left node key, %w", err)
+ return fmt.Errorf("error writing the legacy left node key: %w", err)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.
| return fmt.Errorf("writing mode, %w", err) | |
| } | |
| if mode&ModeLegacyLeftNode != 0 { // legacy leftNodeKey | |
| err = encoding.EncodeBytes(w, node.leftNodeKey) | |
| err = encoding.Encode32BytesHash(w, node.leftNodeKey) | |
| if err != nil { | |
| return fmt.Errorf("writing the legacy left node key, %w", err) | |
| } | |
| return fmt.Errorf("writing mode, %w", err) | |
| } | |
| if mode&ModeLegacyLeftNode != 0 { // legacy leftNodeKey | |
| err = encoding.Encode32BytesHash(w, node.leftNodeKey) | |
| if err != nil { | |
| return fmt.Errorf("error writing the legacy left node key: %w", err) | |
| } |
| return ErrRightNodeKeyEmpty | ||
| } | ||
| if mode&ModeLegacyRightNode != 0 { // legacy rightNodeKey | ||
| err = encoding.EncodeBytes(w, node.rightNodeKey) | ||
| err = encoding.Encode32BytesHash(w, node.rightNodeKey) | ||
| if err != nil { | ||
| return fmt.Errorf("writing the legacy right node key, %w", err) | ||
| } |
There was a problem hiding this comment.
Similarly, the update to encode the rightNodeKey when in legacy mode is correct. The same considerations for error handling and message consistency apply here as well.
- return fmt.Errorf("writing the legacy right node key, %w", err)
+ return fmt.Errorf("error writing the legacy right node key: %w", err)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.
| return ErrRightNodeKeyEmpty | |
| } | |
| if mode&ModeLegacyRightNode != 0 { // legacy rightNodeKey | |
| err = encoding.EncodeBytes(w, node.rightNodeKey) | |
| err = encoding.Encode32BytesHash(w, node.rightNodeKey) | |
| if err != nil { | |
| return fmt.Errorf("writing the legacy right node key, %w", err) | |
| } | |
| return ErrRightNodeKeyEmpty | |
| } | |
| if mode&ModeLegacyRightNode != 0 { // legacy rightNodeKey | |
| err = encoding.Encode32BytesHash(w, node.rightNodeKey) | |
| if err != nil { | |
| return fmt.Errorf("error writing the legacy right node key: %w", err) | |
| } |
|
This needs to be reopened with the proper changes just haven't had a moment to do so |
This is cherry picks of work from both John Reynolds and Dev to ensure main stays up to date with the iavl v1.0.0 branch we will be using in v21 of Osmosis.
Previously, we would block chain progress on pruning orphan nodes. For a chain like Osmosis, this took upwards of 2 hours. This change allows chain progress to continue, while pruning orphan nodes synchronously.
As a drive by change, this also fixes the golang linter in this repo.