From be173a17e1d1f3e30de44d914a9be202d7b112b0 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Wed, 6 Nov 2019 13:16:44 -0500 Subject: [PATCH] [mempool] Restrain carve-out acceptance to only-spending tagged output Offchain multiparty transaction may allow one of the party to spend exclusively one of the output. A party with the ability to spend two or more of this kind of output can use one to hit the descendant limit and the other one to carve-out the transaction with a CPFP good enough to get into the mempool but not get confirmed and so pin the parent transaction. It lets other parties without remedy to bump parent transaction feerate and may incumber a loss of funds on cascade of time-constrained offchain transactions, e.g LN-channels. This change restrain carve-out to enable it to play its end-goal design. A CPFP to get benefits of carve-out relaxation rules must spend first output of parent transaction. Offchain protocols while building pre-signed transaction MUST verify than the first output is a anyone-can-spend or than any party is able to redeem it. This output is "tagged" by being inserted as first one at transaction construction. Explicitly test a non-first-output carve-out spend in mempool_package_onemore which should fail --- src/validation.cpp | 17 +++++++++++++++-- test/functional/mempool_package_onemore.py | 20 +++++++++++--------- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 11072b6038d1c..db8f55505580b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -760,8 +760,21 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // 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 || - !m_pool.CalculateMemPoolAncestors(*entry, setAncestors, 2, m_limit_ancestor_size, m_limit_descendants + 1, m_limit_descendant_size + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) { + + // Verify CPFP transaction is spending carve-out output tagged by contracting/payment channels parties to + // avoid a single-party spend on a N-party pre-signed transaction being used as a malicious carve-out + // output to stuck the parent transaction with a not-good-enough feerate CPFP. + // See, https://github.com/lightningnetwork/lightning-rfc/pull/688#issuecomment-549480380 + bool fSpendsTaggedOutput = true; + for (const CTxIn& txin : tx.vin) { + if (txin.prevout.n != 0) { + fSpendsTaggedOutput = false; + break; + } + } + + if (!fSpendsTaggedOutput || nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || + !m_pool.CalculateMemPoolAncestors(*entry, setAncestors, 2, m_limit_ancestor_size, m_limit_descendants + 1, m_limit_descendant_size + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", errString); } } diff --git a/test/functional/mempool_package_onemore.py b/test/functional/mempool_package_onemore.py index 0739d7e29b373..f2339e544e384 100755 --- a/test/functional/mempool_package_onemore.py +++ b/test/functional/mempool_package_onemore.py @@ -52,8 +52,8 @@ def run_test(self): # 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 + (txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [vout], value, fee, 3) + vout = 1 value = sent_value chain.append([txid, value]) for _ in range(MAX_ANCESTORS - 4): @@ -68,20 +68,22 @@ def run_test(self): # Adding one more transaction on to the chain should fail. assert_raises_rpc_error(-26, "too-long-mempool-chain, too many unconfirmed ancestors [limit: 25]", 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, too many descendants", self.chain_transaction, self.nodes[0], [chain[2][0]], [1], chain[2][1], fee, 1) - assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[1][0]], [1], chain[1][1], fee, 1) + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[2][0]], [0], chain[2][1], fee, 1) + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[1][0]], [0], 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, too many descendants", 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, too many descendants", self.chain_transaction, self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 350) + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[0][0], second_chain], [0, 0], chain[0][1] + second_chain_value, fee, 1) + # ...especially if it chains directly off the first transaction but > 40k weight + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[0][0]], [0], chain[0][1], fee, 350) + # ... especially if it chains directly off the first transaction but not on the first output + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[0][0]], [2], chain[0][1], fee, 1) # But not if it chains directly off the first transaction - (replacable_txid, replacable_orig_value) = self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1) + (replacable_txid, replacable_orig_value) = self.chain_transaction(self.nodes[0], [chain[0][0]], [0], 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) # Make sure we can RBF the chain which used our carve-out rule second_tx_outputs = {self.nodes[0].getrawtransaction(replacable_txid, True)["vout"][0]['scriptPubKey']['addresses'][0]: replacable_orig_value - (Decimal(1) / Decimal(100))} - second_tx = self.nodes[0].createrawtransaction([{'txid': chain[0][0], 'vout': 1}], second_tx_outputs) + second_tx = self.nodes[0].createrawtransaction([{'txid': chain[0][0], 'vout': 0}], second_tx_outputs) signed_second_tx = self.nodes[0].signrawtransactionwithwallet(second_tx) self.nodes[0].sendrawtransaction(signed_second_tx['hex'])