Make iavl.Tree unexported

[liamsi]

The VersionedTree should be sufficient for people to work with. We currently use the Tree only in the cosmos-sdk iavlstore. It would be nice if we could change this there. It would further simplify the exposed API.

There are some test-cases showing even our devs are confused by how to use the tree vs. the versioned tree: #50

cc @jlandrews

also related #62

[silasdavis]

This would break my usage, see where we need access to the tree to store a read tree: https://github.com/hyperledger/burrow/blob/develop/execution/state.go#L202-L218

Also I think IAVL could do with more not fewer things exported. It is a low-level building block and there are various things I have not be able to do because the policy around exporting is in my opinion overly conservative. For example, the nodedb. The Tree itself can be used standalone, and it seem odd to me that VersionedTree does not provide a straightforward way to get previous versions of the Tree. See also my other issue: #68. I do understand the desire to limit the public API for a project, but I think for a building block like IAVL supporting extension and composability (in ways that you may not have in mind when writing the code) is a higher ideal in the case of a library like this.

I think a small amount of additional documentation is sufficient to clear up any confusion.

[ebuchman]

I think we probably need to package up the different pieces ala #62 and expose solid APIs from each package

[UnitylChaos]

@silasdavis So it seems to me like the most straightforward solution here would be to build a well structured way to get immutable snapshots of the tree at specific stored versions, in such a way that they can be loaded on demand and not leak memory. Then we would make Tree just an internal interface for updating the VersionedTree and we could make a separate read only interface which had all the same features of Tree, like iterators and proofs.

I agree on the motivation of wanting to simplify the exposed API, and also can see the downside of doing that in situations where the exposed API is not enough for a particular use case. My view of the current intended API is that VersionedTree provides a mutable map primitive, with the ability to persist particular configurations to disk with incremented version numbers. It's entirely linear though, we can't load an old version, make new changes and save that as a different version n+1. In that frame, it shouldn't be possible to gain a mutable reference to an old version, it should only be possible to load the current mutable version, or to load immutable snapshots of old versions.

[silasdavis]

@jlandrews immutable snapshots would satisfy my current use case yes, but still Tree is a useful type and I I think it's better to have it available - perhaps you can restructure VersionedTree to make sure its internal Tree fields are suitably hidden.

I do agree with you on:

it shouldn't be possible to gain a mutable reference to an old version, it should only be possible to load the current mutable version, or to load immutable snapshots of old versions

Having VersionedTree work as an append-only thing with no rewriting history is a nice feature for VersionedTree

[silasdavis]

I think this is resolved.

[liamsi]

Agreed