[PAN-2763] Add JSON-RPC API to report validator block production information #1687

matkt · 2019-07-13T14:16:23Z

PR description

Add JSON-RPC API to report validator block production information (CLIQUE and IBFT2)

The number of blocks from each proposer in a given block range.
All validators present in the last block of the range should be listed are included even if they didn’t propose a block
The block number of the last block proposed by each validator (if any within the given range).

merge from original

Pull from Main Repository

Merge from official github

Merge from master

merge from official repository

matkt · 2019-07-13T14:23:19Z

Ouput

{
    "jsonrpc": "2.0",
    "id": 1,
    "result": [
        {
            "address": "0x7ffc57839b00206d1ad20c69a1981b489f772031",
            "proposedBlockCount": "0x21",
            "lastProposedBlockNumber": "0x61"
        },
        {
            "address": "0x42eb768f2244c8811c63729a21a3569731535f06",
            "proposedBlockCount": "0x21",
            "lastProposedBlockNumber": "0x63"
        },
        {
            "address": "0xb279182d99e65703f0076e4812653aab85fca0f0",
            "proposedBlockCount": "0x21",
            "lastProposedBlockNumber": "0x62"
        }
    ]
}

Sample input

{"jsonrpc":"2.0","method":"clique_getSignerMetrics","params":["1"], "id":1}

{"jsonrpc":"2.0","method":"clique_getSignerMetrics","params":["1","1000"], "id":1}

{"jsonrpc":"2.0","method":"clique_getSignerMetrics","params":[], "id":1}

{"jsonrpc":"2.0","method":"clique_getSignerMetrics","params":["1000", "pending"], "id":1}

{"jsonrpc":"2.0","method":"clique_getSignerMetrics","params":["1000", "latest"], "id":1}

ajsutton

Thanks for jumping into this - it's a really valuable contribution to add. The JIRA ticket isn't particularly polished or well thought out though. It was added as a bit of a place holder so apologies for where it's unclear or if we wind up having to change requirements a bit. Very pleased to have you working on it and I'll do my best to work with you to clarify anything and get this in.

You probably know a lot of what I cover below, but I'll include it all so hopefully we wind up with the same understanding.

In the JIRA ticket, the terms proposer and validator are being used a bit loosely. When using PoA like Clique or IBFT, for any given block there is a set of validators represented by an Address. That set of validators can be retrieved from the VoteTallyCache with voteTallyCache.getVoteTallyAfterBlock(header).getValidators().

For every block there is also a single proposer which is the validator that actually selected the list of transactions to include and created the proposed block. For Clique the proposer does this alone, for IBFT it then goes through a further process to get enough other validators to also sign the block but in both cases there's still only the one proposer.

What we ultimately want to get out of this JSON-RPC method is a report showing which validators are proposing blocks so we can identify validators that have gone offline and either fix them or vote them out. So very roughly something like:

"proposers": [
  { "address":  "0x7ffc57839b00206d1ad20c69a1981b489f772031", "proposedBlockCount": 315, "lastProposedBlockNumber": 123456 },
  { "address": "0x42eb768f2244c8811c63729a21a3569731535f06", "proposedBlockCount": 314, "lastProposedBlockNumber": 123455 },
  { "address": "0xb279182d99e65703f0076e4812653aab85fca0f0", "proposedBlockCount": 0 }
}

The part about including all validators in the last block being included in the output is so that we get that last entry for 0xb279182d99e65703f0076e4812653aab85fca0f0. They didn't propose a block at all in the period we looked at so would otherwise be omitted but get explicitly included because they're still in the list of validators at the end of the range.

ajsutton · 2019-07-15T00:34:56Z

...main/java/tech/pegasys/pantheon/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java

+    return startBlock < endBlock;
+  }
+
+  protected abstract List<Address> getProposersOfBlock(final BlockHeader header);


There is only ever one proposer of a block - it's the validator that selects the transactions and creates the proposed block, even if multiple validators are later involved in signing off on that block. And you can use the tech.pegasys.pantheon.consensus.common.BlockInterface.getProposerOfBlock method to get that validator without needing to worry about which consensus algorithm is currently in use. You can create the appropriate implementation (either CliqueBlockInterface or IbftBlockInterface) in the (Clique/Ibft)JsonRpcMethodsFactory.

Hopefully that simplifies things a fair bit so the changes should just be isolated to the actual JSON-RPC classes.

ajsutton · 2019-07-15T01:03:08Z

...main/java/tech/pegasys/pantheon/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java

+
+      final Map<Address, ProposerReportBlockProductionResult> proposersMap = new HashMap<>();
+      final Map<Address, ValidatorReportBlockProductionResult> validatorInLastBlockMap =
+          new HashMap<>();


Hopefully the clarifications in my overall comment help to make sense of this but we'd only need the one map here. The basic approach would be:

Iterate through each block in the range and increment the count of number of proposed blocks for the proposer of that block. Note you might still get some proposers who don't yet exist in the map if they've been voted out before the end of the range.

For each validator in the last block of the range not already in proposerMap, add it with 0 proposed blocks. That will ensure they're all present in the report even if none ever propose blocks.

...main/java/tech/pegasys/pantheon/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java

ajsutton · 2019-07-15T01:09:19Z

...main/java/tech/pegasys/pantheon/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java

+                                } else {
+                                  proposersMap.put(
+                                      proposerAddress,
+                                      new ProposerReportBlockProductionResult(proposerAddress));


The Map.computeIfAbsent is really helpful here. If ProposerReportBlockProductionResult defaulted to 0 blocks proposed instead of 1, then you could just use proposersMap.computeIfAbsent(proposerAddress, ProposerReportBlockProductionResult::new).incrementeNbBlock();

ajsutton · 2019-07-15T01:11:46Z

...um/jsonrpc/internal/results/report/block/production/ProposerReportBlockProductionResult.java

+public class ProposerReportBlockProductionResult {
+
+  public final String address;
+  public long nbBlockProposed;


We generally try to avoid abbreviations so something like proposedBlockCount would be a better name.

ajsutton · 2019-07-15T01:24:11Z

...main/java/tech/pegasys/pantheon/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java

+  public JsonRpcResponse response(final JsonRpcRequest request) {
+
+    final long startBlock = parameters.required(request.getParams(), 0, Long.class);
+    final long endBlock = parameters.required(request.getParams(), 1, Long.class);


We should support the usual values for block parameters like latest. parameters().required(request.getParams(), 0, BlockParameter.class); will handle the parsing of that. pending and latest would mean the same thing (current chain head) in this context.

I'd probably make both arguments optional as well. Start would default to 100 blocks before current chain head and end defaults to chain head. That way you can call with no arguments and get a useful result (most of the time you want to know which of the current validators hasn't proposed recently), or specify a starting block to get from their to chain head easily, or both start and end for a custom range.

ajsutton

Looking really good. Just a couple of tweaks and we're there I think. Thanks so much for your work on this.

ajsutton · 2019-07-15T22:44:27Z

...main/java/tech/pegasys/pantheon/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java

+    final long fromBlockNumber = getFromBlockNumber(startBlockParameter);
+    final long toBlockNumber = getEndBlockNumber(endBlockParameter);
+
+    if (isValidParameters(fromBlockNumber, toBlockNumber)) {


We typically exit early in cases like this, so

if (!isValidParameters(fromBlockNumber, toBlockNubmer)) { return new JsonRpcErrorResponse(request.getId(), JsonRpcError.INVALID_PARAMS); }

...main/java/tech/pegasys/pantheon/consensus/common/jsonrpc/AbstractGetSignerMetricsMethod.java

…pan-2763-validator-block-production

… report quantities as hex in the JSON output.

ajsutton · 2019-07-17T00:45:25Z

I realised we should be reporting the values in the JSON response as hex (to match all the other quantity values). This is unfortunately a lot less readable but just how JSON-RPC works.

ajsutton@0bd0892 applies the couple of suggestions I made above, plus the change to use hex. It's on the branch https://github.com/ajsutton/pantheon/commits/validator-metrics if it's easier to just pull that in.

Exit early, handle block parameter aliases (earliest, latest etc) and report quantities as hex in the JSON output.

matkt · 2019-07-17T18:41:22Z

I realised we should be reporting the values in the JSON response as hex (to match all the other quantity values). This is unfortunately a lot less readable but just how JSON-RPC works.

ajsutton@0bd0892 applies the couple of suggestions I made above, plus the change to use hex. It's on the branch https://github.com/ajsutton/pantheon/commits/validator-metrics if it's easier to just pull that in.

Ok thanks you very much for your modification. I will merge your branch

ajsutton · 2019-07-18T00:30:08Z

.../java/tech/pegasys/pantheon/consensus/clique/jsonrpc/methods/CliqueGetSignerMetricsTest.java

+
+    when(blockchainQueries.headBlockNumber()).thenReturn(endBlock);
+
+    List<SignerMetricResult> signerMetricResultList = new java.util.ArrayList<>();


nit: Shouldn't don't need the full package name here

Suggested change

List<SignerMetricResult> signerMetricResultList = new java.util.ArrayList<>();

List<SignerMetricResult> signerMetricResultList = new ArrayList<>();

Also for the other new java.util.ArrayList() calls below.

yes an error that I had not seen. I correct that

ajsutton · 2019-07-18T00:41:16Z

...test/java/tech/pegasys/pantheon/consensus/ibft/jsonrpc/methods/IbftGetSignerMetricsTest.java

+  public void getSignerMetricsWhenNoParams() {
+
+    Long startBlock = 1L;
+    Long endBlock = 3L;


nit: These should just be primitives and we add the final keyword whenever possible in Pantheon (well try to - we're not as consistent as we should be). So:

Suggested change

Long endBlock = 3L;

final long endBlock = 3L;

ajsutton · 2019-07-18T00:48:36Z

...test/java/tech/pegasys/pantheon/consensus/ibft/jsonrpc/methods/IbftGetSignerMetricsTest.java

+    LongStream.range(startBlock, endBlock)
+        .forEach(value -> verify(blockchainQueries).getBlockHeaderByNumber(value));
+
+    verify(blockchainQueries, atLeast(2)).headBlockNumber();


I don't think there's any reason to assert these class to blockchainQueries - they're just implementation details that it needs to get the information. We just check it comes up with the right answer.

ajsutton · 2019-07-18T00:50:43Z

...test/java/tech/pegasys/pantheon/consensus/ibft/jsonrpc/methods/IbftGetSignerMetricsTest.java

+
+    signerMetricResultList.forEach(
+        signerMetricResult ->
+            assertThat(response.getResult().toString()).contains(signerMetricResult.toString()));


These assertions should actually be stronger - we expect that the result only contains the expected signers and the order isn't important (because the list of stats is built off of a HashMap the order isn't guaranteed). So:

assertThat((Collection<SignerMetricResult>) response.getResult()) .containsExactlyInAnyOrderElementsOf(signerMetricResultList);

I wanted to try this proposal but I have an error during the pipeline "unchecked cast assertThat ((Collection ) response.getResult ()) "that's why I used another way

I add the @SuppressWarnings("unchecked") annotation

ajsutton · 2019-07-18T00:52:58Z

...test/java/tech/pegasys/pantheon/consensus/ibft/jsonrpc/methods/IbftGetSignerMetricsTest.java

+
+  private SignerMetricResult generateBlock(final long number) {
+    final SECP256K1.KeyPair proposerKeyPair = SECP256K1.KeyPair.generate();
+    final Address proposerAddressBlock = Util.publicKeyToAddress(proposerKeyPair.getPublicKey());


Creating keypairs is fairly expensive and we really want to test that the number of blocks gets incremented so it's probably worth creating a few key pairs as static fields which are just iterated through here (e.g. final SECP256K1.KeyPair proposerKeyPair = KEYPAIRS[number % KEYPAIRS.length])

…on two different blocks)

merge from master

…pan-2763-validator-block-production

…kInterface implementation anyway.

ajsutton

LGTM. Thanks for picking this up.

I realised during the final review that because we're using a mock for the BlockInterface we can simplify the tests to use Address directly and not need to create real private keys or properly signed blocks. So I've simplified that and applied a few final keywords.

All good to go.

Karim T and others added 8 commits July 2, 2019 22:05

Merge pull request #1 from PegaSysEng/master

7f2555e

merge from original

Merge pull request #2 from PegaSysEng/master

00ed0c1

Pull from Main Repository

Merge pull request #4 from PegaSysEng/master

2ba0e3e

Merge from official github

Merge pull request #5 from PegaSysEng/master

14ffa77

Merge from master

Merge pull request #6 from PegaSysEng/master

943d3ef

merge from official repository

implement clique get report validator block production

49aaaf6

move and modify get report block production response

93509c4

spotlessApply command

23e4ebd

matkt added 3 commits July 13, 2019 21:10

rename method to getSignerMetrics, add ibft implementation

eb6f62c

resolve IbftExtra test

4f8fb27

add test for IbftExtraDataTest class

aa2a2aa

ajsutton self-assigned this Jul 15, 2019

ajsutton reviewed Jul 15, 2019

View reviewed changes

resolve issues raised by the review.

72fd26f

ajsutton reviewed Jul 15, 2019

View reviewed changes

ajsutton added 2 commits July 17, 2019 10:18

Merge branch 'master' of github.com:PegaSysEng/pantheon into feature/…

831a19a

…pan-2763-validator-block-production

Exit early, handle block parameter aliases (earliest, latest etc) and…

0bd0892

… report quantities as hex in the JSON output.

Merge pull request #8 from ajsutton/validator-metrics

bcc6875

Exit early, handle block parameter aliases (earliest, latest etc) and report quantities as hex in the JSON output.

matkt added 5 commits July 17, 2019 22:55

add test for ibft and clique signer metrics

5abc727

resolve test issue

08ffed6

resolve test issue

87db0c5

add missing import for test

f804c88

add toString for SignerMetricsResult, update test

12f9c1c

matkt marked this pull request as ready for review July 17, 2019 22:19

ajsutton reviewed Jul 18, 2019

View reviewed changes

update test after review, improve tests (case of an approver present …

1c52274

…on two different blocks)

matkt and others added 6 commits July 18, 2019 21:46

add SuppressWarnings annotation to signer metrics test

3589457

Merge pull request #9 from PegaSysEng/master

28ab7e3

merge from master

Add final.

c5153d3

Merge branch 'master' of github.com:PegaSysEng/pantheon into feature/…

0a3b6f4

…pan-2763-validator-block-production

Skip creating properly signed blocks since we're mocking out the Bloc…

c71f125

…kInterface implementation anyway.

Use BlockInterface directly instead of the specific subclasses.

89c4943

ajsutton approved these changes Jul 19, 2019

View reviewed changes

ajsutton merged commit 3ab368d into PegaSysEng:master Jul 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAN-2763] Add JSON-RPC API to report validator block production information #1687

[PAN-2763] Add JSON-RPC API to report validator block production information #1687

matkt commented Jul 13, 2019 •

edited

Loading

matkt commented Jul 13, 2019 •

edited

Loading

ajsutton left a comment

ajsutton Jul 15, 2019

ajsutton Jul 15, 2019

ajsutton Jul 15, 2019

ajsutton Jul 15, 2019

ajsutton Jul 15, 2019

ajsutton left a comment

ajsutton Jul 15, 2019

ajsutton commented Jul 17, 2019

matkt commented Jul 17, 2019

ajsutton Jul 18, 2019

ajsutton Jul 18, 2019

matkt Jul 18, 2019

ajsutton Jul 18, 2019

matkt Jul 18, 2019

ajsutton Jul 18, 2019

matkt Jul 18, 2019

ajsutton Jul 18, 2019

matkt Jul 18, 2019

matkt Jul 18, 2019 •

edited

Loading

ajsutton Jul 18, 2019

matkt Jul 18, 2019

ajsutton left a comment


		when(blockchainQueries.headBlockNumber()).thenReturn(endBlock);

		List<SignerMetricResult> signerMetricResultList = new java.util.ArrayList<>();

[PAN-2763] Add JSON-RPC API to report validator block production information #1687

[PAN-2763] Add JSON-RPC API to report validator block production information #1687

Conversation

matkt commented Jul 13, 2019 • edited Loading

PR description

matkt commented Jul 13, 2019 • edited Loading

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton commented Jul 17, 2019

matkt commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matkt Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton left a comment

Choose a reason for hiding this comment

matkt commented Jul 13, 2019 •

edited

Loading

matkt commented Jul 13, 2019 •

edited

Loading

matkt Jul 18, 2019 •

edited

Loading