From 2924ecd453d8ab2e313598fd5baa71c8bd150a8c Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Thu, 29 Oct 2020 19:10:28 +0300 Subject: [PATCH 1/4] core: use ic.SpawnVM() in tests, not vm.New() vm.New() sets some arbitrary trigger while interop context always sets something appropriate according to its state. --- pkg/core/interop_neo_test.go | 9 ++++----- pkg/core/interop_system_test.go | 29 +++++++++++++---------------- pkg/core/native_designate_test.go | 2 +- pkg/core/native_neo_test.go | 5 ++--- 4 files changed, 20 insertions(+), 25 deletions(-) diff --git a/pkg/core/interop_neo_test.go b/pkg/core/interop_neo_test.go index 3ce5fd8cb..64dde00ce 100644 --- a/pkg/core/interop_neo_test.go +++ b/pkg/core/interop_neo_test.go @@ -147,10 +147,9 @@ func TestECDSAVerify(t *testing.T) { ic := chain.newInteropContext(trigger.Application, dao.NewSimple(storage.NewMemoryStore(), netmode.UnitTestNet), nil, nil) runCase := func(t *testing.T, isErr bool, result interface{}, args ...interface{}) { - v := vm.New() - ic.VM = v + ic.SpawnVM() for i := range args { - v.Estack().PushVal(args[i]) + ic.VM.Estack().PushVal(args[i]) } var err error @@ -168,8 +167,8 @@ func TestECDSAVerify(t *testing.T) { return } require.NoError(t, err) - require.Equal(t, 1, v.Estack().Len()) - require.Equal(t, result, v.Estack().Pop().Value().(bool)) + require.Equal(t, 1, ic.VM.Estack().Len()) + require.Equal(t, result, ic.VM.Estack().Pop().Value().(bool)) } msg := []byte("test message") diff --git a/pkg/core/interop_system_test.go b/pkg/core/interop_system_test.go index 964276545..00d5644ea 100644 --- a/pkg/core/interop_system_test.go +++ b/pkg/core/interop_system_test.go @@ -506,23 +506,21 @@ func getTestContractState() (*state.Contract, *state.Contract) { } func loadScript(ic *interop.Context, script []byte, args ...interface{}) { - v := vm.New() - v.LoadScriptWithFlags(script, smartcontract.AllowCall) + ic.SpawnVM() + ic.VM.LoadScriptWithFlags(script, smartcontract.AllowCall) for i := range args { - v.Estack().PushVal(args[i]) + ic.VM.Estack().PushVal(args[i]) } - v.GasLimit = -1 - ic.VM = v + ic.VM.GasLimit = -1 } func loadScriptWithHashAndFlags(ic *interop.Context, script []byte, hash util.Uint160, f smartcontract.CallFlag, args ...interface{}) { - v := vm.New() - v.LoadScriptWithHash(script, hash, f) + ic.SpawnVM() + ic.VM.LoadScriptWithHash(script, hash, f) for i := range args { - v.Estack().PushVal(args[i]) + ic.VM.Estack().PushVal(args[i]) } - v.GasLimit = -1 - ic.VM = v + ic.VM.GasLimit = -1 } func TestContractCall(t *testing.T) { @@ -1029,15 +1027,14 @@ func TestMethodCallback(t *testing.T) { require.Error(t, callback.Invoke(ic)) }) t.Run("CallIsNotAllowed", func(t *testing.T) { - v := vm.New() - ic.VM = v - v.Load(currCs.Script) - v.Estack().PushVal("add") - v.Estack().PushVal(rawHash) + ic.SpawnVM() + ic.VM.Load(currCs.Script) + ic.VM.Estack().PushVal("add") + ic.VM.Estack().PushVal(rawHash) require.NoError(t, callback.CreateFromMethod(ic)) args := stackitem.NewArray([]stackitem.Item{stackitem.Make(1), stackitem.Make(5)}) - v.Estack().InsertAt(vm.NewElement(args), 1) + ic.VM.Estack().InsertAt(vm.NewElement(args), 1) require.Error(t, callback.Invoke(ic)) }) }) diff --git a/pkg/core/native_designate_test.go b/pkg/core/native_designate_test.go index 83d7e31c1..bd5d6622f 100644 --- a/pkg/core/native_designate_test.go +++ b/pkg/core/native_designate_test.go @@ -79,7 +79,7 @@ func TestDesignate_DesignateAsRole(t *testing.T) { des := bc.contracts.Designate tx := transaction.New(netmode.UnitTestNet, []byte{}, 0) ic := bc.newInteropContext(trigger.System, bc.dao, nil, tx) - ic.VM = vm.New() + ic.SpawnVM() ic.VM.LoadScript([]byte{byte(opcode.RET)}) pubs, err := des.GetDesignatedByRole(bc.dao, 0xFF) diff --git a/pkg/core/native_neo_test.go b/pkg/core/native_neo_test.go index 407f79df7..8ab37458f 100644 --- a/pkg/core/native_neo_test.go +++ b/pkg/core/native_neo_test.go @@ -13,7 +13,6 @@ import ( "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/vm" "github.com/nspcc-dev/neo-go/pkg/vm/opcode" "github.com/stretchr/testify/require" ) @@ -32,7 +31,7 @@ func TestNEO_Vote(t *testing.T) { neo := bc.contracts.NEO tx := transaction.New(netmode.UnitTestNet, []byte{byte(opcode.PUSH1)}, 0) ic := bc.newInteropContext(trigger.System, bc.dao, nil, tx) - ic.VM = vm.New() + ic.SpawnVM() ic.Block = bc.newBlock(tx) freq := testchain.ValidatorsCount + testchain.CommitteeSize() @@ -126,7 +125,7 @@ func TestNEO_SetGasPerBlock(t *testing.T) { neo := bc.contracts.NEO tx := transaction.New(netmode.UnitTestNet, []byte{}, 0) ic := bc.newInteropContext(trigger.System, bc.dao, nil, tx) - ic.VM = vm.New() + ic.SpawnVM() ic.VM.LoadScript([]byte{byte(opcode.RET)}) h := neo.GetCommitteeAddress() From 2522271161bb48389d5e9b21367745cbcfe23c1b Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Thu, 29 Oct 2020 19:11:47 +0300 Subject: [PATCH 2/4] vm: use Application trigger by default Don't mess with System, it's too powerful to be the default. --- pkg/vm/vm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index a39c27365..a986cd17c 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -87,7 +87,7 @@ type VM struct { // New returns a new VM object ready to load AVM bytecode scripts. func New() *VM { - return NewWithTrigger(trigger.System) + return NewWithTrigger(trigger.Application) } // NewWithTrigger returns a new VM for executions triggered by t. From 044786b995cd533f3dc649945f3dafd5eb37444c Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Thu, 29 Oct 2020 19:12:58 +0300 Subject: [PATCH 3/4] core: use Application trigger in CalculateClaimable() It doesn't need a System one. --- pkg/core/blockchain.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index f47627bc7..9eae5dd4e 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -1143,7 +1143,7 @@ func (bc *Blockchain) UnsubscribeFromExecutions(ch chan<- *state.AppExecResult) // CalculateClaimable calculates the amount of GAS generated by owning specified // amount of NEO between specified blocks. func (bc *Blockchain) CalculateClaimable(value *big.Int, startHeight, endHeight uint32) *big.Int { - ic := bc.newInteropContext(trigger.System, bc.dao, nil, nil) + ic := bc.newInteropContext(trigger.Application, bc.dao, nil, nil) res, _ := bc.contracts.NEO.CalculateBonus(ic, value, startHeight, endHeight) return res } From eaa260474f4495bac973fa0d536bebb143adfc2f Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Thu, 29 Oct 2020 19:14:49 +0300 Subject: [PATCH 4/4] trigger/core: split System trigger into OnPerist and PostPersist Follow neo-project/neo#2022. --- pkg/core/blockchain.go | 10 +++++----- pkg/core/native/contract.go | 4 ++-- pkg/core/native/native_nep5.go | 4 ++-- pkg/core/native_contract_test.go | 2 +- pkg/core/native_designate_test.go | 2 +- pkg/core/native_neo_test.go | 6 +++--- pkg/core/state/notification_event_test.go | 2 +- pkg/interop/runtime/runtime.go | 3 ++- pkg/rpc/server/server_test.go | 2 +- pkg/smartcontract/trigger/trigger_type.go | 15 +++++++++++---- .../trigger/trigger_type_string.go | 18 ++++++++++++------ pkg/smartcontract/trigger/trigger_type_test.go | 12 ++++++++---- 12 files changed, 49 insertions(+), 31 deletions(-) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index 9eae5dd4e..dad9fa944 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -565,7 +565,7 @@ func (bc *Blockchain) storeBlock(block *block.Block, txpool *mempool.Pool) error writeBuf.Reset() if block.Index > 0 { - aer, err := bc.runPersist(bc.contracts.GetPersistScript(), block, cache) + aer, err := bc.runPersist(bc.contracts.GetPersistScript(), block, cache, trigger.OnPersist) if err != nil { return fmt.Errorf("onPersist failed: %w", err) } @@ -635,7 +635,7 @@ func (bc *Blockchain) storeBlock(block *block.Block, txpool *mempool.Pool) error } } - aer, err := bc.runPersist(bc.contracts.GetPostPersistScript(), block, cache) + aer, err := bc.runPersist(bc.contracts.GetPostPersistScript(), block, cache, trigger.PostPersist) if err != nil { return fmt.Errorf("postPersist failed: %w", err) } @@ -704,8 +704,8 @@ func (bc *Blockchain) storeBlock(block *block.Block, txpool *mempool.Pool) error return nil } -func (bc *Blockchain) runPersist(script []byte, block *block.Block, cache *dao.Cached) (*state.AppExecResult, error) { - systemInterop := bc.newInteropContext(trigger.System, cache, block, nil) +func (bc *Blockchain) runPersist(script []byte, block *block.Block, cache *dao.Cached, trig trigger.Type) (*state.AppExecResult, error) { + systemInterop := bc.newInteropContext(trig, cache, block, nil) v := systemInterop.SpawnVM() v.LoadScriptWithFlags(script, smartcontract.AllowModifyStates|smartcontract.AllowCall) v.SetPriceGetter(getPrice) @@ -719,7 +719,7 @@ func (bc *Blockchain) runPersist(script []byte, block *block.Block, cache *dao.C } return &state.AppExecResult{ TxHash: block.Hash(), // application logs can be retrieved by block hash - Trigger: trigger.System, + Trigger: trig, VMState: v.State(), GasConsumed: v.GasConsumed(), Stack: v.Estack().ToArray(), diff --git a/pkg/core/native/contract.go b/pkg/core/native/contract.go index bd6de2605..a250bfed5 100644 --- a/pkg/core/native/contract.go +++ b/pkg/core/native/contract.go @@ -116,8 +116,8 @@ func (cs *Contracts) GetPostPersistScript() []byte { } func postPersistBase(ic *interop.Context) error { - if ic.Trigger != trigger.System { - return errors.New("'postPersist' should be trigered by system") + if ic.Trigger != trigger.PostPersist { + return errors.New("postPersist must be trigered by system") } return nil } diff --git a/pkg/core/native/native_nep5.go b/pkg/core/native/native_nep5.go index 8ba337f82..76ecb5d7c 100644 --- a/pkg/core/native/native_nep5.go +++ b/pkg/core/native/native_nep5.go @@ -261,8 +261,8 @@ func (c *nep5TokenNative) addTokens(ic *interop.Context, h util.Uint160, amount } func (c *nep5TokenNative) OnPersist(ic *interop.Context) error { - if ic.Trigger != trigger.System { - return errors.New("onPersist should be triggerred by system") + if ic.Trigger != trigger.OnPersist { + return errors.New("onPersist must be triggerred by system") } return nil } diff --git a/pkg/core/native_contract_test.go b/pkg/core/native_contract_test.go index 132e391dc..2858bff41 100644 --- a/pkg/core/native_contract_test.go +++ b/pkg/core/native_contract_test.go @@ -39,7 +39,7 @@ func (tn *testNative) Metadata() *interop.ContractMD { } func (tn *testNative) OnPersist(ic *interop.Context, _ []stackitem.Item) stackitem.Item { - if ic.Trigger != trigger.System { + if ic.Trigger != trigger.OnPersist { panic("invalid trigger") } select { diff --git a/pkg/core/native_designate_test.go b/pkg/core/native_designate_test.go index bd5d6622f..df8cd5f3e 100644 --- a/pkg/core/native_designate_test.go +++ b/pkg/core/native_designate_test.go @@ -78,7 +78,7 @@ func TestDesignate_DesignateAsRole(t *testing.T) { des := bc.contracts.Designate tx := transaction.New(netmode.UnitTestNet, []byte{}, 0) - ic := bc.newInteropContext(trigger.System, bc.dao, nil, tx) + ic := bc.newInteropContext(trigger.OnPersist, bc.dao, nil, tx) ic.SpawnVM() ic.VM.LoadScript([]byte{byte(opcode.RET)}) diff --git a/pkg/core/native_neo_test.go b/pkg/core/native_neo_test.go index 8ab37458f..3aa342c4f 100644 --- a/pkg/core/native_neo_test.go +++ b/pkg/core/native_neo_test.go @@ -30,7 +30,7 @@ func TestNEO_Vote(t *testing.T) { neo := bc.contracts.NEO tx := transaction.New(netmode.UnitTestNet, []byte{byte(opcode.PUSH1)}, 0) - ic := bc.newInteropContext(trigger.System, bc.dao, nil, tx) + ic := bc.newInteropContext(trigger.Application, bc.dao, nil, tx) ic.SpawnVM() ic.Block = bc.newBlock(tx) @@ -124,7 +124,7 @@ func TestNEO_SetGasPerBlock(t *testing.T) { neo := bc.contracts.NEO tx := transaction.New(netmode.UnitTestNet, []byte{}, 0) - ic := bc.newInteropContext(trigger.System, bc.dao, nil, tx) + ic := bc.newInteropContext(trigger.Application, bc.dao, nil, tx) ic.SpawnVM() ic.VM.LoadScript([]byte{byte(opcode.RET)}) @@ -183,7 +183,7 @@ func TestNEO_CalculateBonus(t *testing.T) { neo := bc.contracts.NEO tx := transaction.New(netmode.UnitTestNet, []byte{}, 0) - ic := bc.newInteropContext(trigger.System, bc.dao, nil, tx) + ic := bc.newInteropContext(trigger.Application, bc.dao, nil, tx) ic.SpawnVM() ic.VM.LoadScript([]byte{byte(opcode.RET)}) t.Run("Invalid", func(t *testing.T) { diff --git a/pkg/core/state/notification_event_test.go b/pkg/core/state/notification_event_test.go index 562973b99..7b51a9359 100644 --- a/pkg/core/state/notification_event_test.go +++ b/pkg/core/state/notification_event_test.go @@ -116,7 +116,7 @@ func TestMarshalUnmarshalJSONAppExecResult(t *testing.T) { t.Run("positive, block", func(t *testing.T) { appExecResult := &AppExecResult{ TxHash: random.Uint256(), - Trigger: trigger.System, + Trigger: trigger.OnPersist, VMState: vm.HaltState, GasConsumed: 10, Stack: []stackitem.Item{}, diff --git a/pkg/interop/runtime/runtime.go b/pkg/interop/runtime/runtime.go index 98cc0561b..d62ef8e75 100644 --- a/pkg/interop/runtime/runtime.go +++ b/pkg/interop/runtime/runtime.go @@ -8,7 +8,8 @@ import "github.com/nspcc-dev/neo-go/pkg/interop" // Trigger values to compare with GetTrigger result. const ( - System byte = 0x01 + OnPersist byte = 0x01 + PostPersist byte = 0x02 Application byte = 0x40 Verification byte = 0x20 ) diff --git a/pkg/rpc/server/server_test.go b/pkg/rpc/server/server_test.go index ee294da32..9d2c9a0ad 100644 --- a/pkg/rpc/server/server_test.go +++ b/pkg/rpc/server/server_test.go @@ -770,7 +770,7 @@ func testRPCProtocol(t *testing.T, doRPCCall func(string, string, *testing.T) [] data := checkErrGetResult(t, body, false) var res state.AppExecResult require.NoError(t, json.Unmarshal(data, &res)) - require.Equal(t, trigger.System, res.Trigger) + require.Equal(t, trigger.PostPersist, res.Trigger) require.Equal(t, vm.HaltState, res.VMState) }) diff --git a/pkg/smartcontract/trigger/trigger_type.go b/pkg/smartcontract/trigger/trigger_type.go index e996ca9a9..0fe314da8 100644 --- a/pkg/smartcontract/trigger/trigger_type.go +++ b/pkg/smartcontract/trigger/trigger_type.go @@ -9,8 +9,15 @@ type Type byte // Viable list of supported trigger type constants. const ( - // System is trigger type that indicates that script is being invoke internally by the system. - System Type = 0x01 + // OnPersist is a trigger type that indicates that script is being invoked + // internally by the system during block persistence (before transaction + // processing). + OnPersist Type = 0x01 + + // PostPersist is a trigger type that indicates that script is being invoked + // by the system after block persistence (transcation processing) has + // finished. + PostPersist Type = 0x02 // The verification trigger indicates that the contract is being invoked as a verification function. // The verification function can accept multiple parameters, and should return a boolean value that indicates the validity of the transaction or block. @@ -27,12 +34,12 @@ const ( Application Type = 0x40 // All represents any trigger type. - All Type = System | Verification | Application + All Type = OnPersist | PostPersist | Verification | Application ) // FromString converts string to trigger Type func FromString(str string) (Type, error) { - triggers := []Type{System, Verification, Application, All} + triggers := []Type{OnPersist, PostPersist, Verification, Application, All} for _, t := range triggers { if t.String() == str { return t, nil diff --git a/pkg/smartcontract/trigger/trigger_type_string.go b/pkg/smartcontract/trigger/trigger_type_string.go index 55430ab06..0d3325251 100644 --- a/pkg/smartcontract/trigger/trigger_type_string.go +++ b/pkg/smartcontract/trigger/trigger_type_string.go @@ -8,28 +8,34 @@ func _() { // An "invalid array index" compiler error signifies that the constant values have changed. // Re-run the stringer command to generate them again. var x [1]struct{} - _ = x[System-1] + _ = x[OnPersist-1] + _ = x[PostPersist-2] _ = x[Verification-32] _ = x[Application-64] - _ = x[All-97] + _ = x[All-99] } const ( - _Type_name_0 = "System" + _Type_name_0 = "OnPersistPostPersist" _Type_name_1 = "Verification" _Type_name_2 = "Application" _Type_name_3 = "All" ) +var ( + _Type_index_0 = [...]uint8{0, 9, 20} +) + func (i Type) String() string { switch { - case i == 1: - return _Type_name_0 + case 1 <= i && i <= 2: + i -= 1 + return _Type_name_0[_Type_index_0[i]:_Type_index_0[i+1]] case i == 32: return _Type_name_1 case i == 64: return _Type_name_2 - case i == 97: + case i == 99: return _Type_name_3 default: return "Type(" + strconv.FormatInt(int64(i), 10) + ")" diff --git a/pkg/smartcontract/trigger/trigger_type_test.go b/pkg/smartcontract/trigger/trigger_type_test.go index 166c49d2f..57d6db020 100644 --- a/pkg/smartcontract/trigger/trigger_type_test.go +++ b/pkg/smartcontract/trigger/trigger_type_test.go @@ -9,7 +9,8 @@ import ( func TestStringer(t *testing.T) { tests := map[Type]string{ - System: "System", + OnPersist: "OnPersist", + PostPersist: "PostPersist", Application: "Application", Verification: "Verification", } @@ -20,7 +21,8 @@ func TestStringer(t *testing.T) { func TestEncodeBynary(t *testing.T) { tests := map[Type]byte{ - System: 0x01, + OnPersist: 0x01, + PostPersist: 0x02, Verification: 0x20, Application: 0x40, } @@ -31,7 +33,8 @@ func TestEncodeBynary(t *testing.T) { func TestDecodeBynary(t *testing.T) { tests := map[Type]byte{ - System: 0x01, + OnPersist: 0x01, + PostPersist: 0x02, Verification: 0x20, Application: 0x40, } @@ -42,7 +45,8 @@ func TestDecodeBynary(t *testing.T) { func TestFromString(t *testing.T) { testCases := map[string]Type{ - "System": System, + "OnPersist": OnPersist, + "PostPersist": PostPersist, "Application": Application, "Verification": Verification, "All": All,