From d1e151fa4b7bcd03f2b65e3766509a81d9dcab55 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 19 Oct 2020 23:06:32 +0200 Subject: [PATCH 1/7] Storage limiter --- engine/execution/state/delta/view.go | 11 ++ fvm/context.go | 1 + fvm/errors.go | 17 +++ fvm/state/accounts.go | 7 ++ fvm/state/ledger.go | 37 ++++++- fvm/transactionStorageLimiter.go | 70 ++++++++++++ fvm/transactionStorageLimiter_test.go | 154 ++++++++++++++++++++++++++ 7 files changed, 293 insertions(+), 4 deletions(-) create mode 100644 fvm/transactionStorageLimiter.go create mode 100644 fvm/transactionStorageLimiter_test.go diff --git a/engine/execution/state/delta/view.go b/engine/execution/state/delta/view.go index 424053bf961..a377f20f9c0 100644 --- a/engine/execution/state/delta/view.go +++ b/engine/execution/state/delta/view.go @@ -149,6 +149,10 @@ func (v *View) Set(owner, controller, key string, value flow.RegisterValue) { v.delta.Set(owner, controller, key, value) } +func (v *View) RegisterUpdates() ([]flow.RegisterID, []flow.RegisterValue) { + return v.delta.RegisterUpdates() +} + func (v *View) updateSpock(value []byte) error { _, err := v.spockSecretHasher.Write(value) if err != nil { @@ -159,12 +163,18 @@ func (v *View) updateSpock(value []byte) error { func (v *View) updateRegisterSizeChange(owner, controller, key string, value flow.RegisterValue) error { if key == state.StorageUsedRegisterName { + // Don't check size change on this register + // Size of this register will always be 8 + // If this is the first time storage_used register was set it should be set to the size of itself (8) return nil } + // get old value + // len will be 0 if there is no old value oldValue, err := v.Get(owner, controller, key) if err != nil { return err } + sizeChange := int64(len(value) - len(oldValue)) if sizeChange == 0 { // register size has not changed. Nothing to do @@ -179,6 +189,7 @@ func (v *View) updateRegisterSizeChange(owner, controller, key string, value flo var oldSize uint64 if len(oldSizeRegister) == 0 { // account was just created + // set the storage used to 8 which is the size of the storage used register oldSize = 8 buffer := make([]byte, oldSize) binary.LittleEndian.PutUint64(buffer, 8) diff --git a/fvm/context.go b/fvm/context.go index 5ef31bc754e..7cf960d9f5e 100644 --- a/fvm/context.go +++ b/fvm/context.go @@ -66,6 +66,7 @@ func defaultContext() Context { NewTransactionSequenceNumberChecker(), NewTransactionFeeDeductor(), NewTransactionInvocator(), + NewTransactionStorageLimiter(), }, ScriptProcessors: []ScriptProcessor{ NewScriptInvocator(), diff --git a/fvm/errors.go b/fvm/errors.go index d1610391ef0..935821841c5 100644 --- a/fvm/errors.go +++ b/fvm/errors.go @@ -25,6 +25,8 @@ const ( errCodeInvalidHashAlgorithm = 10 + errCodeOverStorageCapacity = 11 + errCodeExecution = 100 ) @@ -213,6 +215,21 @@ func (e *InvalidHashAlgorithmError) Code() uint32 { return errCodeInvalidHashAlgorithm } +// An OverStorageCapacityError indicates that a given key has an invalid hash algorithm. +type OverStorageCapacityError struct { + Address flow.Address + StorageUsed uint64 + StorageCapacity uint64 +} + +func (e *OverStorageCapacityError) Error() string { + return fmt.Sprintf("address %s storage %d is over capacity %d", e.Address, e.StorageUsed, e.StorageCapacity) +} + +func (e *OverStorageCapacityError) Code() uint32 { + return errCodeOverStorageCapacity +} + type ExecutionError struct { Err runtime.Error } diff --git a/fvm/state/accounts.go b/fvm/state/accounts.go index 1f3a8a8a2f0..5bcf230e757 100644 --- a/fvm/state/accounts.go +++ b/fvm/state/accounts.go @@ -2,6 +2,7 @@ package state import ( "bytes" + "encoding/binary" "errors" "fmt" "math/big" @@ -101,6 +102,12 @@ func (a *Accounts) Create(publicKeys []flow.AccountPublicKey) (flow.Address, err // mark that this account exists a.ledger.Set(string(newAddress.Bytes()), "", keyExists, []byte{1}) + // TODO: This is temporary! Need to fix with next PR before this can be merged to master + // this will be in the transaction that created the account. It's here now so I can run the tests. + buffer := make([]byte, 8) + binary.LittleEndian.PutUint64(buffer, 100000) + a.ledger.Set(string(newAddress.Bytes()), "", StorageCapacityRegisterName, buffer) + a.ledger.Set(string(newAddress.Bytes()), string(newAddress.Bytes()), keyCode, nil) err = a.SetAllPublicKeys(newAddress, publicKeys) diff --git a/fvm/state/ledger.go b/fvm/state/ledger.go index 420b0c5e54e..646b33d7cd9 100644 --- a/fvm/state/ledger.go +++ b/fvm/state/ledger.go @@ -1,6 +1,7 @@ package state import ( + "sort" "strings" "github.com/onflow/flow-go/model/flow" @@ -12,19 +13,20 @@ type Ledger interface { Get(owner, controller, key string) (flow.RegisterValue, error) Touch(owner, controller, key string) Delete(owner, controller, key string) + RegisterUpdates() ([]flow.RegisterID, []flow.RegisterValue) } // A MapLedger is a naive ledger storage implementation backed by a simple map. // // This implementation is designed for testing purposes. type MapLedger struct { - Registers map[string]flow.RegisterValue + Registers map[string]flow.RegisterEntry RegisterTouches map[string]bool } func NewMapLedger() *MapLedger { return &MapLedger{ - Registers: make(map[string]flow.RegisterValue), + Registers: make(map[string]flow.RegisterEntry), RegisterTouches: make(map[string]bool), } } @@ -32,13 +34,20 @@ func NewMapLedger() *MapLedger { func (m MapLedger) Set(owner, controller, key string, value flow.RegisterValue) { k := fullKey(owner, controller, key) m.RegisterTouches[k] = true - m.Registers[k] = value + m.Registers[k] = flow.RegisterEntry{ + Key: flow.RegisterID{ + Owner: owner, + Controller: controller, + Key: key, + }, + Value: value, + } } func (m MapLedger) Get(owner, controller, key string) (flow.RegisterValue, error) { k := fullKey(owner, controller, key) m.RegisterTouches[k] = true - return m.Registers[k], nil + return m.Registers[k].Value, nil } func (m MapLedger) Touch(owner, controller, key string) { @@ -49,6 +58,26 @@ func (m MapLedger) Delete(owner, controller, key string) { delete(m.Registers, fullKey(owner, controller, key)) } +func (m MapLedger) RegisterUpdates() ([]flow.RegisterID, []flow.RegisterValue) { + data := make(flow.RegisterEntries, 0, len(m.Registers)) + + for _, v := range m.Registers { + data = append(data, v) + } + + sort.Sort(&data) + + ids := make([]flow.RegisterID, 0, len(m.Registers)) + values := make([]flow.RegisterValue, 0, len(m.Registers)) + + for _, v := range data { + ids = append(ids, v.Key) + values = append(values, v.Value) + } + + return ids, values +} + func fullKey(owner, controller, key string) string { // https://en.wikipedia.org/wiki/C0_and_C1_control_codes#Field_separators return strings.Join([]string{owner, controller, key}, "\x1F") diff --git a/fvm/transactionStorageLimiter.go b/fvm/transactionStorageLimiter.go new file mode 100644 index 00000000000..529b448d46a --- /dev/null +++ b/fvm/transactionStorageLimiter.go @@ -0,0 +1,70 @@ +package fvm + +import ( + "encoding/binary" + "fmt" + + "github.com/onflow/flow-go/fvm/state" + "github.com/onflow/flow-go/model/flow" +) + +type TransactionStorageLimiter struct{} + +func NewTransactionStorageLimiter() *TransactionStorageLimiter { + return &TransactionStorageLimiter{} +} + +func (d *TransactionStorageLimiter) Process( + _ *VirtualMachine, + _ Context, + _ *TransactionProcedure, + ledger state.Ledger, +) error { + registerIds, _ := ledger.RegisterUpdates() + + checked := map[string]struct{}{} + + for _, id := range registerIds { + owner := id.Owner + + if _, wasChecked := checked[owner]; wasChecked { + // we already checked this account, move on + continue + } + checked[owner] = struct{}{} + + // get capacity. Capacity will be 0 if not set yet. This can only be in the case of a bug. + // It should have been set during account creation + capacity, err := getStorageRegisterValue(ledger, owner, state.StorageCapacityRegisterName) + if err != nil { + return err + } + usage, err := getStorageRegisterValue(ledger, owner, state.StorageUsedRegisterName) + if err != nil { + return err + } + + if usage > capacity { + return &OverStorageCapacityError{ + Address: flow.HexToAddress(owner), + StorageUsed: usage, + StorageCapacity: capacity, + } + } + } + return nil +} + +func getStorageRegisterValue(ledger state.Ledger, owner, key string) (uint64, error) { + bValue, err := ledger.Get(owner, "", key) + if err != nil { + return 0, err + } + if len(bValue) == 0 { + return 0, nil + } + if len(bValue) != 8 { + panic(fmt.Sprintf("storage size of %s should be 8 bytes", key)) + } + return binary.LittleEndian.Uint64(bValue), nil +} diff --git a/fvm/transactionStorageLimiter_test.go b/fvm/transactionStorageLimiter_test.go new file mode 100644 index 00000000000..065a0ee5439 --- /dev/null +++ b/fvm/transactionStorageLimiter_test.go @@ -0,0 +1,154 @@ +package fvm_test + +import ( + "encoding/binary" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/onflow/flow-go/fvm" + "github.com/onflow/flow-go/fvm/state" + "github.com/onflow/flow-go/model/flow" +) + +func TestTransactionStorageLimiter_Process(t *testing.T) { + t.Run("capacity > storage == OK", func(t *testing.T) { + owner := "1" + ledger := newMockLedger( + []string{owner}, + []OwnerKeyValue{ + storageUsedOKV(owner, 99), + storageCapacityOKV(owner, 100), + }) + d := &fvm.TransactionStorageLimiter{} + if err := d.Process(nil, fvm.Context{}, nil, ledger); err != nil { + t.Errorf("Transaction with higher capacity than storage used should work") + } + }) + t.Run("capacity = storage == OK", func(t *testing.T) { + owner := "1" + ledger := newMockLedger( + []string{owner}, + []OwnerKeyValue{ + storageUsedOKV(owner, 100), + storageCapacityOKV(owner, 100), + }) + d := &fvm.TransactionStorageLimiter{} + if err := d.Process(nil, fvm.Context{}, nil, ledger); err != nil { + t.Errorf("Transaction with equal capacity than storage used should work") + } + }) + t.Run("capacity < storage == Not OK", func(t *testing.T) { + owner := "1" + ledger := newMockLedger( + []string{owner}, + []OwnerKeyValue{ + storageUsedOKV(owner, 101), + storageCapacityOKV(owner, 100), + }) + d := &fvm.TransactionStorageLimiter{} + if err := d.Process(nil, fvm.Context{}, nil, ledger); err == nil { + t.Errorf("Transaction with lower capacity than storage used should fail") + } + }) + + t.Run("two registers change with the same account: call get only once", func(t *testing.T) { + owner := "1" + ledger := newMockLedger( + []string{owner, owner}, + []OwnerKeyValue{ + storageUsedOKV(owner, 99), + storageCapacityOKV(owner, 100), + }) + d := &fvm.TransactionStorageLimiter{} + if err := d.Process(nil, fvm.Context{}, nil, ledger); err != nil { + t.Errorf("Transaction should no fail") + } + require.Equal(t, 2, ledger.GetCalls[owner]) + }) + + t.Run("two registers change with different same accounts: call get once per account", func(t *testing.T) { + owner1 := "1" + owner2 := "2" + ledger := newMockLedger( + []string{owner1, owner2}, + []OwnerKeyValue{ + storageUsedOKV(owner1, 99), + storageCapacityOKV(owner1, 100), + storageUsedOKV(owner2, 999), + storageCapacityOKV(owner2, 1000), + }) + d := &fvm.TransactionStorageLimiter{} + if err := d.Process(nil, fvm.Context{}, nil, ledger); err != nil { + t.Errorf("Transaction should no fail") + } + require.Equal(t, 2, ledger.GetCalls[owner1]) + require.Equal(t, 2, ledger.GetCalls[owner2]) + }) +} + +type MockLedger struct { + UpdatedRegisterKeys []flow.RegisterID + StorageValues map[string]map[string]flow.RegisterValue + GetCalls map[string]int +} + +type OwnerKeyValue struct { + Owner string + Key string + Value uint64 +} + +func storageUsedOKV(owner string, value uint64) OwnerKeyValue { + return OwnerKeyValue{ + Owner: owner, + Key: state.StorageUsedRegisterName, + Value: value, + } +} + +func storageCapacityOKV(owner string, value uint64) OwnerKeyValue { + return OwnerKeyValue{ + Owner: owner, + Key: state.StorageCapacityRegisterName, + Value: value, + } +} + +func newMockLedger(updatedKeys []string, ownerKeyStorageValue []OwnerKeyValue) MockLedger { + storageValues := make(map[string]map[string]flow.RegisterValue) + for _, okv := range ownerKeyStorageValue { + _, exists := storageValues[okv.Owner] + if !exists { + storageValues[okv.Owner] = make(map[string]flow.RegisterValue) + } + buf := make([]byte, 8) + binary.LittleEndian.PutUint64(buf, okv.Value) + storageValues[okv.Owner][okv.Key] = buf + } + updatedRegisters := make([]flow.RegisterID, len(updatedKeys)) + for i, key := range updatedKeys { + updatedRegisters[i] = flow.RegisterID{ + Owner: key, + Controller: "", + Key: "", + } + } + + return MockLedger{ + UpdatedRegisterKeys: updatedRegisters, + StorageValues: storageValues, + GetCalls: make(map[string]int), + } +} + +func (l MockLedger) Set(_, _, _ string, _ flow.RegisterValue) {} +func (l MockLedger) Get(owner, _, key string) (flow.RegisterValue, error) { + l.GetCalls[owner] = l.GetCalls[owner] + 1 + return l.StorageValues[owner][key], nil +} +func (l MockLedger) Touch(_, _, _ string) {} +func (l MockLedger) Delete(_, _, _ string) {} +func (l MockLedger) RegisterUpdates() ([]flow.RegisterID, []flow.RegisterValue) { + return l.UpdatedRegisterKeys, []flow.RegisterValue{} +} From 169e0d9682a26d7edc95ad20dfc4f80673ea23fc Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Wed, 21 Oct 2020 20:53:47 +0200 Subject: [PATCH 2/7] added tests --- .../state/bootstrap/bootstrap_test.go | 2 +- fvm/fvm_test.go | 71 ++++++++++- fvm/transactionStorageLimiter.go | 25 +++- fvm/transactionStorageLimiter_test.go | 112 +++++++++++++----- utils/unittest/execution_state.go | 2 +- 5 files changed, 176 insertions(+), 36 deletions(-) diff --git a/engine/execution/state/bootstrap/bootstrap_test.go b/engine/execution/state/bootstrap/bootstrap_test.go index 20ca516d586..3abe0163521 100644 --- a/engine/execution/state/bootstrap/bootstrap_test.go +++ b/engine/execution/state/bootstrap/bootstrap_test.go @@ -44,7 +44,7 @@ func TestBootstrapLedger(t *testing.T) { } func TestBootstrapLedger_ZeroTokenSupply(t *testing.T) { - var expectedStateCommitment, _ = hex.DecodeString("e4e8fcf24e491270ec7546845aa47bcc739a4cc59e0114e10b429a6ba33ea929") + var expectedStateCommitment, _ = hex.DecodeString("846f97a7ab28108f6ef017b178148ecf7520c7b499a739ad94c81c7573746f5c") unittest.RunWithTempDir(t, func(dbDir string) { diff --git a/fvm/fvm_test.go b/fvm/fvm_test.go index cc6dd74fc13..e5a1095a7ff 100644 --- a/fvm/fvm_test.go +++ b/fvm/fvm_test.go @@ -2,6 +2,7 @@ package fvm_test import ( "crypto/rand" + "encoding/base64" "fmt" "strconv" "testing" @@ -16,6 +17,7 @@ import ( "github.com/onflow/flow-go/crypto" "github.com/onflow/flow-go/crypto/hash" + "github.com/onflow/flow-go/engine/execution/state/delta" "github.com/onflow/flow-go/engine/execution/testutil" "github.com/onflow/flow-go/fvm" fvmmock "github.com/onflow/flow-go/fvm/mock" @@ -47,7 +49,8 @@ func vmTest( ctx := fvm.NewContext(opts...) - ledger := state.NewMapLedger() + mapLedger := state.NewMapLedger() + ledger := delta.NewView(mapLedger.Get) err = vm.Run( ctx, @@ -426,6 +429,72 @@ func TestBlockContext_ExecuteTransaction_GasLimit(t *testing.T) { } } +func TestBlockContext_ExecuteTransaction_StorageLimit(t *testing.T) { + rt := runtime.NewInterpreterRuntime() + + chain := flow.Mainnet.Chain() + + vm := fvm.New(rt) + + cache, err := fvm.NewLRUASTCache(CacheSize) + require.NoError(t, err) + + ctx := fvm.NewContext(fvm.WithChain(chain), fvm.WithASTCache(cache)) + + t.Run("Storing too much data fails", func(t *testing.T) { + mapLedger := state.NewMapLedger() + ledger := delta.NewView(mapLedger.Get) + + _ = vm.Run( + ctx, + fvm.Bootstrap(unittest.ServiceAccountPublicKey, unittest.GenesisTokenSupply), + ledger, + ) + + // Create an account private key. + privateKeys, err := testutil.GenerateAccountPrivateKeys(1) + require.NoError(t, err) + + // Bootstrap a ledger, creating accounts with the provided private keys and the root account. + accounts, err := testutil.CreateAccounts(vm, ledger, privateKeys, chain) + require.NoError(t, err) + + b := make([]byte, 100000) // 100k bytes + _, err = rand.Read(b) + require.NoError(t, err) + longString := base64.StdEncoding.EncodeToString(b) + txBody := testutil.CreateContractDeploymentTransaction( + fmt.Sprintf(` + access(all) contract Container { + access(all) resource Counter { + pub var longString: String + init() { + self.longString = "%s" + } + } + } + `, longString), + accounts[0], + chain) + + txBody.SetProposalKey(chain.ServiceAddress(), 0, 0) + txBody.SetPayer(chain.ServiceAddress()) + + err = testutil.SignPayload(txBody, accounts[0], privateKeys[0]) + require.NoError(t, err) + + err = testutil.SignEnvelope(txBody, chain.ServiceAddress(), unittest.ServiceAccountPrivateKey) + require.NoError(t, err) + + tx := fvm.Transaction(txBody) + + err = vm.Run(ctx, tx, ledger) + require.NoError(t, err) + + assert.Equal(t, (&fvm.OverStorageCapacityError{}).Code(), tx.Err.Code()) + }) +} + var createAccountScript = []byte(` transaction { prepare(signer: AuthAccount) { diff --git a/fvm/transactionStorageLimiter.go b/fvm/transactionStorageLimiter.go index 529b448d46a..4c633fd93f4 100644 --- a/fvm/transactionStorageLimiter.go +++ b/fvm/transactionStorageLimiter.go @@ -16,10 +16,12 @@ func NewTransactionStorageLimiter() *TransactionStorageLimiter { func (d *TransactionStorageLimiter) Process( _ *VirtualMachine, - _ Context, + ctx Context, _ *TransactionProcedure, ledger state.Ledger, ) error { + accounts := state.NewAccounts(ledger, ctx.Chain) + registerIds, _ := ledger.RegisterUpdates() checked := map[string]struct{}{} @@ -33,6 +35,16 @@ func (d *TransactionStorageLimiter) Process( } checked[owner] = struct{}{} + // is this an address? + // does it exist? + exists, err := isExistingAccount(accounts, owner) + if err != nil { + return err + } + if !exists { + continue + } + // get capacity. Capacity will be 0 if not set yet. This can only be in the case of a bug. // It should have been set during account creation capacity, err := getStorageRegisterValue(ledger, owner, state.StorageCapacityRegisterName) @@ -68,3 +80,14 @@ func getStorageRegisterValue(ledger state.Ledger, owner, key string) (uint64, er } return binary.LittleEndian.Uint64(bValue), nil } + +func isExistingAccount(accounts *state.Accounts, owner string) (bool, error) { + ownerBytes := []byte(owner) + if len(ownerBytes) != flow.AddressLength { + // not an address + return false, nil + } + + accountExists, err := accounts.Exists(flow.BytesToAddress(ownerBytes)) + return accountExists, err +} diff --git a/fvm/transactionStorageLimiter_test.go b/fvm/transactionStorageLimiter_test.go index 065a0ee5439..5f231264ff1 100644 --- a/fvm/transactionStorageLimiter_test.go +++ b/fvm/transactionStorageLimiter_test.go @@ -12,78 +12,118 @@ import ( ) func TestTransactionStorageLimiter_Process(t *testing.T) { - t.Run("capacity > storage == OK", func(t *testing.T) { - owner := "1" + t.Run("capacity > storage -> OK", func(t *testing.T) { + owner := string(flow.HexToAddress("1").Bytes()) ledger := newMockLedger( []string{owner}, []OwnerKeyValue{ storageUsedOKV(owner, 99), storageCapacityOKV(owner, 100), + accountExists(owner), }) d := &fvm.TransactionStorageLimiter{} - if err := d.Process(nil, fvm.Context{}, nil, ledger); err != nil { - t.Errorf("Transaction with higher capacity than storage used should work") - } + + err := d.Process(nil, fvm.Context{}, nil, ledger) + + require.NoError(t, err, "Transaction with higher capacity than storage used should work") }) - t.Run("capacity = storage == OK", func(t *testing.T) { - owner := "1" + t.Run("capacity = storage -> OK", func(t *testing.T) { + owner := string(flow.HexToAddress("1").Bytes()) ledger := newMockLedger( []string{owner}, []OwnerKeyValue{ storageUsedOKV(owner, 100), storageCapacityOKV(owner, 100), + accountExists(owner), }) d := &fvm.TransactionStorageLimiter{} - if err := d.Process(nil, fvm.Context{}, nil, ledger); err != nil { - t.Errorf("Transaction with equal capacity than storage used should work") - } + + err := d.Process(nil, fvm.Context{}, nil, ledger) + + require.NoError(t, err, "Transaction with equal capacity than storage used should work") }) - t.Run("capacity < storage == Not OK", func(t *testing.T) { - owner := "1" + t.Run("capacity < storage -> Not OK", func(t *testing.T) { + owner := string(flow.HexToAddress("1").Bytes()) ledger := newMockLedger( []string{owner}, []OwnerKeyValue{ storageUsedOKV(owner, 101), storageCapacityOKV(owner, 100), + accountExists(owner), }) d := &fvm.TransactionStorageLimiter{} - if err := d.Process(nil, fvm.Context{}, nil, ledger); err == nil { - t.Errorf("Transaction with lower capacity than storage used should fail") - } - }) - t.Run("two registers change with the same account: call get only once", func(t *testing.T) { - owner := "1" + err := d.Process(nil, fvm.Context{}, nil, ledger) + + require.Error(t, err, "Transaction with lower capacity than storage used should fail") + }) + t.Run("if two registers change on the same account, only check capacity once", func(t *testing.T) { + owner := string(flow.HexToAddress("1").Bytes()) ledger := newMockLedger( []string{owner, owner}, []OwnerKeyValue{ storageUsedOKV(owner, 99), storageCapacityOKV(owner, 100), + accountExists(owner), }) d := &fvm.TransactionStorageLimiter{} - if err := d.Process(nil, fvm.Context{}, nil, ledger); err != nil { - t.Errorf("Transaction should no fail") - } - require.Equal(t, 2, ledger.GetCalls[owner]) - }) - t.Run("two registers change with different same accounts: call get once per account", func(t *testing.T) { - owner1 := "1" - owner2 := "2" + err := d.Process(nil, fvm.Context{}, nil, ledger) + + require.NoError(t, err) + // three touches per account: get exists, get capacity, get used + require.Equal(t, 3, ledger.GetCalls[owner]) + }) + t.Run("two registers change on different accounts, only check capacity once per account", func(t *testing.T) { + owner1 := string(flow.HexToAddress("1").Bytes()) + owner2 := string(flow.HexToAddress("2").Bytes()) ledger := newMockLedger( - []string{owner1, owner2}, + []string{owner1, owner1, owner2, owner2}, []OwnerKeyValue{ storageUsedOKV(owner1, 99), storageCapacityOKV(owner1, 100), + accountExists(owner2), storageUsedOKV(owner2, 999), storageCapacityOKV(owner2, 1000), + accountExists(owner2), }) d := &fvm.TransactionStorageLimiter{} - if err := d.Process(nil, fvm.Context{}, nil, ledger); err != nil { - t.Errorf("Transaction should no fail") - } - require.Equal(t, 2, ledger.GetCalls[owner1]) - require.Equal(t, 2, ledger.GetCalls[owner2]) + + err := d.Process(nil, fvm.Context{}, nil, ledger) + + require.NoError(t, err) + // three touches per account: get exists, get capacity, get used + require.Equal(t, 3, ledger.GetCalls[owner1]) + require.Equal(t, 3, ledger.GetCalls[owner2]) + }) + t.Run("non account registers are ignored", func(t *testing.T) { + owner := "" + ledger := newMockLedger( + []string{owner}, + []OwnerKeyValue{ + storageUsedOKV(owner, 101), + storageCapacityOKV(owner, 100), + accountExists(owner), // it has exists value, but it cannot be parsed as an address + }) + d := &fvm.TransactionStorageLimiter{} + + err := d.Process(nil, fvm.Context{}, nil, ledger) + + require.NoError(t, err) + }) + t.Run("account registers without exists are ignored", func(t *testing.T) { + owner := string(flow.HexToAddress("1").Bytes()) + ledger := newMockLedger( + []string{owner}, + []OwnerKeyValue{ + storageUsedOKV(owner, 101), + storageCapacityOKV(owner, 100), + }) + d := &fvm.TransactionStorageLimiter{} + + err := d.Process(nil, fvm.Context{}, nil, ledger) + + require.NoError(t, err) }) } @@ -115,6 +155,14 @@ func storageCapacityOKV(owner string, value uint64) OwnerKeyValue { } } +func accountExists(owner string) OwnerKeyValue { + return OwnerKeyValue{ + Owner: owner, + Key: "exists", + Value: 1, + } +} + func newMockLedger(updatedKeys []string, ownerKeyStorageValue []OwnerKeyValue) MockLedger { storageValues := make(map[string]map[string]flow.RegisterValue) for _, okv := range ownerKeyStorageValue { diff --git a/utils/unittest/execution_state.go b/utils/unittest/execution_state.go index eb0507711ea..abac1bbcf4e 100644 --- a/utils/unittest/execution_state.go +++ b/utils/unittest/execution_state.go @@ -19,7 +19,7 @@ import ( const ServiceAccountPrivateKeyHex = "e3a08ae3d0461cfed6d6f49bfc25fa899351c39d1bd21fdba8c87595b6c49bb4cc430201" // Pre-calculated state commitment with root account with the above private key -const GenesisStateCommitmentHex = "6415dfb676239ff06990c1264bb47e52f9d8eacb217edee4e118fe2fb34c5e5a" +const GenesisStateCommitmentHex = "f6469bccd13860c8c916d05cc0bfe5c2acfd05895a0300f1d1c3a31d4665db22" var GenesisStateCommitment flow.StateCommitment From e4600840ef2c7a8f5d2372a24bd95ff996fadfaf Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 2 Nov 2020 18:26:47 +0100 Subject: [PATCH 3/7] changes due to accounts refactor --- .../state/bootstrap/bootstrap_test.go | 2 +- fvm/env.go | 7 ++++ fvm/state/accounts.go | 33 ++++++++++++----- fvm/state/accounts_test.go | 15 ++++---- fvm/transactionStorageLimiter.go | 37 +++++-------------- fvm/transactionStorageLimiter_test.go | 5 +-- utils/unittest/execution_state.go | 2 +- 7 files changed, 53 insertions(+), 48 deletions(-) diff --git a/engine/execution/state/bootstrap/bootstrap_test.go b/engine/execution/state/bootstrap/bootstrap_test.go index c9bf95158db..67ee17a434c 100644 --- a/engine/execution/state/bootstrap/bootstrap_test.go +++ b/engine/execution/state/bootstrap/bootstrap_test.go @@ -44,7 +44,7 @@ func TestBootstrapLedger(t *testing.T) { } func TestBootstrapLedger_ZeroTokenSupply(t *testing.T) { - var expectedStateCommitment, _ = hex.DecodeString("489e6a75628cee6ae24662e86d642178c6ab3210d7a213d81de45664bac8cf4f") + var expectedStateCommitment, _ = hex.DecodeString("4989ce4db0a3a46f9db3423735f28124d09c5396acd86b80b4ca9bd0fb075ee0") unittest.RunWithTempDir(t, func(dbDir string) { diff --git a/fvm/env.go b/fvm/env.go index 2b0e1988c94..658530bd805 100644 --- a/fvm/env.go +++ b/fvm/env.go @@ -416,6 +416,13 @@ func (e *transactionEnv) CreateAccount(payer runtime.Address) (address runtime.A return address, err } + // TODO: This is temporary! Need to fix with next PR before this can be merged to master + // this will be in the transaction that created the account. It's here now so I can run the tests. + err = e.accounts.SetStorageCapacity(flowAddress, 100000) + if err != nil { + return address, err + } + if e.ctx.ServiceAccountEnabled { err = e.vm.invokeMetaTransaction( e.ctx, diff --git a/fvm/state/accounts.go b/fvm/state/accounts.go index ff2138f866f..ca4c2384ffc 100644 --- a/fvm/state/accounts.go +++ b/fvm/state/accounts.go @@ -11,10 +11,11 @@ import ( ) const ( - keyExists = "exists" - keyCode = "code" - keyPublicKeyCount = "public_key_count" - storageUsedRegisterName = "storage_used" + keyExists = "exists" + keyCode = "code" + keyPublicKeyCount = "public_key_count" + storageUsedRegisterName = "storage_used" + storageCapacityRegisterName = "storage_capacity" ) var ( @@ -96,11 +97,10 @@ func (a *Accounts) Create(publicKeys []flow.AccountPublicKey, newAddress flow.Ad return err } - // TODO: This is temporary! Need to fix with next PR before this can be merged to master - // this will be in the transaction that created the account. It's here now so I can run the tests. - buffer := make([]byte, 8) - binary.LittleEndian.PutUint64(buffer, 100000) - err = a.setValue(string(newAddress.Bytes()), false, StorageCapacityRegisterName, buffer) + // set storage capacity to 0. + // It must be set with the storage contract before the end of this transaction. + // TODO: for this PR set storage capacity to 100kB, remove this in the next PR to this feature branch + err = a.SetStorageCapacity(newAddress, 100000) if err != nil { return err } @@ -311,6 +311,21 @@ func (a *Accounts) setStorageUsed(owner flow.Address, used uint64) error { return a.setValue(owner, false, storageUsedRegisterName, buffer) } +func (a *Accounts) GetStorageCapacity(owner flow.Address) (uint64, error) { + storageCapacityRegister, err := a.getValue(owner, false, storageCapacityRegisterName) + if err != nil { + return 0, err + } + storageUsed := binary.LittleEndian.Uint64(storageCapacityRegister) + return storageUsed, nil +} + +func (a *Accounts) SetStorageCapacity(owner flow.Address, capacity uint64) error { + buffer := make([]byte, 8) + binary.LittleEndian.PutUint64(buffer, capacity) + return a.setValue(owner, false, storageCapacityRegisterName, buffer) +} + func (a *Accounts) GetValue(address flow.Address, key string) (flow.RegisterValue, error) { return a.getValue(address, false, key) } diff --git a/fvm/state/accounts_test.go b/fvm/state/accounts_test.go index b9472870bb3..95456d9176b 100644 --- a/fvm/state/accounts_test.go +++ b/fvm/state/accounts_test.go @@ -19,7 +19,8 @@ func TestAccounts_Create(t *testing.T) { err := accounts.Create(nil, address) require.NoError(t, err) - require.Equal(t, len(ledger.RegisterTouches), 4) // storage_used + exists + code + key count + // storage_used + storage_capacity + exists + code + key count + require.Equal(t, len(ledger.RegisterTouches), 5) }) t.Run("Fails if account exists", func(t *testing.T) { @@ -85,7 +86,7 @@ func TestAccount_StorageUsed(t *testing.T) { storageUsed, err := accounts.GetStorageUsed(address) require.NoError(t, err) - require.Equal(t, storageUsed, uint64(9)) // exists: 1 byte, storage_used 8 bytes + require.Equal(t, storageUsed, uint64(17)) // exists: 1 byte, storage_ used & capacity 16 bytes }) t.Run("Storage used on register set increases", func(t *testing.T) { @@ -102,7 +103,7 @@ func TestAccount_StorageUsed(t *testing.T) { storageUsed, err := accounts.GetStorageUsed(address) require.NoError(t, err) - require.Equal(t, storageUsed, uint64(9+12)) // exists: 1 byte, storage_used 8 bytes, some_key 12 + require.Equal(t, storageUsed, uint64(17+12)) // exists: 1 byte, storage_ used & capacity 16 bytes, some_key 12 }) t.Run("Storage used on same register set twice to same value stays the same", func(t *testing.T) { @@ -121,7 +122,7 @@ func TestAccount_StorageUsed(t *testing.T) { storageUsed, err := accounts.GetStorageUsed(address) require.NoError(t, err) - require.Equal(t, storageUsed, uint64(9+12)) // exists: 1 byte, storage_used 8 bytes, some_key 12 + require.Equal(t, storageUsed, uint64(17+12)) // exists: 1 byte, storage_ used & capacity 16 bytes, some_key 12 }) t.Run("Storage used on register set twice to larger value increases", func(t *testing.T) { @@ -140,7 +141,7 @@ func TestAccount_StorageUsed(t *testing.T) { storageUsed, err := accounts.GetStorageUsed(address) require.NoError(t, err) - require.Equal(t, storageUsed, uint64(9+13)) // exists: 1 byte, storage_used 8 bytes, some_key 13 + require.Equal(t, storageUsed, uint64(17+13)) // exists: 1 byte, storage_ used & capacity 16 bytes, some_key 13 }) t.Run("Storage used on register set twice to smaller value decreases", func(t *testing.T) { @@ -159,7 +160,7 @@ func TestAccount_StorageUsed(t *testing.T) { storageUsed, err := accounts.GetStorageUsed(address) require.NoError(t, err) - require.Equal(t, storageUsed, uint64(9+11)) // exists: 1 byte, storage_used 8 bytes, some_key 11 + require.Equal(t, storageUsed, uint64(17+11)) // exists: 1 byte, storage_ used & capacity 16 bytes, some_key 11 }) t.Run("Storage used proper value on a complex scenario", func(t *testing.T) { @@ -183,7 +184,7 @@ func TestAccount_StorageUsed(t *testing.T) { storageUsed, err := accounts.GetStorageUsed(address) require.NoError(t, err) - require.Equal(t, storageUsed, uint64(9+34)) // exists: 1 byte, storage_used 8 bytes, other 34 + require.Equal(t, storageUsed, uint64(17+34)) // exists: 1 byte, storage_ used & capacity 16 bytes, other 34 }) } diff --git a/fvm/transactionStorageLimiter.go b/fvm/transactionStorageLimiter.go index 4c633fd93f4..134aae58124 100644 --- a/fvm/transactionStorageLimiter.go +++ b/fvm/transactionStorageLimiter.go @@ -1,9 +1,6 @@ package fvm import ( - "encoding/binary" - "fmt" - "github.com/onflow/flow-go/fvm/state" "github.com/onflow/flow-go/model/flow" ) @@ -16,11 +13,11 @@ func NewTransactionStorageLimiter() *TransactionStorageLimiter { func (d *TransactionStorageLimiter) Process( _ *VirtualMachine, - ctx Context, + _ Context, _ *TransactionProcedure, ledger state.Ledger, ) error { - accounts := state.NewAccounts(ledger, ctx.Chain) + accounts := state.NewAccounts(ledger) registerIds, _ := ledger.RegisterUpdates() @@ -37,7 +34,7 @@ func (d *TransactionStorageLimiter) Process( // is this an address? // does it exist? - exists, err := isExistingAccount(accounts, owner) + address, exists, err := isExistingAccount(accounts, owner) if err != nil { return err } @@ -47,11 +44,11 @@ func (d *TransactionStorageLimiter) Process( // get capacity. Capacity will be 0 if not set yet. This can only be in the case of a bug. // It should have been set during account creation - capacity, err := getStorageRegisterValue(ledger, owner, state.StorageCapacityRegisterName) + capacity, err := accounts.GetStorageCapacity(address) if err != nil { return err } - usage, err := getStorageRegisterValue(ledger, owner, state.StorageUsedRegisterName) + usage, err := accounts.GetStorageUsed(address) if err != nil { return err } @@ -67,27 +64,13 @@ func (d *TransactionStorageLimiter) Process( return nil } -func getStorageRegisterValue(ledger state.Ledger, owner, key string) (uint64, error) { - bValue, err := ledger.Get(owner, "", key) - if err != nil { - return 0, err - } - if len(bValue) == 0 { - return 0, nil - } - if len(bValue) != 8 { - panic(fmt.Sprintf("storage size of %s should be 8 bytes", key)) - } - return binary.LittleEndian.Uint64(bValue), nil -} - -func isExistingAccount(accounts *state.Accounts, owner string) (bool, error) { +func isExistingAccount(accounts *state.Accounts, owner string) (flow.Address, bool, error) { ownerBytes := []byte(owner) if len(ownerBytes) != flow.AddressLength { // not an address - return false, nil + return flow.EmptyAddress, false, nil } - - accountExists, err := accounts.Exists(flow.BytesToAddress(ownerBytes)) - return accountExists, err + flowAddress := flow.BytesToAddress(ownerBytes) + accountExists, err := accounts.Exists(flowAddress) + return flowAddress, accountExists, err } diff --git a/fvm/transactionStorageLimiter_test.go b/fvm/transactionStorageLimiter_test.go index 5f231264ff1..f8e4a3b678b 100644 --- a/fvm/transactionStorageLimiter_test.go +++ b/fvm/transactionStorageLimiter_test.go @@ -7,7 +7,6 @@ import ( "github.com/stretchr/testify/require" "github.com/onflow/flow-go/fvm" - "github.com/onflow/flow-go/fvm/state" "github.com/onflow/flow-go/model/flow" ) @@ -142,7 +141,7 @@ type OwnerKeyValue struct { func storageUsedOKV(owner string, value uint64) OwnerKeyValue { return OwnerKeyValue{ Owner: owner, - Key: state.StorageUsedRegisterName, + Key: "storage_used", Value: value, } } @@ -150,7 +149,7 @@ func storageUsedOKV(owner string, value uint64) OwnerKeyValue { func storageCapacityOKV(owner string, value uint64) OwnerKeyValue { return OwnerKeyValue{ Owner: owner, - Key: state.StorageCapacityRegisterName, + Key: "storage_capacity", Value: value, } } diff --git a/utils/unittest/execution_state.go b/utils/unittest/execution_state.go index 6fe37de26df..fbc55f76384 100644 --- a/utils/unittest/execution_state.go +++ b/utils/unittest/execution_state.go @@ -19,7 +19,7 @@ import ( const ServiceAccountPrivateKeyHex = "e3a08ae3d0461cfed6d6f49bfc25fa899351c39d1bd21fdba8c87595b6c49bb4cc430201" // Pre-calculated state commitment with root account with the above private key -const GenesisStateCommitmentHex = "13035321be788b98c681686ab8460cd989fcfc1dd091bb46c58a031ebcce9cfb" +const GenesisStateCommitmentHex = "5a588ce4bab7a8c20eeb7b25854a78585f35286408eba1fe9c2f64af584701f3" var GenesisStateCommitment flow.StateCommitment From 2b01131075c6b0fd3e778b0e7f5e06c0559d56f4 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 2 Nov 2020 20:19:48 +0100 Subject: [PATCH 4/7] cleanup --- fvm/env.go | 7 ------ fvm/transactionStorageLimiter.go | 4 +-- fvm/transactionStorageLimiter_test.go | 36 +++++++++++++-------------- 3 files changed, 19 insertions(+), 28 deletions(-) diff --git a/fvm/env.go b/fvm/env.go index 658530bd805..2b0e1988c94 100644 --- a/fvm/env.go +++ b/fvm/env.go @@ -416,13 +416,6 @@ func (e *transactionEnv) CreateAccount(payer runtime.Address) (address runtime.A return address, err } - // TODO: This is temporary! Need to fix with next PR before this can be merged to master - // this will be in the transaction that created the account. It's here now so I can run the tests. - err = e.accounts.SetStorageCapacity(flowAddress, 100000) - if err != nil { - return address, err - } - if e.ctx.ServiceAccountEnabled { err = e.vm.invokeMetaTransaction( e.ctx, diff --git a/fvm/transactionStorageLimiter.go b/fvm/transactionStorageLimiter.go index 134aae58124..5a290663178 100644 --- a/fvm/transactionStorageLimiter.go +++ b/fvm/transactionStorageLimiter.go @@ -41,9 +41,7 @@ func (d *TransactionStorageLimiter) Process( if !exists { continue } - - // get capacity. Capacity will be 0 if not set yet. This can only be in the case of a bug. - // It should have been set during account creation + capacity, err := accounts.GetStorageCapacity(address) if err != nil { return err diff --git a/fvm/transactionStorageLimiter_test.go b/fvm/transactionStorageLimiter_test.go index f8e4a3b678b..e5a20594158 100644 --- a/fvm/transactionStorageLimiter_test.go +++ b/fvm/transactionStorageLimiter_test.go @@ -16,8 +16,8 @@ func TestTransactionStorageLimiter_Process(t *testing.T) { ledger := newMockLedger( []string{owner}, []OwnerKeyValue{ - storageUsedOKV(owner, 99), - storageCapacityOKV(owner, 100), + storageUsed(owner, 99), + storageCapacity(owner, 100), accountExists(owner), }) d := &fvm.TransactionStorageLimiter{} @@ -31,8 +31,8 @@ func TestTransactionStorageLimiter_Process(t *testing.T) { ledger := newMockLedger( []string{owner}, []OwnerKeyValue{ - storageUsedOKV(owner, 100), - storageCapacityOKV(owner, 100), + storageUsed(owner, 100), + storageCapacity(owner, 100), accountExists(owner), }) d := &fvm.TransactionStorageLimiter{} @@ -46,8 +46,8 @@ func TestTransactionStorageLimiter_Process(t *testing.T) { ledger := newMockLedger( []string{owner}, []OwnerKeyValue{ - storageUsedOKV(owner, 101), - storageCapacityOKV(owner, 100), + storageUsed(owner, 101), + storageCapacity(owner, 100), accountExists(owner), }) d := &fvm.TransactionStorageLimiter{} @@ -61,8 +61,8 @@ func TestTransactionStorageLimiter_Process(t *testing.T) { ledger := newMockLedger( []string{owner, owner}, []OwnerKeyValue{ - storageUsedOKV(owner, 99), - storageCapacityOKV(owner, 100), + storageUsed(owner, 99), + storageCapacity(owner, 100), accountExists(owner), }) d := &fvm.TransactionStorageLimiter{} @@ -79,11 +79,11 @@ func TestTransactionStorageLimiter_Process(t *testing.T) { ledger := newMockLedger( []string{owner1, owner1, owner2, owner2}, []OwnerKeyValue{ - storageUsedOKV(owner1, 99), - storageCapacityOKV(owner1, 100), + storageUsed(owner1, 99), + storageCapacity(owner1, 100), accountExists(owner2), - storageUsedOKV(owner2, 999), - storageCapacityOKV(owner2, 1000), + storageUsed(owner2, 999), + storageCapacity(owner2, 1000), accountExists(owner2), }) d := &fvm.TransactionStorageLimiter{} @@ -100,8 +100,8 @@ func TestTransactionStorageLimiter_Process(t *testing.T) { ledger := newMockLedger( []string{owner}, []OwnerKeyValue{ - storageUsedOKV(owner, 101), - storageCapacityOKV(owner, 100), + storageUsed(owner, 101), + storageCapacity(owner, 100), accountExists(owner), // it has exists value, but it cannot be parsed as an address }) d := &fvm.TransactionStorageLimiter{} @@ -115,8 +115,8 @@ func TestTransactionStorageLimiter_Process(t *testing.T) { ledger := newMockLedger( []string{owner}, []OwnerKeyValue{ - storageUsedOKV(owner, 101), - storageCapacityOKV(owner, 100), + storageUsed(owner, 101), + storageCapacity(owner, 100), }) d := &fvm.TransactionStorageLimiter{} @@ -138,7 +138,7 @@ type OwnerKeyValue struct { Value uint64 } -func storageUsedOKV(owner string, value uint64) OwnerKeyValue { +func storageUsed(owner string, value uint64) OwnerKeyValue { return OwnerKeyValue{ Owner: owner, Key: "storage_used", @@ -146,7 +146,7 @@ func storageUsedOKV(owner string, value uint64) OwnerKeyValue { } } -func storageCapacityOKV(owner string, value uint64) OwnerKeyValue { +func storageCapacity(owner string, value uint64) OwnerKeyValue { return OwnerKeyValue{ Owner: owner, Key: "storage_capacity", From 51166afa27681d573230c8c41c2e93299a98d20c Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Fri, 6 Nov 2020 17:59:31 +0100 Subject: [PATCH 5/7] review fixes --- .../state/bootstrap/bootstrap_test.go | 2 +- fvm/errors.go | 12 +-- fvm/fvm_test.go | 73 +++++++------------ fvm/state/accounts.go | 18 +++-- fvm/state/ledger.go | 3 - fvm/transactionStorageLimiter.go | 19 +++-- utils/unittest/execution_state.go | 2 +- 7 files changed, 56 insertions(+), 73 deletions(-) diff --git a/engine/execution/state/bootstrap/bootstrap_test.go b/engine/execution/state/bootstrap/bootstrap_test.go index 67ee17a434c..038f72c6c4a 100644 --- a/engine/execution/state/bootstrap/bootstrap_test.go +++ b/engine/execution/state/bootstrap/bootstrap_test.go @@ -44,7 +44,7 @@ func TestBootstrapLedger(t *testing.T) { } func TestBootstrapLedger_ZeroTokenSupply(t *testing.T) { - var expectedStateCommitment, _ = hex.DecodeString("4989ce4db0a3a46f9db3423735f28124d09c5396acd86b80b4ca9bd0fb075ee0") + var expectedStateCommitment, _ = hex.DecodeString("bc1dd2010b35639852bd92bf90af322e8262111c3f6970fef30aab31b90024be") unittest.RunWithTempDir(t, func(dbDir string) { diff --git a/fvm/errors.go b/fvm/errors.go index 935821841c5..8d453493c0f 100644 --- a/fvm/errors.go +++ b/fvm/errors.go @@ -25,7 +25,7 @@ const ( errCodeInvalidHashAlgorithm = 10 - errCodeOverStorageCapacity = 11 + errCodeStorageCapacityExceeded = 11 errCodeExecution = 100 ) @@ -215,19 +215,19 @@ func (e *InvalidHashAlgorithmError) Code() uint32 { return errCodeInvalidHashAlgorithm } -// An OverStorageCapacityError indicates that a given key has an invalid hash algorithm. -type OverStorageCapacityError struct { +// An StorageCapacityExceededError indicates that a given key has an invalid hash algorithm. +type StorageCapacityExceededError struct { Address flow.Address StorageUsed uint64 StorageCapacity uint64 } -func (e *OverStorageCapacityError) Error() string { +func (e *StorageCapacityExceededError) Error() string { return fmt.Sprintf("address %s storage %d is over capacity %d", e.Address, e.StorageUsed, e.StorageCapacity) } -func (e *OverStorageCapacityError) Code() uint32 { - return errCodeOverStorageCapacity +func (e *StorageCapacityExceededError) Code() uint32 { + return errCodeStorageCapacityExceeded } type ExecutionError struct { diff --git a/fvm/fvm_test.go b/fvm/fvm_test.go index e5a1095a7ff..4b92869705c 100644 --- a/fvm/fvm_test.go +++ b/fvm/fvm_test.go @@ -430,41 +430,22 @@ func TestBlockContext_ExecuteTransaction_GasLimit(t *testing.T) { } func TestBlockContext_ExecuteTransaction_StorageLimit(t *testing.T) { - rt := runtime.NewInterpreterRuntime() - - chain := flow.Mainnet.Chain() - - vm := fvm.New(rt) - - cache, err := fvm.NewLRUASTCache(CacheSize) - require.NoError(t, err) - - ctx := fvm.NewContext(fvm.WithChain(chain), fvm.WithASTCache(cache)) - - t.Run("Storing too much data fails", func(t *testing.T) { - mapLedger := state.NewMapLedger() - ledger := delta.NewView(mapLedger.Get) - - _ = vm.Run( - ctx, - fvm.Bootstrap(unittest.ServiceAccountPublicKey, unittest.GenesisTokenSupply), - ledger, - ) - - // Create an account private key. - privateKeys, err := testutil.GenerateAccountPrivateKeys(1) - require.NoError(t, err) + t.Run("Storing too much data fails", vmTest( + func(t *testing.T, vm *fvm.VirtualMachine, chain flow.Chain, ctx fvm.Context, ledger state.Ledger) { + // Create an account private key. + privateKeys, err := testutil.GenerateAccountPrivateKeys(1) + require.NoError(t, err) - // Bootstrap a ledger, creating accounts with the provided private keys and the root account. - accounts, err := testutil.CreateAccounts(vm, ledger, privateKeys, chain) - require.NoError(t, err) + // Bootstrap a ledger, creating accounts with the provided private keys and the root account. + accounts, err := testutil.CreateAccounts(vm, ledger, privateKeys, chain) + require.NoError(t, err) - b := make([]byte, 100000) // 100k bytes - _, err = rand.Read(b) - require.NoError(t, err) - longString := base64.StdEncoding.EncodeToString(b) - txBody := testutil.CreateContractDeploymentTransaction( - fmt.Sprintf(` + b := make([]byte, 100000) // 100k bytes + _, err = rand.Read(b) + require.NoError(t, err) + longString := base64.StdEncoding.EncodeToString(b) + txBody := testutil.CreateContractDeploymentTransaction( + fmt.Sprintf(` access(all) contract Container { access(all) resource Counter { pub var longString: String @@ -474,25 +455,25 @@ func TestBlockContext_ExecuteTransaction_StorageLimit(t *testing.T) { } } `, longString), - accounts[0], - chain) + accounts[0], + chain) - txBody.SetProposalKey(chain.ServiceAddress(), 0, 0) - txBody.SetPayer(chain.ServiceAddress()) + txBody.SetProposalKey(chain.ServiceAddress(), 0, 0) + txBody.SetPayer(chain.ServiceAddress()) - err = testutil.SignPayload(txBody, accounts[0], privateKeys[0]) - require.NoError(t, err) + err = testutil.SignPayload(txBody, accounts[0], privateKeys[0]) + require.NoError(t, err) - err = testutil.SignEnvelope(txBody, chain.ServiceAddress(), unittest.ServiceAccountPrivateKey) - require.NoError(t, err) + err = testutil.SignEnvelope(txBody, chain.ServiceAddress(), unittest.ServiceAccountPrivateKey) + require.NoError(t, err) - tx := fvm.Transaction(txBody) + tx := fvm.Transaction(txBody) - err = vm.Run(ctx, tx, ledger) - require.NoError(t, err) + err = vm.Run(ctx, tx, ledger) + require.NoError(t, err) - assert.Equal(t, (&fvm.OverStorageCapacityError{}).Code(), tx.Err.Code()) - }) + assert.Equal(t, (&fvm.StorageCapacityExceededError{}).Code(), tx.Err.Code()) + })) } var createAccountScript = []byte(` diff --git a/fvm/state/accounts.go b/fvm/state/accounts.go index 1d56fc12322..85125623df4 100644 --- a/fvm/state/accounts.go +++ b/fvm/state/accounts.go @@ -309,19 +309,21 @@ func (a *Accounts) setStorageUsed(address flow.Address, used uint64) error { return a.setValue(address, false, keyStorageUsed, usedBinary) } -func (a *Accounts) GetStorageCapacity(owner flow.Address) (uint64, error) { - storageCapacityRegister, err := a.getValue(owner, false, keyStorageCapacity) +func (a *Accounts) GetStorageCapacity(account flow.Address) (uint64, error) { + storageCapacityRegister, err := a.getValue(account, false, keyStorageCapacity) if err != nil { return 0, err } - storageUsed := binary.LittleEndian.Uint64(storageCapacityRegister) - return storageUsed, nil + storageCapacity, _, err := utils.ReadUint64(storageCapacityRegister) + if err != nil { + return 0, err + } + return storageCapacity, nil } -func (a *Accounts) SetStorageCapacity(owner flow.Address, capacity uint64) error { - buffer := make([]byte, uint64StorageSize) - binary.LittleEndian.PutUint64(buffer, capacity) - return a.setValue(owner, false, keyStorageCapacity, buffer) +func (a *Accounts) SetStorageCapacity(account flow.Address, capacity uint64) error { + capacityBinary := utils.Uint64ToBinary(capacity) + return a.setValue(account, false, keyStorageCapacity, capacityBinary) } func (a *Accounts) GetValue(address flow.Address, key string) (flow.RegisterValue, error) { diff --git a/fvm/state/ledger.go b/fvm/state/ledger.go index 646b33d7cd9..754c76ba763 100644 --- a/fvm/state/ledger.go +++ b/fvm/state/ledger.go @@ -1,7 +1,6 @@ package state import ( - "sort" "strings" "github.com/onflow/flow-go/model/flow" @@ -65,8 +64,6 @@ func (m MapLedger) RegisterUpdates() ([]flow.RegisterID, []flow.RegisterValue) { data = append(data, v) } - sort.Sort(&data) - ids := make([]flow.RegisterID, 0, len(m.Registers)) values := make([]flow.RegisterValue, 0, len(m.Registers)) diff --git a/fvm/transactionStorageLimiter.go b/fvm/transactionStorageLimiter.go index 5a290663178..2e1aec820a5 100644 --- a/fvm/transactionStorageLimiter.go +++ b/fvm/transactionStorageLimiter.go @@ -33,15 +33,19 @@ func (d *TransactionStorageLimiter) Process( checked[owner] = struct{}{} // is this an address? + address, isAddress := addressFromString(owner) + if !isAddress { + continue + } // does it exist? - address, exists, err := isExistingAccount(accounts, owner) + exists, err := accounts.Exists(address) if err != nil { return err } if !exists { continue } - + capacity, err := accounts.GetStorageCapacity(address) if err != nil { return err @@ -52,7 +56,7 @@ func (d *TransactionStorageLimiter) Process( } if usage > capacity { - return &OverStorageCapacityError{ + return &StorageCapacityExceededError{ Address: flow.HexToAddress(owner), StorageUsed: usage, StorageCapacity: capacity, @@ -62,13 +66,12 @@ func (d *TransactionStorageLimiter) Process( return nil } -func isExistingAccount(accounts *state.Accounts, owner string) (flow.Address, bool, error) { +func addressFromString(owner string) (flow.Address, bool) { ownerBytes := []byte(owner) if len(ownerBytes) != flow.AddressLength { // not an address - return flow.EmptyAddress, false, nil + return flow.EmptyAddress, false } - flowAddress := flow.BytesToAddress(ownerBytes) - accountExists, err := accounts.Exists(flowAddress) - return flowAddress, accountExists, err + address := flow.BytesToAddress(ownerBytes) + return address, true } diff --git a/utils/unittest/execution_state.go b/utils/unittest/execution_state.go index fbc55f76384..ac6a9d0ac3f 100644 --- a/utils/unittest/execution_state.go +++ b/utils/unittest/execution_state.go @@ -19,7 +19,7 @@ import ( const ServiceAccountPrivateKeyHex = "e3a08ae3d0461cfed6d6f49bfc25fa899351c39d1bd21fdba8c87595b6c49bb4cc430201" // Pre-calculated state commitment with root account with the above private key -const GenesisStateCommitmentHex = "5a588ce4bab7a8c20eeb7b25854a78585f35286408eba1fe9c2f64af584701f3" +const GenesisStateCommitmentHex = "fa87cbc117134b7654da5baeaaf9433a5850581fcc6e91dd43cf024206aeacb5" var GenesisStateCommitment flow.StateCommitment From 887956867f8fca8f8311ca20ade2fa7097f0bcc6 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 9 Nov 2020 19:44:18 +0100 Subject: [PATCH 6/7] merge fix --- engine/execution/state/bootstrap/bootstrap_test.go | 2 +- fvm/fvm_test.go | 1 + utils/unittest/execution_state.go | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/engine/execution/state/bootstrap/bootstrap_test.go b/engine/execution/state/bootstrap/bootstrap_test.go index 524e4e62b27..151a69a5c24 100644 --- a/engine/execution/state/bootstrap/bootstrap_test.go +++ b/engine/execution/state/bootstrap/bootstrap_test.go @@ -44,7 +44,7 @@ func TestBootstrapLedger(t *testing.T) { } func TestBootstrapLedger_ZeroTokenSupply(t *testing.T) { - var expectedStateCommitment, _ = hex.DecodeString("d1ac84222a9e6312d288f918b16f1c887128ee1a1fb7506ad4c3e55a0b491f7b") + var expectedStateCommitment, _ = hex.DecodeString("f3b24f02064671fd2384c4283a60afbfecf94306796b4881cd915524886f19a5") unittest.RunWithTempDir(t, func(dbDir string) { diff --git a/fvm/fvm_test.go b/fvm/fvm_test.go index 9361eef463b..2ea0acfb5cb 100644 --- a/fvm/fvm_test.go +++ b/fvm/fvm_test.go @@ -474,6 +474,7 @@ func TestBlockContext_ExecuteTransaction_StorageLimit(t *testing.T) { require.NoError(t, err) longString := base64.StdEncoding.EncodeToString(b) txBody := testutil.CreateContractDeploymentTransaction( + "Container", fmt.Sprintf(` access(all) contract Container { access(all) resource Counter { diff --git a/utils/unittest/execution_state.go b/utils/unittest/execution_state.go index ac6a9d0ac3f..218f042da66 100644 --- a/utils/unittest/execution_state.go +++ b/utils/unittest/execution_state.go @@ -19,7 +19,7 @@ import ( const ServiceAccountPrivateKeyHex = "e3a08ae3d0461cfed6d6f49bfc25fa899351c39d1bd21fdba8c87595b6c49bb4cc430201" // Pre-calculated state commitment with root account with the above private key -const GenesisStateCommitmentHex = "fa87cbc117134b7654da5baeaaf9433a5850581fcc6e91dd43cf024206aeacb5" +const GenesisStateCommitmentHex = "7291027f2d25a217217a48fd68d2eadc2284f0c668780798c165e60a211c6ddd" var GenesisStateCommitment flow.StateCommitment From 4c540ff22f71898262e34e3f15c77e8c9cda1a09 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 16 Nov 2020 16:44:45 +0100 Subject: [PATCH 7/7] formatting fix --- fvm/state/accounts.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/fvm/state/accounts.go b/fvm/state/accounts.go index 5796c54723b..fa423af021d 100644 --- a/fvm/state/accounts.go +++ b/fvm/state/accounts.go @@ -14,13 +14,13 @@ import ( ) const ( - keyExists = "exists" - keyCode = "code" - keyContractNames = "contract_names" - keyPublicKeyCount = "public_key_count" - keyStorageUsed = "storage_used" + keyExists = "exists" + keyCode = "code" + keyContractNames = "contract_names" + keyPublicKeyCount = "public_key_count" + keyStorageUsed = "storage_used" keyStorageCapacity = "storage_capacity" - uint64StorageSize = 8 + uint64StorageSize = 8 ) var (