Recent change in behavior for in-memory database: possible regression?

[wuestholz]

I observed a recent change in behavior for the in-memory database. For commit 63b1802 the following piece of code produces a different outcome than in older versions (e.g., 396f1dd8):

db := rawdb.NewMemoryDatabase()
sdb, _ = state.New(common.Hash{}, state.NewDatabase(db))
addr := common.HexToAddress("0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe")
sdb.SetBalance(addr, big.NewInt(42))
fmt.Printf("balance: %v\n", sdb.GetBalance(addr))
nsdb := sdb.Copy()
nsdb.Commit(false)
cnsdb := nsdb.Copy()
fmt.Printf("balance: %v\n", cnsdb.GetBalance(addr))

This looks like a possible regression.

System information

OS & Version: Windows
Commit hash : 63b1802

Expected behaviour

The code should probably print the following:

balance: 42
balance: 42

Actual behaviour

The code prints the following:

balance: 42
balance: 0

Steps to reproduce the behaviour

Run the piece of code above.

[holiman]

Interesting!
Here's a testcase to repro

package core

import (
	"fmt"
	"github.com/ethereum/go-ethereum/common"
	"github.com/ethereum/go-ethereum/core/rawdb"
	"github.com/ethereum/go-ethereum/core/state"
	"math/big"
	"testing"
)

func TestFoo(t *testing.T) {
	db := rawdb.NewMemoryDatabase()
	sdb, _ := state.New(common.Hash{}, state.NewDatabase(db))
	addr := common.HexToAddress("0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe")
	sdb.SetBalance(addr, big.NewInt(42))
	fmt.Printf("balance: %v\n", sdb.GetBalance(addr))
	copyOne := sdb.Copy()
	copyOne.Commit(false)
	copyTwo := copyOne.Copy()
	fmt.Printf("balance: %v\n", sdb.GetBalance(addr))
	fmt.Printf("n balance: %v\n", copyOne.GetBalance(addr))
	fmt.Printf("c balance: %v\n", copyTwo.GetBalance(addr))
	if got, exp := copyTwo.GetBalance(addr), big.NewInt(42); got.Cmp(exp) != 0 {
		t.Fatalf("got %v exp %v", got, exp)
	}
}

This behaviour changed in 223b950 . It's not about Memorydb, but about how the state accumulates writes.

In the example, the trie lookup for addr fails (returns nil) on copyTwo when GetBalance is invoked and the state object is looked up. I'm not quite sure why.
cc @karalabe

[holiman]

So, what seems to happen is that the sdb.trie.t.root is nil, which is carried over to copyOne.
The first copy obtains the changes to pending, so it stil has the changes. However, when it is committed, the trie root is not properly updated (because it's nil to start with). And then, when it's copied once more, the pending has been cleared due to the commit, but we still have a nil trie root.

If I add a commit to sdb before the first copy, the test passes

	sdb.SetBalance(addr, big.NewInt(42))
	sdb.Commit(false)

I am not sure why the changes @karalabe did caused this behaviour to change.

[wuestholz]

@holiman Thank you very much for looking into this so quickly!

Yeah, when looking at the commit history those changes seemed like the most likely candidates for causing this change.

It's weird that the test passes after adding the second commit. I thought that maybe I'm using the API in the wrong way, but it seems like you agree that the old behavior makes more sense.

It would be great to know if there's a chance to reinstate the old behavior. I'm trying to update our Harvey fuzzer to work with the latest Istanbul changes and I'm wondering if I should look into other workarounds.

[karalabe]

We'll get the issue fixed soon.

[wuestholz]

@karalabe Great! Thanks a lot for letting me know!