From b802fa75ce24e3933d6f5f269e9375a0ef3fcdfc Mon Sep 17 00:00:00 2001 From: Karim T Date: Thu, 30 Jan 2020 09:10:00 +0100 Subject: [PATCH] [BESU-179] getSignerMetrics doesn't return missing signers (#343) * fix signer metrics add missing validator * fix end block parameter issue Signed-off-by: Karim TAAM --- .../controller/IbftBesuControllerBuilder.java | 1 + .../IbftLegacyBesuControllerBuilder.java | 2 +- .../clique/jsonrpc/CliqueJsonRpcMethods.java | 8 +++---- .../methods/CliqueGetSignerMetrics.java | 7 ++++-- .../methods/CliqueGetSignerMetricsTest.java | 22 +++++++++++++++++-- .../AbstractGetSignerMetricsMethod.java | 19 +++++++++++----- .../ibft/support/TestContextBuilder.java | 2 +- .../besu/consensus/ibft/IbftContext.java | 8 +++++++ .../ibft/jsonrpc/IbftJsonRpcMethods.java | 18 ++++++++++++++- .../jsonrpc/methods/IbftGetSignerMetrics.java | 7 ++++-- .../methods/IbftGetSignerMetricsTest.java | 19 +++++++++++++++- 11 files changed, 93 insertions(+), 20 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/controller/IbftBesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/IbftBesuControllerBuilder.java index 71b74d8364..3425cc655b 100644 --- a/besu/src/main/java/org/hyperledger/besu/controller/IbftBesuControllerBuilder.java +++ b/besu/src/main/java/org/hyperledger/besu/controller/IbftBesuControllerBuilder.java @@ -243,6 +243,7 @@ public class IbftBesuControllerBuilder extends BesuControllerBuilder context, final MutableBlockchain blockchain) { final EpochManager epochManager = context.getConsensusState().getEpochManager(); + final CliqueBlockInterface cliqueBlockInterface = new CliqueBlockInterface(); final VoteTallyUpdater voteTallyUpdater = - new VoteTallyUpdater(epochManager, new CliqueBlockInterface()); - return new VoteTallyCache( - blockchain, voteTallyUpdater, epochManager, new CliqueBlockInterface()); + new VoteTallyUpdater(epochManager, cliqueBlockInterface); + return new VoteTallyCache(blockchain, voteTallyUpdater, epochManager, cliqueBlockInterface); } } diff --git a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/jsonrpc/methods/CliqueGetSignerMetrics.java b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/jsonrpc/methods/CliqueGetSignerMetrics.java index bbbdeb39b8..2cc51ba002 100644 --- a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/jsonrpc/methods/CliqueGetSignerMetrics.java +++ b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/jsonrpc/methods/CliqueGetSignerMetrics.java @@ -15,6 +15,7 @@ package org.hyperledger.besu.consensus.clique.jsonrpc.methods; import org.hyperledger.besu.consensus.common.BlockInterface; +import org.hyperledger.besu.consensus.common.VoteTallyCache; import org.hyperledger.besu.consensus.common.jsonrpc.AbstractGetSignerMetricsMethod; import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.JsonRpcMethod; @@ -24,8 +25,10 @@ public class CliqueGetSignerMetrics extends AbstractGetSignerMetricsMethod implements JsonRpcMethod { public CliqueGetSignerMetrics( - final BlockInterface blockInterface, final BlockchainQueries blockchainQueries) { - super(blockInterface, blockchainQueries); + final VoteTallyCache voteTallyCache, + final BlockInterface blockInterface, + final BlockchainQueries blockchainQueries) { + super(voteTallyCache, blockInterface, blockchainQueries); } @Override diff --git a/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/jsonrpc/methods/CliqueGetSignerMetricsTest.java b/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/jsonrpc/methods/CliqueGetSignerMetricsTest.java index 17113667e2..8b68508131 100644 --- a/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/jsonrpc/methods/CliqueGetSignerMetricsTest.java +++ b/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/jsonrpc/methods/CliqueGetSignerMetricsTest.java @@ -22,6 +22,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import org.hyperledger.besu.consensus.common.BlockInterface; +import org.hyperledger.besu.consensus.common.VoteTally; +import org.hyperledger.besu.consensus.common.VoteTallyCache; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.exception.InvalidJsonRpcParameters; @@ -33,6 +35,7 @@ import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.Optional; @@ -46,13 +49,14 @@ import org.junit.rules.ExpectedException; public class CliqueGetSignerMetricsTest { private static final Address[] VALIDATORS = { - Address.fromHexString("0x1"), Address.fromHexString("0x2"), Address.fromHexString("0x3"), + Address.fromHexString("0x1"), Address.fromHexString("0x2"), Address.fromHexString("0x3") }; private final String CLIQUE_METHOD = "clique_getSignerMetrics"; private final String JSON_RPC_VERSION = "2.0"; private CliqueGetSignerMetrics method; + private VoteTallyCache voteTallyCache; private BlockchainQueries blockchainQueries; private BlockInterface blockInterface; @@ -60,9 +64,10 @@ public class CliqueGetSignerMetricsTest { @Before public void setup() { + voteTallyCache = mock(VoteTallyCache.class); blockchainQueries = mock(BlockchainQueries.class); blockInterface = mock(BlockInterface.class); - method = new CliqueGetSignerMetrics(blockInterface, blockchainQueries); + method = new CliqueGetSignerMetrics(voteTallyCache, blockInterface, blockchainQueries); } @Test @@ -104,6 +109,8 @@ public class CliqueGetSignerMetricsTest { LongStream.range(startBlock, endBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); + signerMetricResultList.add(new SignerMetricResult(VALIDATORS[0])); // missing validator + final JsonRpcRequestContext request = requestWithParams(); final JsonRpcSuccessResponse response = (JsonRpcSuccessResponse) method.response(request); @@ -158,6 +165,8 @@ public class CliqueGetSignerMetricsTest { LongStream.range(startBlock, headBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); + signerMetricResultList.add(new SignerMetricResult(VALIDATORS[2])); // missing validator + final JsonRpcRequestContext request = requestWithParams(); final JsonRpcSuccessResponse response = (JsonRpcSuccessResponse) method.response(request); @@ -183,6 +192,8 @@ public class CliqueGetSignerMetricsTest { LongStream.range(startBlock, endBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); + signerMetricResultList.add(new SignerMetricResult(VALIDATORS[0])); // missing validator + final JsonRpcRequestContext request = requestWithParams(String.valueOf(startBlock), "latest"); final JsonRpcSuccessResponse response = (JsonRpcSuccessResponse) method.response(request); @@ -205,6 +216,8 @@ public class CliqueGetSignerMetricsTest { LongStream.range(startBlock, endBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); + signerMetricResultList.add(new SignerMetricResult(VALIDATORS[0])); // missing validator + final JsonRpcRequestContext request = requestWithParams(String.valueOf(startBlock), "pending"); final JsonRpcSuccessResponse response = (JsonRpcSuccessResponse) method.response(request); @@ -222,6 +235,8 @@ public class CliqueGetSignerMetricsTest { final List signerMetricResultList = new ArrayList<>(); + when(blockchainQueries.headBlockNumber()).thenReturn(endBlock); + LongStream.range(startBlock, endBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); @@ -246,6 +261,9 @@ public class CliqueGetSignerMetricsTest { when(blockchainQueries.getBlockHeaderByNumber(number)).thenReturn(Optional.of(header)); when(blockInterface.getProposerOfBlock(header)).thenReturn(proposerAddressBlock); + when(voteTallyCache.getVoteTallyAfterBlock(header)) + .thenReturn(new VoteTally(Arrays.asList(VALIDATORS))); + final SignerMetricResult signerMetricResult = new SignerMetricResult(proposerAddressBlock); signerMetricResult.incrementeNbBlock(); signerMetricResult.setLastProposedBlockNumber(number); diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java index ecf143dcee..5861e70fe5 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java @@ -15,6 +15,7 @@ package org.hyperledger.besu.consensus.common.jsonrpc; import org.hyperledger.besu.consensus.common.BlockInterface; +import org.hyperledger.besu.consensus.common.VoteTallyCache; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.BlockParameter; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError; @@ -36,11 +37,15 @@ public abstract class AbstractGetSignerMetricsMethod { private static final long DEFAULT_RANGE_BLOCK = 100; + private final VoteTallyCache voteTallyCache; private final BlockInterface blockInterface; private final BlockchainQueries blockchainQueries; public AbstractGetSignerMetricsMethod( - final BlockInterface blockInterface, final BlockchainQueries blockchainQueries) { + final VoteTallyCache voteTallyCache, + final BlockInterface blockInterface, + final BlockchainQueries blockchainQueries) { + this.voteTallyCache = voteTallyCache; this.blockInterface = blockInterface; this.blockchainQueries = blockchainQueries; } @@ -83,12 +88,12 @@ public abstract class AbstractGetSignerMetricsMethod { // Get All validators present in the last block of the range even // if they didn't propose a block if (currentIndex == lastBlockIndex) { - blockInterface - .validatorsInBlock(header) + voteTallyCache + .getVoteTallyAfterBlock(header) + .getValidators() .forEach( address -> - proposersMap.computeIfAbsent( - proposerAddress, SignerMetricResult::new)); + proposersMap.computeIfAbsent(address, SignerMetricResult::new)); } }); }); @@ -104,9 +109,11 @@ public abstract class AbstractGetSignerMetricsMethod { } private long getEndBlockNumber(final Optional endBlockParameter) { + final long headBlockNumber = blockchainQueries.headBlockNumber(); return endBlockParameter .map(this::resolveBlockNumber) - .orElseGet(blockchainQueries::headBlockNumber); + .filter(blockNumber -> blockNumber <= headBlockNumber) + .orElse(headBlockNumber); } private boolean isValidParameters(final long startBlock, final long endBlock) { diff --git a/consensus/ibft/src/integration-test/java/org/hyperledger/besu/consensus/ibft/support/TestContextBuilder.java b/consensus/ibft/src/integration-test/java/org/hyperledger/besu/consensus/ibft/support/TestContextBuilder.java index 42d322d6bf..39c8921447 100644 --- a/consensus/ibft/src/integration-test/java/org/hyperledger/besu/consensus/ibft/support/TestContextBuilder.java +++ b/consensus/ibft/src/integration-test/java/org/hyperledger/besu/consensus/ibft/support/TestContextBuilder.java @@ -296,7 +296,7 @@ public class TestContextBuilder { new ProtocolContext<>( blockChain, worldStateArchive, - new IbftContext(voteTallyCache, voteProposer, blockInterface)); + new IbftContext(voteTallyCache, voteProposer, epochManager, blockInterface)); final PendingTransactions pendingTransactions = new PendingTransactions( diff --git a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/IbftContext.java b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/IbftContext.java index 2a8247f4dd..879d5b286e 100644 --- a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/IbftContext.java +++ b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/IbftContext.java @@ -15,6 +15,7 @@ package org.hyperledger.besu.consensus.ibft; import org.hyperledger.besu.consensus.common.BlockInterface; +import org.hyperledger.besu.consensus.common.EpochManager; import org.hyperledger.besu.consensus.common.PoaContext; import org.hyperledger.besu.consensus.common.VoteProposer; import org.hyperledger.besu.consensus.common.VoteTallyCache; @@ -24,14 +25,17 @@ public class IbftContext implements PoaContext { private final VoteTallyCache voteTallyCache; private final VoteProposer voteProposer; + private final EpochManager epochManager; private final BlockInterface blockInterface; public IbftContext( final VoteTallyCache voteTallyCache, final VoteProposer voteProposer, + final EpochManager epochManager, final BlockInterface blockInterface) { this.voteTallyCache = voteTallyCache; this.voteProposer = voteProposer; + this.epochManager = epochManager; this.blockInterface = blockInterface; } @@ -43,6 +47,10 @@ public class IbftContext implements PoaContext { return voteProposer; } + public EpochManager getEpochManager() { + return epochManager; + } + @Override public BlockInterface getBlockInterface() { return blockInterface; diff --git a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/jsonrpc/IbftJsonRpcMethods.java b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/jsonrpc/IbftJsonRpcMethods.java index b89097c082..c7a361b921 100644 --- a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/jsonrpc/IbftJsonRpcMethods.java +++ b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/jsonrpc/IbftJsonRpcMethods.java @@ -15,7 +15,10 @@ package org.hyperledger.besu.consensus.ibft.jsonrpc; import org.hyperledger.besu.consensus.common.BlockInterface; +import org.hyperledger.besu.consensus.common.EpochManager; import org.hyperledger.besu.consensus.common.VoteProposer; +import org.hyperledger.besu.consensus.common.VoteTallyCache; +import org.hyperledger.besu.consensus.common.VoteTallyUpdater; import org.hyperledger.besu.consensus.ibft.IbftBlockInterface; import org.hyperledger.besu.consensus.ibft.IbftContext; import org.hyperledger.besu.consensus.ibft.jsonrpc.methods.IbftDiscardValidatorVote; @@ -29,6 +32,7 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.RpcApi; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.JsonRpcMethod; import org.hyperledger.besu.ethereum.api.jsonrpc.methods.ApiGroupJsonRpcMethods; import org.hyperledger.besu.ethereum.api.query.BlockchainQueries; +import org.hyperledger.besu.ethereum.chain.MutableBlockchain; import java.util.Map; @@ -47,17 +51,29 @@ public class IbftJsonRpcMethods extends ApiGroupJsonRpcMethods { @Override protected Map create() { + final MutableBlockchain mutableBlockchain = context.getBlockchain(); final BlockchainQueries blockchainQueries = new BlockchainQueries(context.getBlockchain(), context.getWorldStateArchive()); final VoteProposer voteProposer = context.getConsensusState().getVoteProposer(); final BlockInterface blockInterface = new IbftBlockInterface(); + final VoteTallyCache voteTallyCache = createVoteTallyCache(context, mutableBlockchain); + return mapOf( new IbftProposeValidatorVote(voteProposer), new IbftGetValidatorsByBlockNumber(blockchainQueries, blockInterface), new IbftDiscardValidatorVote(voteProposer), new IbftGetValidatorsByBlockHash(context.getBlockchain(), blockInterface), - new IbftGetSignerMetrics(blockInterface, blockchainQueries), + new IbftGetSignerMetrics(voteTallyCache, blockInterface, blockchainQueries), new IbftGetPendingVotes(voteProposer)); } + + private VoteTallyCache createVoteTallyCache( + final ProtocolContext context, final MutableBlockchain blockchain) { + final EpochManager epochManager = context.getConsensusState().getEpochManager(); + final IbftBlockInterface ibftBlockInterface = new IbftBlockInterface(); + final VoteTallyUpdater voteTallyUpdater = + new VoteTallyUpdater(epochManager, ibftBlockInterface); + return new VoteTallyCache(blockchain, voteTallyUpdater, epochManager, ibftBlockInterface); + } } diff --git a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/jsonrpc/methods/IbftGetSignerMetrics.java b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/jsonrpc/methods/IbftGetSignerMetrics.java index 022e1dd57e..19d4273706 100644 --- a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/jsonrpc/methods/IbftGetSignerMetrics.java +++ b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/jsonrpc/methods/IbftGetSignerMetrics.java @@ -15,6 +15,7 @@ package org.hyperledger.besu.consensus.ibft.jsonrpc.methods; import org.hyperledger.besu.consensus.common.BlockInterface; +import org.hyperledger.besu.consensus.common.VoteTallyCache; import org.hyperledger.besu.consensus.common.jsonrpc.AbstractGetSignerMetricsMethod; import org.hyperledger.besu.ethereum.api.jsonrpc.RpcMethod; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.JsonRpcMethod; @@ -23,8 +24,10 @@ import org.hyperledger.besu.ethereum.api.query.BlockchainQueries; public class IbftGetSignerMetrics extends AbstractGetSignerMetricsMethod implements JsonRpcMethod { public IbftGetSignerMetrics( - final BlockInterface blockInterface, final BlockchainQueries blockchainQueries) { - super(blockInterface, blockchainQueries); + final VoteTallyCache voteTallyCache, + final BlockInterface blockInterface, + final BlockchainQueries blockchainQueries) { + super(voteTallyCache, blockInterface, blockchainQueries); } @Override diff --git a/consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/jsonrpc/methods/IbftGetSignerMetricsTest.java b/consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/jsonrpc/methods/IbftGetSignerMetricsTest.java index 451b1bcda9..cff2ae6099 100644 --- a/consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/jsonrpc/methods/IbftGetSignerMetricsTest.java +++ b/consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/jsonrpc/methods/IbftGetSignerMetricsTest.java @@ -22,6 +22,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import org.hyperledger.besu.consensus.common.BlockInterface; +import org.hyperledger.besu.consensus.common.VoteTally; +import org.hyperledger.besu.consensus.common.VoteTallyCache; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.exception.InvalidJsonRpcParameters; @@ -33,6 +35,7 @@ import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.Optional; @@ -53,6 +56,7 @@ public class IbftGetSignerMetricsTest { private final String JSON_RPC_VERSION = "2.0"; private IbftGetSignerMetrics method; + private VoteTallyCache voteTallyCache; private BlockchainQueries blockchainQueries; private BlockInterface blockInterface; @@ -60,9 +64,10 @@ public class IbftGetSignerMetricsTest { @Before public void setup() { + voteTallyCache = mock(VoteTallyCache.class); blockchainQueries = mock(BlockchainQueries.class); blockInterface = mock(BlockInterface.class); - method = new IbftGetSignerMetrics(blockInterface, blockchainQueries); + method = new IbftGetSignerMetrics(voteTallyCache, blockInterface, blockchainQueries); } @Test @@ -104,6 +109,8 @@ public class IbftGetSignerMetricsTest { LongStream.range(startBlock, endBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); + signerMetricResultList.add(new SignerMetricResult(VALIDATORS[0])); // missing validator + final JsonRpcRequestContext request = requestWithParams(); final JsonRpcSuccessResponse response = (JsonRpcSuccessResponse) method.response(request); @@ -158,6 +165,8 @@ public class IbftGetSignerMetricsTest { LongStream.range(startBlock, headBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); + signerMetricResultList.add(new SignerMetricResult(VALIDATORS[2])); // missing validator + final JsonRpcRequestContext request = requestWithParams(); final JsonRpcSuccessResponse response = (JsonRpcSuccessResponse) method.response(request); @@ -183,6 +192,8 @@ public class IbftGetSignerMetricsTest { LongStream.range(startBlock, endBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); + signerMetricResultList.add(new SignerMetricResult(VALIDATORS[0])); // missing validator + final JsonRpcRequestContext request = requestWithParams(String.valueOf(startBlock), "latest"); final JsonRpcSuccessResponse response = (JsonRpcSuccessResponse) method.response(request); @@ -205,6 +216,8 @@ public class IbftGetSignerMetricsTest { LongStream.range(startBlock, endBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); + signerMetricResultList.add(new SignerMetricResult(VALIDATORS[0])); // missing validator + final JsonRpcRequestContext request = requestWithParams(String.valueOf(startBlock), "pending"); final JsonRpcSuccessResponse response = (JsonRpcSuccessResponse) method.response(request); @@ -222,6 +235,8 @@ public class IbftGetSignerMetricsTest { final List signerMetricResultList = new ArrayList<>(); + when(blockchainQueries.headBlockNumber()).thenReturn(endBlock); + LongStream.range(startBlock, endBlock) .forEach(value -> signerMetricResultList.add(generateBlock(value))); @@ -244,6 +259,8 @@ public class IbftGetSignerMetricsTest { when(blockchainQueries.getBlockHeaderByNumber(number)).thenReturn(Optional.of(header)); when(blockInterface.getProposerOfBlock(header)).thenReturn(proposerAddressBlock); + when(voteTallyCache.getVoteTallyAfterBlock(header)) + .thenReturn(new VoteTally(Arrays.asList(VALIDATORS))); final SignerMetricResult signerMetricResult = new SignerMetricResult(proposerAddressBlock); signerMetricResult.incrementeNbBlock();