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

instagibbs · 2023-09-13T18:33:27Z

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 · 2023-09-13T18:33:31Z

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.

ajtowns · 2023-09-13T23:45:35Z

src/validation.cpp

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?

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.

glozow

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 9b576f0.

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 · 2023-09-14T10:43:29Z

Could you add a test?

instagibbs · 2023-09-14T17:37:11Z

@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 · 2023-09-14T18:26:05Z

Pushed a test that fails on master

test/functional/mempool_limit.py

ariard

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

ariard · 2023-09-18T16:31:05Z

test/functional/mempool_limit.py

should say” spend B”.

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

Shows up at ee589d4

achow101 · 2023-09-20T10:37:20Z

ACK ee589d4

glozow

ACK ee589d4, nits can be ignored

glozow · 2023-09-20T10:47:42Z

src/validation.cpp

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.

glozow · 2023-09-20T11:27:10Z

test/functional/mempool_limit.py

+        rbf_utxo = self.wallet.send_self_transfer(
+            from_node=node,
+            confirmed_only=True
+        )["new_utxo"]
+        self.generate(node, 1)


nit ee589d4: just use get_utxo

Suggested change

rbf_utxo = self.wallet.send_self_transfer(

from_node=node,

confirmed_only=True

)["new_utxo"]

self.generate(node, 1)

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

glozow · 2023-09-20T11:29:40Z

test/functional/mempool_limit.py

+            fee=(mempoolmin_feerate / 1000) * (A_weight // 4) + Decimal('0.000001'),
+            target_weight=A_weight,
+            utxo_to_spend=rbf_utxo,
+            confirmed_only=True


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

ariard

Code Review ACK ee589d4

ariard · 2023-09-20T11:36:09Z

src/validation.cpp

    ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts);
+
+    // Note that these modifications are only applicable to single transaction scenarios;
+    // carve-outs and package RBF are disabled for multi-transaction evaluations.


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

… it in package evaluation

… it in …

DrahtBot added the CI failed label Sep 13, 2023

ajtowns reviewed Sep 13, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 14, 2023

RFC: Remove boost usage from kernel api / headers #28335

Closed

DrahtBot removed the CI failed label Sep 14, 2023

glozow reviewed Sep 14, 2023

View reviewed changes

glozow mentioned this pull request Sep 14, 2023

Package Relay Project Tracking #27463

Open

57 tasks

glozow mentioned this pull request Sep 14, 2023

validate package transactions with their in-package ancestor sets #26711

Closed

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

275579d

instagibbs force-pushed the 2023-09-immutible-m_limit branch from 275687f to 275579d Compare September 14, 2023 17:32

instagibbs force-pushed the 2023-09-immutible-m_limit branch from 6782a61 to 7959451 Compare September 14, 2023 18:29

DrahtBot added the CI failed label Sep 14, 2023

instagibbs force-pushed the 2023-09-immutible-m_limit branch from 7959451 to cddf03e Compare September 14, 2023 18:40

instagibbs mentioned this pull request Sep 14, 2023

Add package evaluation fuzzer #28450

Merged

DrahtBot mentioned this pull request Sep 14, 2023

Fix virtual size limit enforcement in transaction package context #28471

Merged

instagibbs changed the title ~~Make MemPoolAccept::m_limits const~~ Remove MemPoolAccept::m_limits to avoid mutating it in package evaluation Sep 14, 2023

maflcko reviewed Sep 15, 2023

View reviewed changes

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

instagibbs force-pushed the 2023-09-immutible-m_limit branch from cddf03e to 1b7f54b Compare September 15, 2023 12:50

DrahtBot removed the CI failed label Sep 15, 2023

ariard reviewed Sep 18, 2023

View reviewed changes

Add regression test for m_limit mutation

ee589d4

instagibbs force-pushed the 2023-09-immutible-m_limit branch from 1b7f54b to ee589d4 Compare September 19, 2023 13:36

glozow reviewed Sep 20, 2023

View reviewed changes

ariard approved these changes Sep 20, 2023

View reviewed changes

achow101 merged commit 3966b0a into bitcoin:master Sep 20, 2023

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 25, 2023

Merge bitcoin#28472: Remove MemPoolAccept::m_limits to avoid mutating…

f0eeea5

… it in package evaluation

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 26, 2023

Merge bitcoin#28472: Remove MemPoolAccept::m_limits to avoid mutating…

23acd69

… it in …

glozow mentioned this pull request Sep 28, 2023

rpc: allow submitpackage to be called outside of regtest #27609

Merged

bitcoin locked and limited conversation to collaborators Sep 19, 2024

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

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

Uh oh!

Conversation

instagibbs commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge commented Sep 14, 2023

Uh oh!

instagibbs commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Sep 14, 2023

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 20, 2023

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

instagibbs commented Sep 13, 2023 •

edited

Loading

DrahtBot commented Sep 13, 2023 •

edited

Loading

instagibbs commented Sep 14, 2023 •

edited

Loading