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
Concept ACK	murchandamus, glozow
Stale ACK	ismaelsadeeq

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:

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

instagibbs · 2024-05-15T12:13:27Z

rebased due to conflict

t-bast · 2024-05-15T13:47:13Z

While I wouldn't trust myself to correctly code-review this PR, I'd be happy to work on e2e tests that would leverage this for lightning channels fee-bumping (based on eclair) if it can help validate the logic and get this PR merged.

I'd like to highlight again how important this feature is for lightning (and probably for many other L2 protocols on top of bitcoin today). This is the critical step that allows us to mitigate pinning of a commitment transaction, and guarantee that we're able to set the fees of our commitment package to a value that should ensure timely confirmation.

instagibbs · 2024-05-16T15:14:20Z

sorry forgot to link #30072 here, added to OP

instagibbs · 2024-05-20T14:36:32Z

rebased on latest #30072

2fd34ba Add sanity checks for various ATMPArgs booleans (Greg Sanders) 20d8936 [refactor] make some members MemPoolAccept-wide (glozow) cbbfe71 cpfp carveout is excluded in packages (glozow) 69f7ab0 Add m_allow_sibling_eviction as separate ATMPArgs flag (Greg Sanders) 57ee302 Add description for m_test_accept (Greg Sanders) Pull request description: First few commits of #28984 to set the stage for the package RBF logic. These refactors are preparation for evaluating an RBF in a multi-proposed-transaction context instead of only a single proposed transaction. Also, carveouts and sibling evictions only should work in single RBF cases so add logic to preclude multi-tx cases in the future. No behavior changes aside from bailing earlier from failed carve-outs. ACKs for top commit: glozow: reACK 2fd34ba via range-diff sr-gi: utACK [2fd34ba](2fd34ba) theStack: re-ACK 2fd34ba Tree-SHA512: 5071c5b8d9b8d2c9faa278c8c4df31de288cb407a68e4d55544c588caff6c86160cce7825453549c6ed69e29d9ccb5ee2d4a518b18f563bfb12f2ced073fe42a

instagibbs · 2024-05-24T13:57:05Z

rebased on master 🚀

sdaftuar · 2024-05-24T17:25:30Z

src/policy/v3_policy.cpp

@@ -91,7 +91,6 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
            const auto parent_info = [&] {
                if (mempool_ancestors.size() > 0) {
                    auto& mempool_parent = *mempool_ancestors.begin();
-                    Assume(mempool_parent->GetCountWithDescendants() == 1);


My comment is just about the commit message for this commit, not this line of code (sorry):

Relax assumptions aabout in-mempool children of in-mempool

"aabout" --> "about"

TxA (in mempool) <- TxB' (in package, conflicts with TxB) <-
TxD (in package)

s/TxD/TxC/ ?

sdaftuar · 2024-05-24T17:32:35Z

src/validation.cpp

+        direct_conflict_iters.merge(ws.m_iters_conflicting);
+    }
+
+    for (const auto& ws : workspaces) {


Seems weird to loop through all the workspaces, when GetEntriesForConflicts is doing all the work that needs to be done on the first invocation? Maybe just invoke once with the child's txid, as is done further down?

right this is just a logging arg; probably should clean that up to just take Wtxid in future work. re-arranged and passed in child workspace now

sdaftuar · 2024-05-24T17:39:14Z

src/validation.cpp

+    }
+
+    // If the package has in-mempool ancestors, we won't consider a package RBF
+    // since it would result in a cluster larger than 2


Can you just add a comment here that references the comment on line 1520, as a reminder that we can't relax this without revisiting how we track available inputs when processing a package?

made an attempt at a grepp-able cautionary note

doc/policy/packages.md

Relax assumptions about in-mempool children of in-mempool parents. With package RBF, we will allow a package of size 2 with conflicts on its parent and reconsider the parent if its fee is insufficient on its own. Consider: TxA (in mempool) <- TxB (in mempool) TxA (in mempool) <- TxB' (in package, conflicts with TxB) <- TxC (in package) If TxB' fails to RBF TxB due to insufficient feerate, the package TxB' + TxC will be considered. PackageV3Checks called on TxB' will see an in-mempool parent TxA, and see the in-mempool child TxB. We cannot assume there is no in-mempool sibling, rather detect it and fail normally. Prior to package RBF, this would have failed on the first conflict in package.

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 <gloriajzhao@gmail.com> Co-authored-by: Greg Sanders <gsanders87@gmail.com>

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

56 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

Draft

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 722d1a5 to 7fd321d Compare May 13, 2024 16:14

DrahtBot added Needs rebase and removed CI failed labels May 14, 2024

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from 7fd321d to ce3a662 Compare May 15, 2024 12:13

DrahtBot removed the Needs rebase label May 15, 2024

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from ce3a662 to d412681 Compare May 20, 2024 14:35

DrahtBot mentioned this pull request May 23, 2024

policy: restrict all TRUC (v3) transactions to 10kvB #29873

Merged

DrahtBot added the Needs rebase label May 23, 2024

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from d412681 to d05e350 Compare May 24, 2024 13:37

DrahtBot removed the Needs rebase label May 24, 2024

sdaftuar reviewed May 24, 2024

View reviewed changes

instagibbs and others added 2 commits May 24, 2024 14:27

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from d05e350 to cddd60e Compare May 24, 2024 18:38

glozow and others added 7 commits May 24, 2024 14:41

[test] package rbf

7d89593

CheckPackageMempoolAcceptResult: Check package rbf invariants

c36b3bc

mempool: Improve logging of replaced transactions

aa3a4da

Add CTxMempool::GetEntry for Wtxid args

0a15df4

Fuzz: pass mempool to CheckPackageMempoolAcceptResult

5dd74f9

fuzz: add package RBF subpackage size assertion

91a7e8b

doc: update package RBF comment

7b74cbf

instagibbs force-pushed the 2023-12-cluster-size2-package-rbf branch from cddd60e to 7b74cbf Compare May 24, 2024 18:41

DrahtBot added CI failed and removed CI failed labels May 24, 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

DrahtBot commented Dec 1, 2023 •

edited

glozow commented Dec 4, 2023

instagibbs Dec 4, 2023 •

edited

sdaftuar commented Dec 8, 2023

instagibbs commented Dec 8, 2023

instagibbs commented May 15, 2024

t-bast commented May 15, 2024

instagibbs commented May 16, 2024

instagibbs commented May 20, 2024

instagibbs commented May 24, 2024

sdaftuar May 24, 2024

instagibbs May 24, 2024

sdaftuar May 24, 2024

instagibbs May 24, 2024

sdaftuar May 24, 2024

instagibbs May 24, 2024

Cluster size 2 package rbf #28984

Are you sure you want to change the base?

Cluster size 2 package rbf #28984

Conversation

instagibbs commented Dec 1, 2023 • edited

DrahtBot commented Dec 1, 2023 • edited

Code Coverage

Reviews

Conflicts

glozow commented Dec 4, 2023

instagibbs Dec 4, 2023 • edited

Choose a reason for hiding this comment

sdaftuar commented Dec 8, 2023

instagibbs commented Dec 8, 2023

instagibbs commented May 15, 2024

t-bast commented May 15, 2024

instagibbs commented May 16, 2024

instagibbs commented May 20, 2024

instagibbs commented May 24, 2024

sdaftuar May 24, 2024

Choose a reason for hiding this comment

instagibbs May 24, 2024

Choose a reason for hiding this comment

sdaftuar May 24, 2024

Choose a reason for hiding this comment

instagibbs May 24, 2024

Choose a reason for hiding this comment

sdaftuar May 24, 2024

Choose a reason for hiding this comment

instagibbs May 24, 2024

Choose a reason for hiding this comment

instagibbs commented Dec 1, 2023 •

edited

DrahtBot commented Dec 1, 2023 •

edited

instagibbs Dec 4, 2023 •

edited