Clique acceptance tests #290

jframe · 2018-11-21T02:14:22Z

PR description

Adds acceptance tests for the Clique consensus mechanism.

Fixed Issue(s)

…amically

…jframe/pantheon into feature/NC-1524_clique_acceptance_tests

… more accurate clique getSigners test

# Conflicts: # acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/CreateAccountAcceptanceTest.java # acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/AcceptanceTestBase.java # acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/Eth.java # acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/Node.java # acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/PantheonNode.java # acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java # acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/web3/Web3Sha3Transaction.java # pantheon/src/main/java/tech/pegasys/pantheon/cli/EthNetworkConfig.java

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/clique/CliqueMiningAcceptanceTest.java

.../java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/blockchain/ExpectBeneficiary.java

.../java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/blockchain/ExpectBlockNumber.java

...ests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/PantheonWeb3j.java

...src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/CliqueTransactions.java

...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/clique/CliquePropose.java

.../test/java/tech/pegasys/pantheon/tests/acceptance/clique/CliqueDiscardRpcAcceptanceTest.java

...sts/src/test/java/tech/pegasys/pantheon/tests/acceptance/clique/CliqueGetSignersRpcTest.java

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/clique/CliqueMiningAcceptanceTest.java

.../test/java/tech/pegasys/pantheon/tests/acceptance/clique/CliqueProposeRpcAcceptanceTest.java

rain-on · 2018-11-22T22:21:42Z

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/CliqueExtraData.java

@@ -110,20 +107,10 @@ public BytesValue getVanityData() {
    return validators;
  }

-  public static String createGenesisExtraDataString(final List<PrivateKey> privKeys) {
-    final List<Address> validators = convertPrivKeysToAddresses(privKeys);
+  public static String createGenesisExtraDataString(final List<Address> validators) {


In the real world (not acceptance test) - we would often be provided with either:
PrivateKey, or Public Key (or Address I guess) - it'd be great to have the ability to convert from any of these three things, into a genesis ExtraData String.
Happy for the priv --> pub --> address utility to live somewhere else, but we should try and keep it methinks.

…jframe/pantheon into feature/nc-1524_clique_acceptance_tests

ajsutton · 2018-11-26T04:35:22Z

...a/tech/pegasys/pantheon/tests/acceptance/dsl/condition/blockchain/ExpectBlockNotCreated.java

+    try {
+      Thread.sleep(INITIAL_WAIT);
+      final BigInteger lastBlock = node.execute(eth.blockNumber());
+      Thread.sleep(WAIT);


This should be using a waiter that we expect to fail instead of a hard coded sleep - that way if a new block is published it fails immediately. It's a small thing but not having to wait 15 seconds every time makes a big difference if you're trying to work out why the test is failing.

Yeah not nice, but was simple. I've ended up changing it fail immediately if we get a new block.

…lock

# Conflicts: # acceptance-tests/build.gradle

.../test/java/tech/pegasys/pantheon/tests/acceptance/clique/CliqueDiscardRpcAcceptanceTest.java

rain-on · 2018-11-27T23:14:13Z

...sts/src/test/java/tech/pegasys/pantheon/tests/acceptance/clique/CliqueGetSignersRpcTest.java

+    cluster.verify(clique.validatorsAtBlockEqual("0x1", initialNodes));
+
+    minerNode1.waitUntil(wait.chainHeadHasProgressed(minerNode1, 1));
+    cluster.verify(clique.validatorsAtBlockEqual("0x3", allNodes));


should this work with 0x2? or does it have to be 3?

We are already at block 1 when vote in the new validator, so it is working after 2 blocks have progressed.

rain-on · 2018-11-27T23:25:29Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/clique/CliqueMiningAcceptanceTest.java

+    minerNode1.execute(transactions.createTransfer(sender, 50));
+    cluster.verify(sender.balanceEquals(50));
+
+    minerNode2.execute(transactions.createIncrementalTransfers(sender, receiver, 1));


nit: I suspect there's a risk that you haven't actually mined separate blocks - can the test be updated to ensure the transactions are spread across blocks?

The transfers are being done from different nodes so there should be multiple blocks.

Transactions are communicated between nodes. They're only guaranteed to be in different blocks if you wait for their effects to be applied before sending the next transaction. So:

minerNode1.execute(transactions.createTransfer(sender, 50)); cluster.verify(sender.balanceEquals(50));

Must be in one block and any transaction sent after that must be in a different block.

Yeah true, fortunately am following that pattern of during the transfer and then waiting for the effects the applied.

So this what is being done atm:
` minerNode1.execute(transactions.createTransfer(sender, 50));
cluster.verify(sender.balanceEquals(50));

minerNode2.execute(transactions.createIncrementalTransfers(sender, receiver, 1)); cluster.verify(receiver.balanceEquals(1)); minerNode3.execute(transactions.createIncrementalTransfers(sender, receiver, 2)); cluster.verify(receiver.balanceEquals(3));`

So I'm expecting that at least 3 blocks will be created by doing this.

…are potential miners

# Conflicts: # acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/AcceptanceTestBase.java

rain-on

It would be good to write some tests around progressing when a validator has stopped, and also check if signer's of the blocks actually iterates.

jframe added 30 commits October 16, 2018 16:06

[NC-1524] Clique miner acceptance test

8699cb8

Merge branch 'master' into feature/NC-1524_clique_acceptance_tests

bc2fccf

[NC-1524] Clique miner acceptance test - create clique extra data dyn…

907739d

…amically

[NC-1524] Clique miner acceptance test - spotless and error prone

3440d5f

[NC-1524] Clique miner acceptance test - rpc tests

5b3aefb

Merge branch 'master' into feature/NC-1524_clique_acceptance_tests

5f76acb

[NC-1524] Clique miner acceptance test - spotless

7a7f769

[NC-1524] Clique miner acceptance test - fix bad merge

4237915

[NC-1524] Clique miner acceptance test - enable clique rpc

6eaad43

Merge branch 'master' into feature/NC-1524_clique_acceptance_tests

5dfb9c8

[NC-1524] Clique miner acceptance test - discard and proposals rcp tests

5f3d5bb

Merge branch 'master' into feature/NC-1524_clique_acceptance_tests

a113454

Merge branch 'master' into feature/NC-1524_clique_acceptance_tests

74ba546

[NC-1524] Clique miner acceptance test - spotless

73aa666

Merge branch 'master' into feature/NC-1524_clique_acceptance_tests

3be7e0b

[NC-1524] Clique miner acceptance test - PR improvements

d19fa8a

Merge branch 'feature/NC-1524_clique_acceptance_tests' of github.com:…

b12e563

…jframe/pantheon into feature/NC-1524_clique_acceptance_tests

[NC-1524] clean up PantheonNodeConfig constructors

7807a18

[NC-1524] refactor to add a waitForChainHeadToProgress method

03f9eae

[NC-1524] change validator state and wait until blocks progress for a…

de5a1e8

… more accurate clique getSigners test

[NC-1524] test to confirm clique vote details are in block header

ba6a232

[NC-1524] test clique stalls if insufficient validators

af45272

[NC-1524] cleanup

74af4e5

[NC-1524] cleanup

83aa0ae

[NC-1524] use web3j factory with added clique methods

da89fbe

[NC-1524] clique transactions

2dd7596

[NC-1524] move blockchain related conditions into package

adb87fa

[NC-1524] move clique related conditions into package

d829065

[NC-1524] change clique conditions to use clique transactions

05cb7ce

jframe requested review from saltiniroberto and Errorific November 21, 2018 02:16

CjHare added the acceptance test label Nov 21, 2018

CjHare reviewed Nov 22, 2018

View reviewed changes

...ests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/PantheonWeb3j.java Show resolved Hide resolved

CjHare reviewed Nov 22, 2018

View reviewed changes

...src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/CliqueTransactions.java Outdated Show resolved Hide resolved

...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/clique/CliquePropose.java Show resolved Hide resolved

jframe added 3 commits November 22, 2018 15:03

[NC-1524] code review changes

5273625

[NC-1524] code review changes

b0cd99f

Merge branch 'master' into feature/nc-1524_clique_acceptance_tests

04dbd3b

rain-on reviewed Nov 22, 2018

View reviewed changes

jframe added 8 commits November 23, 2018 09:25

[NC-1524] fix broken tests after merge from master

9f814f1

[NC-1524] additional validator checks in getSigners test

219830b

[NC-1524] added no block created expectation

9d35daf

[NC-1524] rename clique proposal methods

2afe001

Merge branch 'master' into feature/nc-1524_clique_acceptance_tests

478f95c

[NC-1524] wait for peers to disconnect in miner instability test

819d5c5

Merge branch 'feature/nc-1524_clique_acceptance_tests' of github.com:…

2689a06

…jframe/pantheon into feature/nc-1524_clique_acceptance_tests

[NC-1524] add initial wait in no block created expectation

cde87bd

ajsutton reviewed Nov 26, 2018

View reviewed changes

jframe added 3 commits November 27, 2018 11:37

[NC-1524] fail no new block condition immediately if there is a new b…

e78b394

…lock

Merge branch 'master' into feature/nc-1524_clique_acceptance_tests

cb88863

# Conflicts: # acceptance-tests/build.gradle

[NC-1524] spotless fixes

3863b53

rain-on reviewed Nov 28, 2018

View reviewed changes

jframe added 3 commits November 28, 2018 11:05

[NC-1524] drop miner from createCliqueMinerNode methods as all nodes …

69e93b9

…are potential miners

[NC-1524] add wait to ensure chain is at expected block

bd5ec67

Merge branch 'master' into feature/nc-1524_clique_acceptance_tests

4178105

# Conflicts: # acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/AcceptanceTestBase.java

rain-on approved these changes Nov 29, 2018

View reviewed changes

Merge branch 'master' into feature/nc-1524_clique_acceptance_tests

026fa0f

jframe merged commit ab744a9 into PegaSysEng:master Nov 29, 2018

jframe deleted the feature/nc-1524_clique_acceptance_tests branch December 6, 2018 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clique acceptance tests #290

Clique acceptance tests #290

jframe commented Nov 21, 2018

rain-on Nov 22, 2018

ajsutton Nov 26, 2018

jframe Nov 27, 2018

rain-on Nov 27, 2018

jframe Nov 28, 2018

rain-on Nov 27, 2018

jframe Nov 28, 2018

ajsutton Nov 28, 2018

jframe Nov 29, 2018

rain-on left a comment

Clique acceptance tests #290

Clique acceptance tests #290

Conversation

jframe commented Nov 21, 2018

PR description

Fixed Issue(s)

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

rain-on left a comment

Choose a reason for hiding this comment