From fd2774ea91b1ace97afeda75f40ec2ba1bf4c0d9 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Wed, 27 Sep 2023 19:24:45 +0300 Subject: [PATCH 1/2] rpcsrv: return core.ErrVerificationFailed from calculatenetworkfee We can ignore core.ErrInvalidSignature (which means that the script has executed, but returned false), but we shouldn't ignore other errors which likely mean that the script is incorrect (or hits some resource limits). Use neorpc.ErrInvalidSignature as a return to separate this case from contract-based verification. Signed-off-by: Roman Khimov --- pkg/services/rpcsrv/server.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/services/rpcsrv/server.go b/pkg/services/rpcsrv/server.go index f973d0489..34872a5a4 100644 --- a/pkg/services/rpcsrv/server.go +++ b/pkg/services/rpcsrv/server.go @@ -951,7 +951,10 @@ func (s *Server) calculateNetworkFee(reqParams params.Params) (any, *neorpc.Erro } w.InvocationScript = inv.Bytes() } - gasConsumed, _ := s.chain.VerifyWitness(signer.Account, tx, &w, int64(s.config.MaxGasInvoke)) + gasConsumed, err := s.chain.VerifyWitness(signer.Account, tx, &w, int64(s.config.MaxGasInvoke)) + if err != nil && !errors.Is(err, core.ErrInvalidSignature) { + return nil, neorpc.WrapErrorWithData(neorpc.ErrInvalidSignature, err.Error()) + } netFee += gasConsumed size += io.GetVarSize(w.VerificationScript) + io.GetVarSize(w.InvocationScript) } From 49a44b0b9db05477f24a90745b6721fd5d2e0782 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Wed, 27 Sep 2023 19:31:21 +0300 Subject: [PATCH 2/2] rpcsrv: use stricter GAS limit for calculatenetworkfee Valid transactions can't use more than MaxVerificationGAS for script execution and this applies to the whole set of signers, so use this value by default unless local instance configuration suggests something lower for generic invocations. Signed-off-by: Roman Khimov --- docs/node-configuration.md | 4 +++- pkg/services/rpcsrv/server.go | 13 +++++++++++-- pkg/services/rpcsrv/server_test.go | 31 ++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/docs/node-configuration.md b/docs/node-configuration.md index 088e75e95..81b9862d3 100644 --- a/docs/node-configuration.md +++ b/docs/node-configuration.md @@ -233,7 +233,9 @@ where: proxy can be used to have proper app-specific CORS settings), but it's an easy way to make RPC interface accessible from the browser. - `MaxGasInvoke` is the maximum GAS allowed to spend during `invokefunction` and - `invokescript` RPC-calls. + `invokescript` RPC-calls. `calculatenetworkfee` also can't exceed this GAS amount + (normally the limit for it is MaxVerificationGAS from Policy, but if MaxGasInvoke + is lower than that then this limit is respected). - `MaxIteratorResultItems` - maximum number of elements extracted from iterator returned by `invoke*` call. When the `MaxIteratorResultItems` value is set to `n`, only `n` iterations are returned and truncated is true, indicating that diff --git a/pkg/services/rpcsrv/server.go b/pkg/services/rpcsrv/server.go index 34872a5a4..15519b20a 100644 --- a/pkg/services/rpcsrv/server.go +++ b/pkg/services/rpcsrv/server.go @@ -917,7 +917,15 @@ func (s *Server) calculateNetworkFee(reqParams params.Params) (any, *neorpc.Erro return 0, neorpc.WrapErrorWithData(neorpc.ErrInvalidParams, fmt.Sprintf("failed to compute tx size: %s", err)) } size := len(hashablePart) + io.GetVarSize(len(tx.Signers)) - var netFee int64 + var ( + netFee int64 + // Verification GAS cost can't exceed this policy. + gasLimit = s.chain.GetMaxVerificationGAS() + ) + if gasLimit > int64(s.config.MaxGasInvoke) { + // But we honor instance configuration as well. + gasLimit = int64(s.config.MaxGasInvoke) + } for i, signer := range tx.Signers { w := tx.Scripts[i] if len(w.InvocationScript) == 0 { // No invocation provided, try to infer one. @@ -951,10 +959,11 @@ func (s *Server) calculateNetworkFee(reqParams params.Params) (any, *neorpc.Erro } w.InvocationScript = inv.Bytes() } - gasConsumed, err := s.chain.VerifyWitness(signer.Account, tx, &w, int64(s.config.MaxGasInvoke)) + gasConsumed, err := s.chain.VerifyWitness(signer.Account, tx, &w, gasLimit) if err != nil && !errors.Is(err, core.ErrInvalidSignature) { return nil, neorpc.WrapErrorWithData(neorpc.ErrInvalidSignature, err.Error()) } + gasLimit -= gasConsumed netFee += gasConsumed size += io.GetVarSize(w.VerificationScript) + io.GetVarSize(w.InvocationScript) } diff --git a/pkg/services/rpcsrv/server_test.go b/pkg/services/rpcsrv/server_test.go index da27c69ce..d2793b6da 100644 --- a/pkg/services/rpcsrv/server_test.go +++ b/pkg/services/rpcsrv/server_test.go @@ -3,6 +3,7 @@ package rpcsrv import ( "bytes" "encoding/base64" + "encoding/binary" "encoding/json" "fmt" gio "io" @@ -26,6 +27,7 @@ import ( "github.com/nspcc-dev/neo-go/pkg/core" "github.com/nspcc-dev/neo-go/pkg/core/block" "github.com/nspcc-dev/neo-go/pkg/core/fee" + "github.com/nspcc-dev/neo-go/pkg/core/interop/interopnames" "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/core/storage/dboper" @@ -3184,6 +3186,21 @@ func testRPCProtocol(t *testing.T, doRPCCall func(string, string, *testing.T) [] body := calcReq(t, tx) _ = checkErrGetResult(t, body, true, neorpc.ErrInvalidVerificationFunctionCode, "signer 0 has no verify method in deployed contract") }) + t.Run("execution limit, fail", func(t *testing.T) { + // 1_6000_0000 GAS with the default 1.5 allowed by Policy + verifScript := []byte{byte(opcode.PUSHINT32), 0x00, 0x58, 0x89, 0x09, byte(opcode.SYSCALL), 0, 0, 0, 0, byte(opcode.PUSHT)} + binary.LittleEndian.PutUint32(verifScript[6:], interopnames.ToID([]byte(interopnames.SystemRuntimeBurnGas))) + tx := &transaction.Transaction{ + Script: []byte{byte(opcode.RET)}, + Signers: []transaction.Signer{{Account: hash.Hash160(verifScript)}}, + Scripts: []transaction.Witness{{ + InvocationScript: []byte{byte(opcode.NOP)}, + VerificationScript: verifScript, + }}, + } + body := calcReq(t, tx) + _ = checkErrGetResult(t, body, true, neorpc.ErrInvalidSignatureCode, "GAS limit exceeded") + }) checkCalc := func(t *testing.T, tx *transaction.Transaction, fee int64) { resp := checkErrGetResult(t, calcReq(t, tx), false, 0) res := new(result.NetworkFee) @@ -3258,6 +3275,20 @@ func testRPCProtocol(t *testing.T, doRPCCall func(string, string, *testing.T) [] invocScript := invocWriter.Bytes() checkContract(t, verAcc, invocScript, 146960) // No C# match, but we believe it's OK and it has a specific invocation script overriding anything server-side. }) + t.Run("execution limit, ok", func(t *testing.T) { + // 1_4000_0000 GAS with the default 1.5 allowed by Policy + verifScript := []byte{byte(opcode.PUSHINT32), 0x00, 0x3b, 0x58, 0x08, byte(opcode.SYSCALL), 0, 0, 0, 0, byte(opcode.PUSHT)} + binary.LittleEndian.PutUint32(verifScript[6:], interopnames.ToID([]byte(interopnames.SystemRuntimeBurnGas))) + tx := &transaction.Transaction{ + Script: []byte{byte(opcode.RET)}, + Signers: []transaction.Signer{{Account: hash.Hash160(verifScript)}}, + Scripts: []transaction.Witness{{ + InvocationScript: []byte{byte(opcode.NOP)}, + VerificationScript: verifScript, + }}, + } + checkCalc(t, tx, 140065570) + }) }) t.Run("sendrawtransaction", func(t *testing.T) { rpc := `{"jsonrpc": "2.0", "id": 1, "method": "sendrawtransaction", "params": ["%s"]}`