Only load to target version when target version doesn't equal to 0, hi… #102

HaoyangLiu · 2018-08-16T06:25:44Z

Currently, the versions in MutableTree contains all previously saved versions of the tree.

type MutableTree struct {
	*ImmutableTree                  // The current, working tree.
	lastSaved      *ImmutableTree   // The most recently saved tree.
	orphans        map[string]int64 // Nodes removed by changes to working tree.
	versions       map[int64]bool   // The previous, saved versions of the tree.
	ndb            *nodeDB
}

The LoadVersion of MutableTree will mark all existing version to be true. Suppose this situation, current tree version is 10, but I want to load to 9. Then versions[10] will be marked true too. However, I think this is not reasonable. Suppose I want to rewrite the latest version 10 to different value, if version[10] is true, then the new value can't be overwritten. But sometimes overwriting is necessary.

func (tree *MutableTree) LoadVersion(targetVersion int64) (int64, error) {
	roots, err := tree.ndb.getRoots()
	if err != nil {
		return 0, err
	}
	if len(roots) == 0 {
		return 0, nil
	}
	latestVersion := int64(0)
	var latestRoot []byte
	for version, r := range roots {
		tree.versions[version] = true
		if version > latestVersion &&
			(targetVersion == 0 || version <= targetVersion) {
			latestVersion = version
			latestRoot = r
		}
	}
        ...

Besides, the above code is the current code for LoadVersion. It has another bug. The variable roots is a map. In golang, map iteration is random. As a result, maybe some lower versions can't be loaded. My PR also fixed this bug. The traversePrefix will return a ascending list.
@liamsi

…gher version won't be loaded

HaoyangLiu · 2018-08-21T11:50:54Z

Hi @liamsi , could you take a look at this PR? Thank very much

liamsi · 2018-08-21T13:50:51Z

Thanks a lot for your pull request!

But sometimes overwriting is necessary.

Can you describe the usecase you are looking at? Will have a closer look today!

silasdavis · 2018-08-21T14:27:36Z

Yes... having a LoadVersion(version) does not make much sense if we cannot SaveVersion() without an error on any version other than the latest. In particular now that we have GetImmutable it makes less sense.

An example of a use case is when you want to implement state rollback to a specific height (c.f. git reset --hard). I plan to implement this and had been assuming IAVL would let me do it. This can be very useful operationally in the presence of bugs or disk corruption on a node and might also being a part of managing forking of chain state (e.g. copy state, rollback to height N, and resume).

Another use case is to provide atomicity of a commit at the software level without using a databse transaction. For example I first commit my IAVL state at version N that gives me a hash, I then store a mapping from that hash -> N, before finally saving my blockchain state (atomically) with a reference to that hash. If anything goes wrong my last committed blockchain state will point to a hash that points to version N-1 and I can just resume from that version. Any intermediate writes don't appear as inconsistent state because they are not committed and I will overwrite version N of the tree during my next commit (provided I am allowed to do so).

It makes sense to be cautious about overwriting state that is meant to be immutable, but I think in this case given that we have Load() that is immutable and safe it can be made clear with a comment that LoadVersion() is intended to allow you to overwrite your history.

However there is an issue with this in that suppose we rollback from version N to version M and then unload the tree (i.e. stop our node) at version L with M <= L < N. If we restart and call Load() we will load version N which is no longer part of a consistent history, i.e. [M, L] has been overwritten with new history but (L, N] contains the future from the old history. This feels like breaks the provision of this library to remember where it got to. If you chose to load an old version I don't think this is the end of the world, but we could consider explicitly clearing any newer versions when loading an older version. We might then rename LoadVersion to LoadVersionOverwrite or something to emphasise the fact.

silasdavis · 2018-08-21T14:40:46Z

mutable_tree.go

 	latestVersion := int64(0)
 	var latestRoot []byte
-	for version, r := range roots {
-		tree.versions[version] = true
+	tree.ndb.traversePrefix([]byte(rootPrefix), func(k, v []byte) {


We ought to be able to avoid traversing the entire prefix here. nodedb should expose an traverseRange which in turn can use db.Iterator([]byte(rootPrefix), ndb.rootKey(targetVersion)) since we know the exact end point.

Come to think of it - do we really need even need to maintain a set of versions that exist? The only place this set is used is to 'fail early' when trying to get a previous tree version. GetImmutable will throw an error if the version is not found. I do not see why that isn't generally good enough. Then we could avoid this traversal completely (other than in loading the latest version). My feeling is loading/checking previous versions is not something that needs to be optimised with a cache - seems better just to check the database when necessary.

Comos-sdk use IAVL to implement its fundamental store. It implements prune strategy:

https://github.com/cosmos/cosmos-sdk/blob/2c6ed0fefc499410a7734ac93005f32a99b47b5b/store/iavlstore.go#L104

and

https://github.com/cosmos/cosmos-sdk/blob/2c6ed0fefc499410a7734ac93005f32a99b47b5b/store/iavlstore.go#L82

So the entire prefix here will not be too huge. But if some developers don't want to implement prune strategy here it is necessary to expose an traverseRange.
Thanks for your impressive comment.

I currently don't prune (for reference I'm using it here: https://github.com/hyperledger/burrow/blob/develop/execution/state.go).

I'm not terribly worried about the size - it does load fairly quickly.

But my afterthought is probably now my main point: I don't think we need the versions set at all, as in, I think we could remove this field: https://github.com/tendermint/iavl/blob/master/mutable_tree.go#L19

Then all we need to do is load targetVersion. Later on rather than checking our versions set we can just handle this error gracefully: https://github.com/tendermint/iavl/blob/master/mutable_tree.go#L270. It means less loading up front - and I suspect most old versions never get accessed again.

For loading the latest version we'll need to traverse anyway.

Would be happy to implement this in a subsequent PR though - what you have here is essential for my use case.

I'm glad my pull request is helpful for your work. Thanks.

HaoyangLiu · 2018-08-21T15:50:11Z

@liamsi
Taking cosmos-sdk for example, suppose we have 4 gaiad nodes, and we have come to a consensus: upgrade gaiad at height 10000. However, before height 10000, only three nodes have been replaced by the upgraded gaiad. So at height 10000, the left node will generate wrong appHash. As a result, the left node can't come to consensus at height 10001. Now for the left node, it is necessary to replay at height 10000 and overwrite the IAVL store at this height. During the replay process, we only load store to height 9999. Once the correct replay result is figured out, we will overwrite the IAVL store at height 10000.
Am I clear here？
The above is a real use case.

liamsi · 2018-08-24T18:14:48Z

Hey @HaoyangLiu @silasdavis. Thank you both for all the input and sorry for letting this slip.

While the changes made look great and the use-cases you describe make perfect sense, we need to make sure that this doesn't break anything. I think this suggestion made by @silasdavis should maybe land in this PR too:

if you chose to load an old version I don't think this is the end of the world, but we could consider explicitly clearing any newer versions when loading an older version. We might then rename LoadVersion to LoadVersionOverwrite or something to emphasise the fact.

/cc @jlandrews @jaekwon

HaoyangLiu · 2018-08-27T03:24:31Z

@liamsi @silasdavis
According to your suggestion, I have commit a new change. Please take a look at it.

jaekwon · 2018-08-27T23:56:33Z

I think we should refactor getRoots() to return a slice of roots.

As is, we still have getRoots() which is only used in once place, a test case (via .roots()), and we have 2 very similar iteration functions... so basically we have 3 functions that iterate over the roots.

Then in LoadVersionOverwrite I'd just iterate over the slice and work with that.

I'd rename LoadVersionOverwrite to LoadVersionForOverwriting, but overall sounds like a good idea.

Just FYI I don't think there was a problem with the previous usage of getRoots(), it seems like the way we've been using it, random iteration wasn't a problem. But using an ordered slice is a bit simpler so...

jaekwon · 2018-08-28T00:10:02Z

@liamsi makes a good point that this probably messes with orphan tracking. If we're going to overwrite, we need to also clean up everything related to versions after the version passed w/ LoadVersionForOverwriting. The index is implemented in nodedb.go, where you can see the comment:

    // Orphans are keyed in the database by their expected lifetime.
    // The first number represents the *last* version at which the orphan needs
    // to exist, while the second number represents the *earliest* version at
    // which it is expected to exist - which starts out by being the version
    // of the node being orphaned.
    orphanPrefix    = "o/"
    orphanPrefixFmt = "o/%010d/"         // o/<last-version>/
    orphanKeyFmt    = "o/%010d/%010d/%X" // o/<last-version>/<first-version>/<hash>

And so on.

I think the easiest way do deal with this is to delete all versions from the desired LoadVersionForOverwriting version+1 until the last version. Deleting versions in the opposite order (backwards) would result in O(N^2) operations, whereas deleting versions in the forward order results in O(N) operations. See the implementation of "deleteOrphans(version)".

Given all of this, I think the best way would be to implement a DeleteVersionsFrom(version), which deletes all versions starting from version, and LoadVersionForOverwriting would just be something like:

func LoadVersionForOverwriting(version int64) {
    LoadVersion(version)
    DeleteVersionsFrom(version+1)
}

HaoyangLiu · 2018-08-28T10:00:41Z

@jaekwon @liamsi
Thanks for your thoughtful suggestion. I made a mistake in random map iteration. It caused issue only in my implementaion. The original code is fine. Besides, according to your suggestion, I refactor the code. Please check it.

liamsi · 2018-08-28T15:34:22Z

Thanks for your input @jaekwon! BTW @jlandrews initially brought up the concerns about the orphan tracking.

@HaoyangLiu thanks for the changes! Do you mind adding tests that document and test the new behaviour? Namely, that the versions are actually deleted (incl. the orphans)? Also, a test that covers the new flag for DeleteVersion?

…fix/load_more_verson_than_target

HaoyangLiu · 2018-09-23T16:10:41Z

@liamsi @jaekwon
I have add a test to cover the new feature. Please take a look at it. Thanks

HaoyangLiu · 2018-09-30T07:49:04Z

@liamsi
Could you help to review this PR again?

liamsi · 2018-10-01T09:14:26Z

Will do during the weekend (mostly AFK till Saturday)

liamsi · 2018-10-08T14:37:55Z

mutable_tree.go

+		return latestVersion, err
+	}
+	tree.deleteVersionsFrom(targetVersion+1)
+	return targetVersion, nil


According to the comment it "returns the version number of the latest version found". Here we just return the argument passed by the caller targetVersion? Either the comment needs to be fixed, or, we'd need to return latestVersion instead (or make it more explicit that they are the same?).

Sorry. It is my mistake. I will change the comment

No worries! Thank you. But then: why do we even need to return this value if it is already known by the caller. It is not even read in the tests. It's a bit confusing.

If the targetVersion is greater than the lastest version, then the lastest version will be return. I have modified the test the check the return value.

Thanks! It's still a bit confusing. So it actually returns the latest version together with an error. How is this useful? Usually, in go if you return an error the other return values aren't considered by the caller (usually nil or default values). But I see that this is what LoadVersion already handles it like this. I think it would be useful if it returned a particular which the caller could check against. But this is probably orthogonal to this PR. No need to change this in this PR.

liamsi · 2018-10-08T15:07:32Z

mutable_tree.go

+		if version > lastestVersion {
+			break
+		}
+		if version == 0 {


What about version < 0?

Also, this check is independent of the for loop (and hence, should live outside of the loop)

Actually, version won't less than zero. If version is less than zero, an error will be return in an early position. But changing version == 0 to version <= 0 is much better.

liamsi · 2018-10-08T15:14:21Z

tree_test.go

+		count++
+		if count > maxLength {
+			break
+		}


nitpicking: use a classical for loop instead of break to increase readability. for count:=1; count <= maxLength; count++ {}

Good suggestion. I will modify the code.

liamsi · 2018-10-08T15:46:50Z

tree_test.go

@@ -13,6 +13,7 @@ import (
 	mathrand "math/rand"

 	cmn "github.com/tendermint/tendermint/libs/common"
+	"strconv"


The order of imports should be:

stdlib

external packages

internal packages

go-imports can automate this for you.

Good suggestion. I have changed the code.

HaoyangLiu · 2018-10-08T16:11:05Z

@liamsi
Do you have any other suggestions?

liamsi · 2018-10-10T13:11:25Z

I think it would be great if the tests also showed that the orphans were actually deleted. But this is also covered by tests calling DeleteVersion. So we should be fine.

liamsi

This PR LGTM. I'd feel more comfortable with a second opinion by @jaekwon / @silasdavis / @jlandrews though.

UnitylChaos

Looks good to me too 👍

silasdavis · 2018-11-29T18:07:37Z

@ebuchman any reason didn't make it into 0.12.0?

liamsi · 2018-11-29T18:47:21Z

It did actually make it. It even made it into 0.11.1 which was a pre-release by Jae: https://github.com/tendermint/iavl/commits/v0.11.1
The changelog which was merged there was never reviewed and this PR is missing in there (the changes are there though).

…i… (cosmos#102) * Only load to target verion when target version doesn't equal to 0, higher version won't be loaded * add LoadVersionOverwrite * Refactor code according to code review * Refactor comment and error handle * Add overwrite test

Only load to target verion when target version doesn't equal to 0, hi…

51a81a7

…gher version won't be loaded

silasdavis reviewed Aug 21, 2018

View reviewed changes

add LoadVersionOverwrite

10f216b

Refactor code according to code review

defa78b

Refactor comment and error handle

bdc4eb8

liamsi changed the title ~~Only load to target verion when target version doesn't equal to 0, hi…~~ Only load to target version when target version doesn't equal to 0, hi… Aug 28, 2018

liamsi added T:enhancement Type: Enhancement proposal labels Aug 29, 2018

HaoyangLiu added 2 commits September 23, 2018 23:05

Merge branch 'develop' of https://github.com/tendermint/iavl into bug…

4dd1d1c

…fix/load_more_verson_than_target

Add overwirte test

070f646

Refactor variable name

89fa912

liamsi reviewed Oct 8, 2018

View reviewed changes

HaoyangLiu added 2 commits October 8, 2018 23:51

Refactor code according to code reviewer suggestions

c24ae24

Refactor import package

f0ca457

liamsi approved these changes Oct 10, 2018

View reviewed changes

UnitylChaos approved these changes Oct 12, 2018

View reviewed changes

liamsi merged commit 13e3ec2 into cosmos:develop Oct 12, 2018

Only load to target version when target version doesn't equal to 0, hi… #102

Only load to target version when target version doesn't equal to 0, hi… #102

Conversation

HaoyangLiu commented Aug 16, 2018

HaoyangLiu commented Aug 21, 2018

liamsi commented Aug 21, 2018

silasdavis commented Aug 21, 2018 • edited Loading

silasdavis Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silasdavis Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoyangLiu commented Aug 21, 2018

liamsi commented Aug 24, 2018

HaoyangLiu commented Aug 27, 2018

jaekwon commented Aug 27, 2018 • edited Loading

jaekwon commented Aug 28, 2018

HaoyangLiu commented Aug 28, 2018

liamsi commented Aug 28, 2018

HaoyangLiu commented Sep 23, 2018

HaoyangLiu commented Sep 30, 2018

liamsi commented Oct 1, 2018

liamsi Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoyangLiu commented Oct 8, 2018

liamsi commented Oct 10, 2018

liamsi left a comment

Choose a reason for hiding this comment

UnitylChaos left a comment

Choose a reason for hiding this comment

silasdavis commented Nov 29, 2018

liamsi commented Nov 29, 2018 • edited Loading

silasdavis commented Aug 21, 2018 •

edited

Loading

silasdavis Aug 21, 2018 •

edited

Loading

silasdavis Aug 22, 2018 •

edited

Loading

jaekwon commented Aug 27, 2018 •

edited

Loading

liamsi Oct 8, 2018 •

edited

Loading

liamsi Oct 8, 2018 •

edited

Loading

liamsi Oct 8, 2018 •

edited

Loading

liamsi Oct 8, 2018 •

edited

Loading

liamsi commented Nov 29, 2018 •

edited

Loading