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: add adr-001 for node key refactoring #608

Merged
merged 29 commits into from
Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
4f9a004
add adr
cool-develope Nov 1, 2022
8646a9a
small fix
cool-develope Nov 1, 2022
6e5b081
remove child hashes
cool-develope Nov 1, 2022
4ee7804
small fix
cool-develope Nov 1, 2022
9468ee2
add migration
cool-develope Nov 1, 2022
0c2d610
add pruning
cool-develope Nov 1, 2022
1459a18
Update docs/architecture/adr-001-node-key-refactoring.md
cool-develope Nov 1, 2022
d8d0bf9
Update docs/architecture/adr-001-node-key-refactoring.md
cool-develope Nov 1, 2022
88885f1
suggestions
cool-develope Nov 1, 2022
5272de4
suggestions
cool-develope Nov 3, 2022
006d76c
update the struct
cool-develope Nov 4, 2022
7cc7280
Update docs/architecture/adr-001-node-key-refactoring.md
cool-develope Nov 8, 2022
4e6044f
Update adr-001-node-key-refactoring.md
cool-develope Nov 8, 2022
0bf7486
orphans
cool-develope Nov 9, 2022
a0dcc0e
Merge branch 'master' into 592/adr
cool-develope Nov 9, 2022
ee11dff
revert removing root store
cool-develope Nov 10, 2022
d9f7d2e
path update
cool-develope Nov 30, 2022
10184ac
small fix
cool-develope Nov 30, 2022
5f94844
Update adr-001-node-key-refactoring.md
cool-develope Nov 30, 2022
85a90e7
Merge branch 'master' into 592/adr
cool-develope Nov 30, 2022
8fb87b6
small fix
cool-develope Dec 2, 2022
6bbf7f9
add prune method
cool-develope Dec 2, 2022
204d881
Update docs/architecture/adr-001-node-key-refactoring.md
cool-develope Jan 18, 2023
f743311
Merge branch 'master' into 592/adr
cool-develope Jan 18, 2023
bf85d92
resolve conflicts
cool-develope Feb 17, 2023
b064ede
Update adr-001-node-key-refactoring.md
cool-develope Feb 17, 2023
6095bc4
Merge branch 'master' into 592/adr
cool-develope Feb 17, 2023
7873267
Merge branch 'master' into 592/adr
cool-develope Feb 21, 2023
fb0f182
comments
cool-develope Feb 21, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/architecture/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,5 @@ If recorded decisions turned out to be lacking, convene a discussion, record the
and then modify the code to match.

## ADR Table of Contents

- [ADR 001: Node Key Refactoring](./adr-001-node-key-refactoring.md)
140 changes: 140 additions & 0 deletions docs/architecture/adr-001-node-key-refactoring.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
# ADR ADR-001: Node Key Refactoring

## Changelog

- 2022-10-31: First draft

## Status

Proposed

## Context

The original key format of IAVL nodes is a hash of the node. It does not take advantage of data locality on LSM-Tree. Nodes are stored with the random hash value, so it increases the number of compactions and makes it difficult to find the node. The new key format will take advantage of data locality in the LSM tree and reduce the number of compactions.

The `orphans` are used to manage node removal in the current design and allow the deletion of removed nodes for the specific version from the disk through the `DeleteVersion` API. It needs to track every time when updating the tree and also requires extra storage to store `orphans`. But there are only 2 use cases for `DeleteVersion`:

1. Rollback of the tree to a previous version
2. Remove unnecessary old nodes

## Decision

- Use the version and the local nonce as a node key like `bigendian(version) | bigendian(nonce)` format. Here the `nonce` is a local sequence id for the same version.
- Store the children node keys (`leftNodeKey` and `rightNodeKey`) in the node body.
- Remove the `version` field from node body writes.
- Remove the `leftHash` and `rightHash` fields, and instead store `hash` field in the node body.
- Remove the `orphans` completely from both tree and storage.

New node structure
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved

```go
type NodeKey struct {
version int64
nonce int32
}

type Node struct {
key []byte
value []byte
hash []byte // keep it in the storage instead of leftHash and rightHash
nodeKey *NodeKey // new field, the key in the storage
leftNodeKey *NodeKey // new field, need to store in the storage
rightNodeKey *NodeKey // new field, need to store in the storage
leftNode *Node
rightNode *Node
size int64
leftNode *Node
rightNode *Node
subtreeHeight int8
}
```

New tree structure

```go
type MutableTree struct {
*ImmutableTree // The current, working tree.
lastSaved *ImmutableTree // The most recently saved tree.
unsavedFastNodeAdditions map[string]*fastnode.Node // FastNodes that have not yet been saved to disk
unsavedFastNodeRemovals map[string]interface{} // FastNodes that have not yet been removed from disk
ndb *nodeDB
skipFastStorageUpgrade bool // If true, the tree will work like no fast storage and always not upgrade fast storage

mtx sync.Mutex
}
```

We will assign the `nodeKey` when saving the current version in `SaveVersion`. It will reduce unnecessary checks in CRUD operations of the tree and keep sorted the order of insertion in the LSM tree.

### Migration

We can migrate nodes through the following steps:

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Shall we remove the old node?
  2. I don't think we should update past versions. If "past" versions will be recalculated then valid proofs issued for a past version will not work any more. This could be an issue for IBC and relayers. cc: @ebuchman

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. yes, we will provide the pruning functionality, it is useful to remove the old unnecessary nodes and reduce the storage.
  2. We won't update the version itself, and just assign the nonce.

- Export the snapshot of the tree from the original version.
- Import the snapshot to the new version.
- Track the nonce for the same version using int32 array of the version length.
- Assign the `nodeKey` when saving the node.

### Pruning
Copy link
Collaborator

Choose a reason for hiding this comment

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

this section needs more details.


We assume keeping only the range versions of `fromVersion` to `toVersion`. Refer to [this issue](https://github.com/cosmos/cosmos-sdk/issues/12989).
cool-develope marked this conversation as resolved.
Show resolved Hide resolved

Here we are introducing a new way how to get orphaned nodes which remove in the `n+1`th version updates.
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved

- Traverse the tree in-order way based on the root of `n+1`th version.
- If we visit the lower version node, pick the node and don't visit further deeply. Pay attention to the order of these nodes.
- Traverse the tree in-order way based on the root of `n`th version.
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
- Iterate the tree until meet the first node among the above nodes(stack) and delete all visited nodes so far from `n`th tree.
- Pop the first node from the stack and iterate again.

If we assume `1 to (n-1)` versions already been removed, when we want to remove the `n`th version, we can just remove the above orphaned nodes.

### Rollback

When we want to rollback to the specific version `n`

- Iterate the version from `n+1`.
- Traverse key-value through `traversePrefix` with `prefix=bigendian(version)`.
- Remove all iterated nodes.

## Consequences
cool-develope marked this conversation as resolved.
Show resolved Hide resolved

### Positive

Using the version and the path, we take advantage of data locality in the LSM tree. Since we commit the sorted data, it can reduce compactions and makes it easy to find the key. Also, it can reduce the key and node size in the storage.
cool-develope marked this conversation as resolved.
Show resolved Hide resolved
cool-develope marked this conversation as resolved.
Show resolved Hide resolved

```
# node body

add `hash`: +32 byte
add `leftNodeKey`, `rightNodeKey`: max (8 + 4) * 2 = +24 byte
remove `leftHash`, `rightHash`: -64 byte
remove `version`: max -8 byte
------------------------------------------------------------
total save 16 byte

# node key

remove `hash`: -32 byte
add `version|nonce`: +12 byte
------------------------------------
total save 20 byte
```

Removing orphans also provides performance improvements including memory and storage saving.
cool-develope marked this conversation as resolved.
Show resolved Hide resolved

### Negative

The `Update` operation will require extra DB access because we need to take children to calculate the hash of updated nodes.
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean to say that it requires extra DB reads? Why is this not needed in Set and Remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because update only needs the re-calc of hash, but Set and Remove requires calcHeightAndSize (re-calculation of height and size) and this needs to splay the children nodes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so we are getting left and right in Set and Remove but not splay in Update since we have leftHash and rightHash in the original version

Copy link
Member

Choose a reason for hiding this comment

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

the trade off here is storage saving on per node basis, right? maybe we mention this as a tradeoff

cool-develope marked this conversation as resolved.
Show resolved Hide resolved
It doesn't require more access in other cases including `Set`, `Remove`, and `Proof`.
cool-develope marked this conversation as resolved.
Show resolved Hide resolved

It is impossible to remove the individual version. The new design requires more restrict pruning strategies.
Copy link
Member

Choose a reason for hiding this comment

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

can you elaborate on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is enough, it will remove orphans and it will be impossible to remove the intermediate version from storage.
And pruning part explains in more detail the new methods.

Copy link
Collaborator

@yihuang yihuang Jan 4, 2023

Choose a reason for hiding this comment

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

Even if removing orphans, it's certainly possible to delete the individual versions in between.

Copy link
Member

Choose a reason for hiding this comment

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

lets edit to include this.

cool-develope marked this conversation as resolved.
Show resolved Hide resolved

When importing the tree, it may require more memory because of int32 array of the version length. We will introduce the new importing strategy to reduce the memory usage.

## References

- https://github.com/cosmos/iavl/issues/548
- https://github.com/cosmos/iavl/issues/137
- https://github.com/cosmos/iavl/issues/571
- https://github.com/cosmos/cosmos-sdk/issues/12989