From 50ee241377cd5ca7d7da5a4f00348e9eae9146cb Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 3 Aug 2023 19:48:55 +0300 Subject: [PATCH] core: move strict script check on deploy under HF condition Follow the https://github.com/neo-project/neo/pull/2881. Signed-off-by: Anna Shaleva --- docs/node-configuration.md | 2 +- pkg/config/hardfork.go | 5 ++- pkg/config/hardfork_string.go | 5 +-- pkg/core/native/management.go | 37 +++++++++++-------- pkg/core/native/management_neotest_test.go | 42 ++++++++++++++++++++++ pkg/core/native/management_test.go | 16 +++++---- 6 files changed, 82 insertions(+), 25 deletions(-) diff --git a/docs/node-configuration.md b/docs/node-configuration.md index 86e938d86..425b15b8c 100644 --- a/docs/node-configuration.md +++ b/docs/node-configuration.md @@ -343,7 +343,7 @@ protocol-related settings described in the table below. | --- | --- | --- | --- | --- | | CommitteeHistory | map[uint32]uint32 | none | Number of committee members after the given height, for example `{0: 1, 20: 4}` sets up a chain with one committee member since the genesis and then changes the setting to 4 committee members at the height of 20. `StandbyCommittee` committee setting must have the number of keys equal or exceeding the highest value in this option. Blocks numbers where the change happens must be divisible by the old and by the new values simultaneously. If not set, committee size is derived from the `StandbyCommittee` setting and never changes. | | GarbageCollectionPeriod | `uint32` | 10000 | Controls MPT garbage collection interval (in blocks) for configurations with `RemoveUntraceableBlocks` enabled and `KeepOnlyLatestState` disabled. In this mode the node stores a number of MPT trees (corresponding to `MaxTraceableBlocks` and `StateSyncInterval`), but the DB needs to be clean from old entries from time to time. Doing it too often will cause too much processing overhead, doing it too rarely will leave more useless data in the DB. This setting is deprecated in favor of the same setting in the ApplicationConfiguration and will be removed in future node versions. If both settings are used, ApplicationConfiguration is prioritized over this one. | -| Hardforks | `map[string]uint32` | [] | The set of incompatible changes that affect node behaviour starting from the specified height. The default value is an empty set which should be interpreted as "each known hard-fork is applied from the zero blockchain height". The list of valid hard-fork names:
• `Aspidochelone` represents hard-fork introduced in [#2469](https://github.com/nspcc-dev/neo-go/pull/2469) (ported from the [reference](https://github.com/neo-project/neo/pull/2712)). It adjusts the prices of `System.Contract.CreateStandardAccount` and `System.Contract.CreateMultisigAccount` interops so that the resulting prices are in accordance with `sha256` method of native `CryptoLib` contract. It also includes [#2519](https://github.com/nspcc-dev/neo-go/pull/2519) (ported from the [reference](https://github.com/neo-project/neo/pull/2749)) that adjusts the price of `System.Runtime.GetRandom` interop and fixes its vulnerability. A special NeoGo-specific change is included as well for ContractManagement's update/deploy call flags behaviour to be compatible with pre-0.99.0 behaviour that was changed because of the [3.2.0 protocol change](https://github.com/neo-project/neo/pull/2653). | +| Hardforks | `map[string]uint32` | [] | The set of incompatible changes that affect node behaviour starting from the specified height. The default value is an empty set which should be interpreted as "each known hard-fork is applied from the zero blockchain height". The list of valid hard-fork names:
• `Aspidochelone` represents hard-fork introduced in [#2469](https://github.com/nspcc-dev/neo-go/pull/2469) (ported from the [reference](https://github.com/neo-project/neo/pull/2712)). It adjusts the prices of `System.Contract.CreateStandardAccount` and `System.Contract.CreateMultisigAccount` interops so that the resulting prices are in accordance with `sha256` method of native `CryptoLib` contract. It also includes [#2519](https://github.com/nspcc-dev/neo-go/pull/2519) (ported from the [reference](https://github.com/neo-project/neo/pull/2749)) that adjusts the price of `System.Runtime.GetRandom` interop and fixes its vulnerability. A special NeoGo-specific change is included as well for ContractManagement's update/deploy call flags behaviour to be compatible with pre-0.99.0 behaviour that was changed because of the [3.2.0 protocol change](https://github.com/neo-project/neo/pull/2653).
• `Basilisk` represents hard-fork introduced in [#3056](https://github.com/nspcc-dev/neo-go/pull/3056) (ported from the [reference](https://github.com/neo-project/neo/pull/2881)). It enables strict smart contract script check against a set of JMP instructions and against method boundaries enabled on contract deploy or update. | | KeepOnlyLatestState | `bool` | `false` | Specifies if MPT should only store the latest state (or a set of latest states, see `P2PStateExcangeExtensions` section for details). If true, DB size will be smaller, but older roots won't be accessible. This value should remain the same for the same database. | This setting is deprecated in favor of the same setting in the ApplicationConfiguration and will be removed in future node versions. If both settings are used, setting any of them to true enables the function. | | Magic | `uint32` | `0` | Magic number which uniquely identifies Neo network. | | MaxBlockSize | `uint32` | `262144` | Maximum block size in bytes. | diff --git a/pkg/config/hardfork.go b/pkg/config/hardfork.go index 5bd775122..be83acfcd 100644 --- a/pkg/config/hardfork.go +++ b/pkg/config/hardfork.go @@ -10,6 +10,9 @@ const ( // https://github.com/neo-project/neo/pull/2712) and #2519 (ported from // https://github.com/neo-project/neo/pull/2749). HFAspidochelone Hardfork = 1 << iota // Aspidochelone + // HFBasilisk represents hard-fork introduced in #3056 (ported from + // https://github.com/neo-project/neo/pull/2881). + HFBasilisk // Basilisk ) // hardforks holds a map of Hardfork string representation to its type. @@ -17,7 +20,7 @@ var hardforks map[string]Hardfork func init() { hardforks = make(map[string]Hardfork) - for _, hf := range []Hardfork{HFAspidochelone} { + for _, hf := range []Hardfork{HFAspidochelone, HFBasilisk} { hardforks[hf.String()] = hf } } diff --git a/pkg/config/hardfork_string.go b/pkg/config/hardfork_string.go index 61aded842..f73923119 100644 --- a/pkg/config/hardfork_string.go +++ b/pkg/config/hardfork_string.go @@ -9,11 +9,12 @@ func _() { // Re-run the stringer command to generate them again. var x [1]struct{} _ = x[HFAspidochelone-1] + _ = x[HFBasilisk-2] } -const _Hardfork_name = "Aspidochelone" +const _Hardfork_name = "AspidocheloneBasilisk" -var _Hardfork_index = [...]uint8{0, 13} +var _Hardfork_index = [...]uint8{0, 13, 21} func (i Hardfork) String() string { i -= 1 diff --git a/pkg/core/native/management.go b/pkg/core/native/management.go index f78d4080e..0841bcc1d 100644 --- a/pkg/core/native/management.go +++ b/pkg/core/native/management.go @@ -10,6 +10,7 @@ import ( "math/big" "unicode/utf8" + "github.com/nspcc-dev/neo-go/pkg/config" "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/interop/contract" @@ -353,7 +354,7 @@ func (m *Management) deployWithData(ic *interop.Context, args []stackitem.Item) if ic.Tx == nil { panic(errors.New("no transaction provided")) } - newcontract, err := m.Deploy(ic.DAO, ic.Tx.Sender(), neff, manif) + newcontract, err := m.Deploy(ic, ic.Tx.Sender(), neff, manif) if err != nil { panic(err) } @@ -375,16 +376,16 @@ func markUpdated(d *dao.Simple, hash util.Uint160, cs *state.Contract) { // Deploy creates a contract's hash/ID and saves a new contract into the given DAO. // It doesn't run _deploy method and doesn't emit notification. -func (m *Management) Deploy(d *dao.Simple, sender util.Uint160, neff *nef.File, manif *manifest.Manifest) (*state.Contract, error) { +func (m *Management) Deploy(ic *interop.Context, sender util.Uint160, neff *nef.File, manif *manifest.Manifest) (*state.Contract, error) { h := state.CreateContractHash(sender, neff.Checksum, manif.Name) - if m.Policy.IsBlocked(d, h) { + if m.Policy.IsBlocked(ic.DAO, h) { return nil, fmt.Errorf("the contract %s has been blocked", h.StringLE()) } - _, err := GetContract(d, h) + _, err := GetContract(ic.DAO, h) if err == nil { return nil, errors.New("contract already exists") } - id, err := m.getNextContractID(d) + id, err := m.getNextContractID(ic.DAO) if err != nil { return nil, err } @@ -392,7 +393,7 @@ func (m *Management) Deploy(d *dao.Simple, sender util.Uint160, neff *nef.File, if err != nil { return nil, fmt.Errorf("invalid manifest: %w", err) } - err = checkScriptAndMethods(neff.Script, manif.ABI.Methods) + err = checkScriptAndMethods(ic, neff.Script, manif.ABI.Methods) if err != nil { return nil, err } @@ -404,7 +405,7 @@ func (m *Management) Deploy(d *dao.Simple, sender util.Uint160, neff *nef.File, Manifest: *manif, }, } - err = PutContractState(d, newcontract) + err = PutContractState(ic.DAO, newcontract) if err != nil { return nil, err } @@ -425,7 +426,7 @@ func (m *Management) updateWithData(ic *interop.Context, args []stackitem.Item) if neff == nil && manif == nil { panic(errors.New("both NEF and manifest are nil")) } - contract, err := m.Update(ic.DAO, ic.VM.GetCallingScriptHash(), neff, manif) + contract, err := m.Update(ic, ic.VM.GetCallingScriptHash(), neff, manif) if err != nil { panic(err) } @@ -436,10 +437,10 @@ func (m *Management) updateWithData(ic *interop.Context, args []stackitem.Item) // Update updates contract's script and/or manifest in the given DAO. // It doesn't run _deploy method and doesn't emit notification. -func (m *Management) Update(d *dao.Simple, hash util.Uint160, neff *nef.File, manif *manifest.Manifest) (*state.Contract, error) { +func (m *Management) Update(ic *interop.Context, hash util.Uint160, neff *nef.File, manif *manifest.Manifest) (*state.Contract, error) { var contract state.Contract - oldcontract, err := GetContract(d, hash) + oldcontract, err := GetContract(ic.DAO, hash) if err != nil { return nil, errors.New("contract doesn't exist") } @@ -463,12 +464,12 @@ func (m *Management) Update(d *dao.Simple, hash util.Uint160, neff *nef.File, ma } contract.Manifest = *manif } - err = checkScriptAndMethods(contract.NEF.Script, contract.Manifest.ABI.Methods) + err = checkScriptAndMethods(ic, contract.NEF.Script, contract.Manifest.ABI.Methods) if err != nil { return nil, err } contract.UpdateCounter++ - err = PutContractState(d, &contract) + err = PutContractState(ic.DAO, &contract) if err != nil { return nil, err } @@ -715,7 +716,7 @@ func (m *Management) emitNotification(ic *interop.Context, name string, hash uti ic.AddNotification(m.Hash, name, stackitem.NewArray([]stackitem.Item{addrToStackItem(&hash)})) } -func checkScriptAndMethods(script []byte, methods []manifest.Method) error { +func checkScriptAndMethods(ic *interop.Context, script []byte, methods []manifest.Method) error { l := len(script) offsets := bitfield.New(l) for i := range methods { @@ -724,5 +725,13 @@ func checkScriptAndMethods(script []byte, methods []manifest.Method) error { } offsets.Set(methods[i].Offset) } - return vm.IsScriptCorrect(script, offsets) + if !ic.IsHardforkEnabled(config.HFBasilisk) { + return nil + } + err := vm.IsScriptCorrect(script, offsets) + if err != nil { + return fmt.Errorf("invalid contract script: %w", err) + } + + return nil } diff --git a/pkg/core/native/management_neotest_test.go b/pkg/core/native/management_neotest_test.go index 1647eccdd..f096b6fd9 100644 --- a/pkg/core/native/management_neotest_test.go +++ b/pkg/core/native/management_neotest_test.go @@ -6,9 +6,13 @@ import ( "github.com/nspcc-dev/neo-go/internal/basicchain" "github.com/nspcc-dev/neo-go/pkg/config" "github.com/nspcc-dev/neo-go/pkg/core/native/nativenames" + "github.com/nspcc-dev/neo-go/pkg/core/state" "github.com/nspcc-dev/neo-go/pkg/neotest" "github.com/nspcc-dev/neo-go/pkg/neotest/chain" + "github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest" + "github.com/nspcc-dev/neo-go/pkg/smartcontract/nef" "github.com/nspcc-dev/neo-go/pkg/util" + "github.com/nspcc-dev/neo-go/pkg/vm/opcode" "github.com/stretchr/testify/require" ) @@ -32,3 +36,41 @@ func TestManagement_GetNEP17Contracts(t *testing.T) { e.NativeHash(t, nativenames.Gas), e.ContractHash(t, 1)}, bc.GetNEP17Contracts()) }) } + +func TestManagement_DeployUpdateHardfork(t *testing.T) { + bc, acc := chain.NewSingleWithCustomConfig(t, func(c *config.Blockchain) { + c.Hardforks = map[string]uint32{ + config.HFBasilisk.String(): 2, + } + }) + e := neotest.NewExecutor(t, bc, acc, acc) + + ne, err := nef.NewFile([]byte{byte(opcode.JMP), 0x05}) + require.NoError(t, err) + + m := &manifest.Manifest{ + Name: "ctr", + ABI: manifest.ABI{ + Methods: []manifest.Method{ + { + Name: "main", + Offset: 0, + }, + }, + }, + } + ctr := &neotest.Contract{ + + Hash: state.CreateContractHash(e.Validator.ScriptHash(), ne.Checksum, m.Name), + NEF: ne, + Manifest: m, + } + + // Block 1: no script check on deploy. + e.DeployContract(t, ctr, nil) + e.AddNewBlock(t) + + // Block 3: script check on deploy. + ctr.Manifest.Name = "other name" + e.DeployContractCheckFAULT(t, ctr, nil, "invalid contract script: invalid offset 5 ip at 0") +} diff --git a/pkg/core/native/management_test.go b/pkg/core/native/management_test.go index cc8e1e1f8..657622e21 100644 --- a/pkg/core/native/management_test.go +++ b/pkg/core/native/management_test.go @@ -19,7 +19,8 @@ func TestDeployGetUpdateDestroyContract(t *testing.T) { mgmt := newManagement() mgmt.Policy = newPolicy() d := dao.NewSimple(storage.NewMemoryStore(), false, false) - err := mgmt.Initialize(&interop.Context{DAO: d}) + ic := &interop.Context{DAO: d} + err := mgmt.Initialize(ic) require.NoError(t, err) require.NoError(t, mgmt.Policy.Initialize(&interop.Context{DAO: d})) script := []byte{byte(opcode.RET)} @@ -35,7 +36,7 @@ func TestDeployGetUpdateDestroyContract(t *testing.T) { h := state.CreateContractHash(sender, ne.Checksum, manif.Name) - contract, err := mgmt.Deploy(d, sender, ne, manif) + contract, err := mgmt.Deploy(ic, sender, ne, manif) require.NoError(t, err) require.Equal(t, int32(1), contract.ID) require.Equal(t, uint16(0), contract.UpdateCounter) @@ -44,12 +45,12 @@ func TestDeployGetUpdateDestroyContract(t *testing.T) { require.Equal(t, *manif, contract.Manifest) // Double deploy. - _, err = mgmt.Deploy(d, sender, ne, manif) + _, err = mgmt.Deploy(ic, sender, ne, manif) require.Error(t, err) // Different sender. sender2 := util.Uint160{3, 2, 1} - contract2, err := mgmt.Deploy(d, sender2, ne, manif) + contract2, err := mgmt.Deploy(ic, sender2, ne, manif) require.NoError(t, err) require.Equal(t, int32(2), contract2.ID) require.Equal(t, uint16(0), contract2.UpdateCounter) @@ -65,7 +66,7 @@ func TestDeployGetUpdateDestroyContract(t *testing.T) { require.NoError(t, err) require.Equal(t, contract, refContract) - upContract, err := mgmt.Update(d, h, ne, manif) + upContract, err := mgmt.Update(ic, h, ne, manif) refContract.UpdateCounter++ require.NoError(t, err) require.Equal(t, refContract, upContract) @@ -106,6 +107,7 @@ func TestManagement_GetNEP17Contracts(t *testing.T) { require.Empty(t, mgmt.GetNEP17Contracts(d)) private := d.GetPrivate() + ic := &interop.Context{DAO: private} // Deploy NEP-17 contract script := []byte{byte(opcode.RET)} @@ -119,7 +121,7 @@ func TestManagement_GetNEP17Contracts(t *testing.T) { Parameters: []manifest.Parameter{}, }) manif.SupportedStandards = []string{manifest.NEP17StandardName} - c1, err := mgmt.Deploy(private, sender, ne, manif) + c1, err := mgmt.Deploy(ic, sender, ne, manif) require.NoError(t, err) // c1 contract hash should be returned, as private DAO already contains changed cache. @@ -140,7 +142,7 @@ func TestManagement_GetNEP17Contracts(t *testing.T) { ReturnType: smartcontract.VoidType, Parameters: []manifest.Parameter{}, }) - c1Updated, err := mgmt.Update(private, c1.Hash, ne, manif) + c1Updated, err := mgmt.Update(&interop.Context{DAO: private}, c1.Hash, ne, manif) require.NoError(t, err) require.Equal(t, c1.Hash, c1Updated.Hash)