Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

ibft mining acceptance test #483

Merged
merged 19 commits into from
Jan 11, 2019

Conversation

jframe
Copy link
Contributor

@jframe jframe commented Dec 21, 2018

PR description

Acceptance test for IBFT to test that a single node and a cluster of 4 nodes work in the basic mining test sending transactions.

Fixed Issue(s)

@jframe jframe added the work in progress Work on this pull request is ongoing label Jan 7, 2019
@jframe jframe removed the work in progress Work on this pull request is ongoing label Jan 9, 2019
@jframe jframe requested a review from CjHare January 9, 2019 03:52
return getRunningNodes().get(0);
}

private static class NodeInfo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the value in this new inner class, my IntelliJ shows isRunning() and isRunning are not called outside NodeInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just way a way adding of associating a the running state with the nodes and their names. Allows me to just verify on the running nodes if I have stopped some.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True the isRunning() doesn't need to exist, I just added both the getter and setters for both fields for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach with NodeInfo and that with the verifyRunningNode are inconsistent with the indirection used elsewhere within Cluster and AT framework.

Let me think about it, there'll be a more consistent way to this goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

new PantheonFactoryConfigurationBuilder()
.setName(name)
.miningEnabled()
.setJsonRpcConfiguration(jsonRpcConfigWithClique())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsonRpcConfigWithClique? (not withIbft?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that's not right. i'll fix that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. it happen to be working because it is enabled by default. I still prefer to be explicit in this case though so am doing it the same as in the clique case and adding the rpc option.

"eip158Block": 3,
"byzantiumBlock": 1035301,
"revisedibft": {
"blockperiodseconds": 5,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to drop these values to make tests faster? (blocktime could be 1 reasonably).

If we do timeout tests, we probably don't want to wait 25 seconds for (request) timeout ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what reasonable time is. in the clique tests I found they were not as reliable when I had the block time at such a small value. will have to think about this a bit. but in general want this to be as small as practical.
agree 25 seconds is too long. ideas on how long? I might go with 5s for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

final PantheonNode validator1 = pantheon.createIbftNode("miner1");
final PantheonNode validator2 = pantheon.createIbftNode("miner2");
final PantheonNode validator3 = pantheon.createIbftNode("miner3");
final PantheonNode nonValidatorNode = pantheon.createIbftNode("miner4");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

textual name (miner4) doesn't line up with variable name (nonValidatorNode) - which should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops. that was accidental, changed back to what it was before consistent with node names.
done.

@Ignore("Temporarily disabled until ibft new block events are being sent")
public void shouldMineOnMultipleNodesEvenWhenClusterContainsNonValidator() throws IOException {
final String[] validators = {"validator1", "validator2", "validator3"};
final PantheonNode validator1 = pantheon.createIbftNodeWithValidators("validator1", validators);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name field could/should reference the validators array? [or even put this into a loop?]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest of the code can't reference the validators array, best I could do is use the validator names in that array to create the validator nodes. I've done that but I think the test is less clear afterwards.

can't use stream as we have a checked exception (IOException) to deal with from the createIbft methods.

final String[] validatorNames = {"validator1", "validator2", "validator3"};
    final List<PantheonNode> validators = new ArrayList<>();
    for (String name: validatorNames) {
      validators.add(pantheon.createIbftNodeWithValidators(name, validatorNames));
    }
    final PantheonNode nonValidatorNode =
        pantheon.createIbftNodeWithValidators("non-validator", validatorNames);
    validators.add(nonValidatorNode);
    cluster.start(validators);

    final Account sender = accounts.createAccount("account1");
    final Account receiver = accounts.createAccount("account2");

    validators.get(0).execute(transactions.createTransfer(sender, 50));
    cluster.verify(sender.balanceEquals(50));

    validators.get(1).execute(transactions.createIncrementalTransfers(sender, receiver, 1));
    cluster.verify(receiver.balanceEquals(1));

    nonValidatorNode.execute(transactions.createIncrementalTransfers(sender, receiver, 2));
    cluster.verify(receiver.balanceEquals(3));

@jframe jframe merged commit ba3c2a5 into PegaSysEng:master Jan 11, 2019
@jframe jframe deleted the feature/ibft_mining_acceptance_test branch April 18, 2019 00:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants