Report node local address as the coinbase in Clique and IBFT. (PegaSy…

…sEng#1758)

Matches behaviour expected of eth_coinbase call by ethstats.net reporter.

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMinerExecutor.java

@@ -32,6 +32,7 @@
 import tech.pegasys.pantheon.util.bytes.BytesValue;
 
 import java.util.List;
+import java.util.Optional;
 import java.util.concurrent.ExecutorService;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -93,6 +94,11 @@ public CliqueBlockMiner startAsyncMining(
     return currentRunningMiner;
   }
 
+  @Override
+  public Optional<Address> getCoinbase() {
+    return Optional.of(localAddress);
+  }
+
   @VisibleForTesting
   BytesValue calculateExtraData(final BlockHeader parentHeader) {
     final List<Address> validators = Lists.newArrayList();

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMiningCoordinator.java

@@ -35,7 +35,7 @@ public CliqueMiningCoordinator(
 
   @Override
   protected boolean newChainHeadInvalidatesMiningOperation(final BlockHeader newChainHeadHeader) {
-    if (!currentRunningMiner.isPresent()) {
+    if (currentRunningMiner.isEmpty()) {
       return true;
     }
 
@@ -56,9 +56,6 @@ private boolean networkBlockBetterThanCurrentMiner(final BlockHeader newChainHea
     final boolean nodeIsMining = miningTracker.canMakeBlockNextRound(parentHeader);
     final boolean nodeIsInTurn = miningTracker.isProposerAfter(parentHeader);
 
-    if (nodeIsMining && nodeIsInTurn) {
-      return false;
-    }
-    return true;
+    return !nodeIsMining || !nodeIsInTurn;
   }
 }

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/blockcreation/IbftBlockCreatorFactory.java

@@ -107,6 +107,10 @@ public BytesValue createExtraData(final int round, final BlockHeader parentHeade
     return extraData.encode();
   }
 
+  public Address getLocalAddress() {
+    return localAddress;
+  }
+
   private static Optional<Vote> toVote(final Optional<ValidatorVote> input) {
     return input
         .map(v -> Optional.of(new Vote(v.getRecipient(), v.getVotePolarity())))

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/blockcreation/IbftMiningCoordinator.java

@@ -21,9 +21,12 @@
 import tech.pegasys.pantheon.ethereum.chain.BlockAddedEvent;
 import tech.pegasys.pantheon.ethereum.chain.BlockAddedObserver;
 import tech.pegasys.pantheon.ethereum.chain.Blockchain;
+import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.Wei;
 import tech.pegasys.pantheon.util.bytes.BytesValue;
 
+import java.util.Optional;
+
 import org.apache.logging.log4j.Logger;
 
 public class IbftMiningCoordinator implements MiningCoordinator, BlockAddedObserver {
@@ -70,6 +73,11 @@ public void setExtraData(final BytesValue extraData) {
     blockCreatorFactory.setExtraData(extraData);
   }
 
+  @Override
+  public Optional<Address> getCoinbase() {
+    return Optional.of(blockCreatorFactory.getLocalAddress());
+  }
+
   @Override
   public void onBlockAdded(final BlockAddedEvent event, final Blockchain blockchain) {
     if (event.isNewCanonicalHead()) {

ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/AbstractMinerExecutor.java

@@ -14,6 +14,7 @@
 
 import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.chain.MinedBlockObserver;
+import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.core.MiningParameters;
 import tech.pegasys.pantheon.ethereum.core.Wei;
@@ -22,6 +23,7 @@
 import tech.pegasys.pantheon.util.Subscribers;
 import tech.pegasys.pantheon.util.bytes.BytesValue;
 
+import java.util.Optional;
 import java.util.concurrent.ExecutorService;
 
 public abstract class AbstractMinerExecutor<
@@ -66,4 +68,6 @@ public void setMinTransactionGasPrice(final Wei minTransactionGasPrice) {
   public Wei getMinTransactionGasPrice() {
     return minTransactionGasPrice;
   }
+
+  public abstract Optional<Address> getCoinbase();
 }

ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/AbstractMiningCoordinator.java

@@ -18,6 +18,7 @@
 import tech.pegasys.pantheon.ethereum.chain.BlockAddedObserver;
 import tech.pegasys.pantheon.ethereum.chain.Blockchain;
 import tech.pegasys.pantheon.ethereum.chain.MinedBlockObserver;
+import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.core.Wei;
 import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState;
@@ -135,6 +136,11 @@ public void setExtraData(final BytesValue extraData) {
     executor.setExtraData(extraData);
   }
 
+  @Override
+  public Optional<Address> getCoinbase() {
+    return executor.getCoinbase();
+  }
+
   protected abstract boolean newChainHeadInvalidatesMiningOperation(
       final BlockHeader newChainHeadHeader);
 }

ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/EthHashMinerExecutor.java

@@ -88,6 +88,7 @@ public void setCoinbase(final Address coinbase) {
     }
   }
 
+  @Override
   public Optional<Address> getCoinbase() {
     return coinbase;
   }

ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/EthHashMiningCoordinator.java

@@ -43,11 +43,6 @@ public void setCoinbase(final Address coinbase) {
     executor.setCoinbase(coinbase);
   }
 
-  @Override
-  public Optional<Address> getCoinbase() {
-    return executor.getCoinbase();
-  }
-
   @Override
   public Optional<Long> hashesPerSecond() {
     final Optional<Long> currentHashesPerSecond =

ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/MiningCoordinator.java

@@ -37,10 +37,7 @@ default void setCoinbase(final Address coinbase) {
         "Current consensus mechanism prevents setting coinbase.");
   }
 
-  default Optional<Address> getCoinbase() {
-    throw new UnsupportedOperationException(
-        "Current consensus mechanism prevents querying of coinbase.");
-  }
+  Optional<Address> getCoinbase();
 
   default Optional<Long> hashesPerSecond() {
     return Optional.empty();

ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbase.java

@@ -38,14 +38,10 @@ public String getName() {
 
   @Override
   public JsonRpcResponse response(final JsonRpcRequest req) {
-    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 (final UnsupportedOperationException ex) {
-      return new JsonRpcErrorResponse(req.getId(), JsonRpcError.INVALID_REQUEST);
+    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);
   }
 }

ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/EthCoinbaseTest.java

@@ -77,17 +77,6 @@ public void shouldReturnErrorWhenCoinbaseNotSpecified() {
     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);
   }