From 352614280cdc6da196fcb35e293a32822e699378 Mon Sep 17 00:00:00 2001 From: Sean Gilligan Date: Mon, 7 Aug 2023 18:33:37 -0700 Subject: [PATCH] BlockLocator: deprecate add & no-args constructor Deprecate `.add()` and no-args constructor in favor of providing complete list of hashes at creation time. Update all usages to use the alternative methods. --- .../java/org/bitcoinj/core/BlockLocator.java | 15 ++++++++++ .../org/bitcoinj/core/GetBlocksMessage.java | 8 +++-- .../org/bitcoinj/core/GetHeadersMessage.java | 8 +++-- .../src/main/java/org/bitcoinj/core/Peer.java | 7 +++-- .../bitcoinj/core/BitcoindComparisonTool.java | 6 ++-- .../test/java/org/bitcoinj/core/PeerTest.java | 30 +++++++++---------- 6 files changed, 46 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/core/BlockLocator.java b/core/src/main/java/org/bitcoinj/core/BlockLocator.java index 9b3607a00..8027b8fe7 100644 --- a/core/src/main/java/org/bitcoinj/core/BlockLocator.java +++ b/core/src/main/java/org/bitcoinj/core/BlockLocator.java @@ -19,6 +19,7 @@ package org.bitcoinj.core; import org.bitcoinj.base.Sha256Hash; import org.bitcoinj.base.internal.InternalUtils; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.stream.Collectors; @@ -30,6 +31,10 @@ import java.util.stream.Stream; public final class BlockLocator { private final List hashes; // unmodifiable list + /** + * @deprecated Use {@link BlockLocator#BlockLocator(List)} + */ + @Deprecated public BlockLocator() { hashes = Collections.emptyList(); } @@ -41,7 +46,15 @@ public final class BlockLocator { this.hashes = Collections.unmodifiableList(hashes); } + // Used by tests + static BlockLocator ofBlocks(Block... blocks) { + return new BlockLocator(Arrays.stream(blocks) + .map(Block::getHash) + .collect(Collectors.toList())); + } + // Create a new BlockLocator by copying an instance and appending an element + @Deprecated private BlockLocator(BlockLocator old, Sha256Hash hashToAdd) { this(Stream.concat(old.hashes.stream(), Stream.of(hashToAdd)) .collect(Collectors.toList()) @@ -50,7 +63,9 @@ public final class BlockLocator { /** * Add a {@link Sha256Hash} to a newly created block locator. + * @deprecated Use {@link BlockLocator#BlockLocator(List)} */ + @Deprecated public BlockLocator add(Sha256Hash hash) { return new BlockLocator(this, hash); } diff --git a/core/src/main/java/org/bitcoinj/core/GetBlocksMessage.java b/core/src/main/java/org/bitcoinj/core/GetBlocksMessage.java index 9ec32bb31..1d73225a8 100644 --- a/core/src/main/java/org/bitcoinj/core/GetBlocksMessage.java +++ b/core/src/main/java/org/bitcoinj/core/GetBlocksMessage.java @@ -25,6 +25,8 @@ import java.io.IOException; import java.io.OutputStream; import java.nio.BufferUnderflowException; import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.List; import static org.bitcoinj.base.internal.Preconditions.check; @@ -54,12 +56,12 @@ public class GetBlocksMessage extends BaseMessage { int startCount = startCountVarInt.intValue(); if (startCount > 500) throw new ProtocolException("Number of locators cannot be > 500, received: " + startCount); - BlockLocator locator = new BlockLocator(); + List hashList = new ArrayList<>(); for (int i = 0; i < startCount; i++) { - locator = locator.add(Sha256Hash.read(payload)); + hashList.add(Sha256Hash.read(payload)); } Sha256Hash stopHash = Sha256Hash.read(payload); - return new GetBlocksMessage(version, locator, stopHash); + return new GetBlocksMessage(version, new BlockLocator(hashList), stopHash); } public GetBlocksMessage(long protocolVersion, BlockLocator locator, Sha256Hash stopHash) { diff --git a/core/src/main/java/org/bitcoinj/core/GetHeadersMessage.java b/core/src/main/java/org/bitcoinj/core/GetHeadersMessage.java index d4fd7837c..f9cf30f0c 100644 --- a/core/src/main/java/org/bitcoinj/core/GetHeadersMessage.java +++ b/core/src/main/java/org/bitcoinj/core/GetHeadersMessage.java @@ -22,6 +22,8 @@ import org.bitcoinj.base.internal.ByteUtils; import java.nio.BufferUnderflowException; import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.List; import static org.bitcoinj.base.internal.Preconditions.check; @@ -48,12 +50,12 @@ public class GetHeadersMessage extends GetBlocksMessage { int startCount = startCountVarInt.intValue(); if (startCount > 500) throw new ProtocolException("Number of locators cannot be > 500, received: " + startCount); - BlockLocator locator = new BlockLocator(); + List hashList = new ArrayList<>(); for (int i = 0; i < startCount; i++) { - locator = locator.add(Sha256Hash.read(payload)); + hashList.add(Sha256Hash.read(payload)); } Sha256Hash stopHash = Sha256Hash.read(payload); - return new GetHeadersMessage(version, locator, stopHash); + return new GetHeadersMessage(version, new BlockLocator(hashList), stopHash); } public GetHeadersMessage(long protocolVersion, BlockLocator locator, Sha256Hash stopHash) { diff --git a/core/src/main/java/org/bitcoinj/core/Peer.java b/core/src/main/java/org/bitcoinj/core/Peer.java index ef39205de..af6a7f965 100644 --- a/core/src/main/java/org/bitcoinj/core/Peer.java +++ b/core/src/main/java/org/bitcoinj/core/Peer.java @@ -1415,7 +1415,6 @@ public class Peer extends PeerSocketHandler { // headers and then request the blocks from that point onwards. "getheaders" does not send us an inv, it just // sends us the data we requested in a "headers" message. - BlockLocator blockLocator = new BlockLocator(); // For now we don't do the exponential thinning as suggested here: // // https://en.bitcoin.it/wiki/Protocol_specification#getblocks @@ -1437,9 +1436,10 @@ public class Peer extends PeerSocketHandler { if (log.isDebugEnabled()) log.debug("{}: blockChainDownloadLocked({}) current head = {}", this, toHash, chainHead.getHeader().getHashAsString()); + List hashes = new ArrayList<>(100); StoredBlock cursor = chainHead; for (int i = 100; cursor != null && i > 0; i--) { - blockLocator = blockLocator.add(cursor.getHeader().getHash()); + hashes.add(cursor.getHeader().getHash()); try { cursor = cursor.getPrev(store); } catch (BlockStoreException e) { @@ -1449,7 +1449,8 @@ public class Peer extends PeerSocketHandler { } // Only add the genesis hash to the locator if we didn't already do so. If the chain is < 100 blocks we already reached it. if (cursor != null) - blockLocator = blockLocator.add(params.getGenesisBlock().getHash()); + hashes.add(params.getGenesisBlock().getHash()); + BlockLocator blockLocator = new BlockLocator(hashes); // Record that we requested this range of blocks so we can filter out duplicate requests in the event of a // block being solved during chain download. diff --git a/core/src/test/java/org/bitcoinj/core/BitcoindComparisonTool.java b/core/src/test/java/org/bitcoinj/core/BitcoindComparisonTool.java index 8eee077ce..70ba905ae 100644 --- a/core/src/test/java/org/bitcoinj/core/BitcoindComparisonTool.java +++ b/core/src/test/java/org/bitcoinj/core/BitcoindComparisonTool.java @@ -213,8 +213,7 @@ public class BitcoindComparisonTool { connectedFuture.get(); - BlockLocator locator = new BlockLocator(); - locator = locator.add(PARAMS.getGenesisBlock().getHash()); + BlockLocator locator = new BlockLocator(Collections.singletonList(PARAMS.getGenesisBlock().getHash())); Sha256Hash hashTo = Sha256Hash.wrap("0000000000000000000000000000000000000000000000000000000000000000"); int rulesSinceFirstFail = 0; @@ -302,8 +301,7 @@ public class BitcoindComparisonTool { if (block.throwsException) blocksRequested.remove(nextBlock.getHash()); //bitcoind.sendMessage(nextBlock); - locator = new BlockLocator(); - locator = locator.add(bitcoindChainHead); + locator = new BlockLocator(Collections.singletonList(bitcoindChainHead)); bitcoind.sendMessage(new GetHeadersMessage(PARAMS.getSerializer().getProtocolVersion(), locator, hashTo)); bitcoind.sendPing().get(); if (!chain.getChainHead().getHeader().getHash().equals(bitcoindChainHead)) { diff --git a/integration-test/src/test/java/org/bitcoinj/core/PeerTest.java b/integration-test/src/test/java/org/bitcoinj/core/PeerTest.java index 1b5468e0f..dff03946b 100644 --- a/integration-test/src/test/java/org/bitcoinj/core/PeerTest.java +++ b/integration-test/src/test/java/org/bitcoinj/core/PeerTest.java @@ -214,10 +214,10 @@ public class PeerTest extends TestWithNetworkConnections { inbound(writeTarget, inv); GetBlocksMessage getblocks = (GetBlocksMessage)outbound(writeTarget); - BlockLocator expectedLocator = new BlockLocator(); - expectedLocator = expectedLocator.add(b1.getHash()); - expectedLocator = expectedLocator.add(TESTNET.getGenesisBlock().getHash()); - + BlockLocator expectedLocator = BlockLocator.ofBlocks( + b1, + TESTNET.getGenesisBlock()); + assertEquals(getblocks.getLocator(), expectedLocator); assertEquals(getblocks.getStopHash(), b3.getHash()); assertNull(outbound(writeTarget)); @@ -384,10 +384,10 @@ public class PeerTest extends TestWithNetworkConnections { }); peer.startBlockChainDownload(); - BlockLocator expectedLocator = new BlockLocator(); - expectedLocator = expectedLocator.add(b2.getHash()); - expectedLocator = expectedLocator.add(b1.getHash()); - expectedLocator = expectedLocator.add(TESTNET.getGenesisBlock().getHash()); + BlockLocator expectedLocator = BlockLocator.ofBlocks( + b2, + b1, + TESTNET.getGenesisBlock()); GetBlocksMessage message = (GetBlocksMessage) outbound(writeTarget); assertEquals(message.getLocator(), expectedLocator); @@ -471,19 +471,19 @@ public class PeerTest extends TestWithNetworkConnections { ); peer.startBlockChainDownload(); GetHeadersMessage getheaders = (GetHeadersMessage) outbound(writeTarget); - BlockLocator expectedLocator = new BlockLocator(); - expectedLocator = expectedLocator.add(b1.getHash()); - expectedLocator = expectedLocator.add(TESTNET.getGenesisBlock().getHash()); + BlockLocator expectedLocator = BlockLocator.ofBlocks( + b1, + TESTNET.getGenesisBlock()); assertEquals(getheaders.getLocator(), expectedLocator); assertEquals(getheaders.getStopHash(), Sha256Hash.ZERO_HASH); // Now send all the headers. HeadersMessage headers = new HeadersMessage(b2.cloneAsHeader(), b3.cloneAsHeader(), b4.cloneAsHeader()); // We expect to be asked for b3 and b4 again, but this time, with a body. - expectedLocator = new BlockLocator(); - expectedLocator = expectedLocator.add(b2.getHash()); - expectedLocator = expectedLocator.add(b1.getHash()); - expectedLocator = expectedLocator.add(TESTNET.getGenesisBlock().getHash()); + expectedLocator = BlockLocator.ofBlocks( + b2, + b1, + TESTNET.getGenesisBlock()); inbound(writeTarget, headers); GetBlocksMessage getblocks = (GetBlocksMessage) outbound(writeTarget); assertEquals(expectedLocator, getblocks.getLocator());