From 2288c2150f0a8d15ea9be072c6da3265677a99d2 Mon Sep 17 00:00:00 2001 From: Andreas Schildbach Date: Tue, 1 Oct 2019 21:37:26 +0200 Subject: [PATCH 1/3] PeerGroup: Make private walletCoinsReceivedEventListener also take care of P2WPKH outputs. P2WPKH outputs are a similar case as P2PK outputs in that inputs that spend them cannot be matched by a bloom filter: their scriptSig is empty and their witness (which would contain a matching pubkey) is not tested for a match. This is a shortcoming of the SegWit spec. Hopefully this fixes https://github.com/bitcoinj/bitcoinj/issues/1690 --- .../java/org/bitcoinj/core/PeerGroup.java | 27 ++++++++++--------- .../java/org/bitcoinj/core/PeerGroupTest.java | 2 ++ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/core/PeerGroup.java b/core/src/main/java/org/bitcoinj/core/PeerGroup.java index b9991d4c4b1..0812c653d21 100644 --- a/core/src/main/java/org/bitcoinj/core/PeerGroup.java +++ b/core/src/main/java/org/bitcoinj/core/PeerGroup.java @@ -176,34 +176,37 @@ public class PeerGroup implements TransactionBroadcaster { @Override public void onCoinsReceived(Wallet wallet, Transaction tx, Coin prevBalance, Coin newBalance) { // We received a relevant transaction. We MAY need to recalculate and resend the Bloom filter, but only - // if we have received a transaction that includes a relevant P2PK output. + // if we have received a transaction that includes a relevant P2PK or P2WPKH output. // - // The reason is that P2PK outputs, when spent, will not repeat any data we can predict in their + // The reason is that P2PK and P2WPKH outputs, when spent, will not repeat any data we can predict in their // inputs. So a remote peer will update the Bloom filter for us when such an output is seen matching the - // existing filter, so that it includes the tx hash in which the P2PK output was observed. Thus + // existing filter, so that it includes the tx hash in which the P2PK/P2WPKH output was observed. Thus // the spending transaction will always match (due to the outpoint structure). // // Unfortunately, whilst this is required for correct sync of the chain in blocks, there are two edge cases. // - // (1) If a wallet receives a relevant, confirmed P2PK output that was not broadcast across the network, + // (1) If a wallet receives a relevant, confirmed P2PK/P2WPKH output that was not broadcast across the network, // for example in a coinbase transaction, then the node that's serving us the chain will update its filter // but the rest will not. If another transaction then spends it, the other nodes won't match/relay it. // - // (2) If we receive a P2PK output broadcast across the network, all currently connected nodes will see + // (2) If we receive a P2PK/P2WPKH output broadcast across the network, all currently connected nodes will see // it and update their filter themselves, but any newly connected nodes will receive the last filter we // calculated, which would not include this transaction. // - // For this reason we check if the transaction contained any relevant P2PKs and force a recalc + // For this reason we check if the transaction contained any relevant P2PKs or P2WPKHs and force a recalc // and possibly retransmit if so. The recalculation process will end up including the tx hash into the // filter. In case (1), we need to retransmit the filter to the connected peers. In case (2), we don't // and shouldn't, we should just recalculate and cache the new filter for next time. for (TransactionOutput output : tx.getOutputs()) { - if (ScriptPattern.isP2PK(output.getScriptPubKey()) && output.isMine(wallet)) { - if (tx.getConfidence().getConfidenceType() == TransactionConfidence.ConfidenceType.BUILDING) - recalculateFastCatchupAndFilter(FilterRecalculateMode.SEND_IF_CHANGED); - else - recalculateFastCatchupAndFilter(FilterRecalculateMode.DONT_SEND); - return; + Script scriptPubKey = output.getScriptPubKey(); + if (ScriptPattern.isP2PK(scriptPubKey) || ScriptPattern.isP2WPKH(scriptPubKey)) { + if (output.isMine(wallet)) { + if (tx.getConfidence().getConfidenceType() == TransactionConfidence.ConfidenceType.BUILDING) + recalculateFastCatchupAndFilter(FilterRecalculateMode.SEND_IF_CHANGED); + else + recalculateFastCatchupAndFilter(FilterRecalculateMode.DONT_SEND); + return; + } } } } diff --git a/core/src/test/java/org/bitcoinj/core/PeerGroupTest.java b/core/src/test/java/org/bitcoinj/core/PeerGroupTest.java index 418d9c2760a..18c697150ea 100644 --- a/core/src/test/java/org/bitcoinj/core/PeerGroupTest.java +++ b/core/src/test/java/org/bitcoinj/core/PeerGroupTest.java @@ -622,6 +622,7 @@ public void testBloomOnP2PK() throws Exception { tx2.addInput(tx.getOutput(0)); TransactionOutPoint outpoint = tx2.getInput(0).getOutpoint(); assertTrue(p1.lastReceivedFilter.contains(key.getPubKey())); + assertTrue(p1.lastReceivedFilter.contains(key.getPubKeyHash())); assertFalse(p1.lastReceivedFilter.contains(tx.getTxId().getBytes())); inbound(p1, tx); // p1 requests dep resolution, p2 is quiet. @@ -635,6 +636,7 @@ public void testBloomOnP2PK() throws Exception { // Now we connect p3 and there is a new bloom filter sent, that DOES match the relevant outpoint. InboundMessageQueuer p3 = connectPeer(3); assertTrue(p3.lastReceivedFilter.contains(key.getPubKey())); + assertTrue(p3.lastReceivedFilter.contains(key.getPubKeyHash())); assertTrue(p3.lastReceivedFilter.contains(outpoint.unsafeBitcoinSerialize())); } From cc445b8fd1d73cada75df81e66dc9e97979d0f47 Mon Sep 17 00:00:00 2001 From: Sean Gilligan Date: Tue, 1 Oct 2019 12:30:43 -0700 Subject: [PATCH 2/3] BtcFormat: Fix "heading used out of sequence" JavaDoc error on JDK 13. --- .../java/org/bitcoinj/utils/BtcFormat.java | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/utils/BtcFormat.java b/core/src/main/java/org/bitcoinj/utils/BtcFormat.java index 9662cd52db9..e47349f4e11 100644 --- a/core/src/main/java/org/bitcoinj/utils/BtcFormat.java +++ b/core/src/main/java/org/bitcoinj/utils/BtcFormat.java @@ -65,7 +65,7 @@ *
  • access to character information that allows for vertical * alignment of tabular columns of formatted values. * - *

    Basic Usage

    + *

    Basic Usage

    * *

    Basic usage is very simple:

    * @@ -87,13 +87,13 @@ * Coin all = f.parseObject("M฿ 21"); // All the money in the world * * - *

    Auto-Denomination versus Fixed-Denomination

    + *

    Auto-Denomination versus Fixed-Denomination

    * *

    There are two provided concrete classes, one that automatically denominates values to * be formatted, {@link BtcAutoFormat}, and another that formats any value in units of a * fixed, specified denomination, {@link BtcFixedFormat}.

    * - *

    Automatic Denomination

    + *

    Automatic Denomination

    * *

    Automatic denomination means that the formatter adjusts the denominational units in which a * formatted number is expressed based on the monetary value that number represents. An @@ -111,7 +111,7 @@ * locale, a value of one bitcoin might be formatted as {@code ฿1.00} where a value * exceeding that by one satoshi would be {@code µ฿1,000,000.01}

    * - *

    Fixed Denomination

    + *

    Fixed Denomination

    * *

    Fixed denomination means that the same denomination of units is used for every value that is * formatted or parsed by a given formatter instance. A fixed-denomination formatter is @@ -122,7 +122,7 @@ * {@code 1.0000 BTC}, or one bitcoin, in millibitcoins, one shifts the decimal point * three places, that is, to {@code 1000.0 mBTC}.

    * - *

    Construction

    + *

    Construction

    * *

    There are two ways to obtain an instance of this class:

    * @@ -136,7 +136,7 @@ * access to some features not available through the factory methods, * such as using custom formatting patterns and currency symbols.

    * - *

    Factory Methods

    + *

    Factory Methods

    * *

    Although formatting and parsing is performed by one of the concrete * subclasses, you can obtain formatters using the various static factory @@ -233,7 +233,7 @@ * fractional quantity of satoshis, and these defaults can be overridden by arguments to the * {@code format()} method. See below for examples.

    * - *

    The {@link Builder} Class

    + *

    The {@link Builder} Class

    * *

    A new {@link BtcFormat.Builder} instance is returned by the {@link #builder()} method. Such * an object has methods that set the configuration parameters of a {@link BtcFormat} @@ -264,7 +264,7 @@ * *

    See the documentation of the {@link BtcFormat.Builder} class for details.

    * - *

    Formatting

    + *

    Formatting

    * *

    You format a Bitcoin monetary value by passing it to the {@link BtcFormat#format(Object)} * method. This argument can be either a {@link Coin}-type object or a @@ -350,7 +350,7 @@ * BtcFormat(4).code(); // unnamed denomination has no code; raises exception * * - *

    Formatting for Tabular Columns

    + *

    Formatting for Tabular Columns

    * *

    When displaying tables of monetary values, you can lessen the * risk of human misreading-error by vertically aligning the decimal @@ -396,7 +396,7 @@ * then see the documentation for the {@link NumberFormat} class * for an explanation of how to do that.

    * - *

    Parsing

    + *

    Parsing

    * *

    The {@link #parse(String)} method accepts a {@link String} argument, and returns a * {@link Coin}-type value. The difference in parsing behavior between instances of {@link @@ -443,9 +443,9 @@ * raise a {@code ParseException}, as will any other detected * parsing error.

    * - *

    Limitations

    + *

    Limitations

    * - *

    Parsing

    + *

    Parsing

    * *

    Parsing is performed by an underlying {@link NumberFormat} object. While this * delivers the benefit of recognizing locale-specific patterns, some have criticized other @@ -455,13 +455,13 @@ * input from end-users, then you should consider whether you would benefit from any of the * work-arounds mentioned in that article.

    * - *

    Exotic Locales

    + *

    Exotic Locales

    * *

    This class is not well-tested in locales that use non-ascii * character sets, especially those where writing proceeds from * right-to-left. Helpful feedback in that regard is appreciated.

    * - *

    Thread-Safety

    + *

    Thread-Safety

    * *

    Instances of this class are immutable.

    * From 62ccf132dc5ac0ab24d6d91530b01b8996c2488f Mon Sep 17 00:00:00 2001 From: Sean Gilligan Date: Tue, 10 Sep 2019 19:02:45 -0700 Subject: [PATCH 3/3] Add Github Actions workflow gradle.yml Use Github Actions built-in Gradle 5.6.2 Github Actions virtual environments all include a pre-installed Gradle that is 5.6 or later, so we can just build with it without using the wrapper. --- .github/workflow/gradle.yml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .github/workflow/gradle.yml diff --git a/.github/workflow/gradle.yml b/.github/workflow/gradle.yml new file mode 100644 index 00000000000..050a2844fc2 --- /dev/null +++ b/.github/workflow/gradle.yml @@ -0,0 +1,22 @@ +name: Java CI + +on: [push, pull_request] + +jobs: + build: + runs-on: ${{ matrix.os }} + strategy: + matrix: + # windows-latest currently fails on some tests + os: [ubuntu-latest, macOS-latest, windows-latest] + java: [ '8', '11' ] + fail-fast: false + name: JAVA ${{ matrix.java }} OS ${{ matrix.os }} + steps: + - uses: actions/checkout@v1 + - name: Set up JDK + uses: actions/setup-java@v1 + with: + java-version: ${{ matrix.java }} + - name: Build with Gradle + run: gradle build --stacktrace