From 91a4bc5bebc415d81f6b2c50b3e4c8d8e4d5b74d Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 7 Apr 2022 16:13:06 +0300 Subject: [PATCH 1/4] core: use proper DAO to get ExecFeeFactor The usage of the Blockchain's one leads to the same ExecFeeFactor within a single block. What we need is to update ExecFeeFactor after each transaction invocation, thus, cached DAO should be used as it contains all relevant changes. --- pkg/compiler/interop_test.go | 2 +- pkg/core/blockchain.go | 9 ++++++++- pkg/core/interop/context.go | 7 +------ pkg/core/interop/crypto/ecdsa_test.go | 2 +- pkg/core/native/native_test/common_test.go | 21 +++++++++++++++------ 5 files changed, 26 insertions(+), 15 deletions(-) diff --git a/pkg/compiler/interop_test.go b/pkg/compiler/interop_test.go index b01cdd373..65c43b1f5 100644 --- a/pkg/compiler/interop_test.go +++ b/pkg/compiler/interop_test.go @@ -200,7 +200,7 @@ func TestAppCall(t *testing.T) { } fc := fakechain.NewFakeChain() - ic := interop.NewContext(trigger.Application, fc, dao.NewSimple(storage.NewMemoryStore(), false, false), contractGetter, nil, nil, nil, zaptest.NewLogger(t)) + ic := interop.NewContext(trigger.Application, fc, dao.NewSimple(storage.NewMemoryStore(), false, false), interop.DefaultBaseExecFee, contractGetter, nil, nil, nil, zaptest.NewLogger(t)) t.Run("valid script", func(t *testing.T) { src := getAppCallScript(fmt.Sprintf("%#v", ih.BytesBE())) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index 6dff70d00..926e612de 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -2304,7 +2304,14 @@ func (bc *Blockchain) ManagementContractHash() util.Uint160 { } func (bc *Blockchain) newInteropContext(trigger trigger.Type, d *dao.Simple, block *block.Block, tx *transaction.Transaction) *interop.Context { - ic := interop.NewContext(trigger, bc, d, bc.contracts.Management.GetContract, bc.contracts.Contracts, block, tx, bc.log) + baseExecFee := int64(interop.DefaultBaseExecFee) + + if block == nil || block.Index != 0 { + // Use provided dao instead of Blockchain's one to fetch possible ExecFeeFactor + // changes that were not yet persisted to Blockchain's dao. + baseExecFee = bc.contracts.Policy.GetExecFeeFactorInternal(d) + } + ic := interop.NewContext(trigger, bc, d, baseExecFee, bc.contracts.Management.GetContract, bc.contracts.Contracts, block, tx, bc.log) ic.Functions = systemInterops switch { case tx != nil: diff --git a/pkg/core/interop/context.go b/pkg/core/interop/context.go index e5418a65f..f4792645a 100644 --- a/pkg/core/interop/context.go +++ b/pkg/core/interop/context.go @@ -67,15 +67,10 @@ type Context struct { } // NewContext returns new interop context. -func NewContext(trigger trigger.Type, bc Ledger, d *dao.Simple, +func NewContext(trigger trigger.Type, bc Ledger, d *dao.Simple, baseExecFee int64, getContract func(*dao.Simple, util.Uint160) (*state.Contract, error), natives []Contract, block *block.Block, tx *transaction.Transaction, log *zap.Logger) *Context { - baseExecFee := int64(DefaultBaseExecFee) dao := d.GetPrivate() - - if bc != nil && (block == nil || block.Index != 0) { - baseExecFee = bc.GetBaseExecFee() - } return &Context{ Chain: bc, Network: uint32(bc.GetConfig().Magic), diff --git a/pkg/core/interop/crypto/ecdsa_test.go b/pkg/core/interop/crypto/ecdsa_test.go index 3331d7f25..5f6839674 100644 --- a/pkg/core/interop/crypto/ecdsa_test.go +++ b/pkg/core/interop/crypto/ecdsa_test.go @@ -72,7 +72,7 @@ func initCheckMultisigVMNoArgs(container *transaction.Transaction) *vm.VM { trigger.Verification, fakechain.NewFakeChain(), dao.NewSimple(storage.NewMemoryStore(), false, false), - nil, nil, nil, + interop.DefaultBaseExecFee, nil, nil, nil, container, nil) ic.Container = container diff --git a/pkg/core/native/native_test/common_test.go b/pkg/core/native/native_test/common_test.go index bf9c900ce..851b18e16 100644 --- a/pkg/core/native/native_test/common_test.go +++ b/pkg/core/native/native_test/common_test.go @@ -55,16 +55,25 @@ func testGetSet(t *testing.T, c *neotest.ContractInvoker, name string, defaultVa c.AddNewBlock(t, txSet, txGet) c.CheckHalt(t, txSet.Hash(), stackitem.Null{}) - if name != "GasPerBlock" { // GasPerBlock is set on the next block - c.CheckHalt(t, txGet.Hash(), stackitem.Make(defaultValue+1)) - } else { + switch name { + case "GasPerBlock": + // GasPerBlock is set on the next block c.CheckHalt(t, txGet.Hash(), stackitem.Make(defaultValue)) c.AddNewBlock(t) randomInvoker.Invoke(t, defaultValue+1, getName) + case "ExecFeeFactor": + // ExecFeeFactor was risen, so the second transaction will fail because + // of gas limit exceeding (its fees are out-of-date). + c.CheckFault(t, txGet.Hash(), "gas limit exceeded") + // Set in a separate block. + committeeInvoker.Invoke(t, stackitem.Null{}, setName, defaultValue+1) + // Get in the next block. + randomInvoker.Invoke(t, defaultValue+1, getName) + default: + c.CheckHalt(t, txGet.Hash(), stackitem.Make(defaultValue+1)) + // Get in the next block. + randomInvoker.Invoke(t, defaultValue+1, getName) } - - // Get in the next block. - randomInvoker.Invoke(t, defaultValue+1, getName) }) } From 544f2c2cb2be9bd2e7004d5c26905a4a3528f31c Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 8 Apr 2022 12:27:25 +0300 Subject: [PATCH 2/4] core: use proper storage price within the whole interop context We shouldn't use StoragePrice from Blockchain because its dao doesn't contain the whole set of changes from previouse transactions in the current block. Instead, we should use an updated storage price for each transaction and retrieve the price from cached DAO. --- pkg/compiler/interop_test.go | 3 +- pkg/core/blockchain.go | 9 +++- pkg/core/interop/context.go | 68 +++++++++++++++------------ pkg/core/interop/crypto/ecdsa_test.go | 3 +- pkg/core/interop_system.go | 2 +- pkg/core/native/interop.go | 2 +- pkg/core/native/management.go | 2 +- 7 files changed, 51 insertions(+), 38 deletions(-) diff --git a/pkg/compiler/interop_test.go b/pkg/compiler/interop_test.go index 65c43b1f5..3243f4a1d 100644 --- a/pkg/compiler/interop_test.go +++ b/pkg/compiler/interop_test.go @@ -12,6 +12,7 @@ import ( "github.com/nspcc-dev/neo-go/pkg/core" "github.com/nspcc-dev/neo-go/pkg/core/dao" "github.com/nspcc-dev/neo-go/pkg/core/interop" + "github.com/nspcc-dev/neo-go/pkg/core/native" "github.com/nspcc-dev/neo-go/pkg/core/state" "github.com/nspcc-dev/neo-go/pkg/core/storage" "github.com/nspcc-dev/neo-go/pkg/crypto/hash" @@ -200,7 +201,7 @@ func TestAppCall(t *testing.T) { } fc := fakechain.NewFakeChain() - ic := interop.NewContext(trigger.Application, fc, dao.NewSimple(storage.NewMemoryStore(), false, false), interop.DefaultBaseExecFee, contractGetter, nil, nil, nil, zaptest.NewLogger(t)) + ic := interop.NewContext(trigger.Application, fc, dao.NewSimple(storage.NewMemoryStore(), false, false), interop.DefaultBaseExecFee, native.DefaultStoragePrice, contractGetter, nil, nil, nil, zaptest.NewLogger(t)) t.Run("valid script", func(t *testing.T) { src := getAppCallScript(fmt.Sprintf("%#v", ih.BytesBE())) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index 926e612de..19045208f 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -2305,13 +2305,18 @@ func (bc *Blockchain) ManagementContractHash() util.Uint160 { func (bc *Blockchain) newInteropContext(trigger trigger.Type, d *dao.Simple, block *block.Block, tx *transaction.Transaction) *interop.Context { baseExecFee := int64(interop.DefaultBaseExecFee) - if block == nil || block.Index != 0 { // Use provided dao instead of Blockchain's one to fetch possible ExecFeeFactor // changes that were not yet persisted to Blockchain's dao. baseExecFee = bc.contracts.Policy.GetExecFeeFactorInternal(d) } - ic := interop.NewContext(trigger, bc, d, baseExecFee, bc.contracts.Management.GetContract, bc.contracts.Contracts, block, tx, bc.log) + baseStorageFee := int64(native.DefaultStoragePrice) + if block == nil || block.Index != 0 { + // Use provided dao instead of Blockchain's one to fetch possible StoragePrice + // changes that were not yet persisted to Blockchain's dao. + baseStorageFee = bc.contracts.Policy.GetStoragePriceInternal(d) + } + ic := interop.NewContext(trigger, bc, d, baseExecFee, baseStorageFee, bc.contracts.Management.GetContract, bc.contracts.Contracts, block, tx, bc.log) ic.Functions = systemInterops switch { case tx != nil: diff --git a/pkg/core/interop/context.go b/pkg/core/interop/context.go index f4792645a..22c1486e8 100644 --- a/pkg/core/interop/context.go +++ b/pkg/core/interop/context.go @@ -41,48 +41,49 @@ type Ledger interface { GetBlock(hash util.Uint256) (*block.Block, error) GetConfig() config.ProtocolConfiguration GetHeaderHash(int) util.Uint256 - GetStoragePrice() int64 } // Context represents context in which interops are executed. type Context struct { - Chain Ledger - Container hash.Hashable - Network uint32 - Natives []Contract - Trigger trigger.Type - Block *block.Block - NonceData [16]byte - Tx *transaction.Transaction - DAO *dao.Simple - Notifications []state.NotificationEvent - Log *zap.Logger - VM *vm.VM - Functions []Function - Invocations map[util.Uint160]int - cancelFuncs []context.CancelFunc - getContract func(*dao.Simple, util.Uint160) (*state.Contract, error) - baseExecFee int64 - signers []transaction.Signer + Chain Ledger + Container hash.Hashable + Network uint32 + Natives []Contract + Trigger trigger.Type + Block *block.Block + NonceData [16]byte + Tx *transaction.Transaction + DAO *dao.Simple + Notifications []state.NotificationEvent + Log *zap.Logger + VM *vm.VM + Functions []Function + Invocations map[util.Uint160]int + cancelFuncs []context.CancelFunc + getContract func(*dao.Simple, util.Uint160) (*state.Contract, error) + baseExecFee int64 + baseStorageFee int64 + signers []transaction.Signer } // NewContext returns new interop context. -func NewContext(trigger trigger.Type, bc Ledger, d *dao.Simple, baseExecFee int64, +func NewContext(trigger trigger.Type, bc Ledger, d *dao.Simple, baseExecFee, baseStorageFee int64, getContract func(*dao.Simple, util.Uint160) (*state.Contract, error), natives []Contract, block *block.Block, tx *transaction.Transaction, log *zap.Logger) *Context { dao := d.GetPrivate() return &Context{ - Chain: bc, - Network: uint32(bc.GetConfig().Magic), - Natives: natives, - Trigger: trigger, - Block: block, - Tx: tx, - DAO: dao, - Log: log, - Invocations: make(map[util.Uint160]int), - getContract: getContract, - baseExecFee: baseExecFee, + Chain: bc, + Network: uint32(bc.GetConfig().Magic), + Natives: natives, + Trigger: trigger, + Block: block, + Tx: tx, + DAO: dao, + Log: log, + Invocations: make(map[util.Uint160]int), + getContract: getContract, + baseExecFee: baseExecFee, + baseStorageFee: baseStorageFee, } } @@ -288,6 +289,11 @@ func (ic *Context) BaseExecFee() int64 { return ic.baseExecFee } +// BaseStorageFee represents price for storing one byte of data in the contract storage. +func (ic *Context) BaseStorageFee() int64 { + return ic.baseStorageFee +} + // SyscallHandler handles syscall with id. func (ic *Context) SyscallHandler(_ *vm.VM, id uint32) error { f := ic.GetFunction(id) diff --git a/pkg/core/interop/crypto/ecdsa_test.go b/pkg/core/interop/crypto/ecdsa_test.go index 5f6839674..9cfb370e1 100644 --- a/pkg/core/interop/crypto/ecdsa_test.go +++ b/pkg/core/interop/crypto/ecdsa_test.go @@ -10,6 +10,7 @@ import ( "github.com/nspcc-dev/neo-go/pkg/core/dao" "github.com/nspcc-dev/neo-go/pkg/core/fee" "github.com/nspcc-dev/neo-go/pkg/core/interop" + "github.com/nspcc-dev/neo-go/pkg/core/native" "github.com/nspcc-dev/neo-go/pkg/core/storage" "github.com/nspcc-dev/neo-go/pkg/core/transaction" "github.com/nspcc-dev/neo-go/pkg/crypto/hash" @@ -72,7 +73,7 @@ func initCheckMultisigVMNoArgs(container *transaction.Transaction) *vm.VM { trigger.Verification, fakechain.NewFakeChain(), dao.NewSimple(storage.NewMemoryStore(), false, false), - interop.DefaultBaseExecFee, nil, nil, nil, + interop.DefaultBaseExecFee, native.DefaultStoragePrice, nil, nil, nil, container, nil) ic.Container = container diff --git a/pkg/core/interop_system.go b/pkg/core/interop_system.go index 3a6facde2..42312f1f3 100644 --- a/pkg/core/interop_system.go +++ b/pkg/core/interop_system.go @@ -126,7 +126,7 @@ func putWithContext(ic *interop.Context, stc *StorageContext, key []byte, value sizeInc = (len(si)-1)/4 + 1 + len(value) - len(si) } } - if !ic.VM.AddGas(int64(sizeInc) * ic.Chain.GetStoragePrice()) { + if !ic.VM.AddGas(int64(sizeInc) * ic.BaseStorageFee()) { return errGasLimitExceeded } ic.DAO.PutStorageItem(stc.ID, key, value) diff --git a/pkg/core/native/interop.go b/pkg/core/native/interop.go index c97850138..92fdf3717 100644 --- a/pkg/core/native/interop.go +++ b/pkg/core/native/interop.go @@ -43,7 +43,7 @@ func Call(ic *interop.Context) error { version, m.MD.Name, ic.VM.Context().GetCallFlags(), m.RequiredFlags) } invokeFee := m.CPUFee*ic.Chain.GetBaseExecFee() + - m.StorageFee*ic.Chain.GetStoragePrice() + m.StorageFee*ic.BaseStorageFee() if !ic.VM.AddGas(invokeFee) { return errors.New("gas limit exceeded") } diff --git a/pkg/core/native/management.go b/pkg/core/native/management.go index 4591133c3..dc9257331 100644 --- a/pkg/core/native/management.go +++ b/pkg/core/native/management.go @@ -198,7 +198,7 @@ func (m *Management) getNefAndManifestFromItems(ic *interop.Context, args []stac return nil, nil, fmt.Errorf("invalid manifest: %w", err) } - gas := ic.Chain.GetStoragePrice() * int64(len(nefBytes)+len(manifestBytes)) + gas := ic.BaseStorageFee() * int64(len(nefBytes)+len(manifestBytes)) if isDeploy { fee := m.minimumDeploymentFee(ic.DAO) if fee > gas { From d3672eb14a8d30400f2d1d9f18840bdd034ed783 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 8 Apr 2022 12:31:55 +0300 Subject: [PATCH 3/4] core: use BaseExecFee from InteropContext instead of Blockchain's one The InteropContext's one contains all relevant fee changes applied in the prevouse transactions of the current block. --- pkg/consensus/consensus.go | 1 + pkg/core/interop/context.go | 1 - pkg/core/native/interop.go | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/consensus/consensus.go b/pkg/consensus/consensus.go index e7a389f03..585e34aa8 100644 --- a/pkg/consensus/consensus.go +++ b/pkg/consensus/consensus.go @@ -56,6 +56,7 @@ type Ledger interface { PoolTx(t *transaction.Transaction, pools ...*mempool.Pool) error SubscribeForBlocks(ch chan<- *coreb.Block) UnsubscribeFromBlocks(ch chan<- *coreb.Block) + GetBaseExecFee() int64 interop.Ledger mempool.Feer } diff --git a/pkg/core/interop/context.go b/pkg/core/interop/context.go index 22c1486e8..9217e8d58 100644 --- a/pkg/core/interop/context.go +++ b/pkg/core/interop/context.go @@ -37,7 +37,6 @@ const ( type Ledger interface { BlockHeight() uint32 CurrentBlockHash() util.Uint256 - GetBaseExecFee() int64 GetBlock(hash util.Uint256) (*block.Block, error) GetConfig() config.ProtocolConfiguration GetHeaderHash(int) util.Uint256 diff --git a/pkg/core/native/interop.go b/pkg/core/native/interop.go index 92fdf3717..3fd2ab57a 100644 --- a/pkg/core/native/interop.go +++ b/pkg/core/native/interop.go @@ -42,7 +42,7 @@ func Call(ic *interop.Context) error { return fmt.Errorf("missing call flags for native %d `%s` operation call: %05b vs %05b", version, m.MD.Name, ic.VM.Context().GetCallFlags(), m.RequiredFlags) } - invokeFee := m.CPUFee*ic.Chain.GetBaseExecFee() + + invokeFee := m.CPUFee*ic.BaseExecFee() + m.StorageFee*ic.BaseStorageFee() if !ic.VM.AddGas(invokeFee) { return errors.New("gas limit exceeded") From 51a54fa2486290b644e158dc6c7d953f791711af Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 8 Apr 2022 12:49:25 +0300 Subject: [PATCH 4/4] core: return default BaseExecFee if blockchain height is 0 For (bc *Blockchain).GetBaseExecFee(). --- pkg/core/blockchain.go | 3 +++ pkg/core/blockchain_core_test.go | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index 19045208f..6ffdba4ed 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -2341,6 +2341,9 @@ func (bc *Blockchain) RegisterPostBlock(f func(func(*transaction.Transaction, *m // GetBaseExecFee return execution price for `NOP`. func (bc *Blockchain) GetBaseExecFee() int64 { + if bc.BlockHeight() == 0 { + return interop.DefaultBaseExecFee + } return bc.contracts.Policy.GetExecFeeFactorInternal(bc.dao) } diff --git a/pkg/core/blockchain_core_test.go b/pkg/core/blockchain_core_test.go index 4a68fb16c..fbfe43213 100644 --- a/pkg/core/blockchain_core_test.go +++ b/pkg/core/blockchain_core_test.go @@ -11,10 +11,12 @@ import ( "github.com/nspcc-dev/neo-go/internal/testchain" "github.com/nspcc-dev/neo-go/pkg/config" "github.com/nspcc-dev/neo-go/pkg/core/block" + "github.com/nspcc-dev/neo-go/pkg/core/dao" "github.com/nspcc-dev/neo-go/pkg/core/state" "github.com/nspcc-dev/neo-go/pkg/core/storage" "github.com/nspcc-dev/neo-go/pkg/core/transaction" "github.com/nspcc-dev/neo-go/pkg/smartcontract" + "github.com/nspcc-dev/neo-go/pkg/smartcontract/trigger" "github.com/nspcc-dev/neo-go/pkg/util" "github.com/nspcc-dev/neo-go/pkg/util/slice" "github.com/nspcc-dev/neo-go/pkg/vm/opcode" @@ -320,3 +322,22 @@ func setSigner(tx *transaction.Transaction, h util.Uint160) { Scopes: transaction.Global, }} } + +// This test checks that value of BaseExecFee returned from corresponding Blockchain's method matches +// the one provided to the constructor of new interop context. +func TestBlockchain_BaseExecFeeBaseStoragePrice_Compat(t *testing.T) { + bc := newTestChain(t) + + check := func(t *testing.T) { + ic := bc.newInteropContext(trigger.Application, dao.NewSimple(storage.NewMemoryStore(), bc.config.StateRootInHeader, bc.config.P2PSigExtensions), bc.topBlock.Load().(*block.Block), nil) + require.Equal(t, bc.GetBaseExecFee(), ic.BaseExecFee()) + require.Equal(t, bc.GetStoragePrice(), ic.BaseStorageFee()) + } + t.Run("zero block", func(t *testing.T) { + check(t) + }) + t.Run("non-zero block", func(t *testing.T) { + require.NoError(t, bc.AddBlock(bc.newBlock())) + check(t) + }) +}