Merge #15681: [mempool] Allow one extra single-ancestor transaction per package

50cede3f5a4d4fbfbb7c420b94e661a6a159bced [mempool] Allow one extra single-ancestor transaction per package (Matt Corallo)

Pull request description:

  This implements the proposed policy change from [1], which allows
  certain classes of contract protocols involving revocation
  punishments to use CPFP. Note that some such use-cases may still
  want some form of one-deep package relay, though even this alone
  may greatly simplify some lightning fee negotiation.

  [1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html

ACKs for top commit:
  ajtowns:
    ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced -- looked over code again, compared with previous commit, compiles, etc.
  sdaftuar:
    ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced
  ryanofsky:
    utACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced. Changes since last review: adding EXTRA_DESCENDANT_TX_SIZE_LIMIT constant, changing max ancestor size from 1,000,000 to nLimitAncestorSize constant (101,000), fixing test comment and getting rid of unused test node.

Tree-SHA512: b052c2a0f384855572b4579310131897b612201214b5abbb225167224e4f550049e300b471dbf320928652571e92ca2d650050b7cf39ac92b3bc1d2bcd386c1c

4 files changed, 108 insertions, 1 deletions

diff --git a/src/validation.cpp b/src/validation.cpp
index 0bc6167bad..f585bc7d2d 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -616,7 +616,21 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
         size_t nLimitDescendantSize = gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000;
         std::string errString;
         if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) {
-            return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
+            setAncestors.clear();
+            // If the new transaction is relatively small (up to 40k weight)
+            // and has at most one ancestor (ie ancestor limit of 2, including
+            // the new transaction), allow it if its parent has exactly the
+            // descendant limit descendants.
+            //
+            // This allows protocols which rely on distrusting counterparties
+            // being able to broadcast descendants of an unconfirmed transaction
+            // to be secure by simply only having two immediately-spendable
+            // outputs - one for each counterparty. For more info on the uses for
+            // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
+            if (nSize >  EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
+                    !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, errString)) {
+                return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
+            }
         }
 
         // A transaction that spends outputs that would be replaced by it is invalid. Now
diff --git a/src/validation.h b/src/validation.h
index 7739d3de4a..d747fdbf27 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -64,6 +64,12 @@ static const unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT = 101;
 static const unsigned int DEFAULT_DESCENDANT_LIMIT = 25;
 /** Default for -limitdescendantsize, maximum kilobytes of in-mempool descendants */
 static const unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT = 101;
+/**
+ * An extra transaction can be added to a package, as long as it only has one
+ * ancestor and is no larger than this. Not really any reason to make this
+ * configurable as it doesn't materially change DoS parameters.
+ */
+static const unsigned int EXTRA_DESCENDANT_TX_SIZE_LIMIT = 10000;
 /** Default for -mempoolexpiry, expiration time for mempool transactions in hours */
 static const unsigned int DEFAULT_MEMPOOL_EXPIRY = 336;
 /** Maximum kilobytes for transactions to store for processing during reorg */
diff --git a/test/functional/mempool_package_onemore.py b/test/functional/mempool_package_onemore.py
new file mode 100755
index 0000000000..f955c1a77f
--- /dev/null
+++ b/test/functional/mempool_package_onemore.py
@@ -0,0 +1,86 @@
+#!/usr/bin/env python3
+# Copyright (c) 2014-2019 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test descendant package tracking carve-out allowing one final transaction in
+   an otherwise-full package as long as it has only one parent and is <= 10k in
+   size.
+"""
+
+from decimal import Decimal
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal, assert_raises_rpc_error, satoshi_round
+
+MAX_ANCESTORS = 25
+MAX_DESCENDANTS = 25
+
+class MempoolPackagesTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+        self.extra_args = [["-maxorphantx=1000"]]
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    # Build a transaction that spends parent_txid:vout
+    # Return amount sent
+    def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs):
+        send_value = satoshi_round((value - fee)/num_outputs)
+        inputs = []
+        for (txid, vout) in zip(parent_txids, vouts):
+            inputs.append({'txid' : txid, 'vout' : vout})
+        outputs = {}
+        for i in range(num_outputs):
+            outputs[node.getnewaddress()] = send_value
+        rawtx = node.createrawtransaction(inputs, outputs)
+        signedtx = node.signrawtransactionwithwallet(rawtx)
+        txid = node.sendrawtransaction(signedtx['hex'])
+        fulltx = node.getrawtransaction(txid, 1)
+        assert len(fulltx['vout']) == num_outputs  # make sure we didn't generate a change output
+        return (txid, send_value)
+
+    def run_test(self):
+        # Mine some blocks and have them mature.
+        self.nodes[0].generate(101)
+        utxo = self.nodes[0].listunspent(10)
+        txid = utxo[0]['txid']
+        vout = utxo[0]['vout']
+        value = utxo[0]['amount']
+
+        fee = Decimal("0.0002")
+        # MAX_ANCESTORS transactions off a confirmed tx should be fine
+        chain = []
+        for _ in range(4):
+            (txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [vout], value, fee, 2)
+            vout = 0
+            value = sent_value
+            chain.append([txid, value])
+        for _ in range(MAX_ANCESTORS - 4):
+            (txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [0], value, fee, 1)
+            value = sent_value
+            chain.append([txid, value])
+        (second_chain, second_chain_value) = self.chain_transaction(self.nodes[0], [utxo[1]['txid']], [utxo[1]['vout']], utxo[1]['amount'], fee, 1)
+
+        # Check mempool has MAX_ANCESTORS + 1 transactions in it
+        assert_equal(len(self.nodes[0].getrawmempool(True)), MAX_ANCESTORS + 1)
+
+        # Adding one more transaction on to the chain should fail.
+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [txid], [0], value, fee, 1)
+        # ...even if it chains on from some point in the middle of the chain.
+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[2][0]], [1], chain[2][1], fee, 1)
+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[1][0]], [1], chain[1][1], fee, 1)
+        # ...even if it chains on to two parent transactions with one in the chain.
+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0], second_chain], [1, 0], chain[0][1] + second_chain_value, fee, 1)
+        # ...especially if its > 40k weight
+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 350)
+        # But not if it chains directly off the first transaction
+        self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1)
+        # and the second chain should work just fine
+        self.chain_transaction(self.nodes[0], [second_chain], [0], second_chain_value, fee, 1)
+
+        # Finally, check that we added two transactions
+        assert_equal(len(self.nodes[0].getrawmempool(True)), MAX_ANCESTORS + 3)
+
+if __name__ == '__main__':
+    MempoolPackagesTest().main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 71738be1d0..6fc48f2649 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -157,6 +157,7 @@ BASE_SCRIPTS = [
     'rpc_invalidateblock.py',
     'feature_rbf.py',
     'mempool_packages.py',
+    'mempool_package_onemore.py',
     'rpc_createmultisig.py',
     'feature_versionbits_warning.py',
     'rpc_preciousblock.py',