[NC-1561] Remove RLPUtils from GetBlockHeadersMessage (#180)

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/EthPeer.java

@@ -127,18 +127,18 @@ public class EthPeer {
   }
 
   public ResponseStream getHeadersByHash(
-      final Hash hash, final int maxHeaders, final boolean reverse, final int skip)
+      final Hash hash, final int maxHeaders, final int skip, final boolean reverse)
       throws PeerNotConnected {
     final GetBlockHeadersMessage message =
-        GetBlockHeadersMessage.create(hash, maxHeaders, reverse, skip);
+        GetBlockHeadersMessage.create(hash, maxHeaders, skip, reverse);
     return sendHeadersRequest(message);
   }
 
   public ResponseStream getHeadersByNumber(
-      final long blockNumber, final int maxHeaders, final boolean reverse, final int skip)
+      final long blockNumber, final int maxHeaders, final int skip, final boolean reverse)
       throws PeerNotConnected {
     final GetBlockHeadersMessage message =
-        GetBlockHeadersMessage.create(blockNumber, maxHeaders, reverse, skip);
+        GetBlockHeadersMessage.create(blockNumber, maxHeaders, skip, reverse);
     return sendHeadersRequest(message);
   }
 

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/messages/GetBlockHeadersMessage.java

@@ -12,24 +12,27 @@
  */
 package tech.pegasys.pantheon.ethereum.eth.messages;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import tech.pegasys.pantheon.ethereum.core.Hash;
 import tech.pegasys.pantheon.ethereum.p2p.NetworkMemoryPool;
 import tech.pegasys.pantheon.ethereum.p2p.api.MessageData;
+import tech.pegasys.pantheon.ethereum.p2p.utils.ByteBufUtils;
 import tech.pegasys.pantheon.ethereum.p2p.wire.AbstractMessageData;
 import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput;
-import tech.pegasys.pantheon.ethereum.rlp.RlpUtils;
+import tech.pegasys.pantheon.ethereum.rlp.RLPInput;
-import tech.pegasys.pantheon.util.bytes.Bytes32;
+import tech.pegasys.pantheon.ethereum.rlp.RLPOutput;
 
-import java.nio.ByteBuffer;
 import java.util.Optional;
 import java.util.OptionalLong;
 
-import com.google.common.primitives.Ints;
 import io.netty.buffer.ByteBuf;
 
 /** PV62 GetBlockHeaders Message. */
 public final class GetBlockHeadersMessage extends AbstractMessageData {
 
+  private GetBlockHeadersData getBlockHeadersData = null;
+
   public static GetBlockHeadersMessage readFrom(final MessageData message) {
     if (message instanceof GetBlockHeadersMessage) {
       message.retain();
@@ -46,33 +49,25 @@ public final class GetBlockHeadersMessage extends AbstractMessageData {
   }
 
   public static GetBlockHeadersMessage create(
-      final long blockNum, final int maxHeaders, final boolean reverse, final int skip) {
+      final long blockNum, final int maxHeaders, final int skip, final boolean reverse) {
+    final GetBlockHeadersData getBlockHeadersData =
+        GetBlockHeadersData.create(blockNum, maxHeaders, skip, reverse);
     final BytesValueRLPOutput tmp = new BytesValueRLPOutput();
-    tmp.startList();
+    getBlockHeadersData.writeTo(tmp);
-    tmp.writeLongScalar(blockNum);
+    final ByteBuf data = NetworkMemoryPool.allocate(tmp.encodedSize());
-    return create(maxHeaders, reverse, skip, tmp);
+    data.writeBytes(tmp.encoded().extractArray());
+    return new GetBlockHeadersMessage(data);
   }
 
   public static GetBlockHeadersMessage create(
-      final Hash hash, final int maxHeaders, final boolean reverse, final int skip) {
+      final Hash hash, final int maxHeaders, final int skip, final boolean reverse) {
+    final GetBlockHeadersData getBlockHeadersData =
+        GetBlockHeadersData.create(hash, maxHeaders, skip, reverse);
     final BytesValueRLPOutput tmp = new BytesValueRLPOutput();
-    tmp.startList();
+    getBlockHeadersData.writeTo(tmp);
-    tmp.writeBytesValue(hash);
+    final ByteBuf data = NetworkMemoryPool.allocate(tmp.encodedSize());
-    return create(maxHeaders, reverse, skip, tmp);
+    data.writeBytes(tmp.encoded().extractArray());
-  }
+    return new GetBlockHeadersMessage(data);
-
-  public static GetBlockHeadersMessage createForSingleHeader(final Hash hash) {
-    return create(hash, 1, false, 0);
-  }
-
-  public static GetBlockHeadersMessage createForContiguousHeaders(
-      final long blockNum, final int maxHeaders) {
-    return create(blockNum, maxHeaders, false, 0);
-  }
-
-  public static GetBlockHeadersMessage createForContiguousHeaders(
-      final Hash blockHash, final int maxHeaders) {
-    return create(blockHash, maxHeaders, false, 0);
   }
 
   private GetBlockHeadersMessage(final ByteBuf data) {
@@ -91,18 +86,7 @@ public final class GetBlockHeadersMessage extends AbstractMessageData {
    * @return Block Number Requested or {@link OptionalLong#EMPTY}
    */
   public OptionalLong blockNumber() {
-    final ByteBuffer raw = data.nioBuffer();
+    return getBlockHeadersData().blockNumber;
-    final int offsetList = RlpUtils.decodeOffset(raw, 0);
-    final int lengthFirst = RlpUtils.decodeLength(raw, offsetList);
-    final int offsetFirst = RlpUtils.decodeOffset(raw, offsetList);
-    if (lengthFirst - offsetFirst == Bytes32.SIZE) {
-      return OptionalLong.empty();
-    } else {
-      final byte[] tmp = new byte[lengthFirst];
-      raw.position(offsetList);
-      raw.get(tmp);
-      return OptionalLong.of(RlpUtils.readLong(0, lengthFirst, tmp));
-    }
   }
 
   /**
@@ -112,52 +96,109 @@ public final class GetBlockHeadersMessage extends AbstractMessageData {
    * @return Block Hash Requested or {@link Optional#EMPTY}
    */
   public Optional<Hash> hash() {
-    final ByteBuffer raw = data.nioBuffer();
+    return getBlockHeadersData().blockHash;
-    final int offsetList = RlpUtils.decodeOffset(raw, 0);
-    final int lengthFirst = RlpUtils.decodeLength(raw, offsetList);
-    final int offsetFirst = RlpUtils.decodeOffset(raw, offsetList);
-    if (lengthFirst - offsetFirst == Bytes32.SIZE) {
-      final byte[] hashBytes = new byte[Bytes32.SIZE];
-      raw.position(offsetFirst + offsetList);
-      raw.get(hashBytes);
-      return Optional.of(Hash.wrap(Bytes32.wrap(hashBytes)));
-    } else {
-      return Optional.empty();
-    }
   }
 
   public int maxHeaders() {
-    final ByteBuffer raw = data.nioBuffer();
+    return getBlockHeadersData().maxHeaders;
-    final int offsetList = RlpUtils.decodeOffset(raw, 0);
-    final byte[] tmp = new byte[raw.capacity()];
-    raw.get(tmp);
-    final int offsetMaxHeaders = RlpUtils.nextOffset(tmp, offsetList);
-    final int lenMaxHeaders = RlpUtils.decodeLength(tmp, offsetMaxHeaders);
-    return Ints.checkedCast(RlpUtils.readLong(offsetMaxHeaders, lenMaxHeaders, tmp));
   }
 
   public int skip() {
-    final ByteBuffer raw = data.nioBuffer();
+    return getBlockHeadersData().skip;
-    final int offsetList = RlpUtils.decodeOffset(raw, 0);
-    final byte[] tmp = new byte[raw.capacity()];
-    raw.get(tmp);
-    final int offsetSkip = RlpUtils.nextOffset(tmp, RlpUtils.nextOffset(tmp, offsetList));
-    final int lenSkip = RlpUtils.decodeLength(tmp, offsetSkip);
-    return Ints.checkedCast(RlpUtils.readLong(offsetSkip, lenSkip, tmp));
   }
 
   public boolean reverse() {
-    return (data.getByte(this.getSize() - 1) & 0xff) != RlpUtils.RLP_ZERO;
+    return getBlockHeadersData().reverse;
   }
 
-  private static GetBlockHeadersMessage create(
+  private GetBlockHeadersData getBlockHeadersData() {
-      final int maxHeaders, final boolean reverse, final int skip, final BytesValueRLPOutput tmp) {
+    if (getBlockHeadersData == null) {
-    tmp.writeIntScalar(maxHeaders);
+      getBlockHeadersData = GetBlockHeadersData.readFrom(ByteBufUtils.toRLPInput(data));
-    tmp.writeIntScalar(skip);
+    }
-    tmp.writeIntScalar(reverse ? 1 : 0);
+    return getBlockHeadersData;
-    tmp.endList();
+  }
-    final ByteBuf data = NetworkMemoryPool.allocate(tmp.encodedSize());
+
-    data.writeBytes(tmp.encoded().extractArray());
+  private static class GetBlockHeadersData {
-    return new GetBlockHeadersMessage(data);
+    private final Optional<Hash> blockHash;
+    private final OptionalLong blockNumber;
+    private final int maxHeaders;
+    private final int skip;
+    private final boolean reverse;
+
+    private GetBlockHeadersData(
+        final Optional<Hash> blockHash,
+        final OptionalLong blockNumber,
+        final int maxHeaders,
+        final int skip,
+        final boolean reverse) {
+      checkArgument(
+          validateBlockHashAndNumber(blockHash, blockNumber),
+          "Either blockHash or blockNumber should be non-empty");
+      this.blockHash = blockHash;
+      this.blockNumber = blockNumber;
+      this.maxHeaders = maxHeaders;
+      this.skip = skip;
+      this.reverse = reverse;
+    }
+
+    private static boolean validateBlockHashAndNumber(
+        final Optional<Hash> blockHash, final OptionalLong blockNumber) {
+      return (blockHash.isPresent() || blockNumber.isPresent())
+          && !(blockHash.isPresent() && blockNumber.isPresent());
+    }
+
+    public static GetBlockHeadersData readFrom(final RLPInput input) {
+      input.enterList();
+
+      final Optional<Hash> blockHash;
+      final OptionalLong blockNumber;
+      if (input.nextSize() == Hash.SIZE) {
+        blockHash = Optional.of(Hash.wrap(input.readBytes32()));
+        blockNumber = OptionalLong.empty();
+      } else {
+        blockHash = Optional.empty();
+        blockNumber = OptionalLong.of(input.readLongScalar());
+      }
+
+      int maxHeaders = input.readIntScalar();
+      int skip = input.readIntScalar();
+      boolean reverse = input.readIntScalar() != 0;
+
+      input.leaveList();
+
+      return new GetBlockHeadersData(blockHash, blockNumber, maxHeaders, skip, reverse);
+    }
+
+    public static GetBlockHeadersData create(
+        final long blockNum, final int maxHeaders, final int skip, final boolean reverse) {
+      return new GetBlockHeadersData(
+          Optional.empty(), OptionalLong.of(blockNum), maxHeaders, skip, reverse);
+    }
+
+    public static GetBlockHeadersData create(
+        final Hash hash, final int maxHeaders, final int skip, final boolean reverse) {
+      return new GetBlockHeadersData(
+          Optional.of(hash), OptionalLong.empty(), maxHeaders, skip, reverse);
+    }
+
+    /**
+     * Write an RLP representation.
+     *
+     * @param out The RLP output to write to
+     */
+    public void writeTo(final RLPOutput out) {
+      out.startList();
+
+      if (blockHash.isPresent()) {
+        out.writeBytesValue(blockHash.get());
+      } else {
+        out.writeLongScalar(blockNumber.getAsLong());
+      }
+      out.writeIntScalar(maxHeaders);
+      out.writeIntScalar(skip);
+      out.writeIntScalar(reverse ? 1 : 0);
+
+      out.endList();
+    }
   }
 }

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/tasks/GetHeadersFromPeerByHashTask.java

@@ -85,7 +85,7 @@ public class GetHeadersFromPeerByHashTask extends AbstractGetHeadersFromPeerTask
   @Override
   protected ResponseStream sendRequest(final EthPeer peer) throws PeerNotConnected {
     LOG.debug("Requesting {} headers from peer {}.", count, peer);
-    return peer.getHeadersByHash(referenceHash, count, reverse, skip);
+    return peer.getHeadersByHash(referenceHash, count, skip, reverse);
   }
 
   @Override

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/tasks/GetHeadersFromPeerByNumberTask.java

@@ -80,7 +80,7 @@ public class GetHeadersFromPeerByNumberTask extends AbstractGetHeadersFromPeerTa
   @Override
   protected ResponseStream sendRequest(final EthPeer peer) throws PeerNotConnected {
     LOG.debug("Requesting {} headers from peer {}.", count, peer);
-    return peer.getHeadersByNumber(blockNumber, count, reverse, skip);
+    return peer.getHeadersByNumber(blockNumber, count, skip, reverse);
   }
 
   @Override

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/EthPeerTest.java

@@ -41,7 +41,7 @@ public class EthPeerTest {
   @Test
   public void getHeadersStream() throws PeerNotConnected {
     final ResponseStreamSupplier getStream =
-        (peer) -> peer.getHeadersByHash(gen.hash(), 5, false, 0);
+        (peer) -> peer.getHeadersByHash(gen.hash(), 5, 0, false);
     final MessageData targetMessage =
         BlockHeadersMessage.create(Arrays.asList(gen.header(), gen.header()));
     final MessageData otherMessage =
@@ -79,7 +79,7 @@ public class EthPeerTest {
     final EthPeer peer = createPeer();
     // Setup headers stream
     final AtomicInteger headersClosedCount = new AtomicInteger(0);
-    peer.getHeadersByHash(gen.hash(), 5, false, 0)
+    peer.getHeadersByHash(gen.hash(), 5, 0, false)
         .then(
             (closed, msg, p) -> {
               if (closed) {
@@ -133,7 +133,7 @@ public class EthPeerTest {
     final AtomicInteger headersMessageCount = new AtomicInteger(0);
     final AtomicInteger headersClosedCount = new AtomicInteger(0);
     final ResponseStream headersStream =
-        peer.getHeadersByHash(gen.hash(), 5, false, 0)
+        peer.getHeadersByHash(gen.hash(), 5, 0, false)
             .then(
                 (closed, msg, p) -> {
                   if (closed) {

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/manager/EthProtocolManagerTest.java

@@ -175,7 +175,7 @@ public final class EthProtocolManagerTest {
       final long startBlock = 5L;
       final int blockCount = 5;
       final MessageData messageData =
-          GetBlockHeadersMessage.create(startBlock, blockCount, false, 0);
+          GetBlockHeadersMessage.create(startBlock, blockCount, 0, false);
       final PeerSendHandler onSend =
           (cap, message, conn) -> {
             if (message.getCode() == EthPV62.STATUS) {
@@ -208,7 +208,7 @@ public final class EthProtocolManagerTest {
       final long startBlock = 5L;
       final int blockCount = 10;
       final MessageData messageData =
-          GetBlockHeadersMessage.create(startBlock, blockCount, false, 0);
+          GetBlockHeadersMessage.create(startBlock, blockCount, 0, false);
       final PeerSendHandler onSend =
           (cap, message, conn) -> {
             if (message.getCode() == EthPV62.STATUS) {
@@ -238,7 +238,7 @@ public final class EthProtocolManagerTest {
     try (final EthProtocolManager ethManager = new EthProtocolManager(blockchain, 1, true, 1)) {
       final long endBlock = 10L;
       final int blockCount = 5;
-      final MessageData messageData = GetBlockHeadersMessage.create(endBlock, blockCount, true, 0);
+      final MessageData messageData = GetBlockHeadersMessage.create(endBlock, blockCount, 0, true);
       final PeerSendHandler onSend =
           (cap, message, conn) -> {
             if (message.getCode() == EthPV62.STATUS) {
@@ -270,7 +270,7 @@ public final class EthProtocolManagerTest {
       final int blockCount = 5;
       final int skip = 1;
       final MessageData messageData =
-          GetBlockHeadersMessage.create(startBlock, blockCount, false, 1);
+          GetBlockHeadersMessage.create(startBlock, blockCount, 1, false);
       final PeerSendHandler onSend =
           (cap, message, conn) -> {
             if (message.getCode() == EthPV62.STATUS) {
@@ -303,7 +303,7 @@ public final class EthProtocolManagerTest {
       final int blockCount = 5;
       final int skip = 1;
       final MessageData messageData =
-          GetBlockHeadersMessage.create(endBlock, blockCount, true, skip);
+          GetBlockHeadersMessage.create(endBlock, blockCount, skip, true);
       final PeerSendHandler onSend =
           (cap, message, conn) -> {
             if (message.getCode() == EthPV62.STATUS) {
@@ -356,7 +356,7 @@ public final class EthProtocolManagerTest {
       final long startBlock = blockchain.getChainHeadBlockNumber() - 1L;
       final int blockCount = 5;
       final MessageData messageData =
-          GetBlockHeadersMessage.create(startBlock, blockCount, false, 0);
+          GetBlockHeadersMessage.create(startBlock, blockCount, 0, false);
       final PeerSendHandler onSend =
           (cap, message, conn) -> {
             if (message.getCode() == EthPV62.STATUS) {
@@ -387,7 +387,7 @@ public final class EthProtocolManagerTest {
       final long startBlock = blockchain.getChainHeadBlockNumber() + 1;
       final int blockCount = 5;
       final MessageData messageData =
-          GetBlockHeadersMessage.create(startBlock, blockCount, false, 0);
+          GetBlockHeadersMessage.create(startBlock, blockCount, 0, false);
       final PeerSendHandler onSend =
           (cap, message, conn) -> {
             if (message.getCode() == EthPV62.STATUS) {
@@ -729,7 +729,7 @@ public final class EthProtocolManagerTest {
       final int requestedBlockCount = 13;
       final int receivedBlockCount = 2;
       final MessageData messageData =
-          GetBlockHeadersMessage.create(startBlock, requestedBlockCount, true, 0);
+          GetBlockHeadersMessage.create(startBlock, requestedBlockCount, 0, true);
       final MockPeerConnection.PeerSendHandler onSend =
           (cap, message, conn) -> {
             if (message.getCode() == EthPV62.STATUS) {

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/messages/GetBlockHeadersMessageTest.java

@@ -33,7 +33,7 @@ public final class GetBlockHeadersMessageTest {
       final int skip = 10;
       final int maxHeaders = 128;
       final GetBlockHeadersMessage initialMessage =
-          GetBlockHeadersMessage.create(hash, maxHeaders, reverse, skip);
+          GetBlockHeadersMessage.create(hash, maxHeaders, skip, reverse);
       final ByteBuf rawBuffer = NetworkMemoryPool.allocate(initialMessage.getSize());
       initialMessage.writeTo(rawBuffer);
       final MessageData raw = new RawMessage(EthPV62.GET_BLOCK_HEADERS, rawBuffer);
@@ -59,7 +59,7 @@ public final class GetBlockHeadersMessageTest {
       final int skip = 10;
       final int maxHeaders = 128;
       final GetBlockHeadersMessage initialMessage =
-          GetBlockHeadersMessage.create(blockNum, maxHeaders, reverse, skip);
+          GetBlockHeadersMessage.create(blockNum, maxHeaders, skip, reverse);
       final ByteBuf rawBuffer = NetworkMemoryPool.allocate(initialMessage.getSize());
       final MessageData raw = new RawMessage(EthPV62.GET_BLOCK_HEADERS, rawBuffer);
       final GetBlockHeadersMessage message = GetBlockHeadersMessage.readFrom(raw);

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/tasks/DetermineCommonAncestorTaskTest.java

@@ -393,8 +393,8 @@ public class DetermineCommonAncestorTaskTest {
     assertThat(result).isCompletedWithValue(genesisBlock.getHeader());
 
     // Make sure we didn't ask for any headers
-    verify(peer, times(0)).getHeadersByHash(any(), anyInt(), anyBoolean(), anyInt());
+    verify(peer, times(0)).getHeadersByHash(any(), anyInt(), anyInt(), anyBoolean());
-    verify(peer, times(0)).getHeadersByNumber(anyLong(), anyInt(), anyBoolean(), anyInt());
+    verify(peer, times(0)).getHeadersByNumber(anyLong(), anyInt(), anyInt(), anyBoolean());
     verify(peer, times(0)).send(any());
   }
 }