mempool: disallow txns under min relay fee, even in packages

glozow commented at 1:33 pm on January 20, 2023: member

Part of package relay, see #27463.

Note that this still allows packages to bump transactions that are below the dynamic mempool minimum feerate, which means this still solves the “mempool is congested and my presigned 1sat/vB tx is screwed” problem for all transactions.

On master, the package policy (only accessible through regtest-only RPC submitpackage) allows 0-fee (or otherwise below min relay feerate) transactions if they are bumped by a child. However, with default package limits, we don’t yet have a DoS-resistant way of ensuring these transactions remain bumped throughout their time in the mempool. Primarily, the fee-bumping child may later be replaced by another transaction that doesn’t bump the parent(s). The parent(s) could potentially stay bumped by other transactions, but not enough to ever be selected by the BlockAssembler (due to blockmintxfee).

For example, (tested here):

The mempool accepts 24 below-minrelayfeerate transactions (“0-fee parents”), all bumped by a single high-fee transaction (“the fee-bumping child”). The fee-bumping child also spends a confirmed UTXO.
Two additional children are added to each 0-fee parent. These children each pay a feerate slightly above the minimum relay feerate (e.g. 1.9sat/vB) such that, for each 0-fee parent, the total fees of its two children divided by the total size of the children and parent is above the minimum relay feerate.
If a block template is built now, all transactions would be selected.
A transaction replaces the the fee-bumping child, spending only the confirmed UTXO and not any of the outputs from the 0-fee parents.
The 0-fee parents now each have 2 children. Their descendant feerates are above minrelayfeerate, which means that they remain in the mempool, even if the mempool evicts all below-minrelayfeerate packages.
If a block template is built now, none of the 0-fee parents or their children would be selected.
Even more low-feerate descendants can be added to these below-minrelayfeerate packages and they will not be evicted until they expire or the mempool reaches capacity.

Unless we have a DoS-resistant way of ensuring package CPFP-bumped transactions are always bumped, allowing package CPFP to bump below-minrelayfeerate transactions can result in these problematic situations. See #27018 which proposes a partial solution with some limitations, and contains discussion about potential improvements to eviction strategy. While no adequate solution exists, for now, avoid these situations by requiring all transactions to meet min relay feerate.

glozow added the label Mempool on Jan 20, 2023

DrahtBot commented at 1:33 pm on January 20, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, ajtowns
Concept ACK	naumenkogs, sdaftuar

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

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.

glozow commented at 1:52 pm on January 20, 2023: member

CC @sdaftuar @ajtowns @instagibbs

ajtowns commented at 7:38 am on January 23, 2023: contributor

We can remove these transactions in TrimToSize(), but 2500 is not within our acceptable limits.

I’m not sure I see why this is a problem? Can’t we have the situation where 1000s of txs get evicted anyway, simply by accepting a high fee rate 400kb tx that then causes a large number of small min feerate txs (with no unconfirmed ancestors/descendants) to be evicted due to hitting the mempool limit? (At least evicting ~350 otherwise unrelated txs that way seems possible – simple 1-in 1-out txs seem to contribute ~1150 bytes to mempool usage each for me, so 350 of them should use about 402kB)

(what if the 0-fee ancestor is bumped by something else?)

We essentially have to do the work to see if the ancestors are bumped by something else anyway, whether they’re 0-fee or not (when we evict the conflicted descendant; its ancestors’ descendant fee rates get updated). When we get to TrimToSize, we just look at the already calculated descendant score, and evict the low ones. But the logic there doesn’t change if the lowest fee rate is 0 or 1 sat/vb?

06:30 < sdaftuar> i think my concern is that the mempool shouldn’t have things in it that would never get mined, eg because they’re below minrelay fee (which we compare against in the mining code iirc)

Wouldn’t it make more sense to address that directly, ie by having TrimToSize trim txs whose descendant score is below min relay fee, even if the mempool isn’t full? Then txs who lose their CPFP child will get trimmed essentially immediately, no matter how they got in, and no matter why the parent might have been removed?

I don’t mind the concept here (which I summarise as “only allow txs below min relay fee via some scheme that deliberately limits the complexity”), but it seems more like a “here’s another special case” instead of “here’s a simple rule that’s easy for everyone to think about”.

glozow commented at 9:34 am on January 23, 2023: member

Slightly out of order:

Wouldn’t it make more sense to address that directly, ie by having TrimToSize trim txs whose descendant score is below min relay fee, even if the mempool isn’t full?

btw I have this as the next commit (https://github.com/bitcoin/bitcoin/pull/25038/commits/898847e1907eda3d87d19ea43099b71d9eded5f4 or “[mempool] evict everything below min relay fee in TrimToSize()” commit in #25038). My concern was whether it would be problematic to have many such transactions to trim, hence these changes beforehand. If we instead decide that we’re ok with this, I’m happy to drop these.

Somewhat relevant, I’d also wonder if we should add a TrimToSize() at the end of reorgs. Same reason, so we don’t add below-minrelayfee from disconnected blocks. And same concern, that it could make the process slower.

I’m not sure I see why this is a problem? Can’t we have the situation where 1000s of txs get evicted anyway, simply by accepting a high fee rate 400kb tx that then causes a large number of small min feerate txs (with no unconfirmed ancestors/descendants) to be evicted due to hitting the mempool limit?

Yes, it is definitely not otherwise impossible to be evicting 1000s of transactions in TrimToSize(). But since package cpfp would be a new way of permitting such transactions, I’d rather lean towards being conservative?

sdaftuar commented at 10:47 am on January 23, 2023: member

Wouldn’t it make more sense to address that directly, ie by having TrimToSize trim txs whose descendant score is below min relay fee, even if the mempool isn’t full?

This is insufficient for preventing transactions that will never be mined from staying in the mempool, because TrimToSize would leave transactions in the mempool if a parent and multiple children appear to have enough fee for the combined package size, even though no single child has an ancestor fee rate above the min relay fee (eg a “children-pay-for-parent” scenario).

So I think while having a call to TrimToSize shouldn’t hurt (and would be needed in the case of v3), we seem to need an approach like this PR to avoid the problem generally.

sdaftuar commented at 9:06 pm on January 23, 2023: member

Concept ACK.

ajtowns commented at 9:09 pm on January 23, 2023: contributor

@glozow

My concern was whether it would be problematic to have many such transactions to trim, hence these changes beforehand. If we instead decide that we’re ok with this, I’m happy to drop these.

I’m not sure that we shouldn’t be concerned about using up too much CPU in such cases; just that if we are, there are other analogous cases we should be concerned about too… @sdaftuar

This is insufficient for preventing transactions that will never be mined from staying in the mempool, because ‘TrimToSize’ would leave transactions in the mempool if a parent and multiple children appear to have enough fee for the combined package size, even though no single child has an ancestor fee rate above the min relay fee (eg a “children-pay-for-parents scenario).

Right, but that’s a reflection of us using ancestor score for mining, but descendant score for mempool removal… So presumably we’d get similar behaviour even if min relay fee is 0 – ie, if we have a parent/children relationship like that where the parent has a high descendant score but the children have low ancestor scores, then they’ll remain in the mempool while we evict txs with a fee rate in between those values due to the mempool being full, even if it turns out those txs would be mined while the child txs would not. (Though the key value there is what people are using for -blockmintxfee not the min relay fee per se…)

Replacing descendant score is a big, possibly impossible, project though, so Concept ACK on working around it in this way in the meantime. I think it would be better to be much clearer on exactly what this is achieving though.

naumenkogs commented at 1:57 pm on January 24, 2023: member

Concept ACK

glozow commented at 12:16 pm on January 30, 2023: member

Replacing descendant score is a big, possibly impossible, project though, so Concept ACK on working around it in this way in the meantime. I think it would be better to be much clearer on exactly what this is achieving though.

Agreed. I’ve rewritten the description to focus on the fact that we can end up with descendant packages of transactions that would never be mined but still get to hang out in the mempool. I’ve also written a test to demonstrate this here, on top of the “evict everything below min relay feerate in TrimToSize()” commit (to show it doesn’t fix this issue), and added a description of this test to the OP so it would be in the commit log. I could adapt the test for this PR, but most of it would not be executed when the changes are in place, so I’m not sure how meaningful it would be.

in src/test/util/setup_common.cpp:447 in 522cb10f2c outdated

442+    CMutableTransaction mtx = CMutableTransaction();
443+    mtx.vin.push_back(CTxIn{COutPoint{InsecureRand256(), 0}});
444+    mtx.vout.push_back(CTxOut(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE))));
445+    const auto tx{MakeTransactionRef(mtx)};
446+    LockPoints lp;
447+    CFeeRate tx_feerate(target_feerate);

sdaftuar commented at 2:52 pm on January 31, 2023:

micro-nit: I don’t think we need this new variable? Could just use target_feerate.GetFee(...) in the next line…

glozow commented at 10:32 am on February 1, 2023:

Thanks, fixed

in src/validation.cpp:852 in fd2b3c8c9b outdated

818@@ -819,9 +819,14 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
819         return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops",
820                 strprintf("%d", nSigOpsCost));
821 
822-    // No individual transactions are allowed below the min relay feerate and mempool min feerate except from
823-    // disconnected blocks and transactions in a package. Package transactions will be checked using
824-    // package feerate later.
825+    // No individual transactions are allowed below the min relay feerate except from disconnected blocks.
826+    if (!bypass_limits && ws.m_modified_fees < m_pool.m_min_relay_feerate.GetFee(ws.m_vsize)) {

sdaftuar commented at 3:55 pm on January 31, 2023:

Maybe worth expanding this comment slightly to explain why this check is here (especially since this check is partially duplicated in the check at line 830 below).

glozow commented at 10:32 am on February 1, 2023:

Added a bit about how this check is different from CheckFeeRate

in test/functional/mempool_limit.py:108 in d32f445ab9 outdated

103+        poor_parent_result = submitpackage_result["tx-results"][tx_poor["wtxid"]]
104+        child_result = submitpackage_result["tx-results"][tx_child["tx"].getwtxid()]
105+        assert_fee_amount(poor_parent_result["fees"]["base"], tx_poor["tx"].get_vsize(), relayfee)
106+        assert_equal(rich_parent_result["fees"]["base"], 0)
107+        assert_equal(child_result["fees"]["base"], DEFAULT_FEE)
108+        # The "rich" parent does not require CPFP so its effective feerate.

sdaftuar commented at 8:17 pm on January 31, 2023:

nit: I think this comment needs some correction, doesn’t parse for me.

glozow commented at 10:33 am on February 1, 2023:

Thanks, fixed

sdaftuar approved

sdaftuar commented at 8:21 pm on January 31, 2023: member

Code review ACK. Only lightly reviewed the test changes. Left some non-blocking nits.

in src/test/txpackage_tests.cpp:663 in d32f445ab9 outdated

661@@ -658,7 +662,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
662     // Package feerate is calculated using modified fees, and prioritisetransaction accepts negative
663     // fee deltas. This should be taken into account. De-prioritise the parent transaction by -1BTC,

naumenkogs commented at 10:25 am on February 1, 2023:

d32f445ab9f9a65d7bd0eec828b8a3de0d07d635 the -1BTC comment is irrelevant now? (i guess it’s technically still relevant, but became more confusing)

glozow commented at 10:37 am on February 1, 2023:

Amended to remove the -1BTC

glozow force-pushed on Feb 1, 2023

in test/functional/mempool_limit.py:91 in 490f3c1a8e outdated

82@@ -76,6 +83,44 @@ def run_test(self):
83         self.log.info('Create a mempool tx that will not pass mempoolminfee')
84         assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee)
85 
86+        self.log.info("Check that submitpackage allows cpfp of a parent below mempool min feerate")
87+        node = self.nodes[0]
88+        peer = node.add_p2p_connection(P2PTxInvStore())
89+
90+        # Package with 2 parents and 1 child. One parent pays for itself using modified fees, and
91+        # another has 0 fees but is bumped by child.

naumenkogs commented at 11:11 am on February 1, 2023:

490f3c1a8eabac737e196e7fffd7765aa941f65a I assume another parent is tx_poor, meaning it pays relayfee rather than 0?

glozow commented at 1:25 pm on February 1, 2023:

Thanks, I’ve updated this comment along with a few others.

in src/validation.cpp:852 in fce52c8004 outdated

825+    // No individual transactions are allowed below the min relay feerate except from disconnected blocks.
826+    // This requirement, unlike CheckFeeRate, cannot be bypassed using m_package_feerates because,
827+    // while a tx could be package CPFP'd when entering the mempool, we do not have a DoS-resistant
828+    // method of ensuring the tx remains bumped. For example, the fee-bumping child could disappear
829+    // due to a replacement.
830+    if (!bypass_limits && ws.m_modified_fees < m_pool.m_min_relay_feerate.GetFee(ws.m_vsize)) {

naumenkogs commented at 11:18 am on February 1, 2023:

fce52c8004744b8af96ff07b419db21979348330 Why using modifiedFees in this check instead of regular fees?

glozow commented at 11:21 am on February 1, 2023:

We always use modified fees for these checks, otherwise we’re breaking the prioritisetransaction RPC’s expected behavior

glozow force-pushed on Feb 1, 2023

sdaftuar commented at 4:10 pm on February 1, 2023: member

Gave this more thought… I think I have a better solution to this problem:

Fundamentally the issue with txs below the minrelayfee in the mempool is a problem because (a) the mining code refuses to select ancestor packages below the blockmintxfee, and (b) TrimToSize() necessarily operates on descendant packages and cannot detect when this is the case.

There seems to be basically no good reason anymore for the mining code to not mine whatever makes it in to the mempool. Philosophically, we ought to ensure that whatever gets into the mempool meets the miner’s feerate criteria, and then just mine whatever is there. Doing otherwise opens up possibilities for DoS, if it’s possible to fill the mempool with transactions that would never be selected.

So instead I think an easier solution to this problem is to (a) drop the blockmintxfee feerate check in the mining code, and (b) modify TrimToSize() to evict descendant packages below the minrelayfee, which should be called whenever there is a reorg or whenever a transaction is added to the mempool (which could be an RBF that leaves a dangling 0-fee transaction lying around). The result of these two changes is that we might pick up a transaction or package in block creation that appears to be below the minrelayfee, but whenever we do so it should open up the door to being able to include transactions that bring the cumulative feerate up to above the minrelayfee, because we’re ensuring that every transaction has a total descendant package that is sufficiently high feerate to be mined. I think this should be ok?

Anyway, one reason I like this alternate approach better is that this PR leaves open a possibility of 0-fee transactions being added to the mempool as a result of a reorg, and I was able to sketch out a possible DoS that takes advantage of seeding the mempool with 0-fee transactions and then using those to fill the mempool up with transactions that would both never be mined and be expensive to evict (via TrimToSize()), which basically would cause the relay fees for everyone on the network to be much higher than they should be. I’m not sure this is a very practical attack, but given that it would still exist I think an alternate approach that covers this case (with no downsides I can see) seems better…

sdaftuar commented at 4:12 pm on February 1, 2023: member

Also, I think it would make sense to drop the blockmintxfee parameter altogether. I think that variable is a holdover from the days when we supported priority transactions, but today it makes no sense to have transactions in your mempool you would never mine (eg if blockmintxfee > minrelayfee), and if you set blockmintxfee to be below minrelayfee, then it mostly has no effect, because how would such low fee transactions make it into your mempool in the first place?

instagibbs commented at 4:54 pm on February 1, 2023: member

@sdaftuar sorry can you spell out what exactly the proposal is by walking through the “24 below-minrelayfeerate transactions” case, and why that’s not a problem?

sdaftuar commented at 5:13 pm on February 1, 2023: member

@sdaftuar sorry can you spell out what exactly the proposal is by walking through the “24 below-minrelayfeerate transactions” case, and why that’s not a problem?

Sure, the proposal is twofold: (1) get rid of any feerate checks in the mining code (ie remove the comparison to blockmintxfee in addPackageTransactions), and (2) modify TrimToSize() so that we evict any descendant package from the mempool if the package is below the minrelayfee. (Technically we need to also invoke this function on the mempool after a reorg, so maybe that’s 3 things, but conceptually it’s the same idea – whenever we might have changed the mempool in a way that could have added a 0-fee transaction, run TrimToSize() to evict it so that we limit junk from being relayed on the network.)

I believe the scenario you’re asking about is one where we have a zero-fee parent in the mempool, with a bunch of children that are all above the minrelayfee themselves, but each one, when taken with the parent will have an ancestor feerate below the minrelayfee. Originally I put this scenario out there as a DoS vector, because the mining code currently will not select any such transactions. However if we change the mining code to skip the relay fee check, then this whole package will eventually get included in a block instead, which means that there is no free relay (because all those fee-paying transactions will be mined). Moreover, if the descendant package in total has a feerate greater than the minrelayfee, then I think apart from edge cases around which transactions make it in at the end of a block, this is basically achieving the miner’s goals anyway of including transactions that in total have a high enough feerate.

If we didn’t include the TrimToSize() modification to evict descendant packages below the minrelayfee, then there could be an issue where 0-fee transactions might get included in a block, which ought to be strictly worse for a miner than omitting such transactions (because they take longer to validate than a block without the transaction, and there’s no additional benefit to the miner for including them) – hence that is an important part of the change as well.

naumenkogs commented at 7:04 am on February 2, 2023: member

I reviewed the code f636daf4c89da7b2b72d05e73de1ca86449a36a6 and it seems correct. However, I think the other approach #27018 makes more sense. Not just because of the particular reorg issue, but in general it seems to cover more mempool garbage situations. @glozow I have two questions though.

In #27018 you say

Though they don’t need to be mutually exclusive, e.g. we could both require individual transactions to meet minrelayfeerate and also evict below-minrelayfeerate entries in TrimToSize().

What would be a practical benefit of this PR (#26933) if #27018 is already merged? (e.g., better UX, attack mitigated, cleaner code, etc.).

If only #27018 is merged and this one is not, what would change to v3 and ephemeral anchors?

glozow commented at 9:45 am on February 2, 2023: member

What would be a practical benefit of this PR (https://github.com/bitcoin/bitcoin/pull/26933) if #27018 is already merged? (e.g., better UX, attack mitigated, cleaner code, etc.).

My reservation with a #27018-only approach is we haven’t answered whether we’re ok with TrimToSize() (and thus reorgs and RBFs) potentially evicting many entries (I’lll re-post the 2500-tx scenario on that PR).

My original intent was to do this PR + evict everything below minrelayfeerate in TrimToSize(), which I think partially (?) addresses the problem with seeding mempool in a reorg (you can’t bump both the reorged tx + a large 0-fee tx with 1-parent-1-child v3, and you can’t attach multiple descendants to anything v3).

If only #27018 is merged and this one is not, what would change to v3 and ephemeral anchors?

Nothing is really changing with v3 regardless. If we do this PR, only v3 transactions can be below minrelayfeerate and bumped through package CPFP. If we don’t do this PR, v3 still has the same rules, it’s just that below-minrelayfeerate is no longer exclusive to v3.

instagibbs commented at 2:35 pm on February 2, 2023: member

Did some naive benchmarking with #27019 and on my basic laptop got 0.72 seconds for evicting 100 packages of size 25 each (2500 txns total).

edit: Using not very scientific methods of repeating the bench with and without eviction steps, it appears to roughly take half that amount of time for the evictions themselves, regardless of topology. ~0.33s

ajtowns commented at 9:05 am on February 4, 2023: contributor

This is insufficient for preventing transactions that will never be mined from staying in the mempool, because TrimToSize would leave transactions in the mempool if a parent and multiple children appear to have enough fee for the combined package size, even though no single child has an ancestor fee rate above the min relay fee (eg a “children-pay-for-parent” scenario).

Would this be fixable by having TrimToSize remove any tx whose ancestor fee rate is below the minrelay fee if it also has no descendants (m_children.empty()) ? You could probably merge that into the descendant feerate index, just by pretending the descendant fee rate is zero if both those conditions are true.

sdaftuar commented at 10:07 am on February 4, 2023: member

@ajtowns I think that gets tricky due to sibling pays for parent situations; in fact I think you could evict something under that criteria in situations where no RBF or anything strange was involved at all:

Package (A, B) accepted to mempool, A is 0 fee and B pays for A.

Package (C, D) is accepted to mempool, C is 0 fee, and D pays for C AND D is a child of A.

D could have an ancestor feerate less than the minrelayfee (if it doesn’t pay for A and C), but nothing wrong with it being in the mempool.

ajtowns commented at 1:15 am on February 10, 2023: contributor

@ajtowns I think that gets tricky due to sibling pays for parent situations; in fact I think you could evict something under that criteria in situations where no RBF or anything strange was involved at all:

Maybe simplifying it, and just have TrimToSize remove any tx with no descendants whose modified fee is below minrelay? (Again, by setting the artificially treating descendantscore as -infinity in that case) Certainly doesn’t cover every case, but would remove anything below minfee that only made it into the mempool via a CPFP that later got evicted?

(The point being to avoid that being a way of getting free-relay while mempools are mostly empty)

instagibbs commented at 4:03 pm on February 10, 2023: member

@ajtowns Trimming anything having a desc feerate of 0 should also be ok along those same lines?

sdaftuar commented at 4:42 pm on April 4, 2023: member

I think in the interests of moving the package relay and v3 project forward, it makes sense to adopt this approach and defer potential improvements like #27018 to the future. My thinking is that this PR is a conservative way forward that seems obviously safe; our eviction logic on the other hand has a number of shortcomings, which I think will be best to fix up first before making more changes to use the eviction logic in new scenarios. We can work on the mempool and eviction in parallel and (hopefully) once we have something that works better, we can revisit this limitation and see if we can lift it in the future.

So (re-?) concept ACK… Will try to give this a review again soon.

glozow force-pushed on Apr 5, 2023

glozow commented at 10:37 am on April 5, 2023: member

rebased, there was a silent merge conflict

in src/test/txpackage_tests.cpp:734 in 85c1cd0673 outdated

733                                                          /*output_destination=*/child_spk,
734-                                                         /*output_amount=*/coinbase_value - 200, /*submit=*/false);
735+                                                         /*output_amount=*/coinbase_value - 800, /*submit=*/false);
736     CTransactionRef tx_child_cheap = MakeTransactionRef(mtx_child_cheap);
737     package_still_too_low.push_back(tx_child_cheap);
738+    BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*tx_parent_cheap)) <= 800);

sdaftuar commented at 1:03 pm on April 11, 2023:

Not sure if I’m just reading too fast, but was the intent here to check that the mempool minfee for the child is <= 800? Seems like we did the parent checks up above, but maybe I’m missing something…?

glozow commented at 11:10 am on April 12, 2023:

Yes you’re right, this should be checking the child. Edited this to be checking that “This would be enough to pay for the child alone.”

glozow force-pushed on Apr 12, 2023

instagibbs commented at 2:24 pm on April 12, 2023: member

concept ACK

I find both variants unsatisfactory for different reasons, but while we’re working on something potentially better, I think this small change is the most reasonable, as it can be removed simply if improved upon. Meanwhile, we can continue moving forward with package relay/RBF/et al.

ajtowns commented at 3:32 pm on April 12, 2023: contributor

I guess I don’t really understand how this PR on its own helps anything – we don’t do package relay yet, so except for a regtest only rpc, these changes should be unobservable?

Would it make sense to replace this with a PR that enabled package relay without v3 or package RBF, and included this policy? I think that would be the first ~10 commits from #25038? That seems like it would be more meaningful progress…

instagibbs commented at 4:01 pm on April 12, 2023: member

I think either way is “meaningful progress”, but I’m not averse to coupling the commits if we’re in agreement on this PR’s commits.

Having this in would also allow #26711 to move forward

in src/test/util/setup_common.cpp:454 in 359b1ff9bb outdated

448+    CMutableTransaction mtx = CMutableTransaction();
449+    mtx.vin.push_back(CTxIn{COutPoint{g_insecure_rand_ctx.rand256(), 0}});
450+    mtx.vout.push_back(CTxOut(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE))));
451+    const auto tx{MakeTransactionRef(mtx)};
452+    LockPoints lp;
453+    const auto tx_fee = target_feerate.GetFee(GetVirtualTransactionSize(*tx)) -

instagibbs commented at 5:06 pm on April 12, 2023:

I’m confused, why is the subtraction of incremental relay total fee necessary?

glozow commented at 5:08 pm on April 12, 2023:

New mempool min feerate is the feerate of removed + incremental: https://github.com/bitcoin/bitcoin/blob/7f4ab67e7be615b2425a85fb65872fc2327f58c3/src/txmempool.cpp#L1063-L1068

instagibbs commented at 5:10 pm on April 12, 2023:

Ah, adding a comment to this effect helps. Seems really out of place otherwise.

in src/test/txpackage_tests.cpp:568 in 8d2c5b6e5b outdated

565+    // parent3 will be a new transaction. Put a low feerate to make it invalid on its own.
566     auto mtx_parent3 = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[3], /*input_vout=*/0,
567                                                      /*input_height=*/0, /*input_signing_key=*/coinbaseKey,
568                                                      /*output_destination=*/acs_spk,
569-                                                     /*output_amount=*/CAmount(50 * COIN), /*submit=*/false);
570+                                                     /*output_amount=*/CAmount(50 * COIN - 200), /*submit=*/false);

instagibbs commented at 5:22 pm on April 12, 2023:

name the 200 low_fee_amt and reuse everywhere?

in src/test/txpackage_tests.cpp:669 in 8d2c5b6e5b outdated

660@@ -657,9 +661,9 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
661     package_cpfp.push_back(tx_child);
662 
663     // Package feerate is calculated using modified fees, and prioritisetransaction accepts negative
664-    // fee deltas. This should be taken into account. De-prioritise the parent transaction by -1BTC,
665-    // bringing the package feerate to 0.
666-    m_node.mempool->PrioritiseTransaction(tx_parent->GetHash(), -1 * COIN);
667+    // fee deltas. This should be taken into account. De-prioritise the parent transaction
668+    // to bring the package feerate to 0.
669+    m_node.mempool->PrioritiseTransaction(tx_parent->GetHash(), child_value - coinbase_value);

instagibbs commented at 5:29 pm on April 12, 2023:

Could we use CTxMemPool::info on parent and child to assert that the (fee + nFeeDelta) summed == 0 for the test so I don’t have to think so hard to know this is right

glozow commented at 12:47 pm on April 14, 2023:

We can’t because these get rejected and info() only works on transactions that make it into the mempool.

in src/test/txpackage_tests.cpp:767 in 8d2c5b6e5b outdated

764         auto it_parent = submit_prioritised_package.m_tx_results.find(tx_parent_cheap->GetWitnessHash());
765         auto it_child = submit_prioritised_package.m_tx_results.find(tx_child_cheap->GetWitnessHash());
766         BOOST_CHECK(it_parent != submit_prioritised_package.m_tx_results.end());
767         BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID);
768-        BOOST_CHECK(it_parent->second.m_base_fees.value() == 0);
769+        BOOST_CHECK(it_parent->second.m_base_fees.value() == 200);

instagibbs commented at 5:33 pm on April 12, 2023:

use parent_fee

in src/test/txpackage_tests.cpp:771 in 8d2c5b6e5b outdated

769+        BOOST_CHECK(it_parent->second.m_base_fees.value() == 200);
770         BOOST_CHECK(it_parent->second.m_effective_feerate.value() == expected_feerate);
771         BOOST_CHECK(it_child != submit_prioritised_package.m_tx_results.end());
772         BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID);
773-        BOOST_CHECK(it_child->second.m_base_fees.value() == 200);
774+        BOOST_CHECK(it_child->second.m_base_fees.value() == 600);

instagibbs commented at 5:33 pm on April 12, 2023:

use child_fee

glozow commented at 6:04 pm on April 12, 2023: member

Would it make sense to replace this with a PR that enabled package relay without v3 or package RBF, and included this policy? I think that would be the first ~10 commits from #25038? That seems like it would be more meaningful progress…

Right, the changes for package relay only (i.e. no v3, no package RBF) would be this, persist packages across restart, and allow any ancestor package instead of just child-with-parents. Basically this PR + the first 7 commits of #25038 + the original #26711. I’ve now pushed all of it to #26711 so hopefully it’s more clear what that looks like.

Putting more of it in this PR would be more progress of course, but I’m given to believe that less stuff per PR == easier to review. I could add all the commits that are shared between #25038 and #26711 here? Happy to cut the PRs in whichever way makes the most sense to y’all.

instagibbs approved

instagibbs commented at 6:15 pm on April 12, 2023: member

ACK 6fb01d26c57f6af7205721e528bbafee8fa41027

ajtowns commented at 2:26 pm on April 13, 2023: contributor

Right, the changes for package relay only (i.e. no v3, no package RBF) would be this, persist packages across restart, and allow any ancestor package instead of just child-with-parents.

Sorry, I more just mean “what’s the minimal set of additional commits that need this that results in something that’s useful on its own?”

Maybe the current overarching “useful thing” is just “package mempool acceptance works in regtest via rpc in a way that would be acceptable to expose on mainnet for packages put together by random peers”, and that’s already a multi-step thing, and this PR is already a step forward on that path? If so, maybe it would be worth creating an issue with a task list of the steps?

avoid the risk of below minrelaytxfee txs hanging around forever in the mempool
- v1, v2 txs should never be below minrelaytxfee (26933)
- regularly trim mempool txs with descendent fee of 0 (or less), even when the mempool is not full (27018)
- drop the default blockminfee to 0, to avoid mining non-full blocks when there are fee paying txs in the mempool, so that any below minrelaytxfee txs eventually get mined anyway (27018)
apply mempool min fee rules to packages correctly (26711)
when loading a saved mempool, prevent parents from being evicted as they might be cpfp’ed by later transactions that haven’t been loaded yet (26711)
handle arbitrary packages of possibly completely unrelated txs sensibly (26711)
support v3 tx rules? (25038)
support ephemeral anchors? (26403)
support package rbf? (25038)

(huh, abbreviating package relay as “PR” is instantly confusing)

glozow commented at 12:48 pm on April 14, 2023: member

@ajtowns thanks for the feedback, I’m not really sure where to draw the line between useful and not useful but I’ve made a tracking issue #27463. I’ve marked places where I think the outcome is meaningful (e.g. “after this, we could expose on p2p and it would be safe”).

ajtowns commented at 5:51 am on April 17, 2023: contributor

ACK 6fb01d26c57f6af7205721e528bbafee8fa41027

ajtowns commented at 7:36 am on April 17, 2023: contributor

I’m not really sure where to draw the line between useful and not useful

I think “submitpackage” would count as obviously useful if it was available on mainnet – even without package relay, a miner could expose that via a web form to allow people to do cpfp bumps when the (miner’s) mempool is full, eg.

I was viewing that RPC more as “here’s a way of exposing some code we’re working on so we can test it while it’s in development”, but if you look at it as “here’s something that would be useful already, except we don’t want to expose it because if someone submits a sub-minrelayfee tx it’ll clog up their mempool without ever having any possibility of getting relayed”, then this PR fixes that problem.

I think with this PR merged, we could reasonably expose submitpackage outside of regtest, though should keep it debug-only/experimental (after this PR, I think it’s never worse than having 500MB in the mempool and submitting a min mempool fee tx, and we already allow that).

glozow referenced this in commit 2b635eea3f on Apr 17, 2023

glozow force-pushed on Apr 17, 2023

[test util] mock mempool minimum feerate ac463e87df

[test] package cpfp bumps parents <mempoolminfee but >=minrelaytxfee c4554fe894

[policy] disallow transactions under min relay fee, even in packages

Avoid adding transactions below min relay feerate because, even if they
were bumped through CPFP when entering the mempool, we do not have a
DoS-resistant way of ensuring they always remain bumped.  In the future,
this rule can be relaxed (e.g. to allow packages to bump 0-fee
transactions) if we find a way to do so.

563a2ee4f5

glozow force-pushed on Apr 17, 2023

glozow commented at 9:14 am on April 17, 2023: member

6fb01d2…563a2ee addressed

[validation] set PackageValidationState when mempool full b51ebccc28

[test] mempool full in package accept bf77fc9cb4

glozow commented at 12:01 pm on April 17, 2023: member

Last push fixes + tests the case where package meets mempool min feerate, but hits maxmempool and is evicted immediately (the user-facing effect is that submitpackage now throws a different and more helpful JSONRPCError). Don’t know why I hadn’t been testing that already, my bad.

I think “submitpackage” would count as obviously useful if it was available on mainnet – even without package relay, a miner could expose that via a web form to allow people to do cpfp bumps when the (miner’s) mempool is full, eg.

That makes sense to me as a useful use case, though I’m unsure if we should encourage miners to accept txns the rest of the network can’t see.

I think with this PR merged, we could reasonably expose submitpackage outside of regtest, though should keep it debug-only/experimental (after this PR, I think it’s never worse than having 500MB in the mempool and submitting a min mempool fee tx, and we already allow that).

Thought about this for a bit. While there are edge cases where the package won’t go in, and submitpackage doesn’t have a maxfeerate/maxburn check, I suppose this is not unsafe. Could open the RPC in a followup?

in test/functional/mempool_limit.py:126 in bf77fc9cb4

118@@ -119,7 +119,31 @@ def run_test(self):
119 
120         # The node will broadcast each transaction, still abiding by its peer's fee filter
121         peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns])
122-        self.generate(node, 1)
123+
124+        self.log.info("Check a package that passes mempoolminfee but is evicted immediately after submission")
125+        mempoolmin_feerate = node.getmempoolinfo()["mempoolminfee"]
126+        current_mempool = node.getrawmempool(verbose=False)
127+        worst_feerate_btcvb = Decimal("21000000")

instagibbs commented at 2:20 pm on April 17, 2023:

should this just be None and have a check that it’s non-None later? or is this value important?

glozow commented at 5:44 pm on April 17, 2023:

no special meaning, just me implementing “find the min” in a very elementary way :sweat_smile: will fix if I retouch

instagibbs commented at 2:27 pm on April 17, 2023: member

re-ACK https://github.com/bitcoin/bitcoin/pull/26933/commits/bf77fc9cb45209b9c560208c65abc94209cd7919

DrahtBot requested review from ajtowns on Apr 17, 2023

instagibbs commented at 2:38 pm on April 17, 2023: member

(the user-facing effect is that submitpackage now throws a different and more helpful JSONRPCError)

For those following at home, the legacy reason it was giving was the first transaction in package’s individual submission error, aka missing mempool min fee.

ajtowns commented at 9:49 am on April 26, 2023: contributor

reACK bf77fc9cb45209b9c560208c65abc94209cd7919

DrahtBot removed review request from ajtowns on Apr 26, 2023

glozow merged this on Apr 26, 2023

glozow closed this on Apr 26, 2023

sidhujag referenced this in commit ff16533cac on Apr 28, 2023

achow101 referenced this in commit 54bdb6e074 on Oct 5, 2023

Sjors commented at 2:11 pm on November 23, 2023: member

Just ran into this today while testing #27463.

To make testing of package submission easier, should we have an option to allow this on test networks? Or perhaps a way to set a minimum for GetMinFee()? @sdaftuar does mempool clustering fix this? E.g. is it just a matter of (occasionally) evicting chunks below -minrelaytxfee? From the above discussion I get the impression that the answer is yes, but most of this was discussed before that proposal came out.

instagibbs commented at 2:18 pm on November 23, 2023: member

@Sjors testing it includes making sure nothing below minrelay can enter the mempool, unfortunately until v3(where 0-fee will be allowed) or cluster mempool is deployed, which makes trimming very fast

glozow deleted the branch on Jan 15, 2024

mempool: disallow txns under min relay fee, even in packages #26933

Reviews

Conflicts