[NC-1582] iBFT 2.0 ExtraData #37

saltiniroberto · 2018-10-11T05:59:19Z

iBFT ExtraData class for iBFT 2.0

For the extraData iBFT 2.0 specifications, please see par. "Block Header Description" of the iBFT 2.0 Specification Document

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraData.java

rain-on · 2018-10-12T06:13:17Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraData.java

+
+  private final BytesValue vanityData;
+  private final List<Signature> seals;
+  private final Address voteRecipient;


This address and VoteType should be encapsulated in a single object, which in turn is optional. Similar to what Chris did (or almost did?) in Clique.

I had the object, no optional. The object represented the 3 states of auth, drop, and empty given all 3 states have values when read or written.

Fair suggestion but it would be great if we could agree beforehand on whether we want an Optional vote field in ExtraData or whether we want a Vote that can be empty?

This class is good in that it exactly represents what is in the block header.
My concern is that a vote is only valid if both recipient and 'polarity' exist. But maybe the IbftExtraDataValidator can ensure this data struct is valid.

The smell may be that in the past, the vote type field was primitive type (i.e. was a long up until we needed it to be a vote) - We may now fail to deserialise the header, rather than failing at validation.

Added Vote class

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraData.java

rain-on · 2018-10-15T05:13:50Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraData.java

+        input.size() > EXTRA_VANITY_LENGTH,
+        "Invalid BytesValue supplied - too short to produce a valid IBFT Extra Data object.");
+
+    final BytesValue vanityData = input.slice(0, EXTRA_VANITY_LENGTH);


Confirming the VanityData will not be RLPd?

This is exactly as it is in iBFT 1.0. So, yes, we are not RLP'ing vanity.

I have actually decided to RLP the vanity data as it is a better approach

rain-on · 2018-10-15T05:14:53Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraData.java

+    rlpInput.enterList(); // This accounts for the "root node" which contains IBFT data items.
+    final List<Address> validators = rlpInput.readList(Address::readFrom);
+    final Address voteRecipient = Address.readFrom(rlpInput);
+    final Optional<Ibft2VoteType> vote = Ibft2VoteType.readFrom(rlpInput);


Is the Vote Optional? Or will it always be there?

I made the vote optional just to handle the case that the vote value is different from either AUTH or DROP.
Alternative is to throw an RLP exception if that is the case.
What do you suggest?

I don't really know - but this is ultimately the smell - we're interpreting data rather than just deserialising. Maybe Vote should always exist, but be a long/int?

Changed to non Optional and type changed to Vote

Errorific · 2018-10-17T00:29:25Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/Vote.java

+    return new Vote(address, Ibft2VoteType.DROP);
+  }
+
+  public static Vote noVote() {


I'd like to see this be a 3rd Ibft2VoteType as currently noVotes get identified by isDrop and won't be easily filterable as a non actionable type if we use this representation in places other than the extra data.

or at least update the isDrop() function to not include noVotes & maybe add an isNoVote()

Conditioned isDrop to return true if the recipient address is different from 0.
Also, added isNoVote and tests for testing these methods and the creating methods

After discussion with Trent, I will change the vote field in extraData back to Optional to identify that there is no vote information in the extra data and remove the noVote method from Vote

rain-on · 2018-10-17T03:57:15Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/Vote.java

+    return voteType;
+  }
+
+  public static void writTo(final Optional<Vote> optionalVote, final RLPOutput rlpOutput) {


Suggested change

public static void writTo(final Optional<Vote> optionalVote, final RLPOutput rlpOutput) {

public static void writeTo(final Optional<Vote> optionalVote, final RLPOutput rlpOutput) {

rain-on · 2018-10-17T03:58:38Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/Vote.java

+  public static void writTo(final Optional<Vote> optionalVote, final RLPOutput rlpOutput) {
+    if (optionalVote.isPresent()) {
+      rlpOutput.startList();
+      rlpOutput.writeBytesValue(optionalVote.get().recipient);


pull the optional into a local variable - or else embed in a lambda:
optionalVote.ifPresent(v -> { ...}).orElse(rlp.writeNull());

As far as I am aware, you cannot apply orElse after ifPresent

rain-on · 2018-10-17T03:59:36Z

consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/Vote.java

+  }
+
+  public static Optional<Vote> readFrom(final RLPInput rlpInput) {
+    if (!rlpInput.nextIsNull()) {


nit: Flip the conditional and exit early if(rlpInput.nextIsNull()) { return Optional.empty;}
the use the reset of the code outside the conditional

rain-on · 2018-10-17T22:45:29Z

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraDataTest.java

+public class Ibft2ExtraDataTest {
+
+  @Test
+  public void correctlyCodedRoundConstitutesValidContent() {


This seems to be largely testing the RLP of the round number - with some side benefits of the other fields. Which aspect of the IBFT Extra Data does using a byte array for the round prove?

That you are not coding it as scalar, but you always expect to be coded as 4 bytes regardless of the leading zeroes

rain-on · 2018-10-17T22:53:27Z

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraDataTest.java

+    assertThat(extraData.getValidators()).isEqualTo(validators);
+  }
+
+  @Test(expected = RLPException.class)


People seem to be a fan of using assertj's assertThrownBy rather than the junit 'expected'.

rain-on · 2018-10-17T22:55:29Z

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraDataTest.java

+
+  @Test
+  public void fullyPopulatedDataProducesCorrectlyFormedExtraDataObject() {
+    final List<Address> validators = Arrays.asList(Address.ECREC, Address.SHA256);


probably better using custom addresses - i.e. Address.fromHexString("<>") rather than the pre-canned ones.

Does the validator list need to be ordered? (I personally think not, but that's my perspective).

Not for coding the extraData. Sorting is only relevant for the validation

Changed. I was using the pre-canned one because I know that they are real addresses. But it doesn't really matter

rain-on · 2018-10-17T22:56:18Z

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraDataTest.java

+  }
+
+  @Test
+  public void fullyPopulatedDateIsEncodedCorrectly() {


'date'? or data?

rain-on · 2018-10-17T22:57:04Z

consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/Ibft2ExtraDataTest.java

+    assertThat(actualExtraData).isEqualToComparingFieldByField(expectedExtraData);
+  }
+
+  @Test(expected = RLPException.class)


use assertj assertThatThrownBy

saltiniroberto requested review from Errorific, jframe and rain-on October 11, 2018 05:59

Added Ibft2ExtraData

b2a7b45

saltiniroberto force-pushed the ibft2_extra_data branch from bd6d65d to b2a7b45 Compare October 15, 2018 04:00

rain-on reviewed Oct 15, 2018

View reviewed changes

saltiniroberto and others added 5 commits October 16, 2018 15:52

Added Vote class and RLPed vanity data

132bd51

Merge branch 'master' into ibft2_extra_data

48594e3

Added finals

9aa5218

Fixed up test failures

fa8480e

Merge branch 'master' into ibft2_extra_data

6f0a2c0

Errorific reviewed Oct 17, 2018

View reviewed changes

saltiniroberto added 5 commits October 17, 2018 11:46

Added isNoVote and added test

8b79442

Merge

8527661

Moved vote field in extraData back to Optional

4ba2441

Merge remote-tracking branch 'upstream/master' into ibft2_extra_data

08e40fa

Applied new spotless rules

8e59f98

rain-on reviewed Oct 17, 2018

View reviewed changes

saltiniroberto added 2 commits October 17, 2018 15:25

After review

a7b3924

Moved enc/dec of an Optional vote into Ibft2ExtraData

87529d1

rain-on reviewed Oct 17, 2018

View reviewed changes

saltiniroberto added 6 commits October 18, 2018 10:23

Replaced expect with assertThatThrownnBy

788f0f5

Fixed spelling

4bacb22

Replaced canned addresses with fromHexString

f549f7c

Added test agains reference string

19abb71

Small refactoring on tests

40d11d6

Optimised

2fbfb5a

rain-on approved these changes Oct 18, 2018

View reviewed changes

Merge branch 'master' into ibft2_extra_data

a7d7dd3

saltiniroberto merged commit fdd08a8 into PegaSysEng:master Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NC-1582] iBFT 2.0 ExtraData #37

[NC-1582] iBFT 2.0 ExtraData #37

saltiniroberto commented Oct 11, 2018

rain-on Oct 12, 2018

Errorific Oct 15, 2018

saltiniroberto Oct 15, 2018 •

edited

Loading

rain-on Oct 15, 2018

saltiniroberto Oct 16, 2018

rain-on Oct 15, 2018

saltiniroberto Oct 15, 2018

saltiniroberto Oct 16, 2018

rain-on Oct 15, 2018

saltiniroberto Oct 15, 2018

rain-on Oct 15, 2018

saltiniroberto Oct 16, 2018

Errorific Oct 17, 2018

Errorific Oct 17, 2018

saltiniroberto Oct 17, 2018

saltiniroberto Oct 17, 2018

rain-on Oct 17, 2018

saltiniroberto Oct 17, 2018

rain-on Oct 17, 2018

saltiniroberto Oct 17, 2018

rain-on Oct 17, 2018

saltiniroberto Oct 17, 2018

rain-on Oct 17, 2018

saltiniroberto Oct 17, 2018

rain-on Oct 17, 2018

saltiniroberto Oct 17, 2018

rain-on Oct 17, 2018

rain-on Oct 17, 2018

saltiniroberto Oct 17, 2018

saltiniroberto Oct 17, 2018

rain-on Oct 17, 2018

saltiniroberto Oct 17, 2018

rain-on Oct 17, 2018

saltiniroberto Oct 17, 2018

	public static void writTo(final Optional<Vote> optionalVote, final RLPOutput rlpOutput) {
	public static void writeTo(final Optional<Vote> optionalVote, final RLPOutput rlpOutput) {

[NC-1582] iBFT 2.0 ExtraData #37

[NC-1582] iBFT 2.0 ExtraData #37

Conversation

saltiniroberto commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saltiniroberto Oct 15, 2018 • edited Loading

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

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

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

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

saltiniroberto Oct 15, 2018 •

edited

Loading