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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
765+ setAncestors.clear();
766+ // If the new transaction is relatively small (up to 40k weight)
767+ // and has at most one ancestor (ie ancestor limit of 2, including
768+ // the new transaction), allow it if its parent has exactly the
769+ // descendant limit descendants.
770+ if (nSize <= 10000 && !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, 1000000, nLimitDescendants + 1, nLimitDescendantSize + 10000, errString)) {
@TheBlueMatt I have a spill-over question from the PR review club which covered this PR last week. I am unsure of why the LimitAncestorSize for the carve-out tx has been increased to the block-size.
EDIT: @harding has helped tidy up my understanding of this:
In the case of a (to-be-fee-bumped) LN parent TX with two hooks (A and B), which is close or equal to the descendant-size-limit, only a single (carve-out) child can be accepted, since the initial package size limit has been reached with the parent TX alone. This would seem like a race between A and B for the carve-out.
In regards to the 1M ancestor-size-limit for the carve-out, is it increased to 1M to prevent the ancestor-size-limit from interfering with the carve-out? I suppose this prevents a custom ancestor-size-limit setting from blocking the carve-out and subsequent network propagation.
74+ # ...even if it chains on to two parent transactions with one in the chain.
75+ 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)
76+ # But not if it chains directly off the first transaction
77+ self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1)
78+ # and the second chain should work just fine
79+ self.chain_transaction(self.nodes[0], [second_chain], [0], second_chain_value, fee, 1)
I don’t think this addresses the edge case that we want. The line you added above:
assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 350)
Fails both the nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT and !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, errString) checks. We want a test that differentitates just the weight > 40k case.
0@@ -0,0 +1,87 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2014-2019 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test descendant package tracking carge-out allowing one final transaction in
23+ def skip_test_if_missing_module(self):
24+ self.skip_if_no_wallet()
25+
26+ # Build a transaction that spends parent_txid:vout
27+ # Return amount sent
28+ def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs):
self: could be made a function?
29+ send_value = satoshi_round((value - fee)/num_outputs)
30+ inputs = []
31+ for (txid, vout) in zip(parent_txids, vouts):
32+ inputs.append({'txid' : txid, 'vout' : vout})
33+ outputs = {}
34+ for i in range(num_outputs):
for _ is more idiomatic when _ is unused :-) Applies also for the other for i:s below :-)
760@@ -761,7 +761,14 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
761 size_t nLimitDescendantSize = gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000;
762 std::string errString;
763 if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) {
764- return state.DoS(0, false, REJECT_NONSTANDARD, "too-long-mempool-chain", false, errString);
765+ setAncestors.clear();
766+ // If the new transaction is relatively small (up to 40k weight)
767+ // and has at most one ancestor (ie ancestor limit of 2, including
768+ // the new transaction), allow it if its parent has exactly the
769+ // descendant limit descendants.
Concept Ack.
After much discussion, this does seem to be the minimal-intervention fix. Thanks Matt.
Concept ACK.
I did some review and light testing, and I noticed that there seems to be a general problem with our RBF logic: if we have some parent transaction whose descendant count is maxed out in the mempool, then we’re unable to rbf any child transaction, because we evaluate the descendant limits of a new transactions ancestors prior to looking at what might be evicted by that transaction. (However, we can generally RBF the parent itself!)
So in particular, that means that the new transaction that would be carved out by this policy would not be able to be RBF’ed in the situation that the parent transactions’ descendant limit has been reached.
I’m guessing we may want to fix that to provide additional footgun protection to users that would take advantage of this new behavior, though I’ll leave it to the PR author to decide if it is worth doing in this PR or separately.
Concept ACK. Might be worth adding an explicit test case for RBF case @sdaftuar points out:
# 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)
+
+ # Sadly, RBFing that should fail, however
+ assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, 0, [chain[0][0]], [1], chain[0][1], fee*2, 1)
+
# and the second chain should work just fine
self.chain_transaction(self.nodes[0], [second_chain], [0], second_chain_value, fee, 1)
(means createrawtransaction needs to be called with replaceable=True as well)
Could change chain_transaction(self, node, ...) to always use node=self.nodes[0] which would simplify calls, and deal with @practicalswift’s nit.
Could define a const for the 10k magic number. Is there any reason to have 1M instead of reusing nLimitAncestorSize? (Could not allow the carve out at all if nLimitAncestors <= 1 though that’s probably getting a bit ridiculous)
I think this should work for two-party lightning channels, because the outputs will be a bunch of CSV-limited ones that won’t be in the mempool, and one CPFP output for each party. So at most one output can be spammed, and that only up to the descendent limit, so the plus-one here should work fine. If there were three or more immediately-spendable outputs (for channel factories and multiparty eltoo eg), I don’t think this would be enough. But that’s fine today.
I guess it’s not very useful to log when this carve out triggers – it’s meant to prevent attacks from being effective, so while it’s there there won’t be any attacks so it’ll hardly ever be used, but if it weren’t there there would be attacks and it would be useful.
Should this carve out be limited to only apply when it’s actually raising the parent’s effective fee rate? I think you’d have to loop over all the descendents of your parent, work out the best ancestor fee rate of any of them, then see if the new tx and the parent have a better combined fee rate than that. That seems like it would add a chunk of complexity though, so probably isn’t worth it.
This seems like it’s a sufficient workaround for current problems, and is a pretty minimal change, while doing anything much better would be lots more complicated. So: Approach ACK.
16+MAX_DESCENDANTS = 25
17+
18+class MempoolPackagesTest(BitcoinTestFramework):
19+ def set_test_params(self):
20+ self.num_nodes = 2
21+ self.extra_args = [["-maxorphantx=1000"], ["-maxorphantx=1000", "-limitancestorcount=5"]]
620+ // This allows protocols which rely on distrusting counterparties
621+ // being able to broadcast descendants of an unconfirmed transaction
622+ // to be secure by simply only having two immediately-spendable
623+ // outputs - one for each counterparty. For more info on the uses for
624+ // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
625+ if (nSize > 10000 || !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, 1000000, nLimitDescendants + 1, nLimitDescendantSize + 10000, errString)) {
23+ def skip_test_if_missing_module(self):
24+ self.skip_if_no_wallet()
25+
26+ # Build a transaction that spends parent_txid:vout
27+ # Return amount sent
28+ def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs):
54+ for _ in range(4):
55+ (txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [vout], value, fee, 2)
56+ vout = 0
57+ value = sent_value
58+ chain.append([txid, value])
59+ for _ in range(MAX_ANCESTORS - 4):
Having two basically identical loops just to add an extra output to the first four transactions seems like an odd choice. Maybe prefer
0for depth in range(MAX_ANCESTORS):
1 num_outputs = 2 if depth < 4 else 1
2 ...
60+ (txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [0], value, fee, 1)
61+ value = sent_value
62+ chain.append([txid, value])
63+ (second_chain, second_chain_value) = self.chain_transaction(self.nodes[0], [utxo[1]['txid']], [utxo[1]['vout']], utxo[1]['amount'], fee, 1)
64+
65+ # Check mempool has MAX_ANCESTORS transactions in it, and descendant and ancestor
67+ assert_equal(len(self.nodes[0].getrawmempool(True)), MAX_ANCESTORS + 1)
68+
69+ # Adding one more transaction on to the chain should fail.
70+ assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [txid], [0], value, fee, 1)
71+ # ...even if it chains on from some point in the middle of the chain.
72+ assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[2][0]], [1], chain[2][1], fee, 1)
57+ value = sent_value
58+ chain.append([txid, value])
59+ for _ in range(MAX_ANCESTORS - 4):
60+ (txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [0], value, fee, 1)
61+ value = sent_value
62+ chain.append([txid, value])
Imo, tests below would be more reable if this were
0chain.append(txid)
1values.append(value)
to get rid of all the 0/1 indexing
ACK f1facdd3e27d3b5c0536ab27d1f928984b8ccc25 with minimal testing. I verified new test fails without policy change and succeeds with it. I left only minor review comments which can be ignored.
I do like ajtowns’s RBF test from #15681 (comment), and think it’d be nice if that were added.
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
@ajtowns I’d prefer to not add the proposed test change, as @sdaftuar’s comment points out a critical limitation, and one I’d like to get resolved before this gets released, though not necessarily in this PR. I’d rather just fix the bug than add a test to ensure that the bug exists.
As for general policy questions: this doesn’t result in any new transactions being accepted which violate min fee, nor does it change eviction criteria when mempool fills. What this does do, is tweak our already-somewhat-arbitrary-but-sadly-neccessary anti-DoS policies to allow one more (potentially-in-the-future) common case so that we can maximize fee revenue in the case this does get used. Thus, I don’t see any reason to need to bend over backwards to make it more restrictive, if anything, less restrictive is better.
ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced – looked over code again, compared with previous commit, compiles, etc.
I think this is a sufficient improvement even with the problem @sdaftuar points out:
It would obviously still be better to find a more general fix, but this seems to me a worthwhile improvement without introducing much of a maintenance cost.
Patch to mempool_package_onemore.py for checking the “can RBF if I’ve added too many descendants myself” case. Maybe useful for reviewers, not suitable for inclusion since it kills the checks for the intended usecase:
0+++ b/test/functional/mempool_package_onemore.py
1@@ -33,7 +33,7 @@ class MempoolPackagesTest(BitcoinTestFramework):
2 outputs = {}
3 for i in range(num_outputs):
4 outputs[node.getnewaddress()] = send_value
5- rawtx = node.createrawtransaction(inputs, outputs)
6+ rawtx = node.createrawtransaction(inputs, outputs, 0, True)
7 signedtx = node.signrawtransactionwithwallet(rawtx)
8 txid = node.sendrawtransaction(signedtx['hex'])
9 fulltx = node.getrawtransaction(txid, 1)
10@@ -74,6 +74,12 @@ class MempoolPackagesTest(BitcoinTestFramework):
11 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)
12 # ...especially if its > 40k weight
13 assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 350)
14+
15+ # can even RBF the original tx
16+ self.chain_transaction(self.nodes[0], [chain[0][0]], [0], chain[0][1], fee*25, 10)
17+ assert_equal(len(self.nodes[0].getrawmempool(True)), 3) # parent, second_chain, RBF'd original
18+ return
19+
20 # But not if it chains directly off the first transaction
21 self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1)
22 # and the second chain should work just fine
621+ // being able to broadcast descendants of an unconfirmed transaction
622+ // to be secure by simply only having two immediately-spendable
623+ // outputs - one for each counterparty. For more info on the uses for
624+ // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
625+ if (nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
626+ !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, errString)) {
errString through to the second call of CalculateMemPoolAncestors() means that the error string in the CValidationState object will be incorrect because of the changed ancestor/descendant size/count parameters. Really we should declare a dummy_error_string to pass into the second call, and use the original errString in the call to state.Invalid() below.
0@@ -0,0 +1,86 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2014-2019 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test descendant package tracking carve-out allowing one final transaction in
6+ an otherwise-full package as long as it has only one parent and is <= 10k in
7+ size.
41+ return (txid, send_value)
42+
43+ def run_test(self):
44+ # Mine some blocks and have them mature.
45+ self.nodes[0].generate(101)
46+ utxo = self.nodes[0].listunspent(10)
utxos or define it as self.nodes[0].listunspent(10)[0]
13+from test_framework.util import assert_equal, assert_raises_rpc_error, satoshi_round
14+
15+MAX_ANCESTORS = 25
16+MAX_DESCENDANTS = 25
17+
18+class MempoolPackagesTest(BitcoinTestFramework):
MempoolPackageOnemoreTest
Quick code-review ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced.
There’s one logging bug that I think we should fix. I’ve added a bunch of nits to the test, but I agree with other reviewers that we could probably avoid a bunch of test code duplication by merging mempool_package_onemore.py with mempool_packages.py.
