Implement LazyLoadVersion

[alexanderbez]

Implement lazy loading of a given version.

ref: cosmos/cosmos-sdk#4326

[alexanderbez]

Let me know if this looks correct @liamsi or if there are any cases I need to be aware of. I've successfully tested this in Gaia.

[melekes]

🌟

[liamsi]

The changes look good to me 👍 My only concern is that this adds yet another public method to the API and as a user it might not be clear when to use LazyLoadVersion over LoadVersion. A test that showcases the different behaviour, or a comment in the godoc when to use which method might help here. I'm also wondering if LazyLoadVersion shouldn't be the default (and only) method.

[alexanderbez]

@liamsi that's a good point. And as you've brought up, I can't think of a reason why one would use LoadVersion() over LazyLoadVersion(). It's not immediately clear to me why you'd need to load all the roots -- I was hoping you'd have more context or a clue here.

If we are in agreement, how about we replace LoadVersion with LazyLoadVersion?

[liamsi]

Sorry, I don't have any more context on this. Maybe @jaekwon (or @ethanfrey) have?

If we are in agreement, how about we replace LoadVersion with LazyLoadVersion?

As far as I understand it does not make sense to load all the roots. I'm OK with replacing LoadVersion with LazyLoadVersion.

[zmanian]

Okay I am not 100% sure we should replace LoadVersion()

LoadVersion is not DOSable. LazyLoadVersion is. Ie. Once you load a tree with LoadVersion() the tree cannot be DOSed by creating a query pattern that requires loading lots of different tree versions to bring the node to a halt.

LoadVersion() can be used safely with a pruning node and queries against snapshots.

LazyLoadVersion() opts a node operator into new risks if the tendermint rpc is world accessible.

[alexanderbez]

Valid concerns @zmanian. From doing another pass through of the IAVL implementation and it's relation/usage in the SDK's multistore (I've done this now a few times), I don't quite see where a DOS attack vector could occur? I could very well be wrong here. Here's what I can tell:

• Height queries always call ndb.getRoot() regardless
• Root node lookups ndb.getNode() can be cached (regardless of which method we call)
• The only difference between LoadVersion and LazyLoadVersion is the former reads all the roots AND sets them in tree.versions[version].
• I don't see how tree.versions would correlate to any DOSing or the prevention thereof. It's mainly only used when it comes to deleting.

[zmanian]

I'm in favor of replacing LoadVersion entirely at the movement.

[alexanderbez]

Ok so actually taking a stab at replacing LoadVersion with LazyLoadVersion, I see why the need for LoadVersion exists:

The original LoadVersion() loads all the roots and as a result, when you load previous committed/saved versions and then try to overwrite a value and save, you'll get an error (as you should). This obviously does not hold with LazyLoadVersion. I'm sure there are other cases similar to this.

That being the case, I'm going to leave LoadVersion as-is and keep LazyLoadVersion and update the godoc to state it should only be used in cases where only reads are necessary, not writes. In fact, this was the original motivation/use-case.