Use scalars for consensus round encoding (#2002)

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

consensus/qbft/build.gradle

@@ -117,7 +117,7 @@ task ('validateReferenceTestSubmodule') {
   description = "Checks that the reference tests submodule is not accidentally changed"
   doLast {
     def result = new ByteArrayOutputStream()
-    def expectedHash = '2fe00d76d97e35aa2df6449b5f567c41cf6f58fc'
+    def expectedHash = '5a5751bfba33dc7024805bb1f2a8d6f8d96a1549'
     def submodulePath = java.nio.file.Path.of("${rootProject.projectDir}", "consensus/qbft/src/reference-test/resources").toAbsolutePath()
     try {
       exec {

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/payload/CommitPayload.java

@@ -26,7 +26,7 @@ import org.hyperledger.besu.ethereum.rlp.RLPOutput;
 import java.util.Objects;
 import java.util.StringJoiner;
 
-public class CommitPayload implements Payload {
+public class CommitPayload extends QbftPayload {
   private static final int TYPE = QbftV1.COMMIT;
   private final ConsensusRoundIdentifier roundIdentifier;
   private final Hash digest;
@@ -43,7 +43,7 @@ public class CommitPayload implements Payload {
 
   public static CommitPayload readFrom(final RLPInput rlpInput) {
     rlpInput.enterList();
-    final ConsensusRoundIdentifier roundIdentifier = ConsensusRoundIdentifier.readFrom(rlpInput);
+    final ConsensusRoundIdentifier roundIdentifier = readConsensusRound(rlpInput);
     final Hash digest = Payload.readDigest(rlpInput);
     final SECPSignature commitSeal =
         rlpInput.readBytes(SignatureAlgorithmFactory.getInstance()::decodeSignature);
@@ -55,7 +55,7 @@ public class CommitPayload implements Payload {
   @Override
   public void writeTo(final RLPOutput rlpOutput) {
     rlpOutput.startList();
-    roundIdentifier.writeTo(rlpOutput);
+    writeConsensusRound(rlpOutput);
     rlpOutput.writeBytes(digest);
     rlpOutput.writeBytes(commitSeal.encodedBytes());
     rlpOutput.endList();

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/payload/PreparePayload.java

@@ -24,7 +24,7 @@ import org.hyperledger.besu.ethereum.rlp.RLPOutput;
 import java.util.Objects;
 import java.util.StringJoiner;
 
-public class PreparePayload implements Payload {
+public class PreparePayload extends QbftPayload {
   private static final int TYPE = QbftV1.PREPARE;
   private final ConsensusRoundIdentifier roundIdentifier;
   private final Hash digest;
@@ -36,7 +36,7 @@ public class PreparePayload implements Payload {
 
   public static PreparePayload readFrom(final RLPInput rlpInput) {
     rlpInput.enterList();
-    final ConsensusRoundIdentifier roundIdentifier = ConsensusRoundIdentifier.readFrom(rlpInput);
+    final ConsensusRoundIdentifier roundIdentifier = readConsensusRound(rlpInput);
     final Hash digest = Payload.readDigest(rlpInput);
     rlpInput.leaveList();
     return new PreparePayload(roundIdentifier, digest);
@@ -45,7 +45,7 @@ public class PreparePayload implements Payload {
   @Override
   public void writeTo(final RLPOutput rlpOutput) {
     rlpOutput.startList();
-    roundIdentifier.writeTo(rlpOutput);
+    writeConsensusRound(rlpOutput);
     rlpOutput.writeBytes(digest);
     rlpOutput.endList();
   }

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/payload/ProposalPayload.java

@@ -16,7 +16,6 @@ package org.hyperledger.besu.consensus.qbft.payload;
 
 import org.hyperledger.besu.consensus.common.bft.BftBlockHeaderFunctions;
 import org.hyperledger.besu.consensus.common.bft.ConsensusRoundIdentifier;
-import org.hyperledger.besu.consensus.common.bft.payload.Payload;
 import org.hyperledger.besu.consensus.qbft.messagedata.QbftV1;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.rlp.RLPInput;
@@ -26,7 +25,7 @@ import java.util.Objects;
 
 import com.google.common.base.MoreObjects;
 
-public class ProposalPayload implements Payload {
+public class ProposalPayload extends QbftPayload {
 
   private static final int TYPE = QbftV1.PROPOSAL;
   private final ConsensusRoundIdentifier roundIdentifier;
@@ -40,7 +39,7 @@ public class ProposalPayload implements Payload {
 
   public static ProposalPayload readFrom(final RLPInput rlpInput) {
     rlpInput.enterList();
-    final ConsensusRoundIdentifier roundIdentifier = ConsensusRoundIdentifier.readFrom(rlpInput);
+    final ConsensusRoundIdentifier roundIdentifier = readConsensusRound(rlpInput);
     final Block proposedBlock =
         Block.readFrom(rlpInput, BftBlockHeaderFunctions.forCommittedSeal());
     rlpInput.leaveList();
@@ -51,7 +50,7 @@ public class ProposalPayload implements Payload {
   @Override
   public void writeTo(final RLPOutput rlpOutput) {
     rlpOutput.startList();
-    roundIdentifier.writeTo(rlpOutput);
+    writeConsensusRound(rlpOutput);
     proposedBlock.writeTo(rlpOutput);
     rlpOutput.endList();
   }

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/payload/QbftPayload.java

@@ -0,0 +1,32 @@
+/*
+ * Copyright 2020 ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.consensus.qbft.payload;
+
+import org.hyperledger.besu.consensus.common.bft.ConsensusRoundIdentifier;
+import org.hyperledger.besu.consensus.common.bft.payload.Payload;
+import org.hyperledger.besu.ethereum.rlp.RLPInput;
+import org.hyperledger.besu.ethereum.rlp.RLPOutput;
+
+public abstract class QbftPayload implements Payload {
+
+  protected void writeConsensusRound(final RLPOutput out) {
+    out.writeLongScalar(getRoundIdentifier().getSequenceNumber());
+    out.writeIntScalar(getRoundIdentifier().getRoundNumber());
+  }
+
+  protected static ConsensusRoundIdentifier readConsensusRound(final RLPInput in) {
+    return new ConsensusRoundIdentifier(in.readLongScalar(), in.readIntScalar());
+  }
+}

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/payload/RoundChangePayload.java

@@ -15,7 +15,6 @@
 package org.hyperledger.besu.consensus.qbft.payload;
 
 import org.hyperledger.besu.consensus.common.bft.ConsensusRoundIdentifier;
-import org.hyperledger.besu.consensus.common.bft.payload.Payload;
 import org.hyperledger.besu.consensus.qbft.messagedata.QbftV1;
 import org.hyperledger.besu.ethereum.rlp.RLPInput;
 import org.hyperledger.besu.ethereum.rlp.RLPOutput;
@@ -25,7 +24,7 @@ import java.util.Optional;
 
 import com.google.common.base.MoreObjects;
 
-public class RoundChangePayload implements Payload {
+public class RoundChangePayload extends QbftPayload {
   private static final int TYPE = QbftV1.ROUND_CHANGE;
   private final ConsensusRoundIdentifier roundChangeIdentifier;
   private final Optional<PreparedRoundMetadata> preparedRoundMetadata;
@@ -50,7 +49,7 @@ public class RoundChangePayload implements Payload {
   public void writeTo(final RLPOutput rlpOutput) {
     // RLP encode of the message data content (round identifier and prepared certificate)
     rlpOutput.startList();
-    roundChangeIdentifier.writeTo(rlpOutput);
+    writeConsensusRound(rlpOutput);
 
     rlpOutput.startList();
     preparedRoundMetadata.ifPresent(prm -> prm.writeTo(rlpOutput));
@@ -61,7 +60,7 @@ public class RoundChangePayload implements Payload {
 
   public static RoundChangePayload readFrom(final RLPInput rlpInput) {
     rlpInput.enterList();
-    final ConsensusRoundIdentifier roundIdentifier = ConsensusRoundIdentifier.readFrom(rlpInput);
+    final ConsensusRoundIdentifier roundIdentifier = readConsensusRound(rlpInput);
     final Optional<PreparedRoundMetadata> preparedRoundMetadata;
 
     rlpInput.enterList();

consensus/qbft/src/reference-test/resources

@@ -1 +1 @@
-Subproject commit 2fe00d76d97e35aa2df6449b5f567c41cf6f58fc
+Subproject commit 5a5751bfba33dc7024805bb1f2a8d6f8d96a1549

consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/payload/QbftPayloadTest.java

@@ -0,0 +1,79 @@
+/*
+ * Copyright 2020 ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.consensus.qbft.payload;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.hyperledger.besu.consensus.common.bft.ConsensusRoundIdentifier;
+import org.hyperledger.besu.ethereum.rlp.BytesValueRLPOutput;
+import org.hyperledger.besu.ethereum.rlp.RLP;
+import org.hyperledger.besu.ethereum.rlp.RLPInput;
+import org.hyperledger.besu.ethereum.rlp.RLPOutput;
+
+import org.junit.Test;
+
+public class QbftPayloadTest {
+
+  private final QbftPayload payload =
+      new QbftPayload() {
+        @Override
+        public void writeTo(final RLPOutput rlpOutput) {
+          writeConsensusRound(rlpOutput);
+        }
+
+        @Override
+        public int getMessageType() {
+          return 0;
+        }
+
+        @Override
+        public ConsensusRoundIdentifier getRoundIdentifier() {
+          return new ConsensusRoundIdentifier(5, 10);
+        }
+      };
+
+  @Test
+  public void roundAndSequenceAreEncodedAsScalars() {
+    final BytesValueRLPOutput out = new BytesValueRLPOutput();
+    out.startList();
+    payload.writeTo(out);
+    out.endList();
+    final RLPInput rlpIn = RLP.input(out.encoded());
+
+    rlpIn.enterList();
+    assertThat(rlpIn.readLongScalar()).isEqualTo(payload.getRoundIdentifier().getSequenceNumber());
+    assertThat(rlpIn.readIntScalar()).isEqualTo(payload.getRoundIdentifier().getRoundNumber());
+    rlpIn.leaveList();
+  }
+
+  @Test
+  public void readingConsensusRoundExpectsScalarValues() {
+    final long sequence = 100;
+    final int round = 8;
+    final BytesValueRLPOutput out = new BytesValueRLPOutput();
+    out.startList();
+    out.writeLongScalar(sequence);
+    out.writeIntScalar(round);
+    out.endList();
+
+    final RLPInput rlpIn = RLP.input(out.encoded());
+    rlpIn.enterList();
+    final ConsensusRoundIdentifier roundIdentifier = QbftPayload.readConsensusRound(rlpIn);
+    rlpIn.leaveList();
+
+    assertThat(roundIdentifier.getSequenceNumber()).isEqualTo(sequence);
+    assertThat(roundIdentifier.getRoundNumber()).isEqualTo(round);
+  }
+}

consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/validation/RoundChangePayloadValidatorTest.java

@@ -139,18 +139,6 @@ public class RoundChangePayloadValidatorTest {
     assertThat(messageValidator.validate(signedPayload)).isFalse();
   }
 
-  @Test
-  public void roundChangeWithNegativeTargetRoundFails() {
-    final RoundChangePayload payload =
-        new RoundChangePayload(
-            new ConsensusRoundIdentifier(chainHeight, -3),
-            Optional.of(new PreparedRoundMetadata(preparedBlockHash, 1)));
-
-    final SignedData<RoundChangePayload> signedPayload =
-        createSignedPayload(payload, validators.getNode(0).getNodeKey());
-    assertThat(messageValidator.validate(signedPayload)).isFalse();
-  }
-
   private SignedData<RoundChangePayload> createSignedPayload(
       final RoundChangePayload payload, final NodeKey nodeKey) {
     final SECPSignature signature = nodeKey.sign(hashForSignature(payload));