Clarify tree.Hash / Skipped test TestProof

[liamsi]

Find out why TestProof fails (tree root never seems to match here).
It's not clear why this is a race-condition. Either 1) fix test, 2) clarify why it is skipped, or 3) remove test (only if the test does not make sense).

https://github.com/tendermint/iavl/blob/35f66e53d9b01e83b30de68b931f54b2477a94c9/basic_test.go#L392-L422

[liamsi]

This is what I think the problem is:

While tree.Iterate is called with tree.Hash() as the root hash, tree.Hash() is called on the latest versioned tree (the persisted tree, see here).
tree.Iterate(...) on the other side operates on the whole tree (incl. the non-persisted / orphaned nodes); see here

The test passes again if:

===================================================================
--- basic_test.go	(revision fd37a0fa3a7454423233bc3d5ea828f38e0af787)
+++ basic_test.go	(date 1525356743000)
@@ -387,7 +387,7 @@
 }
 
 func TestProof(t *testing.T) {
-	t.Skipf("This test has a race condition causing it to occasionally panic.")
+	//t.Skipf("This test has a race condition causing it to occasionally panic.")
 
 	// Construct some random tree
 	db := db.NewMemDB()
@@ -412,7 +412,7 @@
 		assert.NoError(t, err)
 		assert.Equal(t, value, value2)
 		if assert.NotNil(t, proof) {
-			testProof(t, proof.(*KeyExistsProof), key, value, tree.Hash())
+			testProof(t, proof.(*KeyExistsProof), key, value, tree.orphaningTree.Hash())
 		}
 		return false
 	})

Looks like there should be 2 VersionedTree.Hash() methods (one with orphans and one without), or, there should be a VersionedTree.Iterate which ignores the orphaned nodes (like Hash() does), too.

What should be the intended behaviour here? @jaekwon @ebuchman

[liamsi]

After discussing with @jlandrews, will open a PR with the above changes which will fix the test.

[tac0turtle]

resolved in #92