From 9d01f7aa262a5001e4ec915394922ae5e34f7328 Mon Sep 17 00:00:00 2001 From: Ivan Tomilov Date: Tue, 19 Sep 2017 12:52:38 +0300 Subject: [PATCH 1/2] fixed that shh_newMessageFilter was erroneously routed to the upstream instead of local (#345) An issue arose that shh_newMessageFilter was routed to the upstream instead of local node. This PR fixes that. It also revisits routing logic and makes all requests go to the local node by default. --- geth/rpc/client.go | 7 ++- geth/rpc/route.go | 99 ++++++++++++++++++++++++------------------ geth/rpc/route_test.go | 20 +++++---- 3 files changed, 70 insertions(+), 56 deletions(-) diff --git a/geth/rpc/client.go b/geth/rpc/client.go index 2f480e26b..903269059 100644 --- a/geth/rpc/client.go +++ b/geth/rpc/client.go @@ -72,9 +72,8 @@ func (c *Client) Call(result interface{}, method string, args ...interface{}) er // // It uses custom routing scheme for calls. func (c *Client) CallContext(ctx context.Context, result interface{}, method string, args ...interface{}) error { - if c.router.routeLocally(method) { - return c.local.CallContext(ctx, result, method, args...) + if c.router.routeRemote(method) { + return c.upstream.CallContext(ctx, result, method, args...) } - - return c.upstream.CallContext(ctx, result, method, args...) + return c.local.CallContext(ctx, result, method, args...) } diff --git a/geth/rpc/route.go b/geth/rpc/route.go index 10cce9943..f707bc497 100644 --- a/geth/rpc/route.go +++ b/geth/rpc/route.go @@ -15,60 +15,73 @@ func newRouter(upstreamEnabled bool) *router { upstreamEnabled: upstreamEnabled, } - for _, m := range localMethods { + for _, m := range remoteMethods { r.methods[m] = true } return r } -// routeLocally returns true if given method should be routed to -// the local node -func (r *router) routeLocally(method string) bool { - // if upstream is disabled, always route to - // the local node +// routeRemote returns true if given method should be routed to the remote node +func (r *router) routeRemote(method string) bool { if !r.upstreamEnabled { - return true + return false } // else check route using the methods list return r.methods[method] } -// localMethods contains methods that should be routed to -// the local node; the rest is considered to be routed to -// the upstream node. -// TODO: in the future, default option may be "local node", -// so it'd be convinient to keep track of "remoteMethods" here. -var localMethods = [...]string{ - //Whisper commands - "shh_post", - "shh_version", - "shh_newIdentity", - "shh_hasIdentity", - "shh_newGroup", - "shh_addToGroup", - "shh_newFilter", - "shh_uninstallFilter", - "shh_getFilterChanges", - "shh_getMessages", - - // DB commands - "db_putString", - "db_getString", - "db_putHex", - "db_getHex", - - // Other commands - "net_version", - "net_peerCount", - "net_listening", - - // blockchain commands - "eth_sign", - "eth_accounts", - "eth_getCompilers", - "eth_compileLLL", - "eth_compileSolidity", - "eth_compileSerpent", +// remoteMethods contains methods that should be routed to +// the upstream node; the rest is considered to be routed to +// the local node. +// TODO(tiabc): Write a test on each of these methods to ensure they're all routed to the proper node and ensure they really work. +// Although it's tempting to only list methods coming to the local node as there're fewer of them +// but it's deceptive: we want to ensure that only known requests leave our zone of responsibility. +// Also, we want new requests in newer Geth versions not to be accidentally routed to the upstream. +// The list of methods: https://github.com/ethereum/wiki/wiki/JSON-RPC +var remoteMethods = [...]string{ + "eth_protocolVersion", + "eth_syncing", + "eth_coinbase", + "eth_mining", + "eth_hashrate", + "eth_gasPrice", + //"eth_accounts", // goes to the local because we handle sub-accounts + "eth_blockNumber", + "eth_getBalance", + "eth_getStorageAt", + "eth_getTransactionCount", + "eth_getBlockTransactionCountByHash", + "eth_getBlockTransactionCountByNumber", + "eth_getUncleCountByBlockHash", + "eth_getUncleCountByBlockNumber", + "eth_getCode", + //"eth_sign", // goes to the local because only the local node has an injected account to sign the payload with + "eth_sendTransaction", + "eth_sendRawTransaction", + "eth_call", + "eth_estimateGas", + "eth_getBlockByHash", + "eth_getBlockByNumber", + "eth_getTransactionByHash", + "eth_getTransactionByBlockHashAndIndex", + "eth_getTransactionByBlockNumberAndIndex", + "eth_getTransactionReceipt", + "eth_getUncleByBlockHashAndIndex", + "eth_getUncleByBlockNumberAndIndex", + //"eth_getCompilers", // goes to the local because there's no need to send it anywhere + //"eth_compileLLL", // goes to the local because there's no need to send it anywhere + //"eth_compileSolidity", // goes to the local because there's no need to send it anywhere + //"eth_compileSerpent", // goes to the local because there's no need to send it anywhere + "eth_newFilter", + "eth_newBlockFilter", + "eth_newPendingTransactionFilter", + "eth_uninstallFilter", + "eth_getFilterChanges", + "eth_getFilterLogs", + "eth_getLogs", + "eth_getWork", + "eth_submitWork", + "eth_submitHashrate", } diff --git a/geth/rpc/route_test.go b/geth/rpc/route_test.go index 2c9db90a8..06df0ed6a 100644 --- a/geth/rpc/route_test.go +++ b/geth/rpc/route_test.go @@ -6,28 +6,30 @@ import ( ) // some of the upstream examples -var upstreamMethods = []string{"some_weirdo_method", "eth_syncing", "eth_getBalance", "eth_call", "eth_getTransactionReceipt"} +var localMethods = []string{"some_weirdo_method", "shh_newMessageFilter", "net_version"} func TestRouteWithUpstream(t *testing.T) { router := newRouter(true) - for _, method := range localMethods { - require.True(t, router.routeLocally(method)) + for _, method := range remoteMethods { + require.True(t, router.routeRemote(method)) } - for _, method := range upstreamMethods { - require.False(t, router.routeLocally(method)) + for _, method := range localMethods { + t.Run(method, func(t *testing.T) { + require.False(t, router.routeRemote(method)) + }) } } func TestRouteWithoutUpstream(t *testing.T) { router := newRouter(false) - for _, method := range localMethods { - require.True(t, router.routeLocally(method)) + for _, method := range remoteMethods { + require.True(t, router.routeRemote(method)) } - for _, method := range upstreamMethods { - require.True(t, router.routeLocally(method)) + for _, method := range localMethods { + require.True(t, router.routeRemote(method)) } } From ba963cc1bddbcf450e0665519baaa759d975e31f Mon Sep 17 00:00:00 2001 From: Adam Babik Date: Tue, 19 Sep 2017 13:19:18 +0200 Subject: [PATCH 2/2] estimate gas if missing in sending tx using upstream (#346) I added a call to eth_estimateGas in case gas param is missing when using the upstream. This is a little bit ugly because I create an anonymous struct to match eth_call params. I spotted this struct in go-ethereum but I don't see it's exported. --- geth/api/backend_txqueue_test.go | 53 ++++++++++++++++++++++++++++++ geth/node/txqueue_manager.go | 56 ++++++++++++++++++++++++++++++-- 2 files changed, 107 insertions(+), 2 deletions(-) diff --git a/geth/api/backend_txqueue_test.go b/geth/api/backend_txqueue_test.go index 7790c1375..d9e923fd9 100644 --- a/geth/api/backend_txqueue_test.go +++ b/geth/api/backend_txqueue_test.go @@ -195,6 +195,59 @@ func (s *BackendTestSuite) TestSendEtherTx() { s.Zero(s.backend.TxQueueManager().TransactionQueue().Count(), "tx queue must be empty at this point") } +func (s *BackendTestSuite) TestSendEtherTxUpstream() { + s.StartTestBackend(params.RopstenNetworkID, WithUpstream("https://ropsten.infura.io/z6GCTmjdP3FETEJmMBI4")) + defer s.StopTestBackend() + + time.Sleep(TestConfig.Node.SyncSeconds * time.Second) // allow to sync + + err := s.backend.AccountManager().SelectAccount(TestConfig.Account1.Address, TestConfig.Account1.Password) + s.NoError(err) + + completeQueuedTransaction := make(chan struct{}) + + // replace transaction notification handler + var txHash = gethcommon.Hash{} + node.SetDefaultNodeNotificationHandler(func(jsonEvent string) { // nolint: dupl + var envelope node.SignalEnvelope + err := json.Unmarshal([]byte(jsonEvent), &envelope) + s.NoError(err, "cannot unmarshal JSON: %s", jsonEvent) + + if envelope.Type == node.EventTransactionQueued { + event := envelope.Event.(map[string]interface{}) + log.Info("transaction queued (will be completed shortly)", "id", event["id"].(string)) + + txHash, err = s.backend.CompleteTransaction( + common.QueuedTxID(event["id"].(string)), + TestConfig.Account1.Password, + ) + s.NoError(err, "cannot complete queued transaction[%v]", event["id"]) + + log.Info("contract transaction complete", "URL", "https://ropsten.etherscan.io/tx/"+txHash.Hex()) + close(completeQueuedTransaction) + } + }) + + // This call blocks, up until Complete Transaction is called. + // Explicitly not setting Gas to get it estimated. + txHashCheck, err := s.backend.SendTransaction(nil, common.SendTxArgs{ + From: common.FromAddress(TestConfig.Account1.Address), + To: common.ToAddress(TestConfig.Account2.Address), + GasPrice: (*hexutil.Big)(big.NewInt(28000000000)), + Value: (*hexutil.Big)(big.NewInt(1000000000000)), + }) + s.NoError(err, "cannot send transaction") + + select { + case <-completeQueuedTransaction: + case <-time.After(1 * time.Minute): + s.FailNow("completing transaction timed out") + } + + s.Equal(txHash.Hex(), txHashCheck.Hex(), "transaction hash returned from SendTransaction is invalid") + s.Zero(s.backend.TxQueueManager().TransactionQueue().Count(), "tx queue must be empty at this point") +} + func (s *BackendTestSuite) TestDoubleCompleteQueuedTransactions() { require := s.Require() require.NotNil(s.backend) diff --git a/geth/node/txqueue_manager.go b/geth/node/txqueue_manager.go index 94be53596..4b9904a0e 100644 --- a/geth/node/txqueue_manager.go +++ b/geth/node/txqueue_manager.go @@ -227,12 +227,16 @@ func (m *TxQueueManager) completeRemoteTransaction(queuedTx *common.QueuedTx, pa chainID := big.NewInt(int64(config.NetworkID)) nonce := uint64(txCount) - gas := (*big.Int)(queuedTx.Args.Gas) gasPrice := (*big.Int)(queuedTx.Args.GasPrice) dataVal := []byte(queuedTx.Args.Data) priceVal := (*big.Int)(queuedTx.Args.Value) - tx := types.NewTransaction(nonce, *queuedTx.Args.To, priceVal, gas, gasPrice, dataVal) + gas, err := m.estimateGas(queuedTx.Args) + if err != nil { + return emptyHash, err + } + + tx := types.NewTransaction(nonce, *queuedTx.Args.To, priceVal, (*big.Int)(gas), gasPrice, dataVal) signedTx, err := types.SignTx(tx, types.NewEIP155Signer(chainID), selectedAcct.AccountKey.PrivateKey) if err != nil { return emptyHash, err @@ -253,6 +257,54 @@ func (m *TxQueueManager) completeRemoteTransaction(queuedTx *common.QueuedTx, pa return signedTx.Hash(), nil } +func (m *TxQueueManager) estimateGas(args common.SendTxArgs) (*hexutil.Big, error) { + if args.Gas != nil { + return args.Gas, nil + } + + client := m.nodeManager.RPCClient() + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + var gasPrice hexutil.Big + if args.GasPrice != nil { + gasPrice = (hexutil.Big)(*args.GasPrice) + } + + var value hexutil.Big + if args.Value != nil { + value = (hexutil.Big)(*args.Value) + } + + params := struct { + From gethcommon.Address `json:"from"` + To *gethcommon.Address `json:"to"` + Gas hexutil.Big `json:"gas"` + GasPrice hexutil.Big `json:"gasPrice"` + Value hexutil.Big `json:"value"` + Data hexutil.Bytes `json:"data"` + }{ + From: args.From, + To: args.To, + GasPrice: gasPrice, + Value: value, + Data: []byte(args.Data), + } + + var estimatedGas hexutil.Big + if err := client.CallContext( + ctx, + &estimatedGas, + "eth_estimateGas", + params, + ); err != nil { + log.Warn("failed to estimate gas", "err", err) + return nil, err + } + + return &estimatedGas, nil +} + // CompleteTransactions instructs backend to complete sending of multiple transactions func (m *TxQueueManager) CompleteTransactions(ids []common.QueuedTxID, password string) map[common.QueuedTxID]common.RawCompleteTransactionResult { results := make(map[common.QueuedTxID]common.RawCompleteTransactionResult)