Json RPCs report Invalid_Request for unsupported ops (#40)

With the advent of Clique, some JSON RPCs pertaining to mining are
no longer going to produce appropriate results.

The Get/Set coinbase RPCs cannot affect a Clique based miner, as such
the miner throws an "Unsupported Operation" exception. The JSON
handler is responsible for catching this, and returning an
appropriate error code.
Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

ethereum/jsonrpc/src/main/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbase.java

@@ -25,10 +25,14 @@ public class EthCoinbase implements JsonRpcMethod {
 
   @Override
   public JsonRpcResponse response(final JsonRpcRequest req) {
-    final Optional<Address> coinbase = miningCoordinator.getCoinbase();
-    if (coinbase.isPresent()) {
-      return new JsonRpcSuccessResponse(req.getId(), coinbase.get().toString());
+    try {
+      final Optional<Address> coinbase = miningCoordinator.getCoinbase();
+      if (coinbase.isPresent()) {
+        return new JsonRpcSuccessResponse(req.getId(), coinbase.get().toString());
+      }
+      return new JsonRpcErrorResponse(req.getId(), JsonRpcError.COINBASE_NOT_SPECIFIED);
+    } catch (UnsupportedOperationException ex) {
+      return new JsonRpcErrorResponse(req.getId(), JsonRpcError.INVALID_REQUEST);
     }
-    return new JsonRpcErrorResponse(req.getId(), JsonRpcError.COINBASE_NOT_SPECIFIED);
   }
 }

ethereum/jsonrpc/src/main/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/miner/MinerSetCoinbase.java

@@ -5,6 +5,8 @@ import net.consensys.pantheon.ethereum.core.Address;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.JsonRpcRequest;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.methods.JsonRpcMethod;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.parameters.JsonRpcParameter;
+import net.consensys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcError;
+import net.consensys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcErrorResponse;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcResponse;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessResponse;
 
@@ -26,10 +28,12 @@ public class MinerSetCoinbase implements JsonRpcMethod {
 
   @Override
   public JsonRpcResponse response(final JsonRpcRequest req) {
-    final Address coinbase = parameters.required(req.getParams(), 0, Address.class);
-
-    miningCoordinator.setCoinbase(coinbase);
-
-    return new JsonRpcSuccessResponse(req.getId(), true);
+    try {
+      final Address coinbase = parameters.required(req.getParams(), 0, Address.class);
+      miningCoordinator.setCoinbase(coinbase);
+      return new JsonRpcSuccessResponse(req.getId(), true);
+    } catch (UnsupportedOperationException ex) {
+      return new JsonRpcErrorResponse(req.getId(), JsonRpcError.INVALID_REQUEST);
+    }
   }
 }

ethereum/jsonrpc/src/test/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbaseTest.java

@@ -5,7 +5,7 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
-import net.consensys.pantheon.ethereum.blockcreation.EthHashMiningCoordinator;
+import net.consensys.pantheon.ethereum.blockcreation.AbstractMiningCoordinator;
 import net.consensys.pantheon.ethereum.core.Address;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.JsonRpcRequest;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcError;
@@ -24,7 +24,7 @@ import org.mockito.junit.MockitoJUnitRunner;
 @RunWith(MockitoJUnitRunner.class)
 public class EthCoinbaseTest {
 
-  @Mock private EthHashMiningCoordinator miningCoordinator;
+  @Mock private AbstractMiningCoordinator<?, ?> miningCoordinator;
   private EthCoinbase method;
   private final String JSON_RPC_VERSION = "2.0";
   private final String ETH_METHOD = "eth_coinbase";
@@ -65,6 +65,17 @@ public class EthCoinbaseTest {
     assertThat(actualResponse).isEqualToComparingFieldByField(expectedResponse);
   }
 
+  @Test
+  public void shouldReturnAnInvalidRequestIfUnderlyingOperationThrowsUnsupportedOperation() {
+    final JsonRpcRequest request = requestWithParams();
+    final JsonRpcResponse expectedResponse =
+        new JsonRpcErrorResponse(request.getId(), JsonRpcError.INVALID_REQUEST);
+    when(miningCoordinator.getCoinbase()).thenThrow(UnsupportedOperationException.class);
+
+    final JsonRpcResponse actualResponse = method.response(request);
+    assertThat(actualResponse).isEqualToComparingFieldByField(expectedResponse);
+  }
+
   private JsonRpcRequest requestWithParams(final Object... params) {
     return new JsonRpcRequest(JSON_RPC_VERSION, ETH_METHOD, params);
   }

ethereum/jsonrpc/src/test/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/miner/MinerSetCoinbaseTest.java

@@ -2,14 +2,18 @@ package net.consensys.pantheon.ethereum.jsonrpc.internal.methods.miner;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.catchThrowable;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.verify;
 
-import net.consensys.pantheon.ethereum.blockcreation.EthHashMiningCoordinator;
+import net.consensys.pantheon.ethereum.blockcreation.AbstractMiningCoordinator;
 import net.consensys.pantheon.ethereum.core.Address;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.JsonRpcRequest;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.exception.InvalidJsonRpcParameters;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.parameters.JsonRpcParameter;
+import net.consensys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcError;
+import net.consensys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcErrorResponse;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcResponse;
 import net.consensys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessResponse;
 
@@ -24,7 +28,7 @@ public class MinerSetCoinbaseTest {
 
   private MinerSetCoinbase method;
 
-  @Mock private EthHashMiningCoordinator miningCoordinator;
+  @Mock private AbstractMiningCoordinator<?, ?> miningCoordinator;
 
   @Before
   public void before() {
@@ -67,6 +71,23 @@ public class MinerSetCoinbaseTest {
     assertThat(response).isEqualToComparingFieldByField(expectedResponse);
   }
 
+  @Test
+  public void shouldReturnAnInvalidRequestIfUnderlyingOperationThrowsUnsupportedOperation() {
+    final JsonRpcRequest request = minerSetCoinbaseRequest("0x0");
+    final JsonRpcResponse expectedResponse =
+        new JsonRpcErrorResponse(request.getId(), JsonRpcError.INVALID_REQUEST);
+
+    doAnswer(
+            invocation -> {
+              throw new UnsupportedOperationException();
+            })
+        .when(miningCoordinator)
+        .setCoinbase(any());
+
+    final JsonRpcResponse response = method.response(request);
+    assertThat(response).isEqualToComparingFieldByField(expectedResponse);
+  }
+
   private JsonRpcRequest minerSetCoinbaseRequest(final String hexString) {
     if (hexString != null) {
       return new JsonRpcRequest("2.0", "miner_setCoinbase", new Object[] {hexString});