Remove MemPoolAccept::m_limits to avoid mutating it in package evaluation #28472

instagibbs commented at 6:33 PM on September 13, 2023: member

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.

DrahtBot commented at 6:33 PM on September 13, 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	achow101, glozow, ariard

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:

#28471 (Fix virtual size limit enforcement in transaction package context by instagibbs)
#26711 (validate package transactions with their in-package ancestor sets by glozow)

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.

DrahtBot added the label CI failed on Sep 13, 2023

in src/validation.cpp:911 in 275687f03f outdated

 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();

ajtowns commented at 11:45 PM on September 13, 2023:

This prevents 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?

glozow commented at 10:47 AM on September 20, 2023:

I think there aren't any situations in which we'd have RBFing and 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.

DrahtBot removed the label CI failed on Sep 14, 2023

glozow commented at 8:36 AM on September 14, 2023: member

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.

dergoegge commented at 10:43 AM on September 14, 2023: member

Could you add a test?

Remove MemPoolAccept::m_limits, only have local copies for carveouts 275579d8c1

instagibbs force-pushed on Sep 14, 2023

instagibbs commented at 5:37 PM on September 14, 2023: member

@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

ancestor limit: 2
descendant limit: 1

A (already in mempool, to be RBF'd by A')
A' -> B (A is RBF'd  and where B > 4kWu)

if 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

instagibbs commented at 6:26 PM on September 14, 2023: member

Pushed a test that fails on master

instagibbs force-pushed on Sep 14, 2023

DrahtBot added the label CI failed on Sep 14, 2023

instagibbs force-pushed on Sep 14, 2023

instagibbs renamed this:
~~Make MemPoolAccept::m_limits const~~
Remove MemPoolAccept::m_limits to avoid mutating it in package evaluation
on Sep 14, 2023

in test/functional/mempool_limit.py:129 in cddf03e3fb outdated

 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"]])

maflcko commented at 6:55 AM on September 15, 2023:

 test  2023-09-14T23:10:39.063000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 140, in try_rpc
                                       fun(*args, **kwds)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 129, in __call__
                                       raise JSONRPCException(response['error'], status)
                                   test_framework.authproxy.JSONRPCException: c15f8dbec585be90e17a0757b8a6ad3f21863ae9db90a5d69ad31e5c047c8f7e failed: mempool min fee not met (-26)
                                   During handling of the above exception, another exception occurred:
                                   Traceback (most recent call last):
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
                                       self.run_test()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/mempool_limit.py", line 375, in run_test
                                       self.test_rbf_carveout_disallowed()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/mempool_limit.py", line 127, in test_rbf_carveout_disallowed
                                       assert_raises_rpc_error(-26, "too-long-mempool-chain", node.submitpackage, [tx_B["hex"], tx_C["hex"]])
                                     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
                                       assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 146, in try_rpc
                                       raise AssertionError(
                                   AssertionError: Expected substring not found in error message:
                                   substring: 'too-long-mempool-chain'
                                   error message: 'c15f8dbec585be90e17a0757b8a6ad3f21863ae9db90a5d69ad31e5c047c8f7e failed: mempool min fee not met'.

maflcko commented at 6:55 AM on September 15, 2023:

https://cirrus-ci.com/task/6070864190373888?logs=ci#L3352

instagibbs commented at 12:51 PM on September 15, 2023:

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

instagibbs force-pushed on Sep 15, 2023

DrahtBot removed the label CI failed on Sep 15, 2023

in test/functional/mempool_limit.py:90 in 1b7f54b033 outdated

  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

ariard commented at 4:31 PM on September 18, 2023:

should say” spend B”.

instagibbs commented at 1:34 PM on September 19, 2023:

tried pushing an update, it doesn't seem to be showing yet... tried to fix this up anyways!

ariard commented at 11:34 AM on September 20, 2023:

Shows up at ee589d44

ariard commented at 4:49 PM on September 18, 2023: contributor

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).

Add regression test for m_limit mutation ee589d4466

instagibbs force-pushed on Sep 19, 2023

achow101 commented at 10:37 AM on September 20, 2023: member

ACK ee589d4466bb0548a6f2215afe8abd0735768dab

in test/functional/mempool_limit.py:99 in ee589d4466

  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)

glozow commented at 11:27 AM on September 20, 2023:

nit ee589d4466bb0548a6f2215afe8abd0735768dab: just use get_utxo

        rbf_utxo = self.wallet.get_utxo(confirmed_only=True)

in test/functional/mempool_limit.py:109 in ee589d4466

 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

glozow commented at 11:29 AM on September 20, 2023:

nit ee589d4466bb0548a6f2215afe8abd0735768dab: this arg doesn't do anything as you've already specified which utxo to spend. Doesn't hurt but you can omit it.

glozow commented at 11:33 AM on September 20, 2023: member

ACK ee589d4466bb0548a6f2215afe8abd0735768dab, nits can be ignored

in src/validation.cpp:875 in ee589d4466

 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.

ariard commented at 11:36 AM on September 20, 2023:

nit: the two types of carve-out (RBF and CPFP) are disabled, could be commented here.

ariard approved

ariard commented at 11:39 AM on September 20, 2023: contributor

Code Review ACK ee589d446

achow101 merged this on Sep 20, 2023

achow101 closed this on Sep 20, 2023

Frank-GER referenced this in commit f0eeea5b83 on Sep 25, 2023

sidhujag referenced this in commit 23acd69f7d on Sep 26, 2023

bitcoin locked this on Sep 19, 2024