Basic repo cleanup by ValarDragon · Pull Request #43 · osmosis-labs/iavl

ValarDragon · 2022-08-20T12:20:03Z

This commit does some repository cleanup, at the moment it does:

Moves key format logic to its own package
Moves key format instantiation to its own file
Makes methods for getting key bytes no longer defined off nodeDB, and put it into their own file
Move various utils functions to a utils package
Deletes repair.go
Deletes useless ndb.roots() method, that is only used in a test to avoid checking an error lmao
Make an encoding package
Move tree-methods off of the node, to instead be defined on the tree
Move fast node to its own package
Rename node.height -> node.subtreeHeight
move ndb.SaveBranch to instead be tree.saveBranch which just calls a nodeDB.saveNode function.
Make an interface for DRY'ing the node save function with fast node save

If you want to review this, it should be reviewed commit by commit

…de db save fn

tac0turtle

Lgtm. Could we get changelog entries for things

p0mvn · 2022-08-24T16:15:08Z

docs/tree/immutable_tree.md

 Using the root and the nodeDB, the ImmutableTree can retrieve any node that is a part of the IAVL tree at this version.

-Users can get information about the IAVL tree by calling getter functions such as `Size()` and `Height()` which will return the tree's size and height by querying the root node's size and height.
+Users can get information about the IAVL tree by calling getter functions such as `Size()` and `Height()` which will return the tree's size and depth by querying the root node's size and depth.


Suggested change

Users can get information about the IAVL tree by calling getter functions such as `Size()` and `Height()` which will return the tree's size and depth by querying the root node's size and depth.

Users can get information about the IAVL tree by calling getter functions such as `Size()` and `Depth()` which will return the tree's size and depth by querying the root node's size and depth.

Would it actually be depth though? It seems to be returning "subtree height"

p0mvn · 2022-08-24T16:17:45Z

types/fast_node.go

@@ -29,13 +30,13 @@ func NewFastNode(key []byte, value []byte, version int64) *FastNode {

 // DeserializeFastNode constructs an *FastNode from an encoded byte slice.
 func DeserializeFastNode(key []byte, buf []byte) (*FastNode, error) {


Should we move the regular Node to this package as well?

It really feels like they belong together / grouped into the same package

I think they should, just takes more refactors to get there

p0mvn · 2022-08-24T16:19:33Z

utils/db_helper.go

+import dbm "github.com/tendermint/tm-db"
+
+// Traverse all keys with a certain prefix. Return error if any, nil otherwise
+func IterateThroughAllSubtreeKeys(db dbm.DB, prefix []byte, fn func(k, v []byte) error) error {


Can we have a test for this?

p0mvn · 2022-08-24T16:19:59Z

utils/db_helper.go

+	return nil
+}
+
+func GatherSubtreeKVPairs(db dbm.DB, prefix []byte) (keys [][]byte, values [][]byte, err error) {


Is this used anywhere?

p0mvn · 2022-08-24T16:20:17Z

utils/encoding.go


-// encodeBytesSlice length-prefixes the byte slice and returns it.
-func encodeBytesSlice(bz []byte) ([]byte, error) {
+// utils.EncodeBytesSlice length-prefixes the byte slice and returns it.


Suggested change

// utils.EncodeBytesSlice length-prefixes the byte slice and returns it.

// EncodeBytesSlice length-prefixes the byte slice and returns it.

p0mvn · 2022-08-24T16:20:32Z

utils/encoding.go


-// encodeVarint writes a varint-encoded integer to an io.Writer.
-func encodeVarint(w io.Writer, i int64) error {
+// utils.EncodeVarint writes a varint-encoded integer to an io.Writer.


Suggested change

// utils.EncodeVarint writes a varint-encoded integer to an io.Writer.

// EncodeVarint writes a varint-encoded integer to an io.Writer.

p0mvn · 2022-08-24T16:20:40Z

utils/encoding.go


-// encodeVarintSize returns the byte size of the given integer as a varint.
-func encodeVarintSize(i int64) int {
+// utils.EncodeVarintSize returns the byte size of the given integer as a varint.


Suggested change

// utils.EncodeVarintSize returns the byte size of the given integer as a varint.

// EncodeVarintSize returns the byte size of the given integer as a varint.

p0mvn · 2022-08-24T16:22:58Z

mutable_tree.go

+// TODO: Make better static guarantee in tree,
+// for what nodes exist in deserialized memory, and just not getting extra nodes
+// and a clear decision criteria between "no branch node", "new branch with as of yet unknown hash"
+func (tree *MutableTree) saveBranch(node *Node) []byte {


Seems this was moved but would be great to eventually cover this with a test

p0mvn · 2022-08-24T16:24:24Z

mutable_tree.go

+	if node.version <= genesisVersion {
+		tree.ndb.resetBatch()
+	}
+	// TODO: This should be deletable? Is it just here for garbage collection purposes?


I think you are right. We should probably run benchmarks before removing them. Wdyt?

p0mvn · 2022-08-24T16:24:30Z

mutable_tree.go

+	tree.ndb.SaveNode(node)
+
+	// resetBatch only working on generate a genesis block
+	// TODO: What is this? It should almost certainly be deleted / moved elsewhere.


p0mvn · 2022-08-24T16:34:19Z

immutable_tree.go

 }

-// Height returns the height of the tree.
+// Height returns the depth of the tree.


Can we rename this to SubtreeHeight() and update the docs? The docs seem to say that this returns depth

Some minimal repo cleanup

50c2850

ValarDragon force-pushed the dev/repo_cleanup branch from baa713d to 50c2850 Compare August 20, 2022 12:21

ValarDragon added 3 commits August 20, 2022 14:22

move logger code to utils

0b4aefc

Move encoding to its own package

986eeb1

Rename node.Height to depth

57c39c6

ValarDragon changed the title ~~Some minimal repo cleanup~~ Basic repo cleanup Aug 20, 2022

ValarDragon added 14 commits August 20, 2022 15:06

Make some db iterator helpers, to lower nodedb role

f79154e

Minor function reordering / renames

5d05969

Move colors to utils package

a3b101a

Move child getting methods onto tree

da9f212

Move get by index to being a tree method

97ed6b6

Migrate more node methods to immutable tree

e6e785a

Finish moving tree based node methods to tree

761fa46

fix build

bcf9c79

Move fast node struct to types package

5a98527

Move SaveBranch to mutable tree

081d428

Move test-only functions to test file

7424454

Move node & fast node writes to be under an interface, make common no…

5d7be2b

…de db save fn

rename depth -> subtreeHeight

7e5d7df

fix accidental find/replace over-matches

837ef36

tac0turtle approved these changes Aug 21, 2022

View reviewed changes

p0mvn approved these changes Aug 24, 2022

View reviewed changes

p0mvn reviewed Aug 24, 2022

View reviewed changes

comments, changelog & fastnode_migration file

49821ba

	Users can get information about the IAVL tree by calling getter functions such as `Size()` and `Height()` which will return the tree's size and depth by querying the root node's size and depth.
	Users can get information about the IAVL tree by calling getter functions such as `Size()` and `Depth()` which will return the tree's size and depth by querying the root node's size and depth.

		@@ -29,13 +30,13 @@ func NewFastNode(key []byte, value []byte, version int64) *FastNode {

		// DeserializeFastNode constructs an *FastNode from an encoded byte slice.
		func DeserializeFastNode(key []byte, buf []byte) (*FastNode, error) {

	// utils.EncodeBytesSlice length-prefixes the byte slice and returns it.
	// EncodeBytesSlice length-prefixes the byte slice and returns it.

	// utils.EncodeVarint writes a varint-encoded integer to an io.Writer.
	// EncodeVarint writes a varint-encoded integer to an io.Writer.

	// utils.EncodeVarintSize returns the byte size of the given integer as a varint.
	// EncodeVarintSize returns the byte size of the given integer as a varint.

Conversation

ValarDragon commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tac0turtle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ValarDragon commented Aug 20, 2022 •

edited

Loading