From 0092330fe1bd67eaab4c5eef6459f66a698d0bbf Mon Sep 17 00:00:00 2001 From: AnnaShaleva Date: Mon, 21 Feb 2022 13:41:14 +0300 Subject: [PATCH] rpc: carefully review places where Client.cache.initDone is used 1. Keep initDone check only for the places where cache is directly accessed. We don't need to check it in other places, otherwise we have a mess of duplicating checks. 2. Fix bug in code related to block deserialisation. There's no magic, so checking that initialisation is done is not enough for proper block deserialisation. We need to manually fill StateRootEnabled field. 3. Since transaction doesn't need network magic to compute its hash, we don't need to perform Client initialisation before transaction-related requests. 4. Check that cache is initialised before accessing network magic. 5. Refactor the way Policy contract hash is fetched for Client requests. We don't really need Client initialisation for that, it's OK to fetch Policy hash on-the-fly. --- cli/smartcontract/smart_contract.go | 6 ++- cli/wallet/nep11.go | 6 ++- cli/wallet/nep17.go | 6 ++- pkg/rpc/client/client.go | 4 +- pkg/rpc/client/nep11.go | 6 --- pkg/rpc/client/nep17.go | 8 ---- pkg/rpc/client/policy.go | 23 ++++------ pkg/rpc/client/rpc.go | 69 ++++++++++++++++------------- pkg/rpc/client/rpc_test.go | 8 +++- pkg/rpc/client/wsclient.go | 7 ++- pkg/rpc/client/wsclient_test.go | 1 + pkg/rpc/server/client_test.go | 8 ++-- 12 files changed, 81 insertions(+), 71 deletions(-) diff --git a/cli/smartcontract/smart_contract.go b/cli/smartcontract/smart_contract.go index 058e0eee5..b374353e4 100644 --- a/cli/smartcontract/smart_contract.go +++ b/cli/smartcontract/smart_contract.go @@ -681,7 +681,11 @@ func invokeWithArgs(ctx *cli.Context, acc *wallet.Account, wall *wallet.Wallet, if err != nil { return sender, cli.NewExitError(fmt.Errorf("failed to create tx: %w", err), 1) } - if err := paramcontext.InitAndSave(c.GetNetwork(), tx, acc, out); err != nil { + m, err := c.GetNetwork() + if err != nil { + return sender, cli.NewExitError(fmt.Errorf("failed to save tx: %w", err), 1) + } + if err := paramcontext.InitAndSave(m, tx, acc, out); err != nil { return sender, cli.NewExitError(err, 1) } fmt.Fprintln(ctx.App.Writer, tx.Hash().StringLE()) diff --git a/cli/wallet/nep11.go b/cli/wallet/nep11.go index d5adb21c0..335d35bf9 100644 --- a/cli/wallet/nep11.go +++ b/cli/wallet/nep11.go @@ -274,7 +274,11 @@ func signAndSendNEP11Transfer(ctx *cli.Context, c *client.Client, acc *wallet.Ac tx.SystemFee += int64(sysgas) if outFile := ctx.String("out"); outFile != "" { - if err := paramcontext.InitAndSave(c.GetNetwork(), tx, acc, outFile); err != nil { + m, err := c.GetNetwork() + if err != nil { + return cli.NewExitError(fmt.Errorf("failed to save tx: %w", err), 1) + } + if err := paramcontext.InitAndSave(m, tx, acc, outFile); err != nil { return cli.NewExitError(err, 1) } } else { diff --git a/cli/wallet/nep17.go b/cli/wallet/nep17.go index 417c488fe..a11766d83 100644 --- a/cli/wallet/nep17.go +++ b/cli/wallet/nep17.go @@ -647,7 +647,11 @@ func signAndSendNEP17Transfer(ctx *cli.Context, c *client.Client, acc *wallet.Ac tx.SystemFee += int64(sysgas) if outFile := ctx.String("out"); outFile != "" { - if err := paramcontext.InitAndSave(c.GetNetwork(), tx, acc, outFile); err != nil { + m, err := c.GetNetwork() + if err != nil { + return cli.NewExitError(fmt.Errorf("failed to save tx: %w", err), 1) + } + if err := paramcontext.InitAndSave(m, tx, acc, outFile); err != nil { return cli.NewExitError(err, 1) } } else { diff --git a/pkg/rpc/client/client.go b/pkg/rpc/client/client.go index 4cae3a11a..4f41bd10b 100644 --- a/pkg/rpc/client/client.go +++ b/pkg/rpc/client/client.go @@ -116,8 +116,8 @@ func New(ctx context.Context, endpoint string, opts Options) (*Client, error) { // Init sets magic of the network client connected to, stateRootInHeader option // and native NEO, GAS and Policy contracts scripthashes. This method should be -// called before any transaction-, header- or block-related requests in order to -// deserialize responses properly. +// called before any header- or block-related requests in order to deserialize +// responses properly. func (c *Client) Init() error { version, err := c.GetVersion() if err != nil { diff --git a/pkg/rpc/client/nep11.go b/pkg/rpc/client/nep11.go index d9f759b5d..40217f517 100644 --- a/pkg/rpc/client/nep11.go +++ b/pkg/rpc/client/nep11.go @@ -46,9 +46,6 @@ func (c *Client) NEP11TokenInfo(tokenHash util.Uint160) (*wallet.Token, error) { // given account and sends it to the network returning just a hash of it. func (c *Client) TransferNEP11(acc *wallet.Account, to util.Uint160, tokenHash util.Uint160, tokenID string, data interface{}, gas int64, cosigners []SignerAccount) (util.Uint256, error) { - if !c.cache.initDone { - return util.Uint256{}, errNetworkNotInitialized - } tx, err := c.CreateNEP11TransferTx(acc, tokenHash, gas, cosigners, to, tokenID, data) if err != nil { return util.Uint256{}, err @@ -144,9 +141,6 @@ func (c *Client) NEP11NDOwnerOf(tokenHash util.Uint160, tokenID []byte) (util.Ui // sends it to the network returning just a hash of it. func (c *Client) TransferNEP11D(acc *wallet.Account, to util.Uint160, tokenHash util.Uint160, amount int64, tokenID []byte, data interface{}, gas int64, cosigners []SignerAccount) (util.Uint256, error) { - if !c.cache.initDone { - return util.Uint256{}, errNetworkNotInitialized - } from, err := address.StringToUint160(acc.Address) if err != nil { return util.Uint256{}, fmt.Errorf("bad account address: %w", err) diff --git a/pkg/rpc/client/nep17.go b/pkg/rpc/client/nep17.go index bd296b854..22dcf6ea6 100644 --- a/pkg/rpc/client/nep17.go +++ b/pkg/rpc/client/nep17.go @@ -142,10 +142,6 @@ func (c *Client) CreateTxFromScript(script []byte, acc *wallet.Account, sysFee, // impossible (e.g. due to locked cosigner's account) an error is returned. func (c *Client) TransferNEP17(acc *wallet.Account, to util.Uint160, token util.Uint160, amount int64, gas int64, data interface{}, cosigners []SignerAccount) (util.Uint256, error) { - if !c.cache.initDone { - return util.Uint256{}, errNetworkNotInitialized - } - tx, err := c.CreateNEP17TransferTx(acc, to, token, amount, gas, data, cosigners) if err != nil { return util.Uint256{}, err @@ -156,10 +152,6 @@ func (c *Client) TransferNEP17(acc *wallet.Account, to util.Uint160, token util. // MultiTransferNEP17 is similar to TransferNEP17, buf allows to have multiple recipients. func (c *Client) MultiTransferNEP17(acc *wallet.Account, gas int64, recipients []TransferTarget, cosigners []SignerAccount) (util.Uint256, error) { - if !c.cache.initDone { - return util.Uint256{}, errNetworkNotInitialized - } - tx, err := c.CreateNEP17MultiTransferTx(acc, gas, recipients, cosigners) if err != nil { return util.Uint256{}, err diff --git a/pkg/rpc/client/policy.go b/pkg/rpc/client/policy.go index 358d2bfe6..8fe8f0a2b 100644 --- a/pkg/rpc/client/policy.go +++ b/pkg/rpc/client/policy.go @@ -10,25 +10,16 @@ import ( // GetFeePerByte invokes `getFeePerByte` method on a native Policy contract. func (c *Client) GetFeePerByte() (int64, error) { - if !c.cache.initDone { - return 0, errNetworkNotInitialized - } return c.invokeNativePolicyMethod("getFeePerByte") } // GetExecFeeFactor invokes `getExecFeeFactor` method on a native Policy contract. func (c *Client) GetExecFeeFactor() (int64, error) { - if !c.cache.initDone { - return 0, errNetworkNotInitialized - } return c.invokeNativePolicyMethod("getExecFeeFactor") } // GetStoragePrice invokes `getStoragePrice` method on a native Policy contract. func (c *Client) GetStoragePrice() (int64, error) { - if !c.cache.initDone { - return 0, errNetworkNotInitialized - } return c.invokeNativePolicyMethod("getStoragePrice") } @@ -43,10 +34,11 @@ func (c *Client) GetMaxNotValidBeforeDelta() (int64, error) { // invokeNativePolicy method invokes Get* method on a native Policy contract. func (c *Client) invokeNativePolicyMethod(operation string) (int64, error) { - if !c.cache.initDone { - return 0, errNetworkNotInitialized + policyHash, err := c.GetNativeContractHash(nativenames.Policy) + if err != nil { + return 0, fmt.Errorf("failed to get native Policy hash: %w", err) } - return c.invokeNativeGetMethod(c.cache.nativeHashes[nativenames.Policy], operation) + return c.invokeNativeGetMethod(policyHash, operation) } func (c *Client) invokeNativeGetMethod(hash util.Uint160, operation string) (int64, error) { @@ -63,10 +55,11 @@ func (c *Client) invokeNativeGetMethod(hash util.Uint160, operation string) (int // IsBlocked invokes `isBlocked` method on native Policy contract. func (c *Client) IsBlocked(hash util.Uint160) (bool, error) { - if !c.cache.initDone { - return false, errNetworkNotInitialized + policyHash, err := c.GetNativeContractHash(nativenames.Policy) + if err != nil { + return false, fmt.Errorf("failed to get native Policy hash: %w", err) } - result, err := c.InvokeFunction(c.cache.nativeHashes[nativenames.Policy], "isBlocked", []smartcontract.Parameter{{ + result, err := c.InvokeFunction(policyHash, "isBlocked", []smartcontract.Parameter{{ Type: smartcontract.Hash160Type, Value: hash, }}, nil) diff --git a/pkg/rpc/client/rpc.go b/pkg/rpc/client/rpc.go index 41aed20d8..d00ede814 100644 --- a/pkg/rpc/client/rpc.go +++ b/pkg/rpc/client/rpc.go @@ -94,14 +94,15 @@ func (c *Client) getBlock(params request.RawParams) (*block.Block, error) { err error b *block.Block ) - if !c.cache.initDone { - return nil, errNetworkNotInitialized - } if err = c.performRequest("getblock", params, &resp); err != nil { return nil, err } r := io.NewBinReaderFromBuf(resp) - b = block.New(c.StateRootInHeader()) + sr, err := c.StateRootInHeader() + if err != nil { + return nil, err + } + b = block.New(sr) b.DecodeBinary(r) if r.Err != nil { return nil, r.Err @@ -127,9 +128,11 @@ func (c *Client) getBlockVerbose(params request.RawParams) (*result.Block, error resp = &result.Block{} err error ) - if !c.cache.initDone { - return nil, errNetworkNotInitialized + sr, err := c.StateRootInHeader() + if err != nil { + return nil, err } + resp.Header.StateRootEnabled = sr if err = c.performRequest("getblock", params, resp); err != nil { return nil, err } @@ -157,14 +160,16 @@ func (c *Client) GetBlockHeader(hash util.Uint256) (*block.Header, error) { resp []byte h *block.Header ) - if !c.cache.initDone { - return nil, errNetworkNotInitialized - } if err := c.performRequest("getblockheader", params, &resp); err != nil { return nil, err } + sr, err := c.StateRootInHeader() + if err != nil { + return nil, err + } r := io.NewBinReaderFromBuf(resp) h = new(block.Header) + h.StateRootEnabled = sr h.DecodeBinary(r) if r.Err != nil { return nil, r.Err @@ -399,17 +404,13 @@ func (c *Client) GetRawMemPool() ([]util.Uint256, error) { return *resp, nil } -// GetRawTransaction returns a transaction by hash. You should initialize network magic -// with Init before calling GetRawTransaction. +// GetRawTransaction returns a transaction by hash. func (c *Client) GetRawTransaction(hash util.Uint256) (*transaction.Transaction, error) { var ( params = request.NewRawParams(hash.StringLE()) resp []byte err error ) - if !c.cache.initDone { - return nil, errNetworkNotInitialized - } if err = c.performRequest("getrawtransaction", params, &resp); err != nil { return nil, err } @@ -421,8 +422,7 @@ func (c *Client) GetRawTransaction(hash util.Uint256) (*transaction.Transaction, } // GetRawTransactionVerbose returns a transaction wrapper with additional -// metadata by transaction's hash. You should initialize network magic -// with Init before calling GetRawTransactionVerbose. +// metadata by transaction's hash. // NOTE: to get transaction.ID and transaction.Size, use t.Hash() and io.GetVarSize(t) respectively. func (c *Client) GetRawTransactionVerbose(hash util.Uint256) (*result.TransactionOutputRaw, error) { var ( @@ -430,9 +430,6 @@ func (c *Client) GetRawTransactionVerbose(hash util.Uint256) (*result.Transactio resp = &result.TransactionOutputRaw{} err error ) - if !c.cache.initDone { - return nil, errNetworkNotInitialized - } if err = c.performRequest("getrawtransaction", params, resp); err != nil { return nil, err } @@ -687,7 +684,11 @@ func (c *Client) SignAndPushTx(tx *transaction.Transaction, acc *wallet.Account, txHash util.Uint256 err error ) - if err = acc.SignTx(c.GetNetwork(), tx); err != nil { + m, err := c.GetNetwork() + if err != nil { + return txHash, fmt.Errorf("failed to sign tx: %w", err) + } + if err = acc.SignTx(m, tx); err != nil { return txHash, fmt.Errorf("failed to sign tx: %w", err) } // try to add witnesses for the rest of the signers @@ -695,7 +696,7 @@ func (c *Client) SignAndPushTx(tx *transaction.Transaction, acc *wallet.Account, var isOk bool for _, cosigner := range cosigners { if signer.Account == cosigner.Signer.Account { - err = cosigner.Account.SignTx(c.GetNetwork(), tx) + err = cosigner.Account.SignTx(m, tx) if err != nil { // then account is non-contract-based and locked, but let's provide more detailed error if paramNum := len(cosigner.Account.Contract.Parameters); paramNum != 0 && cosigner.Account.Contract.Deployed { return txHash, fmt.Errorf("failed to add contract-based witness for signer #%d (%s): "+ @@ -771,9 +772,6 @@ func getSigners(sender *wallet.Account, cosigners []SignerAccount) ([]transactio // Note: client should be initialized before SignAndPushP2PNotaryRequest call. func (c *Client) SignAndPushP2PNotaryRequest(mainTx *transaction.Transaction, fallbackScript []byte, fallbackSysFee int64, fallbackNetFee int64, fallbackValidFor uint32, acc *wallet.Account) (*payload.P2PNotaryRequest, error) { var err error - if !c.cache.initDone { - return nil, errNetworkNotInitialized - } notaryHash, err := c.GetNativeContractHash(nativenames.Notary) if err != nil { return nil, fmt.Errorf("failed to get native Notary hash: %w", err) @@ -835,7 +833,11 @@ func (c *Client) SignAndPushP2PNotaryRequest(mainTx *transaction.Transaction, fa VerificationScript: []byte{}, }, } - if err = acc.SignTx(c.GetNetwork(), fallbackTx); err != nil { + m, err := c.GetNetwork() + if err != nil { + return nil, fmt.Errorf("failed to sign fallback tx: %w", err) + } + if err = acc.SignTx(m, fallbackTx); err != nil { return nil, fmt.Errorf("failed to sign fallback tx: %w", err) } fallbackHash := fallbackTx.Hash() @@ -844,7 +846,7 @@ func (c *Client) SignAndPushP2PNotaryRequest(mainTx *transaction.Transaction, fa FallbackTransaction: fallbackTx, } req.Witness = transaction.Witness{ - InvocationScript: append([]byte{byte(opcode.PUSHDATA1), 64}, acc.PrivateKey().SignHashable(uint32(c.GetNetwork()), req)...), + InvocationScript: append([]byte{byte(opcode.PUSHDATA1), 64}, acc.PrivateKey().SignHashable(uint32(m), req)...), VerificationScript: acc.GetVerificationScript(), } actualHash, err := c.SubmitP2PNotaryRequest(req) @@ -990,13 +992,20 @@ func (c *Client) AddNetworkFee(tx *transaction.Transaction, extraFee int64, accs } // GetNetwork returns the network magic of the RPC node client connected to. -func (c *Client) GetNetwork() netmode.Magic { - return c.cache.network +func (c *Client) GetNetwork() (netmode.Magic, error) { + if !c.cache.initDone { + return 0, errNetworkNotInitialized + } + return c.cache.network, nil } // StateRootInHeader returns true if state root is contained in block header. -func (c *Client) StateRootInHeader() bool { - return c.cache.stateRootInHeader +// You should initialize Client cache with Init() before calling StateRootInHeader. +func (c *Client) StateRootInHeader() (bool, error) { + if !c.cache.initDone { + return false, errNetworkNotInitialized + } + return c.cache.stateRootInHeader, nil } // GetNativeContractHash returns native contract hash by its name. diff --git a/pkg/rpc/client/rpc_test.go b/pkg/rpc/client/rpc_test.go index ee1b6c71b..8c099e810 100644 --- a/pkg/rpc/client/rpc_test.go +++ b/pkg/rpc/client/rpc_test.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "encoding/hex" "encoding/json" + "errors" "fmt" "math/big" "net/http" @@ -1913,7 +1914,8 @@ func TestGetNetwork(t *testing.T) { t.Fatal(err) } // network was not initialised - require.Equal(t, netmode.Magic(0), c.GetNetwork()) + _, err = c.GetNetwork() + require.True(t, errors.Is(err, errNetworkNotInitialized)) require.Equal(t, false, c.cache.initDone) }) @@ -1923,7 +1925,9 @@ func TestGetNetwork(t *testing.T) { t.Fatal(err) } require.NoError(t, c.Init()) - require.Equal(t, netmode.UnitTestNet, c.GetNetwork()) + m, err := c.GetNetwork() + require.NoError(t, err) + require.Equal(t, netmode.UnitTestNet, m) }) } diff --git a/pkg/rpc/client/wsclient.go b/pkg/rpc/client/wsclient.go index f2ed6aed8..5239d84b6 100644 --- a/pkg/rpc/client/wsclient.go +++ b/pkg/rpc/client/wsclient.go @@ -141,7 +141,12 @@ readloop: var val interface{} switch event { case response.BlockEventID: - val = block.New(c.StateRootInHeader()) + sr, err := c.StateRootInHeader() + if err != nil { + // Client is not initialised. + break + } + val = block.New(sr) case response.TransactionEventID: val = &transaction.Transaction{} case response.NotificationEventID: diff --git a/pkg/rpc/client/wsclient_test.go b/pkg/rpc/client/wsclient_test.go index f625a99b0..7a8eda105 100644 --- a/pkg/rpc/client/wsclient_test.go +++ b/pkg/rpc/client/wsclient_test.go @@ -143,6 +143,7 @@ func TestWSClientEvents(t *testing.T) { wsc, err := NewWS(context.TODO(), httpURLtoWS(srv.URL), Options{}) require.NoError(t, err) + wsc.cache.initDone = true // Our server mock is restricted, so perform initialisation manually. wsc.cache.network = netmode.UnitTestNet for range events { select { diff --git a/pkg/rpc/server/client_test.go b/pkg/rpc/server/client_test.go index 18c2b418d..4e9c8c4d3 100644 --- a/pkg/rpc/server/client_test.go +++ b/pkg/rpc/server/client_test.go @@ -555,9 +555,11 @@ func TestSignAndPushP2PNotaryRequest(t *testing.T) { c, err := client.New(context.Background(), httpSrv.URL, client.Options{}) require.NoError(t, err) + acc, err := wallet.NewAccount() + require.NoError(t, err) t.Run("client wasn't initialized", func(t *testing.T) { - _, err := c.SignAndPushP2PNotaryRequest(nil, nil, 0, 0, 0, nil) + _, err := c.SignAndPushP2PNotaryRequest(transaction.New([]byte{byte(opcode.RET)}, 123), []byte{byte(opcode.RET)}, -1, 0, 100, acc) require.NotNil(t, err) }) @@ -567,8 +569,6 @@ func TestSignAndPushP2PNotaryRequest(t *testing.T) { require.NotNil(t, err) }) - acc, err := wallet.NewAccount() - require.NoError(t, err) t.Run("bad fallback script", func(t *testing.T) { _, err := c.SignAndPushP2PNotaryRequest(nil, []byte{byte(opcode.ASSERT)}, -1, 0, 0, acc) require.NotNil(t, err) @@ -649,7 +649,7 @@ func TestCalculateNotaryFee(t *testing.T) { t.Run("client not initialized", func(t *testing.T) { _, err := c.CalculateNotaryFee(0) - require.NotNil(t, err) + require.NoError(t, err) // Do not require client initialisation for this. }) }