From 84b00d46aa599cacb90a65b2dbe82c16ebeb2b40 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 3 Apr 2023 15:53:37 +0300 Subject: [PATCH 1/6] rpc: emit Null in case of `Any` parameter with zero-len value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise it leads to the following error in the TestActor_CallWithNilParam: ``` === RUN TestActor_CallWithNilParam logger.go:130: 2023-04-03T15:58:27.672+0300 INFO initial gas supply is not set or wrong, setting default value {"InitialGASSupply": "52000000"} logger.go:130: 2023-04-03T15:58:27.672+0300 INFO P2PNotaryRequestPayloadPool size is not set or wrong, setting default value {"P2PNotaryRequestPayloadPoolSize": 1000} logger.go:130: 2023-04-03T15:58:27.672+0300 INFO MaxBlockSize is not set or wrong, setting default value {"MaxBlockSize": 262144} logger.go:130: 2023-04-03T15:58:27.672+0300 INFO MaxBlockSystemFee is not set or wrong, setting default value {"MaxBlockSystemFee": 900000000000} logger.go:130: 2023-04-03T15:58:27.672+0300 INFO MaxTransactionsPerBlock is not set or wrong, using default value {"MaxTransactionsPerBlock": 512} logger.go:130: 2023-04-03T15:58:27.672+0300 INFO MaxValidUntilBlockIncrement is not set or wrong, using default value {"MaxValidUntilBlockIncrement": 5760} logger.go:130: 2023-04-03T15:58:27.675+0300 INFO no storage version found! creating genesis block logger.go:130: 2023-04-03T15:58:27.675+0300 INFO ExtensiblePoolSize is not set or wrong, using default value {"ExtensiblePoolSize": 20} logger.go:130: 2023-04-03T15:58:27.675+0300 INFO SessionPoolSize is not set or wrong, setting default value {"SessionPoolSize": 20} logger.go:130: 2023-04-03T15:58:27.675+0300 INFO MaxWebSocketClients is not set or wrong, setting default value {"MaxWebSocketClients": 64} logger.go:130: 2023-04-03T15:58:27.675+0300 INFO starting rpc-server {"endpoint": "localhost:0"} logger.go:130: 2023-04-03T15:58:27.677+0300 DEBUG done processing headers {"headerIndex": 1, "blockHeight": 0, "took": "436.313µs"} logger.go:130: 2023-04-03T15:58:27.679+0300 DEBUG done processing headers {"headerIndex": 2, "blockHeight": 1, "took": "272.891µs"} logger.go:130: 2023-04-03T15:58:27.680+0300 DEBUG done processing headers {"headerIndex": 3, "blockHeight": 2, "took": "276.949µs"} logger.go:130: 2023-04-03T15:58:27.681+0300 DEBUG done processing headers {"headerIndex": 4, "blockHeight": 3, "took": "286.028µs"} logger.go:130: 2023-04-03T15:58:27.681+0300 DEBUG done processing headers {"headerIndex": 5, "blockHeight": 4, "took": "268.673µs"} logger.go:130: 2023-04-03T15:58:27.681+0300 INFO bad notification {"contract": "565cff9508ebc75aadd7fe59f38dac610ab6093c", "event": "Transfer", "error": "parameter 0 type mismatch: Hash160 vs ByteString"} logger.go:130: 2023-04-03T15:58:27.682+0300 DEBUG done processing headers {"headerIndex": 6, "blockHeight": 5, "took": "380.988µs"} logger.go:130: 2023-04-03T15:58:27.683+0300 DEBUG done processing headers {"headerIndex": 7, "blockHeight": 6, "took": "273.543µs"} logger.go:130: 2023-04-03T15:58:27.683+0300 DEBUG done processing headers {"headerIndex": 8, "blockHeight": 7, "took": "275.163µs"} logger.go:130: 2023-04-03T15:58:27.684+0300 DEBUG done processing headers {"headerIndex": 9, "blockHeight": 8, "took": "259.578µs"} logger.go:130: 2023-04-03T15:58:27.685+0300 DEBUG done processing headers {"headerIndex": 10, "blockHeight": 9, "took": "266.882µs"} logger.go:130: 2023-04-03T15:58:27.686+0300 DEBUG done processing headers {"headerIndex": 11, "blockHeight": 10, "took": "295.3µs"} logger.go:130: 2023-04-03T15:58:27.687+0300 DEBUG done processing headers {"headerIndex": 12, "blockHeight": 11, "took": "295.568µs"} logger.go:130: 2023-04-03T15:58:27.688+0300 DEBUG done processing headers {"headerIndex": 13, "blockHeight": 12, "took": "258.197µs"} logger.go:130: 2023-04-03T15:58:27.689+0300 DEBUG done processing headers {"headerIndex": 14, "blockHeight": 13, "took": "261.602µs"} logger.go:130: 2023-04-03T15:58:27.689+0300 DEBUG done processing headers {"headerIndex": 15, "blockHeight": 14, "took": "268.922µs"} logger.go:130: 2023-04-03T15:58:27.690+0300 DEBUG done processing headers {"headerIndex": 16, "blockHeight": 15, "took": "276.176µs"} logger.go:130: 2023-04-03T15:58:27.691+0300 DEBUG done processing headers {"headerIndex": 17, "blockHeight": 16, "took": "256.068µs"} logger.go:130: 2023-04-03T15:58:27.692+0300 DEBUG done processing headers {"headerIndex": 18, "blockHeight": 17, "took": "262.303µs"} logger.go:130: 2023-04-03T15:58:27.692+0300 DEBUG done processing headers {"headerIndex": 19, "blockHeight": 18, "took": "265.087µs"} logger.go:130: 2023-04-03T15:58:27.693+0300 DEBUG done processing headers {"headerIndex": 20, "blockHeight": 19, "took": "260.758µs"} logger.go:130: 2023-04-03T15:58:27.694+0300 DEBUG done processing headers {"headerIndex": 21, "blockHeight": 20, "took": "263.482µs"} logger.go:130: 2023-04-03T15:58:27.694+0300 DEBUG done processing headers {"headerIndex": 22, "blockHeight": 21, "took": "327.812µs"} logger.go:130: 2023-04-03T15:58:27.696+0300 DEBUG done processing headers {"headerIndex": 23, "blockHeight": 22, "took": "284.104µs"} logger.go:130: 2023-04-03T15:58:27.697+0300 WARN contract invocation failed {"tx": "82279bfe9bada282ca0f8cb8e0bb124b921af36f00c69a518320322c6f4fef60", "block": 23, "error": "at instruction 0 (ABORT): ABORT"} logger.go:130: 2023-04-03T15:58:27.697+0300 DEBUG processing rpc request {"method": "getversion", "params": "[]"} logger.go:130: 2023-04-03T15:58:27.698+0300 DEBUG processing rpc request {"method": "invokefunction", "params": "[565cff9508ebc75aadd7fe59f38dac610ab6093c putValue ]"} client_test.go:2562: Error Trace: /home/anna/Documents/GitProjects/nspcc-dev/neo-go/pkg/services/rpcsrv/client_test.go:2562 Error: Should be true Test: TestActor_CallWithNilParam Messages: at instruction 6 (PACK): OPACK: invalid length logger.go:130: 2023-04-03T15:58:27.699+0300 INFO shutting down RPC server {"endpoint": "127.0.0.1:46005"} logger.go:130: 2023-04-03T15:58:27.700+0300 INFO persisted to disk {"blocks": 23, "keys": 1236, "headerHeight": 23, "blockHeight": 23, "took": "908.825µs"} --- FAIL: TestActor_CallWithNilParam (0.03s) FAIL ``` See also the ref. https://github.com/neo-project/neo/blob/df534f6b0c700e1c7b3eb1315c4560fedce793b8/src/Neo/SmartContract/ContractParameter.cs#L141 and the way how parameters are handled by ref. RPC server: https://github.com/neo-project/neo-modules/blob/4b3a76e1b760798b2e9bc8706bc5f4cf22c1f5ae/src/RpcServer/RpcServer.SmartContract.cs#L202 and FromJSON implementation: https://github.com/neo-project/neo/blob/df534f6b0c700e1c7b3eb1315c4560fedce793b8/src/Neo/SmartContract/ContractParameter.cs#L70 --- pkg/services/rpcsrv/client_test.go | 32 +++++++++++++++++++++++++ pkg/services/rpcsrv/params/txBuilder.go | 2 +- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/pkg/services/rpcsrv/client_test.go b/pkg/services/rpcsrv/client_test.go index 031dab5ee..d74b83afb 100644 --- a/pkg/services/rpcsrv/client_test.go +++ b/pkg/services/rpcsrv/client_test.go @@ -18,6 +18,7 @@ import ( "github.com/google/uuid" "github.com/gorilla/websocket" + "github.com/nspcc-dev/neo-go/internal/basicchain" "github.com/nspcc-dev/neo-go/internal/testchain" "github.com/nspcc-dev/neo-go/pkg/config" "github.com/nspcc-dev/neo-go/pkg/core" @@ -2499,3 +2500,34 @@ func TestWSClient_SubscriptionsCompat(t *testing.T) { checkRelevant(t, false) }) } + +func TestActor_CallWithNilParam(t *testing.T) { + chain, rpcSrv, httpSrv := initServerWithInMemoryChain(t) + defer chain.Close() + defer rpcSrv.Shutdown() + + c, err := rpcclient.New(context.Background(), httpSrv.URL, rpcclient.Options{}) + require.NoError(t, err) + acc, err := wallet.NewAccount() + require.NoError(t, err) + act, err := actor.New(c, []actor.SignerAccount{ + { + Signer: transaction.Signer{ + Account: acc.ScriptHash(), + }, + Account: acc, + }, + }) + require.NoError(t, err) + + rubles, err := chain.GetContractScriptHash(basicchain.RublesContractID) + require.NoError(t, err) + + // We don't have a suitable contract, thus use Rubles with simple put method, + // it should fail at the moment of conversion Null value to ByteString (not earlier, + // and that's the point of the test!). + res, err := act.Call(rubles, "putValue", "123", (*util.Uint160)(nil)) + require.NoError(t, err) + + require.True(t, strings.Contains(res.FaultException, "invalid conversion: Null/ByteString"), res.FaultException) +} diff --git a/pkg/services/rpcsrv/params/txBuilder.go b/pkg/services/rpcsrv/params/txBuilder.go index 9872c7cc6..9d2c9ea2d 100644 --- a/pkg/services/rpcsrv/params/txBuilder.go +++ b/pkg/services/rpcsrv/params/txBuilder.go @@ -84,7 +84,7 @@ func ExpandArrayIntoScript(script *io.BinWriter, slice []Param) error { return err } case smartcontract.AnyType: - if fp.Value.IsNull() { + if fp.Value.IsNull() || len(fp.Value.RawMessage) == 0 { emit.Opcodes(script, opcode.PUSHNULL) } default: From 50c88050349e4169bfc651fdaf70b3013a79bdaf Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Wed, 31 May 2023 18:31:20 +0300 Subject: [PATCH 2/6] *: drop legacy addresses from examples They can't even be used now because they'll fail the conversion. Signed-off-by: Roman Khimov --- cli/cmdargs/parser.go | 4 ++-- docs/compiler.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/cmdargs/parser.go b/cli/cmdargs/parser.go index b90fa60d7..315577de5 100644 --- a/cli/cmdargs/parser.go +++ b/cli/cmdargs/parser.go @@ -84,8 +84,8 @@ const ( * 'dead' is a byte array with a value of 'dead' * 'string:dead' is a string with a value of 'dead' * 'filebytes:my_data.txt' is bytes decoded from a content of my_data.txt - * 'AK2nJJpJr6o664CWJKi1QRXjqeic2zRp8y' is a hash160 with a value - of '23ba2703c53263e8d6e522dc32203339dcd8eee9' + * 'NSiVJYZej4XsxG5CUpdwn7VRQk8iiiDMPM' is a hash160 with a value + of '682cca3ebdc66210e5847d7f8115846586079d4a' * '\4\2' is an integer with a value of 42 * '\\4\2' is a string with a value of '\42' * 'string:string' is a string with a value of 'string' diff --git a/docs/compiler.md b/docs/compiler.md index a38a2dcf6..59b7759dc 100644 --- a/docs/compiler.md +++ b/docs/compiler.md @@ -403,11 +403,11 @@ see `contract` command help for more details. They all work via RPC, so it's a mandatory parameter. Example call (contract `f84d6a337fbc3d3a201d41da99e86b479e7a2554` with method -`balanceOf` and method's parameter `AK2nJJpJr6o664CWJKi1QRXjqeic2zRp8y` using +`balanceOf` and method's parameter `NVTiAjNgagDkTr5HTzDmQP9kPwPHN5BgVq` using given RPC server and wallet and paying 0.00001 extra GAS for this transaction): ``` -$ ./bin/neo-go contract invokefunction -r http://localhost:20331 -w my_wallet.json -g 0.00001 f84d6a337fbc3d3a201d41da99e86b479e7a2554 balanceOf AK2nJJpJr6o664CWJKi1QRXjqeic2zRp8y +$ ./bin/neo-go contract invokefunction -r http://localhost:20331 -w my_wallet.json -g 0.00001 f84d6a337fbc3d3a201d41da99e86b479e7a2554 balanceOf NVTiAjNgagDkTr5HTzDmQP9kPwPHN5BgVq ``` ### Generating contract bindings From 70aed34d77d916ca9d2551b871332fb2ba85a3f5 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Thu, 29 Jun 2023 11:18:30 +0300 Subject: [PATCH 3/6] interop/contract: fix state rollbacks for nested contexts Our wrapping optimization relied on the caller context having a TRY block, but each context (including internal calls!) has an exception handling stack of its own, which means that for an invocation stack of entry A.someMethodFromEntry() # this one has a TRY A.internalMethodViaCALL() # this one doesn't B.someMethod() we get `HasTryBlock() == false` for `A.internalMethodViaCALL()` context, which leads to missing wrapper and missing rollbacks if B is to THROW. What this patch does instead is it checks for any context within contract boundaries. Fixes #3045. Signed-off-by: Roman Khimov --- pkg/core/interop/contract/call.go | 2 +- pkg/core/interop/contract/call_test.go | 3 +++ pkg/vm/context.go | 10 ---------- pkg/vm/vm.go | 22 ++++++++++++++++++++++ 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/pkg/core/interop/contract/call.go b/pkg/core/interop/contract/call.go index 170e596a4..e1b8b72be 100644 --- a/pkg/core/interop/contract/call.go +++ b/pkg/core/interop/contract/call.go @@ -117,7 +117,7 @@ func callExFromNative(ic *interop.Context, caller util.Uint160, cs *state.Contra ic.Invocations[cs.Hash]++ f = ic.VM.Context().GetCallFlags() & f - wrapped := ic.VM.Context().HasTryBlock() && // If the method is not wrapped into try-catch block, then changes should be discarded anyway if exception occurs. + wrapped := ic.VM.ContractHasTryBlock() && // If the method is not wrapped into try-catch block, then changes should be discarded anyway if exception occurs. f&(callflag.All^callflag.ReadOnly) != 0 // If the method is safe, then it's read-only and doesn't perform storage changes or emit notifications. baseNtfCount := len(ic.Notifications) baseDAO := ic.DAO diff --git a/pkg/core/interop/contract/call_test.go b/pkg/core/interop/contract/call_test.go index 50648a4b6..909e6a7c4 100644 --- a/pkg/core/interop/contract/call_test.go +++ b/pkg/core/interop/contract/call_test.go @@ -304,6 +304,9 @@ func TestSnapshotIsolation_Exceptions(t *testing.T) { for i := 0; i < nNtfB1; i++ { runtime.Notify("NotificationFromB before panic", i) } + internalCaller(keyA, valueA, nNtfA) + } + func internalCaller(keyA, valueA []byte, nNtfA int) { contract.Call(interop.Hash160{` + hashAStr + `}, "doAndPanic", contract.All, keyA, valueA, nNtfA) } func CheckStorageChanges() bool { diff --git a/pkg/vm/context.go b/pkg/vm/context.go index 95d693296..1d44c29c5 100644 --- a/pkg/vm/context.go +++ b/pkg/vm/context.go @@ -315,16 +315,6 @@ func (v *VM) PushContextScriptHash(n int) error { return nil } -func (c *Context) HasTryBlock() bool { - for i := 0; i < c.tryStack.Len(); i++ { - eCtx := c.tryStack.Peek(i).Value().(*exceptionHandlingContext) - if eCtx.State == eTry { - return true - } - } - return false -} - // MarshalJSON implements the JSON marshalling interface. func (c *Context) MarshalJSON() ([]byte, error) { var aux = contextAux{ diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index 950dfed7e..4ee109516 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -1804,6 +1804,28 @@ func throwUnhandledException(item stackitem.Item) { panic(msg) } +// ContractHasTryBlock checks if the currently executing contract has a TRY +// block in one of its contexts. +func (v *VM) ContractHasTryBlock() bool { + var topctx *Context // Currently executing context. + for i := 0; i < len(v.istack); i++ { + ictx := v.istack[len(v.istack)-1-i] // It's a stack, going backwards like handleException(). + if topctx == nil { + topctx = ictx + } + if ictx.sc != topctx.sc { + return false // Different contract -> no one cares. + } + for j := 0; j < ictx.tryStack.Len(); j++ { + eCtx := ictx.tryStack.Peek(j).Value().(*exceptionHandlingContext) + if eCtx.State == eTry { + return true + } + } + } + return false +} + // CheckMultisigPar checks if the sigs contains sufficient valid signatures. func CheckMultisigPar(v *VM, curve elliptic.Curve, h []byte, pkeys [][]byte, sigs [][]byte) bool { if len(sigs) == 1 { From 60e6fe05a76a42af79083b1dcbc22c902c580626 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 25 Apr 2023 12:01:55 +0300 Subject: [PATCH 4/6] docs: improve Notary's `till` value documentation Close #2986. Signed-off-by: Anna Shaleva --- docs/notary.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/notary.md b/docs/notary.md index 7c5e112dd..c11e23f9b 100644 --- a/docs/notary.md +++ b/docs/notary.md @@ -261,10 +261,14 @@ with the `data` parameter matching the following requirements: - `to` denotes the receiver of the deposit. It can be nil in case `to` equals the GAS sender. - `till` denotes chain's height before which deposit is locked and can't be - withdrawn. `till` can't be set if you're not the deposit owner. Default `till` - value is the current chain height + 5760. `till` can't be less than the current chain - height. `till` can't be less than the currently set `till` value for that deposit if - the deposit already exists. + withdrawn. `till` can't be less than the current chain height. `till` + can't be less than the current `till` value for the deposit if the deposit + already exists. `till` can be set to the provided value iff the transaction + sender is the owner of the deposit, otherwise the provided `till` value will + be overridden by the system. If the sender is not the deposit owner, the + overridden `till` value is either set to be the current chain height + 5760 + (for the newly added deposit) or set to the old `till` value (for the existing + deposit). Note, that the first deposit call for the `to` address can't transfer less than 2×`FEE` GAS. Deposit is allowed for renewal, i.e. consequent `deposit` calls for the same `to` From 3bf8192f9816bcd03dfc29f3817778366d358507 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 19 Apr 2023 17:18:04 +0300 Subject: [PATCH 5/6] rpcclient: make Notary Actor follow the request creation doc All Notary contract witnesses in incomplete transaction (both main and fallback) may either have invocation scripts pushing dummy signature on stack or be empty, both ways are OK. Notary actor keeps main tx's Notary witness empty and keeps fallback tx's Notary witness filled with dummy signature. Signed-off-by: Anna Shaleva --- docs/notary.md | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/docs/notary.md b/docs/notary.md index c11e23f9b..8fe10b3d2 100644 --- a/docs/notary.md +++ b/docs/notary.md @@ -356,9 +356,9 @@ subpackage with an example written in Go doc. with `Deployed` set to `false` and `Script` set to the signer's verification script. - An account for a notary signer is **just a placeholder** and should have - `Contract` field with `Deployed` set to `false`, i.e. the default value for - `Contract` field. That's needed to skip notary verification during regular - network fee calculation at the next step. + `Contract` field with `Deployed` set to `true`. Its `Invocation` witness script + parameters will be guessed by the `verify` method signature of Notary contract + during the network fee calculation at the next step. 6. Fill in the main transaction `Nonce` field. 7. Construct a list of main transactions witnesses (that will be `Scripts` @@ -367,11 +367,17 @@ subpackage with an example written in Go doc. - A contract-based witness should have `Invocation` script that pushes arguments on stack (it may be empty) and empty `Verification` script. If multiple notary requests provide different `Invocation` scripts, the first one will be used - to construct contract-based witness. + to construct contract-based witness. If non-empty `Invocation` script is + specified then it will be taken into account during network fee calculation. + In case of an empty `Invocation` script, its parameters will be guessed from + the contract's `verify` signature during network fee calculation. - A **Notary contract witness** (which is also a contract-based witness) should - have empty `Verification` script. `Invocation` script should be of the form - [opcode.PUSHDATA1, 64, make([]byte, 64)...], i.e. to be a placeholder for - a notary contract signature. + have empty `Verification` script. `Invocation` script should be either empty + (allowed for main transaction and forbidden for fallback transaction) or of + the form [opcode.PUSHDATA1, 64, make([]byte, 64)...] (allowed for main + transaction and required for fallback transaction by the Notary subsystem to + pass verification), i.e. to be a placeholder for a notary contract signature. + Both ways are OK for network fee calculation. - A standard signature witness must have regular `Verification` script filled even if the `Invocation` script is to be collected from other notary requests. From e993003b466c538003e332ee2c41b52c0597bfcf Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Thu, 29 Jun 2023 12:20:11 +0300 Subject: [PATCH 6/6] CHANGELOG: release 0.101.2 Signed-off-by: Roman Khimov --- CHANGELOG.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dfc70cc2..51f514767 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,22 @@ This document outlines major changes between releases. +## 0.101.2 "Excavation" (29 Jun 2023) + +One more (and unexpected one!) 3.5.0-compatible version that fixes a VM bug +leading to mainnet state incompatibility (since 3672783) which in turn leads +to inability to process new blocks (since 3682290). + +Mainnet chain needs to be resynchronized for this version. + +Improvements: + * documentation updates (#3029, #2990, #2981) + +Bugs fixed: + * incorrect handling of empty Any-type parameter for RPC invocations (#2959) + * incorrect state rollbacks in case of exception during cross-contract call + when the call is made from non-TRYing context (#3046) + ## 0.101.1 "Shallowness" (17 Mar 2023) Another 3.5.0-compatible version that delivers important bug fixes and