This fixes two issues with our state handling when querying state via

`CallSim` and `CallCodeSim`.

State is separated into a mutable write side and an immutable read side
(from last commit/height) via RWTree and then via MutableForest in an
MVCC style manner. Reads should hit the last committed version. However
there were two bugs in TransactServer's `CallTxSim` and `CallCodeSim`:

1. During the execution of a simulated call if a block is committed
reads can be made from consecutive blocks and so may not see a
consistent view of the state.

2. `ImmutableForest`'s `loadOrCreateTree` was unsynchronised and
committed a multitude of sins: creating and returning multiple trees for the
same prefix when it should ensure one instance exists in cache and most problematically allowing access to uninitialised trees by pushing into LRU
cache before `Load`ing the tree from DB.

The fix for 1 is to pull a snapshot `ImmutableForest` when Call*Sim are
called which is fixed to the latest height at the time of the call.

The fix for 2 was to synchronise `loadOrCreateTree` and only push
initialised trees into the `treeCache`.

simulated_call_test.go adds a test that triggers the second bug
(requires a cold cache and a prefix with previous existing versions)

Additionally:

- Make MutableForest and ImmutableForest fully thread-safe on reads and
writes.

Signed-off-by: Silas Davis <[email protected]>

acm/acmstate/state_cache.go

@@ -262,21 +262,26 @@ func (cache *Cache) get(address crypto.Address) (*accountInfo, error) {
 	cache.RLock()
 	accInfo := cache.accounts[address]
 	cache.RUnlock()
-	if accInfo == nil {
-		cache.Lock()
-		defer cache.Unlock()
-		accInfo = cache.accounts[address]
-		if accInfo == nil {
-			account, err := cache.backend.GetAccount(address)
-			if err != nil {
-				return nil, err
-			}
-			accInfo = &accountInfo{
-				account: account,
-				storage: make(map[binary.Word256][]byte),
-			}
-			cache.accounts[address] = accInfo
-		}
+	if accInfo != nil {
+		return accInfo, nil
+	}
+	// Take write lock to fill cache
+	cache.Lock()
+	defer cache.Unlock()
+	// Check for an interleaved cache fill
+	accInfo = cache.accounts[address]
+	if accInfo != nil {
+		return accInfo, nil
+	}
+	// Pull from backend
+	account, err := cache.backend.GetAccount(address)
+	if err != nil {
+		return nil, err
+	}
+	accInfo = &accountInfo{
+		account: account,
+		storage: make(map[binary.Word256][]byte),
 	}
+	cache.accounts[address] = accInfo
 	return accInfo, nil
 }

cmd/burrow/commands/errors.go

@@ -0,0 +1,29 @@
+package commands
+
+import (
+	"encoding/json"
+
+	"github.com/hyperledger/burrow/execution/errors"
+	cli "github.com/jawher/mow.cli"
+)
+
+func Errors(output Output) func(cmd *cli.Cmd) {
+	return func(cmd *cli.Cmd) {
+
+		jsonOpt := cmd.BoolOpt("j json", false, "output errors as a JSON object")
+
+		cmd.Spec = "[ --json ]"
+
+		cmd.Action = func() {
+			if *jsonOpt {
+				bs, err := json.MarshalIndent(errors.Codes, "", "\t")
+				if err != nil {
+					output.Fatalf("Could not marshal error codes: %w", err)
+				}
+				output.Printf(string(bs))
+			} else {
+				output.Printf(errors.Codes.String())
+			}
+		}
+	}
+}

cmd/burrow/main.go

@@ -87,6 +87,8 @@ func burrow(output commands.Output) *cli.Cli {
 	app.Command("compile", "Compile solidity files embedding the compilation results as a fixture in a Go file",
 		commands.Compile(output))
 
+	app.Command("errors", "Print error codes",
+		commands.Errors(output))
 	return app
 }
 

core/processes.go

@@ -6,6 +6,8 @@ import (
 	"net/http"
 	"time"
 
+	"github.com/hyperledger/burrow/acm/acmstate"
+
 	"github.com/hyperledger/burrow/bcm"
 	"github.com/hyperledger/burrow/consensus/abci"
 	"github.com/hyperledger/burrow/execution"
@@ -320,7 +322,9 @@ func GRPCLauncher(kern *Kernel, conf *rpc.ServerConfig, keyConfig *keys.KeysConf
 
 			txCodec := txs.NewProtobufCodec()
 			rpctransact.RegisterTransactServer(grpcServer,
-				rpctransact.NewTransactServer(kern.State, kern.Blockchain, kern.Transactor, txCodec, kern.Logger))
+				rpctransact.NewTransactServer(func() (acmstate.Reader, error) {
+					return kern.State.AtLatestVersion()
+				}, kern.Blockchain, kern.Transactor, txCodec, kern.Logger))
 
 			rpcevents.RegisterExecutionEventsServer(grpcServer, rpcevents.NewExecutionEventsServer(kern.State,
 				kern.Emitter, kern.Blockchain, kern.Logger))

dump/dump.go

@@ -70,7 +70,7 @@ func (ds *Dumper) Transmit(sink Sink, startHeight, endHeight uint64, options Opt
 	if endHeight == 0 || endHeight > lastHeight {
 		endHeight = lastHeight
 	}
-	st, err := ds.state.LoadHeight(endHeight)
+	st, err := ds.state.AtHeight(endHeight)
 	if err != nil {
 		return err
 	}

execution/accounts.go

@@ -36,6 +36,7 @@ func NewAccounts(reader acmstate.Reader, keyClient keys.KeyClient, mutexCount in
 		keyClient: keyClient,
 	}
 }
+
 func (accs *Accounts) SigningAccount(address crypto.Address) (*SigningAccount, error) {
 	signer, err := keys.AddressableSigner(accs.keyClient, address)
 	if err != nil {

execution/errors/codes.go

@@ -4,6 +4,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"reflect"
+	"strconv"
+	"strings"
 )
 
 type codes struct {
@@ -89,3 +91,16 @@ func (es *codes) Get(number uint32) *Code {
 	}
 	return es.codes[number]
 }
+
+func (es *codes) String() string {
+	sb := new(strings.Builder)
+	for _, c := range es.codes {
+		sb.WriteString(strconv.FormatUint(uint64(c.Number), 10))
+		sb.WriteString(": ")
+		sb.WriteString(c.Name)
+		sb.WriteString(" - ")
+		sb.WriteString(c.Description)
+		sb.WriteRune('\n')
+	}
+	return sb.String()
+}

execution/errors/errors.go

@@ -21,7 +21,7 @@ type Source interface {
 	Error() error
 }
 
-var Codes = codes{
+var Codes = &codes{
 	None:                   code("none"),
 	UnknownAddress:         code("unknown address"),
 	InsufficientBalance:    code("insufficient balance"),

execution/errors/errors_test.go

@@ -25,3 +25,7 @@ func TestCode_String(t *testing.T) {
 	err := Codes.CodeOutOfBounds
 	fmt.Println(err.Error())
 }
+
+func TestPrintCodes(t *testing.T) {
+	fmt.Printf("%v", Codes)
+}

execution/execution.go

@@ -54,6 +54,7 @@ type ExecutorState interface {
 	proposal.Reader
 	validator.IterableReader
 }
+
 type BatchExecutor interface {
 	// Provides access to write lock for a BatchExecutor so reads can be prevented for the duration of a commit
 	sync.Locker
@@ -284,7 +285,7 @@ func (exe *executor) validateInputsAndStorePublicKeys(txEnv *txs.Envelope) error
 	for s, in := range txEnv.Tx.GetInputs() {
 		err := exe.updateSignatory(txEnv.Signatories[s])
 		if err != nil {
-			return fmt.Errorf("failed to update public key for input %v: %v", in.Address, err)
+			return fmt.Errorf("failed to update public key for input %v: %w", in.Address, err)
 		}
 		acc, err := exe.stateCache.GetAccount(in.Address)
 		if err != nil {

execution/execution_test.go

@@ -234,7 +234,7 @@ func TestSendPermission(t *testing.T) {
 	tx = payload.NewSendTx()
 	require.NoError(t, tx.AddInput(exe.stateCache, users[0].GetPublicKey(), 5))
 	require.NoError(t, tx.AddInput(exe.stateCache, users[1].GetPublicKey(), 5))
-	require.NoError(t, tx.AddOutput(users[2].GetAddress(), 10))
+	tx.AddOutput(users[2].GetAddress(), 10)
 	err = exe.signExecuteCommit(tx, users[:2]...)
 	require.Error(t, err)
 }
@@ -1203,6 +1203,11 @@ func TestMerklePanic(t *testing.T) {
 	acc0 := getAccount(t, st, privAccounts[0].GetAddress())
 	acc1 := getAccount(t, st, privAccounts[1].GetAddress())
 
+	acc, err := st.GetAccount(acc0.Address)
+
+	require.NoError(t, err)
+	require.NotNil(t, acc)
+
 	// SendTx.
 	{
 		tx := &payload.SendTx{

execution/simulated_call.go

@@ -18,7 +18,6 @@ import (
 // Cannot be used to create new contracts
 func CallSim(reader acmstate.Reader, blockchain bcm.BlockchainInfo, fromAddress, address crypto.Address, data []byte,
 	logger *logging.Logger) (*exec.TxExecution, error) {
-
 	cache := acmstate.NewCache(reader)
 	exe := contexts.CallContext{
 		VMS:           vms.NewConnectedVirtualMachines(engine.Options{}),
@@ -29,14 +28,15 @@ func CallSim(reader acmstate.Reader, blockchain bcm.BlockchainInfo, fromAddress,
 		Logger:        logger,
 	}
 
-	txe := exec.NewTxExecution(txs.Enclose(blockchain.ChainID(), &payload.CallTx{
-		Input: &payload.TxInput{
-			Address: fromAddress,
-		},
-		Address:  &address,
-		Data:     data,
-		GasLimit: contexts.GasLimit,
-	}))
+	txe := exec.NewTxExecution(txs.Enclose(blockchain.ChainID(),
+		&payload.CallTx{
+			Input: &payload.TxInput{
+				Address: fromAddress,
+			},
+			Address:  &address,
+			Data:     data,
+			GasLimit: contexts.GasLimit,
+		}))
 
 	// Set height for downstream synchronisation purposes
 	txe.Height = blockchain.LastBlockHeight()

execution/simulated_call_test.go

@@ -0,0 +1,131 @@
+package execution
+
+import (
+	"context"
+	"fmt"
+	"math/big"
+	"testing"
+
+	"github.com/hyperledger/burrow/acm"
+	"github.com/hyperledger/burrow/acm/acmstate"
+	"github.com/hyperledger/burrow/bcm"
+	"github.com/hyperledger/burrow/crypto"
+	"github.com/hyperledger/burrow/execution/engine"
+	"github.com/hyperledger/burrow/execution/evm"
+	"github.com/hyperledger/burrow/execution/evm/abi"
+	"github.com/hyperledger/burrow/execution/exec"
+	"github.com/hyperledger/burrow/execution/solidity"
+	"github.com/hyperledger/burrow/execution/state"
+	"github.com/hyperledger/burrow/genesis"
+	"github.com/hyperledger/burrow/permission"
+	"github.com/stretchr/testify/require"
+	dbm "github.com/tendermint/tm-db"
+	"golang.org/x/sync/errgroup"
+)
+
+var genesisDoc, _, _ = genesis.NewDeterministicGenesis(100).GenesisDoc(1, 1)
+
+// This test looks at caching problems that arise when doing concurrent reads via CallSim. It requires a cold cache
+// for bug to be exhibited.
+// The root cause of the original bug was a race condition that could lead to reading a uninitialised tree from the
+// MutableForest tree cache.
+func TestCallSimDelegate(t *testing.T) {
+	// Roll up our sleeves and swear fealty to the witch-king
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	g, ctx := errgroup.WithContext(ctx)
+
+	db := dbm.NewMemDB()
+	st, err := state.MakeGenesisState(db, genesisDoc)
+	require.NoError(t, err)
+
+	from := crypto.PrivateKeyFromSecret("raaah", crypto.CurveTypeEd25519)
+	contractAddress := crypto.Address{1, 2, 3, 4, 5}
+	blockchain := &bcm.Blockchain{}
+	sink := exec.NewNoopEventSink()
+
+	// Function to set storage value for later
+	setDelegate := func(up state.Updatable, value crypto.Address) error {
+		call, _, err := abi.EncodeFunctionCall(string(solidity.Abi_DelegateProxy), "setDelegate", logger, value)
+		if err != nil {
+			return err
+		}
+
+		cache := acmstate.NewCache(st)
+		_, err = evm.Default().Execute(cache, blockchain, sink,
+			engine.CallParams{
+				CallType: exec.CallTypeCall,
+				Origin:   from.GetAddress(),
+				Caller:   from.GetAddress(),
+				Callee:   contractAddress,
+				Input:    call,
+				Gas:      big.NewInt(9999999),
+			}, solidity.DeployedBytecode_DelegateProxy)
+
+		if err != nil {
+			return err
+		}
+		return cache.Sync(up)
+	}
+
+	// Initialise sender smart contract state
+	_, _, err = st.Update(func(up state.Updatable) error {
+		err = up.UpdateAccount(&acm.Account{
+			Address:     from.GetAddress(),
+			PublicKey:   from.GetPublicKey(),
+			Balance:     9999999,
+			Permissions: permission.DefaultAccountPermissions,
+		})
+		if err != nil {
+			return err
+		}
+		return up.UpdateAccount(&acm.Account{
+			Address: contractAddress,
+			EVMCode: solidity.DeployedBytecode_DelegateProxy,
+		})
+	})
+	require.NoError(t, err)
+
+	// Set a series of values of storage slot so we get a deep version tree (which we need to trigger the bug)
+	delegate := crypto.Address{0xBE, 0xEF, 0, 0xFA, 0xCE, 0, 0xBA, 0}
+	for i := 0; i < 0xBF; i++ {
+		delegate[7] = byte(i)
+		_, _, err = st.Update(func(up state.Updatable) error {
+			return setDelegate(up, delegate)
+		})
+		require.NoError(t, err)
+	}
+
+	// This is important in order to illicit the former bug - we need a cold LRU tree cache in MutableForest
+	st, err = state.LoadState(db, st.Version())
+	require.NoError(t, err)
+
+	getIntCall, _, err := abi.EncodeFunctionCall(string(solidity.Abi_DelegateProxy), "getDelegate", logger)
+	require.NoError(t, err)
+	n := 1000
+
+	for i := 0; i < n; i++ {
+		g.Go(func() error {
+			txe, err := CallSim(st, blockchain, from.GetAddress(), contractAddress, getIntCall, logger)
+			if err != nil {
+				return err
+			}
+			err = txe.GetException().AsError()
+			if err != nil {
+				return err
+			}
+			address, err := crypto.AddressFromBytes(txe.GetResult().Return[12:])
+			if err != nil {
+				return err
+			}
+			if address != delegate {
+				// The bug for which this test was written will return the zero address here since it is accessing
+				// an uninitialised tree
+				return fmt.Errorf("getDelegate returned %v but expected %v", address, delegate)
+			}
+			return nil
+		})
+	}
+
+	require.NoError(t, g.Wait())
+}

execution/solidity/delgate_proxy.sol

@@ -0,0 +1,13 @@
+pragma solidity ^0.5;
+
+contract DelegateProxy {
+    address internal proxied;
+
+    function setDelegate(address _proxied) public {
+        proxied = _proxied;
+    }
+
+    function getDelegate() public view returns (address) {
+        return proxied;
+    }
+}

execution/solidity/delgate_proxy.sol.go

@@ -0,0 +1,7 @@
+package solidity
+
+import hex "github.com/tmthrgd/go-hex"
+
+var Bytecode_DelegateProxy = hex.MustDecodeString("608060405234801561001057600080fd5b5061016a806100206000396000f3fe608060405234801561001057600080fd5b50600436106100365760003560e01c8063bc7f3b501461003b578063ca5eb5e114610085575b600080fd5b6100436100c9565b604051808273ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff16815260200191505060405180910390f35b6100c76004803603602081101561009b57600080fd5b81019080803573ffffffffffffffffffffffffffffffffffffffff1690602001909291905050506100f2565b005b60008060009054906101000a900473ffffffffffffffffffffffffffffffffffffffff16905090565b806000806101000a81548173ffffffffffffffffffffffffffffffffffffffff021916908373ffffffffffffffffffffffffffffffffffffffff1602179055505056fea265627a7a7231582061c5d5f36c9a31fab0943b47e1f33f7c79322c9e41c8fa02eec89de42114050f64736f6c634300050f0032")
+var DeployedBytecode_DelegateProxy = hex.MustDecodeString("608060405234801561001057600080fd5b50600436106100365760003560e01c8063bc7f3b501461003b578063ca5eb5e114610085575b600080fd5b6100436100c9565b604051808273ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff16815260200191505060405180910390f35b6100c76004803603602081101561009b57600080fd5b81019080803573ffffffffffffffffffffffffffffffffffffffff1690602001909291905050506100f2565b005b60008060009054906101000a900473ffffffffffffffffffffffffffffffffffffffff16905090565b806000806101000a81548173ffffffffffffffffffffffffffffffffffffffff021916908373ffffffffffffffffffffffffffffffffffffffff1602179055505056fea265627a7a7231582061c5d5f36c9a31fab0943b47e1f33f7c79322c9e41c8fa02eec89de42114050f64736f6c634300050f0032")
+var Abi_DelegateProxy = []byte(`[{"constant":true,"inputs":[],"name":"getDelegate","outputs":[{"internalType":"address","name":"","type":"address"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"_proxied","type":"address"}],"name":"setDelegate","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"}]`)