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

@@ -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) {
+	if c.router.routeRemote(method) {
-		return c.local.CallContext(ctx, result, method, args...)
+		return c.upstream.CallContext(ctx, result, method, args...)
 	}
-
+	return c.local.CallContext(ctx, result, method, args...)
-	return c.upstream.CallContext(ctx, result, method, args...)
 }

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
+// routeRemote returns true if given method should be routed to the remote node
-// the local node
+func (r *router) routeRemote(method string) bool {
-func (r *router) routeLocally(method string) bool {
-	// if upstream is disabled, always route to
-	// the local node
 	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
+// remoteMethods contains methods that should be routed to
-// the local node; the rest is considered to be routed to
+// the upstream node; the rest is considered to be routed to
-// the upstream node.
+// the local node.
-// TODO: in the future, default option may be "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.
-// so it'd be convinient to keep track of "remoteMethods" here.
+// Although it's tempting to only list methods coming to the local node as there're fewer of them
-var localMethods = [...]string{
+// but it's deceptive: we want to ensure that only known requests leave our zone of responsibility.
-	//Whisper commands
+// Also, we want new requests in newer Geth versions not to be accidentally routed to the upstream.
-	"shh_post",
+// The list of methods: https://github.com/ethereum/wiki/wiki/JSON-RPC
-	"shh_version",
+var remoteMethods = [...]string{
-	"shh_newIdentity",
+	"eth_protocolVersion",
-	"shh_hasIdentity",
+	"eth_syncing",
-	"shh_newGroup",
+	"eth_coinbase",
-	"shh_addToGroup",
+	"eth_mining",
-	"shh_newFilter",
+	"eth_hashrate",
-	"shh_uninstallFilter",
+	"eth_gasPrice",
-	"shh_getFilterChanges",
+	//"eth_accounts", // goes to the local because we handle sub-accounts
-	"shh_getMessages",
+	"eth_blockNumber",
-
+	"eth_getBalance",
-	// DB commands
+	"eth_getStorageAt",
-	"db_putString",
+	"eth_getTransactionCount",
-	"db_getString",
+	"eth_getBlockTransactionCountByHash",
-	"db_putHex",
+	"eth_getBlockTransactionCountByNumber",
-	"db_getHex",
+	"eth_getUncleCountByBlockHash",
-
+	"eth_getUncleCountByBlockNumber",
-	// Other commands
+	"eth_getCode",
-	"net_version",
+	//"eth_sign", // goes to the local because only the local node has an injected account to sign the payload with
-	"net_peerCount",
+	"eth_sendTransaction",
-	"net_listening",
+	"eth_sendRawTransaction",
-
+	"eth_call",
-	// blockchain commands
+	"eth_estimateGas",
-	"eth_sign",
+	"eth_getBlockByHash",
-	"eth_accounts",
+	"eth_getBlockByNumber",
-	"eth_getCompilers",
+	"eth_getTransactionByHash",
-	"eth_compileLLL",
+	"eth_getTransactionByBlockHashAndIndex",
-	"eth_compileSolidity",
+	"eth_getTransactionByBlockNumberAndIndex",
-	"eth_compileSerpent",
+	"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",
 }

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 {
+	for _, method := range remoteMethods {
-		require.True(t, router.routeLocally(method))
+		require.True(t, router.routeRemote(method))
 	}
 
-	for _, method := range upstreamMethods {
+	for _, method := range localMethods {
-		require.False(t, router.routeLocally(method))
+		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 {
+	for _, method := range remoteMethods {
-		require.True(t, router.routeLocally(method))
+		require.True(t, router.routeRemote(method))
 	}
 
-	for _, method := range upstreamMethods {
+	for _, method := range localMethods {
-		require.True(t, router.routeLocally(method))
+		require.True(t, router.routeRemote(method))
 	}
 }