From 2584fed97db46bdfb8d93f0852bc0ee291d98cbf Mon Sep 17 00:00:00 2001 From: tmohay <37158202+rain-on@users.noreply.github.com> Date: Fri, 12 Oct 2018 12:39:22 +1100 Subject: [PATCH] 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 --- .../jsonrpc/internal/methods/EthCoinbase.java | 12 ++++++--- .../methods/miner/MinerSetCoinbase.java | 14 +++++++---- .../internal/methods/EthCoinbaseTest.java | 15 +++++++++-- .../methods/miner/MinerSetCoinbaseTest.java | 25 +++++++++++++++++-- 4 files changed, 53 insertions(+), 13 deletions(-) diff --git a/ethereum/jsonrpc/src/main/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbase.java b/ethereum/jsonrpc/src/main/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbase.java index 09fee18dfe..eb21d2a3f0 100644 --- a/ethereum/jsonrpc/src/main/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbase.java +++ b/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
coinbase = miningCoordinator.getCoinbase(); - if (coinbase.isPresent()) { - return new JsonRpcSuccessResponse(req.getId(), coinbase.get().toString()); + try { + final Optional
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); } } diff --git a/ethereum/jsonrpc/src/main/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/miner/MinerSetCoinbase.java b/ethereum/jsonrpc/src/main/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/miner/MinerSetCoinbase.java index e6bb810513..eb63a13a12 100644 --- a/ethereum/jsonrpc/src/main/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/miner/MinerSetCoinbase.java +++ b/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); + } } } diff --git a/ethereum/jsonrpc/src/test/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbaseTest.java b/ethereum/jsonrpc/src/test/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbaseTest.java index e1a3d20795..7db19534e9 100644 --- a/ethereum/jsonrpc/src/test/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbaseTest.java +++ b/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); } diff --git a/ethereum/jsonrpc/src/test/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/miner/MinerSetCoinbaseTest.java b/ethereum/jsonrpc/src/test/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/miner/MinerSetCoinbaseTest.java index 5172c50eb3..1d25ec6f9b 100644 --- a/ethereum/jsonrpc/src/test/java/net/consensys/pantheon/ethereum/jsonrpc/internal/methods/miner/MinerSetCoinbaseTest.java +++ b/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});