Cluster size 2 package rbf #28984

instagibbs · 2023-12-01T22:12:06Z

Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less.

Proposed validation steps:

If the transaction package is of size 1, legacy rbf rules apply.
Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2. If larger, fail.
The package rbf may not evict more than 100 transactions from the mempool(bip125 rule 5)
The package is a single chunk
Every directly conflicted mempool transaction is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2).
Diagram check: We ensure that the replacement is strictly superior, improving the mempool
The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package (equivalent to bip125 rule 3 and 4)

Post-cluster mempool this will likely be expanded to general package rbf, but this is what we can safely support today.

DrahtBot · 2023-12-01T22:12:09Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, ismaelsadeeq, theStack, murchandamus, achow101

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:

#30272 (doc: use TRUC instead of v3 and add release note by glozow)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)
#27432 (contrib: add tool to convert compact-serialized UTXO set to SQLite database by theStack)
#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)

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 · 2023-12-04T04:54:20Z

Added this to the tracking issue

instagibbs · 2023-12-04T19:26:16Z

src/policy/rbf.cpp

+ const CFeeRate replacement_miner_score(replacement_fees, replacement_vsize);
+
+ for (const auto& entry : direct_conflicts) {
+ const bool conflict_is_v3{entry->GetSharedTx()->nVersion == 3};


this can't be hit yet; post-v3 it can. I can remove this, or leave a note, or both. It makes some package rbfs more useful

(edit: it's used in #29001 functional tests )

src/test/rbf_tests.cpp

sdaftuar · 2023-12-08T16:12:07Z

So thinking conceptually about this, I'm most concerned by the new CheckMinerScores() criterion, where we only check the replacement package's feerate against the ancestor feerates of the indirect conflicts.

I think there are two issues with this approach:

If you look at GetModFeesWithAncestors()/GetSizeWithAncestors(), that can be greater than the transaction's individual feerate (ie if the parent of a transaction has a higher feerate than the child you're conflicting with). So this can cause the test to be too conservative and introduce some kind of weird pinning. Granted, this is a new replacement functionality so maybe not the end of the world if it is sometimes too conservative, but it seems like a bug.
Also if you look at the ancestor feerate of a transaction, that can underestimate its mining score, such as if some low feerate ancestor is being paid for by another transaction. So that can allow for replacements that are not incentive compatible (similar to how this is possible today with our current single-transaction RBF rules).

Even though the current RBF logic is broken already, I'd prefer to avoid adding on to that while rolling out the package RBF implementation, just so that users don't get used to some kinds of replacements working that really shouldn't be.

So I was wondering, can we deal with this by just restricting the topology of what we allow a package RBF to conflict with? Let's say we allow a replacement if the conflict set consists of either (1) a single transaction with no ancestors (and of course no descendants, since any descendants would be in the conflict set as well), or (2) two transactions that are in a parent-child topology, and that have no other in-mempool ancestors (or descendants, of course).

In those situations, the maximum miner score you would have to consider is either the individual feerate of the parent tx or the ancestor feerate of the child, so the new logic should work perfectly.

instagibbs · 2023-12-08T21:48:57Z

After offline discussion with @sdaftuar , I've worked on a set of prospective changes and pushed them to this branch as follow-on commits.

Key change is that we will now only allow package RBF when the conflicted transactions are all in "clusters" of up to size 2. This allows us to calculate mining scores for each conflicted transaction, which means that post-cluster mempool, if we did this right, IIUC, the behavior should match with more general package RBF.

glozow

ACK dd364f9. non-blocking nits that can go in a followup. Also needs a release note.

src/test/util/txmempool.cpp

src/validation.cpp

doc/policy/packages.md

instagibbs · 2024-06-12T13:53:38Z

Added a release note

theStack

Spent most time reviewing the core commit 612847a so far, which looks correct to me (though I'm not super-familiar with the details of the feerate diagram checks that are used in this PR for the first time in production code). Still planning on looking deeper at the tests and do another full review round. Left some non-blocking nits on the way.

src/validation.cpp

src/test/txpackage_tests.cpp

test/functional/mempool_package_rbf.py

doc/policy/packages.md

Support package RBF where the conflicting package would result in a mempool cluster of size two, and each of its direct conflicts are also part of an up-to-size-2 mempool cluster. This restricted topology allows for exact calculation of miner scores for each side of the equation, reducing the surface area for new pins, or incentive-incompatible replacements. This allows wallets to create simple CPFP packages that can fee bump other simple CPFP packages. This, leveraged with other restrictions such as V3 transactions, can create pin-resistant applications. Future package RBF relaxations can be considered when appropriate. Co-authored-by: glozow <[email protected]> Co-authored-by: Greg Sanders <[email protected]>

instagibbs · 2024-06-13T13:54:44Z

@theStack part of splitting out the diagram stuff to a prep PR is that you can just assume it means "economically rational to replace" 👍

glozow

reACK 94ed4fb via range-diff

ismaelsadeeq

re-ACK 94ed4fb

theStack

Code-review ACK 94ed4fb

murchandamus

utACK 94ed4fb

doc/policy/packages.md

murchandamus · 2024-06-17T15:47:34Z

src/test/util/txmempool.cpp

+ // Each subpackage is allowed MAX_REPLACEMENT_CANDIDATES replacements (only checking individually here)
+ if (atmp_result.m_replaced_transactions.size() > MAX_REPLACEMENT_CANDIDATES) {
+ return strprintf("tx %s result replaced too many transactions",
+ wtxid.ToString());
+ }


"result replaced" seems like the wrong tense to me, given that the transaction was not accepted. Should this perhaps be:

Suggested change

// Each subpackage is allowed MAX_REPLACEMENT_CANDIDATES replacements (only checking individually here)

if (atmp_result.m_replaced_transactions.size() > MAX_REPLACEMENT_CANDIDATES) {

return strprintf("tx %s result replaced too many transactions",

wtxid.ToString());

}

// Each subpackage is allowed MAX_REPLACEMENT_CANDIDATES replacements (only checking individually here)

if (atmp_result.m_replaced_transactions.size() > MAX_REPLACEMENT_CANDIDATES) {

return strprintf("tx %s would replace too many transactions",

wtxid.ToString());

}

done in follow-up

I don't agree with this suggestion - we are checking the result of a transaction that was accepted here. So "would replace" is incorrect and "replaced" is correct.

src/test/util/txmempool.cpp

murchandamus · 2024-06-17T16:17:53Z

test/functional/mempool_package_rbf.py

+ assert_equal(f"package RBF failed: insufficient anti-DoS fees, rejecting replacement {package_txns3[1].rehash()}, not enough additional fees to relay; 0.00 < 0.00000{sum([tx.get_vsize() for tx in package_txns3])}", pkg_results3["package_msg"])
+
+ self.assert_mempool_contents(expected=package_txns1)
+ self.generate(node, 1)


Maybe also add a success case in the end to prove that all the reasons for rejection were enumerated?

Suggested change

self.generate(node, 1)

self.log.info("Check non-heavy child with higher absolute fee will replace")

package_hex3_1, package_txns3_1 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_CHILD_FEE + Decimal("0.00000001") )

pkg_results3_1 = node.submitpackage(package_hex3_1)

self.assert_mempool_contents(expected=package_txns3_1)

self.generate(node, 1)

wrote a better boundary test in this vein in follow-up, thanks. You actually need incremental added, not 1 sat.

murchandamus · 2024-06-17T16:21:50Z

test/functional/mempool_package_rbf.py

+ assert 'package RBF failed: package feerate is less than parent feerate' in pkg_results5["package_msg"]
+
+ self.assert_mempool_contents(expected=package_txns4)
+ self.generate(node, 1)


Suggested change

self.generate(node, 1)

package_hex5_1, package_txns5_1 = self.create_simple_package(coin, parent_fee=DEFAULT_CHILD_FEE, child_fee=DEFAULT_CHILD_FEE)

pkg_results5_1 = node.submitpackage(package_hex5_1)

self.assert_mempool_contents(expected=package_txns5_1)

self.generate(node, 1)

took as inspiration to make an actual boundary-condition test in follow-up, thanks

instagibbs · 2024-06-17T18:37:06Z

opened followup at #30295

achow101 · 2024-06-17T21:09:11Z

ACK 94ed4fb

3f00aae package rbf: cpfp structure requires package > parent feerate (Greg Sanders) ad7f1f6 test package rbf boundary conditions more closely (Greg Sanders) ff4558d doc: reword package RBF documentation (Greg Sanders) de669a8 doc: replace mention of V3 with TRUC (Greg Sanders) Pull request description: Some suggested nits/changes from #28984 ACKs for top commit: glozow: ACK 3f00aae murchandamus: ACK 3f00aae Tree-SHA512: 79434cc8aba25a43e99793298cdc99cad807db2c3a2e780a31953f244b95eecd97b90559abd67fbf30996c00966675fa257253a7812ec4727420226162c629ae

hebasto · 2024-07-14T06:46:50Z

Ported to the CMake-based build system in hebasto#264.

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from 38ce2cb to 5b216db Compare December 1, 2023 22:13

DrahtBot added the CI failed label Dec 2, 2023

glozow added the TX fees and policy label Dec 4, 2023

glozow mentioned this pull request Dec 4, 2023

Package Relay Project Tracking #27463

Open

54 tasks

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch 3 times, most recently from 57e5fc7 to 56848f9 Compare December 4, 2023 18:54

instagibbs marked this pull request as ready for review December 4, 2023 18:55

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from 56848f9 to be9d163 Compare December 4, 2023 19:14

instagibbs commented Dec 4, 2023

View reviewed changes

DrahtBot removed the CI failed label Dec 4, 2023

instagibbs commented Dec 5, 2023

View reviewed changes

src/test/rbf_tests.cpp Outdated Show resolved Hide resolved

instagibbs mentioned this pull request Dec 5, 2023

Ephemeral Anchors #29001

Closed

DrahtBot mentioned this pull request Dec 7, 2023

[WIP] Cluster mempool implementation #28676

Draft

8 tasks

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch 2 times, most recently from 7aee728 to 1d859b0 Compare December 8, 2023 15:27

glozow mentioned this pull request Dec 11, 2023

policy: nVersion=3 and Package RBF #25038

Closed

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch 3 times, most recently from f9ce967 to 385b6ca Compare December 11, 2023 20:04

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from db21a37 to dd364f9 Compare June 11, 2024 15:45

glozow reviewed Jun 12, 2024

View reviewed changes

src/test/util/txmempool.cpp Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

doc/policy/packages.md Outdated Show resolved Hide resolved

doc/policy/packages.md Outdated Show resolved Hide resolved

DrahtBot requested a review from ismaelsadeeq June 12, 2024 10:45

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from dd364f9 to 8f2f64c Compare June 12, 2024 13:48

glozow mentioned this pull request Jun 12, 2024

doc: use TRUC instead of v3 and add release note #30272

Merged

theStack reviewed Jun 13, 2024

View reviewed changes

sdaftuar and others added 7 commits June 13, 2024 09:52

[test] package rbf

4d15bcf

Fuzz: pass mempool to CheckPackageMempoolAcceptResult

316d7b6

CheckPackageMempoolAcceptResult: Check package rbf invariants

d3466e4

mempool: Improve logging of replaced transactions

6e3c439

doc: update package RBF comment

afd52d8

Add release note for size 2 package rbf

94ed4fb

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from 400ad72 to 94ed4fb Compare June 13, 2024 13:53

glozow reviewed Jun 14, 2024

View reviewed changes

ismaelsadeeq approved these changes Jun 14, 2024

View reviewed changes

theStack approved these changes Jun 14, 2024

View reviewed changes

glozow requested a review from sdaftuar June 14, 2024 16:44

murchandamus reviewed Jun 17, 2024

View reviewed changes

achow101 merged commit 41544b8 into bitcoin:master Jun 17, 2024
16 checks passed

instagibbs mentioned this pull request Jun 17, 2024

#28984 package rbf followups #30295

Merged

hebasto added the Needs CMake port label Jun 30, 2024

hebasto mentioned this pull request Jul 14, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#264

Closed

hebasto removed the Needs CMake port label Jul 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster size 2 package rbf #28984

Cluster size 2 package rbf #28984

instagibbs commented Dec 1, 2023 •

edited

Loading

DrahtBot commented Dec 1, 2023 •

edited

Loading

glozow commented Dec 4, 2023

instagibbs Dec 4, 2023 •

edited

Loading

sdaftuar commented Dec 8, 2023

instagibbs commented Dec 8, 2023

glozow left a comment

instagibbs commented Jun 12, 2024

theStack left a comment

instagibbs commented Jun 13, 2024

glozow left a comment

ismaelsadeeq left a comment

theStack left a comment

murchandamus left a comment

murchandamus Jun 17, 2024

instagibbs Jun 17, 2024

glozow Jul 4, 2024

murchandamus Jun 17, 2024

instagibbs Jun 17, 2024

murchandamus Jun 17, 2024

instagibbs Jun 17, 2024

instagibbs commented Jun 17, 2024

achow101 commented Jun 17, 2024

hebasto commented Jul 14, 2024

- self.generate(node, 1)
+ self.log.info("Check non-heavy child with higher absolute fee will replace")
+ package_hex3_1, package_txns3_1 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_CHILD_FEE + Decimal("0.00000001") )
+ pkg_results3_1 = node.submitpackage(package_hex3_1)
+ self.assert_mempool_contents(expected=package_txns3_1)
+ self.generate(node, 1)

- self.generate(node, 1)
+ package_hex5_1, package_txns5_1 = self.create_simple_package(coin, parent_fee=DEFAULT_CHILD_FEE, child_fee=DEFAULT_CHILD_FEE)
+ pkg_results5_1 = node.submitpackage(package_hex5_1)
+ self.assert_mempool_contents(expected=package_txns5_1)
+ self.generate(node, 1)

Cluster size 2 package rbf #28984

Cluster size 2 package rbf #28984

Conversation

instagibbs commented Dec 1, 2023 • edited Loading

DrahtBot commented Dec 1, 2023 • edited Loading

Code Coverage

Reviews

Conflicts

glozow commented Dec 4, 2023

instagibbs Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

sdaftuar commented Dec 8, 2023

instagibbs commented Dec 8, 2023

glozow left a comment

Choose a reason for hiding this comment

instagibbs commented Jun 12, 2024

theStack left a comment

Choose a reason for hiding this comment

instagibbs commented Jun 13, 2024

glozow left a comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

theStack left a comment

Choose a reason for hiding this comment

murchandamus left a comment

Choose a reason for hiding this comment

murchandamus Jun 17, 2024

Choose a reason for hiding this comment

instagibbs Jun 17, 2024

Choose a reason for hiding this comment

glozow Jul 4, 2024

Choose a reason for hiding this comment

murchandamus Jun 17, 2024

Choose a reason for hiding this comment

instagibbs Jun 17, 2024

Choose a reason for hiding this comment

murchandamus Jun 17, 2024

Choose a reason for hiding this comment

instagibbs Jun 17, 2024

Choose a reason for hiding this comment

instagibbs commented Jun 17, 2024

achow101 commented Jun 17, 2024

hebasto commented Jul 14, 2024

instagibbs commented Dec 1, 2023 •

edited

Loading

DrahtBot commented Dec 1, 2023 •

edited

Loading

instagibbs Dec 4, 2023 •

edited

Loading