Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

Commit

Permalink
This fixes two issues with our state handling when querying state via
Browse files Browse the repository at this point in the history
`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 <silas@monax.io>
  • Loading branch information
Silas Davis committed Apr 5, 2021
1 parent aad850b commit 314357e
Show file tree
Hide file tree
Showing 34 changed files with 676 additions and 306 deletions.
35 changes: 20 additions & 15 deletions acm/acmstate/state_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
29 changes: 29 additions & 0 deletions cmd/burrow/commands/errors.go
Original file line number Diff line number Diff line change
@@ -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())
}
}
}
}
2 changes: 2 additions & 0 deletions cmd/burrow/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
6 changes: 5 additions & 1 deletion core/processes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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))
Expand Down
2 changes: 1 addition & 1 deletion dump/dump.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
1 change: 1 addition & 0 deletions execution/accounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
15 changes: 15 additions & 0 deletions execution/errors/codes.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"encoding/json"
"fmt"
"reflect"
"strconv"
"strings"
)

type codes struct {
Expand Down Expand Up @@ -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()
}
2 changes: 1 addition & 1 deletion execution/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
4 changes: 4 additions & 0 deletions execution/errors/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
3 changes: 2 additions & 1 deletion execution/execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
7 changes: 6 additions & 1 deletion execution/execution_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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{
Expand Down
18 changes: 9 additions & 9 deletions execution/simulated_call.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}),
Expand All @@ -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()
Expand Down
131 changes: 131 additions & 0 deletions execution/simulated_call_test.go
Original file line number Diff line number Diff line change
@@ -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())
}
13 changes: 13 additions & 0 deletions execution/solidity/delgate_proxy.sol
Original file line number Diff line number Diff line change
@@ -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;
}
}
7 changes: 7 additions & 0 deletions execution/solidity/delgate_proxy.sol.go
Original file line number Diff line number Diff line change
@@ -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"}]`)
Loading

0 comments on commit 314357e

Please sign in to comment.