Without remoing it, if we ever call PreChecks() multiple times for any reason during any one MempoolAccept, subsequent invocations may have incorrect limits, allowing longer/larger chains than should be allowed.
Currently this is only an issue with submitpackage, so this is not exposed on mainnet.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
908@@ -905,11 +909,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
909 assert(ws.m_iters_conflicting.size() == 1);
910 CTxMemPool::txiter conflict = *ws.m_iters_conflicting.begin();
911
912- m_limits.descendant_count += 1;
913- m_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants();
914+ maybe_rbf_limits.descendant_count += 1;
915+ maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants();
PackageMempoolChecks and SubmitPackage from seeing the bumped limits. Maybe have const m_base_limits and m_current_limits and have PreChecks() reset m_current_limits=m_base_limits before bumping it?
PackageMempoolChecks/SubmitPackage right now? This has been mentioned in the comment which was added to maybe_rbf_limits. We’ll just need to think about this again when we try to add package RBF.
After #26711, are there scenarios where it would make sense to give these carveouts to transactions in SubmitPackage? At that point we only use those functions for transactions that need to be bumped. They won’t be allowed to do package RBF (so no RBF carveout), and CPFP carveout will never work (it’d be given to the parent which exceeds limits, but parent+child will bust 26). Also see https://github.com/bitcoin/bitcoin/pull/25038/commits/9b576f0fb1a697feb4df5f3f8d7f0b0431f8d881.
Also, I just realized we could maybe delete MemPoolAccept::m_limits now that we can just query pool.m_limits. Unless we need to remember the limit amendments for SubmitPackage, I think these amendments should probably just have a local scope.
@glozow good idea, I think removing it entirely makes a lot of sense if your assumptions are correct, and they sound correct to me.
@dergoegge ~Ok, I don’t think this is easy(possible?) to hit in master as it shows easier when PreChecks are being called more often, and allowed to fail the first time, such as #26711~ it’s possible
Example on ~#26711~ master
0ancestor limit: 2
1descendant limit: 1
2
3A (already in mempool, to be RBF'd by A')
4A' -> B (A is RBF'd and where B > 4kWu)
5
6if you run PreChecks once over A', then B, it's now 2 and 2(thanks to RBF of A), making A'+B allowed even though it shouldn't be
122+ target_weight=40001, # EXTRA_DESCENDANT_TX_SIZE_LIMIT + 1
123+ utxo_to_spend=tx_B["new_utxo"],
124+ confirmed_only=True
125+ )
126+
127+ assert_raises_rpc_error(-26, "too-long-mempool-chain", node.submitpackage, [tx_B["hex"], tx_C["hex"]])
0 test 2023-09-14T23:10:39.063000Z TestFramework (ERROR): Assertion failed
1 Traceback (most recent call last):
2 File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 140, in try_rpc
3 fun(*args, **kwds)
4 File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
5 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
6 File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 129, in __call__
7 raise JSONRPCException(response['error'], status)
8 test_framework.authproxy.JSONRPCException: c15f8dbec585be90e17a0757b8a6ad3f21863ae9db90a5d69ad31e5c047c8f7e failed: mempool min fee not met (-26)
9 During handling of the above exception, another exception occurred:
10 Traceback (most recent call last):
11 File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
12 self.run_test()
13 File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/mempool_limit.py", line 375, in run_test
14 self.test_rbf_carveout_disallowed()
15 File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/mempool_limit.py", line 127, in test_rbf_carveout_disallowed
16 assert_raises_rpc_error(-26, "too-long-mempool-chain", node.submitpackage, [tx_B["hex"], tx_C["hex"]])
17 File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 131, in assert_raises_rpc_error
18 assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
19 File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 146, in try_rpc
20 raise AssertionError(
21 AssertionError: Expected substring not found in error message:
22 substring: 'too-long-mempool-chain'
23 error message: 'c15f8dbec585be90e17a0757b8a6ad3f21863ae9db90a5d69ad31e5c047c8f7e failed: mempool min fee not met'.
hmm, not sure why miniwallet is making something that doesn’t hit minfee, pushed specific fee arg to avoid this
edit: I guess if miniwallet doesn’t check minfee, trimming could cause it
85+
86+ # We set chain limits to 2 ancestors, 1 descendant, then try to get a parents-and-child chain of 2 in mempool
87+ #
88+ # A: Solo transaction to be RBF'd (to bump descendant limit for package later)
89+ # B: First transaction in package, RBFs A by itself under individual evaluation, which would give it +1 descendant limit
90+ # C: Second transaction in package, spends C. If the +1 descendant limit persisted, would make it into mempool
After #26711, are there scenarios where it would make > sense to give these carveouts to transactions in SubmitPackage? At that point we only use > those functions for transactions that need to be bumped.
I can’t see a scenario, at least for LN-chain of transactions, where these carveouts would make sense (maybe if you have 24 commitment txn bumped by one CPFP, though this is not safe for other reasons).
94+ # Generate a confirmed utxo we will double-spend
95+ rbf_utxo = self.wallet.send_self_transfer(
96+ from_node=node,
97+ confirmed_only=True
98+ )["new_utxo"]
99+ self.generate(node, 1)
nit ee589d4466bb0548a6f2215afe8abd0735768dab: just use get_utxo
0 rbf_utxo = self.wallet.get_utxo(confirmed_only=True)
104+ tx_A = self.wallet.send_self_transfer(
105+ from_node=node,
106+ fee=(mempoolmin_feerate / 1000) * (A_weight // 4) + Decimal('0.000001'),
107+ target_weight=A_weight,
108+ utxo_to_spend=rbf_utxo,
109+ confirmed_only=True
869@@ -873,6 +870,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
870 if (!bypass_limits && !args.m_package_feerates && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false;
871
872 ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts);
873+
874+ // Note that these modifications are only applicable to single transaction scenarios;
875+ // carve-outs and package RBF are disabled for multi-transaction evaluations.