feat: add adr-001 for node key refactoring

[cool-develope]

ref: #592, #593, #597

[tac0turtle]

a good question by celestia, how does this effect the commitment structure? Are proofs still the same?

[cool-develope]

a good question by celestia, how does this effect the commitment structure? Are proofs still the same?

it doesn't affect, as you can see in #597 it doesn't update any proof part and still passing tests

[tzdybal]

a good question by celestia, how does this effect the commitment structure? Are proofs still the same?

it doesn't affect, as you can see in #597 it doesn't update any proof part and still passing tests

#597 still contains version field, which is still hashed (see:

iavl/node.go

Line 364 in 36a150e

err = encoding.EncodeVarint(w, node.version)

). Removing version will affect all the hashes, and therefore migration will invalidate all existing proofs.

[cool-develope]

Removing version will affect all the hashes, and therefore migration will invalidate all existing proofs.

@tzdybal , good catch. @tac0turtle how about your thougt? Will we keep the version or refactor the ics23 part?

[tac0turtle]

@tzdybal , good catch. @tac0turtle how about your thougt? Will we keep the version or refactor the ics23 part?

what do you mean by refactor ics23?

So there are two paths, introduce this in a breaking release of the sdk which a coordinated upgraded is needed or leave it alone. for testing purposes and data collection we probably want to leave it but in the future we could break it. BUT we should check with IBC about this.

[cool-develope]

what do you mean by refactor ics23?

I mean update the validation part in ics23.

So there are two paths, introduce this in a breaking release of the sdk which a coordinated upgraded is needed or leave it alone. for testing purposes and data collection we probably want to leave it but in the future we could break it. BUT we should check with IBC about this.

Sounds good, then let's keep the version at this time. I will create the issue and we can revisit in the future

[alexanderbez]

Why do we need these fields, if we already have leftNode and rightNode?

[cool-develope]

leftNode and rightNode is only meaningful on memory side, we need these fields to get children from the storage side.

[aaronc]

I'm unclear as to how an integer improves data locality. Can you explain this a bit more? It would seem that it puts nodes that are created at similar times near each other in the B-tree, but this doesn't actually improve things like range scans other than by reducing the size of the node key which is significant.

[cool-develope]

I'm unclear as to how an integer improves data locality. Can you explain this a bit more? It would seem that it puts nodes that are created at similar times near each other in the B-tree, but this doesn't actually improve things like range scans other than by reducing the size of the node key which is significant.

Reducing key size itself improves data locality. I am not sure if leveldb uses compressed trie or general bTree, but we can reduce the node count and improve the tree density.

[yihuang]

A different nonce assignment strategy can be useful in for example migration, we can do a sequential iteration on existing db, should be much faster than doing random access to find the nodes version by version

The migration would be complicated, it requires a more detail design. I don't think we can keep the current import/export structure in the new design.

https://github.com/cosmos/iavl/blob/master/export.go#L21
The current export node is already nonce agnostic, it just rely on a post-order traversal of the tree, I think we can keep that, so the nonce assignment strategy can be a node local decision.

[tac0turtle]

can you elaborate on this?

[cool-develope]

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.

[yihuang]

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

[tac0turtle]

lets edit to include this.

[yihuang]

Another round of review:

Keep order of insertion sorted

when there are multiple stores in the same db, the insertion is not sorted by keys anymore.

Migration

You also need to record the mapping of hash -> new node key to update the references in node body.

Pruning

We'll need both in-order and pre-order somehow, in-order to keep the key order, pre-order to skip subtree early on.

Remove the individual version

It's still possible to remove individual version with new pruning alrogithm, we just need to be aware of predecessor version and avoid deleting nodes older than that.

Export Snapshot

Can we clarify that the new node key format don't affect the snapshot export format? so the nonce assignment strategy is not part of consensus, but a node local decision.

[aaronc]

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

[cool-develope]

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

[cool-develope]

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

[tac0turtle]

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