Cluster size 2 package rbf

instagibbs commented at 10:12 pm on December 1, 2023: member

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 commented at 10:12 pm on December 1, 2023: contributor

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.

instagibbs force-pushed on Dec 1, 2023

DrahtBot added the label CI failed on Dec 2, 2023

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

glozow commented at 4:54 am on December 4, 2023: member

Added this to the tracking issue

instagibbs force-pushed on Dec 4, 2023

instagibbs marked this as ready for review on Dec 4, 2023

instagibbs force-pushed on Dec 4, 2023

in src/policy/rbf.cpp:194 in a6730af067 outdated

189+{
190+        // Note that this assumes no in-mempool ancestors
191+        const CFeeRate replacement_miner_score(replacement_fees, replacement_vsize);
192+
193+        for (const auto& entry : direct_conflicts) {
194+            const bool conflict_is_v3{entry->GetSharedTx()->nVersion == 3};

instagibbs commented at 7:26 pm on December 4, 2023:

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 )

DrahtBot removed the label CI failed on Dec 4, 2023

in src/test/rbf_tests.cpp:249 in 1996defeef outdated

228@@ -228,6 +229,36 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
229     const auto spends_conflicting_confirmed = make_tx({m_coinbase_txns[0], m_coinbase_txns[1]}, {45 * CENT});
230     BOOST_CHECK(HasNoNewUnconfirmed(*spends_conflicting_confirmed.get(), pool, {entry1_normal, entry3_low}) == std::nullopt);
231 
232+    // Tests for CheckMinerScores
233+
234+    // These tests use modified fees (including prioritisation), not base fees.
235+    BOOST_CHECK(CheckMinerScores(entry5_low->GetFee() + entry6_low_prioritised->GetFee() + 1,

instagibbs commented at 3:28 pm on December 5, 2023:

commit message is old; will fix

instagibbs force-pushed on Dec 8, 2023

sdaftuar commented at 4:12 pm on December 8, 2023: member

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 commented at 9:48 pm on December 8, 2023: member

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 force-pushed on Dec 11, 2023

instagibbs commented at 8:06 pm on December 11, 2023: member

@sdaftuar should be ready for another look

instagibbs force-pushed on Dec 11, 2023

in src/policy/rbf.cpp:196 in 557da6c902 outdated

191+            if (direct_conflict->GetCountWithAncestors() > 2) {
192+                return strprintf("Child transaction has too many ancestors");
193+            }
194+            if (direct_conflict->GetCountWithAncestors() == 2) {
195+                const auto& parent = direct_conflict->GetMemPoolParentsConst().begin();
196+                if (parent->get().GetCountWithDescendants() > 1) {

sdaftuar commented at 8:23 pm on December 11, 2023:

Should this be “> 2”?

instagibbs commented at 8:36 pm on December 11, 2023:

I have a failing unit test for this :facepalm:

pushed fix

instagibbs force-pushed on Dec 11, 2023

sdaftuar commented at 1:12 pm on December 12, 2023: member

size-2 package rbf can have 6 types of conflicts:

parent conflicts with solo

parent conflicts with child

parent conflicts with parent

child conflicts with parent

child conflicts with solo

child conflicts with child

I think if you state it this way, then there are more cases to write out (but they all simplify down to the same thing) – the parent transaction in the incoming package can directly conflict with {solo tx, child tx, parent tx, both child tx and parent tx}, and so can the child transaction in the package (ie it can also conflict with a solo tx, child tx, parent tx, and both), and you can have both of those going on at the same time, AND we permit the incoming package to conflict with up to 100 such in-mempool transactions that satisfy this “cluster-size-2” restriction.

So the way I’d try to describe this PR is that an incoming package with in-mempool conflicts must satisfy these RBF policy rules:

Every direct conflict is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2).
The 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)
The package’s child tx has greater feerate than the parent tx
The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package
The package feerate (total package fee / total package vsize) must exceed the min(individual feerate, ancestor feerate) of every transaction that would be evicted (direct and indirect conflicts). For transactions in a cluster of size <= 2, min(feerate, ancestor feerate) exactly captures the mining score of a transaction.

Did I miss anything?

instagibbs commented at 2:19 pm on December 12, 2023: member

@sdaftuar I took your suggestions, reordered and slightly reworded. Matches my implementation.

in src/validation.cpp:1351 in 671de055c8 outdated

1351-    // because it's unnecessary. Also, CPFP carve out can increase the limit for individual
1352-    // transactions, but this exemption is not extended to packages in CheckPackageLimits().
1353-    std::string err_string;
1354-    if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) {
1355+    // Apply package mempool ancestor/descendant limits.
1356+    if (!PackageMempoolChecks(txns, m_total_vsize, package_state)) {

sdaftuar commented at 2:31 pm on December 12, 2023:

Is there a reason to drop the txns.size() > 1 test in this if clause?

instagibbs commented at 7:40 pm on December 12, 2023:

It’s essentially an existing no-op. IIRC AcceptMultipleTransactions is only called in unit tests with size 1, otherwise it’s always > 1. (maybe I should just kill those tests and Assume()?

in test/functional/mempool_package_onemore.py:58 in 671de055c8 outdated

54@@ -55,14 +55,23 @@ def run_test(self):
55         assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0], second_chain])
56         # ...especially if its > 40k weight
57         assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0]], num_outputs=350)
58+        # ...not if it's submitted with other transactions

sdaftuar commented at 2:36 pm on December 12, 2023:

nit: I found the “not” in this comment confusing.

instagibbs commented at 6:05 pm on January 9, 2024:

reworded

in test/functional/mempool_package_onemore.py:67 in 671de055c8 outdated

63+        pkg_result = self.nodes[0].submitpackage(txns_hex)
64+        assert "too-long-mempool-chain" in pkg_result["tx-results"][txns[0].getwtxid()]["error"]
65+        assert_equal(pkg_result["tx-results"][txns[1].getwtxid()]["error"], "bad-txns-inputs-missingorspent")
66         # But not if it chains directly off the first transaction
67-        replacable_tx = self.wallet.send_self_transfer_multi(from_node=self.nodes[0], utxos_to_spend=[chain[0]])['tx']
68+        self.nodes[0].sendrawtransaction(replacable_tx["hex"])

murchandamus commented at 3:54 pm on December 12, 2023:

Nit: Did you mean replacEable?

instagibbs commented at 6:05 pm on January 9, 2024:

fixed

in src/validation.cpp:1063 in 56e2fb281a outdated

993@@ -972,7 +994,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
994         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string);
995     }
996 
997-    m_rbf = !ws.m_conflicts.empty();
998+    m_rbf |= !ws.m_conflicts.empty();

sdaftuar commented at 5:38 pm on December 12, 2023:

Perhaps a comment here would be helpful to explain why this change is needed.

instagibbs commented at 6:05 pm on January 9, 2024:

added

in src/validation.cpp:1538 in 228a5ed165 outdated

1429-        assert(!args.m_allow_replacement);
1430-        assert(!m_rbf);
1431+        // package to spend. Since we already checked conflicts, no transaction can spend a coin
1432+        // needed by another transaction in the package. We also need to make sure that no package
1433+        // tx replaces (or replaces the ancestor of) the parent of another package tx. As long as we
1434+        // check these two things, we don't need to track the coins spent.

sdaftuar commented at 6:01 pm on December 12, 2023:

I was trying to figure out exactly why this code is safe in a package RBF context. I think for a generalized package RBF, this would not be safe, because we do not verify that a child transaction in an incoming package (say) isn’t spending a coin in the mempool that would be conflicted by some other transaction in the package.

However, in this particular case, we are enforcing in PackageMempoolChecks() (further down) that the incoming package has no other in-mempool ancestors (ie the new package is a cluster of size 2), and so I think that condition is what precludes the scenario I’m concerned about above. (Specifically: if, say, we allowed the incoming package to have 1 in-mempool ancestor A, then it’s possible that a package could consist of transactions (P, C) such that P spent one of A’s outputs, while transaction C conflicted with A, which I don’t think would be caught anywhere right now.)

If my understanding here is correct, I think it’d be worth updating the comment to explain this a bit more (and perhaps we could add a test that covers the scenario I described).

instagibbs commented at 8:41 pm on December 12, 2023:

Pushed a functional test covering this case specifically which is rejected due to existence of mempool ancestors.

I’ll try and rephrase soon.

instagibbs commented at 6:05 pm on January 9, 2024:

rephrased into the two cases

sdaftuar commented at 7:34 pm on December 12, 2023: member

I read the code more carefully (other than the tests, which I just skimmed) – so far this looks good, just had a few comments. Planning to do some more thorough testing next.

DrahtBot added the label CI failed on Dec 12, 2023

in doc/policy/packages.md:67 in 671de055c8 outdated

47@@ -48,8 +48,13 @@ The following rules are enforced for all packages:
48      heavily connected, i.e. some transaction in the package is the ancestor or descendant of all

ismaelsadeeq commented at 12:24 pm on December 13, 2023:

In 671de055c8b3ff48b16b93d8389b80fead6342ac Commit title indicate " [doc] cpfp carveout is excluded in packages "

But the diff is not a only a doc change.

instagibbs commented at 6:05 pm on January 9, 2024:

changed wording of commit

sdaftuar commented at 1:14 pm on December 13, 2023: member

I thought of a couple issues with the current version of the PR that I wanted to summarize (longer form here):

The mempool might not be made “strictly better”, in the sense that there are more fees available for every size of transactions selected, using the RBF criteria in this PR. (I’m trying to figure out if there are any simple heuristics we can apply to resolve this issue.)
It occurred to me that users might run into a problem if they submit a package (A, B), where B has in-mempool conflicts and A does not, if A would relay fine on its own. Consider this scenario:
- node receives package (A, B), either via submitpackage or a future package-relay protocol
- node applies the package RBF rules here and accepts the package
- node announces transactions A and B to peers
- a peer downloads the transactions separately
- the peer validates tx A, and it makes it into the mempool on its own (perhaps it itself had no conflicts and satisfied min fee requirements)
- the peer validates tx B and rejects it, because under single-tx-rbf rules, B is an invalid replacement (it has a new unconfirmed input compared with what it conflicts with, namely tx A)

It seems to me that the only solution for issue 2 would be to somehow ensure that tx A would not be eligible for relay on its own. Are there any better ideas?

in src/policy/rbf.cpp:186 in 228a5ed165 outdated

180@@ -181,3 +181,54 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
181     }
182     return std::nullopt;
183 }
184+

ismaelsadeeq commented at 3:50 pm on December 13, 2023:

In 228a5ed16586ef93e7ec5c5da3ef4df0d5fc322f

0#include <algorithm>

instagibbs commented at 6:05 pm on January 9, 2024:

replaced section of code entirely

in src/validation.cpp:524 in 228a5ed165 outdated

506@@ -507,7 +507,7 @@ class MemPoolAccept
507                             /* m_bypass_limits */ false,

ismaelsadeeq commented at 4:13 pm on December 13, 2023:

Would be nice if this commit to be split into two, RBF utility functions and test in one commit and package rbf validation in another?

instagibbs commented at 6:05 pm on January 9, 2024:

replaced entirely(I think?)

instagibbs commented at 2:18 pm on December 14, 2023: member

I responded here: https://delvingbitcoin.org/t/cluster-mempool-rbf-thoughts/156/24?u=instagibbs

issue 1: awaiting further details on potential diagram check integration vs suggested heuristic I gave (currently reviewing this work) issue 2: known issue, wallet authors shouldn’t do that until cluster mempool fixes that (don’t cross-sponsor)

in test/functional/mempool_package_rbf.py:183 in 39c09efade outdated

178+        assert_greater_than_or_equal(1000, package_txns1[-1].get_vsize())
179+        node.submitpackage(package_hex1)
180+        self.assert_mempool_contents(expected=package_txns1)
181+
182+        # Package 2 has a higher feerate but lower absolute fee
183+        package_hex2, package_txns2 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE - fee_delta, child_fee=DEFAULT_FEE + fee_delta - Decimal("0.000000001"))

instagibbs commented at 7:07 pm on December 15, 2023:

too many zeroes on this, it’s a 1/10 of a sat

in test/functional/mempool_package_rbf.py:198 in 39c09efade outdated

193+        self.log.info("Check Package RBF must pay for the entire package's bandwidth")
194+        coin = self.coins.pop()
195+        package_hex4, package_txns4 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_FEE)
196+        node.submitpackage(package_hex4)
197+        self.assert_mempool_contents(expected=package_txns4, unexpected=[])
198+        package_hex5, package_txns5 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE - fee_delta, child_fee=DEFAULT_FEE + fee_delta + Decimal("0.000000001"))

instagibbs commented at 7:08 pm on December 15, 2023:

too many zeroes on this, it’s a 1/10 of a sat

DrahtBot removed the label CI failed on Dec 16, 2023

in src/policy/rbf.cpp:227 in 228a5ed165 outdated

222+        const CFeeRate replacement_miner_score(replacement_fees, replacement_vsize);
223+
224+        for (const auto& entry : original_transactions) {
225+            CFeeRate original_miner_score(entry->GetModFeesWithAncestors(), entry->GetSizeWithAncestors());
226+            original_miner_score = std::min(original_miner_score, CFeeRate(entry->GetModifiedFee(), entry->GetTxSize()));
227+            if (replacement_miner_score < original_miner_score) {

murchandamus commented at 9:17 pm on December 28, 2023:

Shouldn’t this be greater or equal to exceed the original?

The package feerate (total package fee / total package vsize) must exceed the min(individual feerate, ancestor feerate) of every transaction that would be evicted (direct and indirect conflicts).

instagibbs commented at 7:03 pm on January 5, 2024:

Probably yeah, tests should catch this case regardless. Will add one after deciding what way to go with heuristic vs diagram check

instagibbs commented at 6:05 pm on January 9, 2024:

replaced with diagram check; should be fine now since it must be strictly superior.

murchandamus commented at 9:50 pm on December 28, 2023: contributor

Concept ACK, did a quick skim review, will review more thoroughly

ismaelsadeeq commented at 1:30 pm on December 30, 2023: member

Concept ACK

instagibbs commented at 6:59 pm on January 5, 2024: member

Considering an alternative branch with fee diagram checks instead of heuristic here: https://github.com/instagibbs/bitcoin/commits/feefrac_package_rbf

in src/policy/rbf.h:110 in 228a5ed165 outdated

105@@ -106,4 +106,18 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
106                                       CFeeRate relay_fee,
107                                       const uint256& txid);
108 
109+/** Ensure we are only attempting RBF against connected components we can easily
110+ * compute mining scores from.

glozow commented at 11:04 am on January 8, 2024:

Maybe add “Each entry must be part of a cluster with size <=2.”

instagibbs commented at 6:05 pm on January 9, 2024:

this all got moved internal to mempool

in src/policy/rbf.cpp:212 in 228a5ed165 outdated

207+            if (child->get().GetCountWithAncestors() != 2) {
208+                return strprintf("Child in package RBF has too many ancestors");
209+            }
210+        } else {
211+            return strprintf("Too many descendants of direct conflict: %zu", desc_count);
212+        }

glozow commented at 11:32 am on January 8, 2024:

Looks correct. If you’re willing to entertain a suggestion to make it more readable, I think the following logic is easier to follow and the strings less ambiguous. Since “desc count’ and “anc count” usually mean inclusive of tx, it’s confusing if it’s different in this function.

 0        // Ancestor and descendant counts are inclusive of the tx itself.
 1        const auto ancestor_count{direct_conflict->GetCountWithAncestors()};
 2        const auto descendant_count{direct_conflict->GetCountWithDescendants()};
 3        const bool has_ancestor{ancestor_count > 1};
 4        const bool has_descendant{descendant_count > 1};
 5
 6        const auto& txid_string{direct_conflict->GetSharedTx()->GetHash().ToString()};
 7
 8        // The only allowed configurations are:
 9        // 1 ancestor and 0 descendant
10        // 0 ancestor and 1 descendant
11        // 0 ancestor and 0 descendant
12        if (ancestor_count > 2) {
13            return strprintf("%s has %u ancestors, max 1 allowed", ancestor_count, txid_string", txid_string);
14        } else if (descendant_count > 2) {
15            return strprintf("%s has %u descendants, max 1 allowed", ancestor_count, txid_string", txid_string);
16        } else if (has_ancestor && has_descendant) {
17            return strprintf("%s has both ancestor and descendant", txid_string);
18        }
19
20        // Additionally enforce that:
21        // If we have a parent, we are its only child.
22        // If we have a child,  we are its only parent.
23        if (has_descendant) {
24            const auto& our_child = direct_conflict->GetMemPoolChildrenConst().begin();
25            if (our_child->get().GetCountWithAncestors() > 2) {
26                return strprintf("%s is not the only parent of child %s",
27                                 txid_string, our_child->get().GetSharedTx()->GetHash().ToString());
28            }
29        } else if (has_ancestor) {
30            const auto& our_parent = direct_conflict->GetMemPoolParentsConst().begin();
31            if (our_parent->get().GetCountWithDescendants() > 2) {
32                return strprintf("%s is not the only child of parent %s",
33                                 txid_string, our_parent->get().GetSharedTx()->GetHash().ToString());
34            }
35        }

instagibbs commented at 6:04 pm on January 9, 2024:

cleaner, thanks. will take(minus typos :) )

in src/validation.cpp:1172 in 228a5ed165 outdated

1140+    }
1141+
1142+    // Use the child as the transaction for attributing errors to.
1143+    const Txid child_hash = child_ws.m_ptx->GetHash();
1144+    if (const auto err_string{PaysForRBF(m_conflicting_fees, m_total_modified_fees, m_total_vsize,
1145+                                         m_pool.m_incremental_relay_feerate, child_hash)}) {

glozow commented at 11:42 am on January 8, 2024:

nit: good place to use annotations to double-check we’re passing in the right fees:

0    if (const auto err_string{PaysForRBF(/*original_fees=*/m_conflicting_fees, /*replacement_fees=*/m_total_modified_fees, m_total_vsize,
1                                         m_pool.m_incremental_relay_feerate, child_hash)}) {

instagibbs commented at 6:04 pm on January 9, 2024:

taken

in src/test/rbf_tests.cpp:322 in 228a5ed165 outdated

280+    BOOST_CHECK(CheckConflictTopology({entry9_unchained}) == std::nullopt);
281+
282+    // Add 1 descendant, still ok
283+    add_descendants(tx9, 1, pool);
284+    BOOST_CHECK(CheckConflictTopology({entry9_unchained}) == std::nullopt);
285+

glozow commented at 11:45 am on January 8, 2024:

Maybe add a test case for CheckConflictTopology where our 1 parent has 2 children, or our 1 child has 2 parents

instagibbs commented at 6:04 pm on January 9, 2024:

Pretty sure following section covers it, adding two descendants one by one to tx10 and checking the resulting failure

in src/validation.cpp:1593 in 56e2fb281a outdated

1389@@ -1366,7 +1390,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1390             const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids :
1391                 std::vector<Wtxid>{ws.m_ptx->GetWitnessHash()};
1392             results.emplace(ws.m_ptx->GetWitnessHash(),
1393-                            MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions),
1394+                            MempoolAcceptResult::Success(std::move(m_replaced_transactions),

glozow commented at 11:56 am on January 8, 2024:

56e2fb281a0e62cb70e610370c2dce9e79bc05f0

We copy this list into every transaction’s MempoolAcceptResult (which does makes sense since both the parent and the child participated in the replacement of these txns - that’s probably helpful for showing package RBFs in the RPC results). But the duplicates are a problem when we add to vExtraTxnForCompact in p2p: https://github.com/bitcoin/bitcoin/blob/04b9df0f9fd95e907b2e8bf823d63e9dfb37b4ad/src/net_processing.cpp#L4254-L4256

Possible solutions:

Only add m_replaced_transactions to the last result in the SubmitPackage loop (along the lines of “the child is the true ‘replacer’ of all these txns”). I think this is fine since this var is per-subpackage.
Add logic to net_processing to deal with duplicates. Probably the most robust solution, and allows us to keep the lists per tx in the RPC results.
Add a new field to PackageMempoolAcceptResult and don’t populate the MempoolAcceptResults when there is a pcakage RBF (ehhh kind of ugly).

instagibbs commented at 1:47 pm on January 8, 2024:

dealing with duplicates seems strictly superior, will take a look, thanks!

instagibbs commented at 1:50 pm on January 8, 2024:

the more work solutions would be to refactor everything to have a “subpackage eval” state, but juice is probably not worth the squeeze until cluster mempool, where “chunk eval” is likely the natural boundary

glozow commented at 12:02 pm on January 9, 2024:

yeah I think a temporary set<Wtxid> within net processing is probably the easiest

instagibbs commented at 8:05 pm on January 9, 2024:

left a comment for future work

in src/validation.cpp:1227 in 56e2fb281a outdated

1152                 hash.ToString(),
1153                 tx.GetWitnessHash().ToString(),
1154-                FormatMoney(ws.m_modified_fees - ws.m_conflicting_fees),
1155-                (int)entry->GetTxSize() - (int)ws.m_conflicting_size);
1156+                FormatMoney(ws.m_modified_fees - m_conflicting_fees),
1157+                (int)entry->GetTxSize() - (int)m_conflicting_size);

glozow commented at 12:33 pm on January 8, 2024:

Just realized this log is pretty nonsensical for package RBF, and also whenever there are multiple transactions being replaced in by a single one.

Maybe this log should be:

0        LogPrint(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n",
1                it->GetTx().GetHash().ToString(),
2                it->GetTx().GetWitnessHash().ToString(),
3                it->GetFee(),
4                it->GetTxSize(),
5                hash.ToString(),
6                tx.GetWitnessHash().ToString(),
7                entry->GetFee(),
8                entry->GetTxSize());

And we should add a replacement summary log at the end of SubmitPackage and AcceptSingleTransaction:

0    if (!m_replaced_transactions.empty()) {
1        LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with %u new one(s) for %s additional fees, %d delta bytes\n",
2                 workspaces.size(), m_replaced_transactions.size(),
3                 m_total_modified_fees - m_conflicting_fees,
4                 m_total_vsize - m_conflicting_size);
5    }

0    if (!m_replaced_transactions.empty()) {
1        LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with 1 new one for %s additional fees, %d delta bytes\n",
2                 m_replaced_transactions.size(),
3                 m_total_modified_fees - m_conflicting_fees,
4                 m_total_vsize - m_conflicting_size);
5    }

instagibbs commented at 8:05 pm on January 9, 2024:

done

glozow commented at 12:35 pm on January 8, 2024: member

Combed through the code a bit more. Generally approach ACK for “cluster 2 replace cluster 2” idea.

instagibbs force-pushed on Jan 9, 2024

instagibbs commented at 4:30 pm on January 9, 2024: member

pushed update with fee diagram checks instead of heuristics. @sipa please see https://github.com/bitcoin/bitcoin/pull/28984/commits/9dda95d58442e4884a57216472c991c62f87ef1f and similar

Addressing other comments next.

instagibbs force-pushed on Jan 9, 2024

DrahtBot added the label CI failed on Jan 9, 2024

DrahtBot commented at 6:05 pm on January 9, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/20309843311

instagibbs force-pushed on Jan 9, 2024

sipa commented at 10:02 pm on January 9, 2024: member

@instagibbs See https://github.com/sipa/bitcoin/commits/pr28984 for a commit that works exactly for all FeeFrac objects. It uses C++20 three-way comparisons to also reduce the line count by 25%.

instagibbs force-pushed on Jan 10, 2024

DrahtBot removed the label CI failed on Jan 10, 2024

instagibbs commented at 6:08 pm on January 12, 2024: member

broke off all the incentive compatibility check stuff into its own PR: #29242

Putting this in draft for now to divert attention there

instagibbs marked this as a draft on Jan 12, 2024

DrahtBot added the label CI failed on Jan 16, 2024

achow101 referenced this in commit 7143d43884 on Feb 10, 2024

DrahtBot added the label Needs rebase on Feb 10, 2024

instagibbs force-pushed on Feb 12, 2024

DrahtBot removed the label Needs rebase on Feb 12, 2024

DrahtBot removed the label CI failed on Feb 12, 2024

DrahtBot added the label Needs rebase on Mar 9, 2024

glozow referenced this in commit c2dbbc35b9 on Mar 26, 2024

instagibbs force-pushed on Mar 26, 2024

instagibbs marked this as ready for review on Mar 26, 2024

instagibbs commented at 3:06 pm on March 26, 2024: member

Rebased on master, and ready for review

note that I will likely rebase this on top of #29735 when merged, to pull in fill_mempool

instagibbs force-pushed on Mar 26, 2024

DrahtBot removed the label Needs rebase on Mar 26, 2024

instagibbs force-pushed on Mar 26, 2024

DrahtBot added the label CI failed on Mar 26, 2024

DrahtBot removed the label CI failed on Mar 27, 2024

glozow referenced this in commit bdb33ec519 on Apr 11, 2024

instagibbs force-pushed on Apr 11, 2024

instagibbs commented at 1:38 pm on April 15, 2024: member

ready for review

in src/net_processing.cpp:1802 in b0ab6875bb outdated

1797@@ -1798,6 +1798,8 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c
1798 
1799 void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx)
1800 {
1801+    // Note: If package evaluation is exposed, we need to make sure the same
1802+    // tx isn't added multiple times in one invocation.

glozow commented at 4:26 pm on April 17, 2024:

This seems like a merge conflict with #28970, maybe add the fix here instead?

instagibbs commented at 4:56 pm on April 18, 2024:

I think #28970 is handling this properly, as it’s the only place where we expose package evaluation over p2p. Can just remove this commit?

in src/validation.cpp:1105 in 34c513554c outdated

1101@@ -1098,16 +1102,15 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws)
1102     }
1103     if (const auto err_string{PaysForRBF(m_conflicting_fees, ws.m_modified_fees, ws.m_vsize,
1104                                          m_pool.m_incremental_relay_feerate, hash)}) {
1105-        // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not
1106-        // TX_RECONSIDERABLE, because it cannot be bypassed using package validation.
1107-        // This must be changed if package RBF is enabled.
1108-        return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
1109+        // Result may change in another package

glozow commented at 4:29 pm on April 17, 2024:

34c513554c7be97d91446126bb3ef0de2884fc19

This seems slightly inaccurate, as it wouldn’t currently be in a package if ReplacementChecks is happening?

instagibbs commented at 5:52 pm on April 17, 2024:

“in a package”?

instagibbs commented at 1:20 pm on April 19, 2024:

pushed updated text

glozow commented at 5:13 pm on April 17, 2024: member

I think we had some docs written for doc/policy/packages.md and doc/policy/mempool_replacements.md?

in src/test/util/txmempool.cpp:81 in f0c6e68d5d outdated

85+            }
86+
87+            // Replacements can't happen for subpackages larger than 2
88+            if (!atmp_result.m_replaced_transactions.value().empty() &&
89+                atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() > 2) {
90+                 return strprintf("tx %s was part of a too-large package RBF subpackage",

glozow commented at 6:12 pm on April 17, 2024:

Maybe you can also check the topology/cluster size of this tx in mempool?

instagibbs commented at 1:21 pm on April 19, 2024:

added a small check here using GetEntry that takes Wtxids.

instagibbs force-pushed on Apr 18, 2024

DrahtBot added the label CI failed on Apr 18, 2024

DrahtBot commented at 7:49 pm on April 18, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/23993793129

DrahtBot removed the label CI failed on Apr 18, 2024

in doc/policy/packages.md:51 in 033736bcd9 outdated

47@@ -48,8 +48,13 @@ The following rules are enforced for all packages:
48      heavily connected, i.e. some transaction in the package is the ancestor or descendant of all
49      the other transactions.
50 
51-The following rules are only enforced for packages to be submitted to the mempool (not enforced for
52-test accepts):
53+* [CPFP Carve Out](./mempool-limits.md#CPFP-Carve-Out) is disabled. (#21800)

ismaelsadeeq commented at 1:12 pm on April 22, 2024:

At first when I see CPFP carveout disabled I thought we are not longer supporting single transaction CPFP carveout because the linked document is describing rules for a single transaction CPFP carveout.

But from the commit message and reading the code I understand thats not the case, I think we should be explicit that it’s package CPFP carvout that is disabled.

0* [Package CPFP Carve Out](./mempool-limits.md#CPFP-Carve-Out) is disabled. (#21800)

instagibbs commented at 4:38 pm on April 26, 2024:

added an extra phrase, hope that helps?

in doc/policy/packages.md:74 in 033736bcd9 outdated

52-test accepts):
53+* [CPFP Carve Out](./mempool-limits.md#CPFP-Carve-Out) is disabled. (#21800)
54+
55+   - *Rationale*: This carve out cannot be accurately applied when there are multiple transactions'
56+     ancestors and descendants being considered at the same time.
57+

ismaelsadeeq commented at 1:14 pm on April 22, 2024:

This also implies that single conflict package RBF carvout is not supported, is this worth mentioning?

instagibbs commented at 2:41 pm on April 22, 2024:

The carve-out, by definition, doesn’t have support in (sub)package contexts. Specific suggestions to clarify are welcome of course.

in test/functional/mempool_package_onemore.py:71 in 033736bcd9 outdated

67-        replacable_tx = self.wallet.send_self_transfer_multi(from_node=self.nodes[0], utxos_to_spend=[chain[0]])['tx']
68+        self.nodes[0].sendrawtransaction(replaceable_tx["hex"])
69         # and the second chain should work just fine
70         self.chain_tx([second_chain])
71 
72         # Make sure we can RBF the chain which used our carve-out rule

ismaelsadeeq commented at 1:15 pm on April 22, 2024:

0        # Ensure an individual transaction with single direct conflict can RBF the chain which used our carve-out rule

instagibbs commented at 4:38 pm on April 26, 2024:

taken

in test/functional/mempool_package_onemore.py:74 in 033736bcd9 outdated

73-        replacable_tx.vout[0].nValue -= 1000000
74-        self.nodes[0].sendrawtransaction(replacable_tx.serialize().hex())
75+        replacement_tx = replaceable_tx["tx"]
76+        replacement_tx.vout[0].nValue -= 1000000
77+        self.nodes[0].sendrawtransaction(replacement_tx.serialize().hex())
78

ismaelsadeeq commented at 1:17 pm on April 22, 2024:

Maybe test that Package RBF carveout is not supported?

0
1        # But We can not package RBF the chain which used our carveout rule
2        replaceable_tx_conflict = self.wallet.create_self_transfer_multi(utxos_to_spend=[chain[0]])
3        txns_conflict = [replaceable_tx_conflict["tx"], self.wallet.create_self_transfer_multi(utxos_to_spend=replaceable_tx_conflict["new_utxos"])["tx"]]
4        txns_conflict_hex = [tx.serialize().hex() for tx in txns_conflict]
5        assert_equal(self.nodes[0].testmempoolaccept(txns_conflict_hex)[0]["reject-reason"], "bip125-replacement-disallowed")
6        pkg_result = self.nodes[0].submitpackage(txns_conflict_hex)
7        assert "too-long-mempool-chain" in pkg_result["tx-results"][txns_conflict[0].getwtxid()]["error"]
8        assert_equal(pkg_result["tx-results"][txns_conflict[1].getwtxid()]["error"], "bad-txns-inputs-missingorspent")

instagibbs commented at 4:38 pm on April 26, 2024:

would need to do a bit more work to make sure it actually would be attempting a package rbf. I think we can rely on our conflict topology check tests to ensure this, at least for default mempool chains?

ismaelsadeeq commented at 9:27 am on April 29, 2024:

I learned that we can RBF a transaction that gets into the mempool using carveout. I wanted to test that you wouldn’t RBF that transaction using a package.

Even if the arrangement attempts to RBF or not, I think the error in the suggestion is how it would fail. We would set m_allow_carveout to false while evaluating the first parent transaction and fail its evaluation due to a TX_MEMPOOL_POLICY violation of too-long-mempool-chain. hence, all the descendants would fail due to missing input.

I just think this is a “nice-to-have” test, but not necessarily required for this PR.

instagibbs commented at 1:11 pm on April 29, 2024:

looked a bit closer, how is this different from the “# …even if it’s submitted with other transactions” case above in the test? To check package rbf the only difference is making sure it’s evaluated as a single subpackage(by having the child feerate be larger)

ismaelsadeeq commented at 1:22 pm on April 29, 2024:

Ahh I see, it’s the same code path.

instagibbs commented at 1:24 pm on April 29, 2024:

oh right, neither of these cases will do a package rbf, as the package has in-mempool ancestors.

It would have to be a scenario where the ancestor limits are set to 1, then a package RBF of size 2 replaces that single tx, which would fail prechecks?

ismaelsadeeq commented at 1:39 pm on April 29, 2024:

Hmm, I think it will always be the same code path that will be hit because the parent transaction (in the cluster size 2 replacement package) will be detected as conflict with the transaction that was carved out (because it has to go through PreChecks), and we don’t allow package RBF carveout, so the subpackage evaluation with the parent and child will not be executed?

instagibbs commented at 4:55 pm on April 29, 2024:

even if the test would manage to hit the case where it would have considered a package rbf(this case won’t), it should be caught in both PreChecks and package mempool checks separately as chain limits are being busted.

ismaelsadeeq commented at 1:29 pm on April 22, 2024: member

Continued Reviewing.

826ec4c47c0d18a2d9b437b33ea4ceba67e870c1
033736bcd9fc16e244e52e72fe7c7ff030690ece
fa3f94a858
8df3b9f7a9
a6e2372488
a81d265fb0
59e2271167
ce04c35217
2da61cab7e
ae2fd7b1d6
d7d697ba40
2eb09886f3

in doc/policy/packages.md:77 in 2eb09886f3 outdated

55+   - *Rationale*: This carve out cannot be accurately applied when there are multiple transactions'
56+     ancestors and descendants being considered at the same time.
57+
58+The following rules are only enforced for packages to be submitted to the mempool (not
59+enforced for test accepts):
60

ismaelsadeeq commented at 1:32 pm on April 22, 2024:

In packages.md there is note stating “Replace By Fee is currently disabled for packages.” This is no longer the case, some variants of package can now be RBF’d I think we should indicate that we now support cluster size 2 package replacement into node’s mempool stating the new acceptance rules.

instagibbs commented at 4:37 pm on April 26, 2024:

added updated comment

in src/validation.cpp:1154 in a6e2372488 outdated

1150+        // Aggregate all conflicts into one set.
1151+        direct_conflict_iters.merge(ws.m_iters_conflicting);
1152+    }
1153+
1154+
1155+    // Calculate all conflicting entries and enforce Rules 2 and 5.

ismaelsadeeq commented at 1:49 pm on April 25, 2024:

Question: here you mean the package RBF rules in OP not BIP125 right?

instagibbs commented at 4:37 pm on April 26, 2024:

Deleting this comment as I don’t think it’s helpful

in src/test/txpackage_tests.cpp:1020 in a81d265fb0 outdated

918+            coinbaseKey, parent_spk, coinbase_value - 800, /*submit=*/false));
919+        CTransactionRef tx_child_2 = MakeTransactionRef(CreateValidMempoolTransaction(
920+            tx_parent_2, /*input_vout=*/0, /*input_height=*/101,
921+            child_key, child_spk, coinbase_value - 800 - 200, /*submit=*/false));
922+
923+        CTransactionRef tx_parent_3 = MakeTransactionRef(CreateValidMempoolTransaction(

ismaelsadeeq commented at 3:55 pm on April 25, 2024:

nit: tx_parent_3 is the same transaction tx_parent_1 maybe just use tx_parent_1 as parent of tx_child_3

instagibbs commented at 4:37 pm on April 26, 2024:

conceptually the test is cleaner if each parent is different, so I updated the parent to be slightly different, and asserted the parents are all have different txids

in src/test/txpackage_tests.cpp:936 in a81d265fb0 outdated

931+        Package package1{tx_parent_1, tx_child_1};
932+        // 1 parent paying 800sat, 1 child paying 200sat. Both v2.
933+        Package package2{tx_parent_2, tx_child_2};
934+        // 1 parent paying 200sat, 1 child paying 1300. Both v2.
935+        Package package3{tx_parent_3, tx_child_3};
936+        // In all packages, the parents conflict with each other

ismaelsadeeq commented at 3:56 pm on April 25, 2024:

package1 and package3 has the same parent

instagibbs commented at 4:37 pm on April 26, 2024:

fixed

in test/functional/mempool_package_rbf.py:37 in a81d265fb0 outdated

32+        self.extra_args = [[
33+            "-datacarriersize=100000",
34+            "-maxmempool=5",
35+        ]]
36+
37+    def fill_mempool(self):

ismaelsadeeq commented at 10:04 am on April 26, 2024:

You moved fill_mempool to util in #29735

instagibbs commented at 4:37 pm on April 26, 2024:

forgot to remove, done

in test/functional/mempool_package_rbf.py:237 in a81d265fb0 outdated

232+
233+        pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]])
234+        assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_parent['tx'].rehash()}; too many potential replacements (125 > 100)\n", pkg_results["package_msg"])
235+        self.generate(node, 1)
236+
237+    def test_package_rbf_conflicting_conflicts(self):

ismaelsadeeq commented at 11:29 am on April 26, 2024:

I think this might be a better name

0    def test_package_rbf_with_conflicting_packages(self):

instagibbs commented at 4:37 pm on April 26, 2024:

done

in test/functional/mempool_package_rbf.py:261 in a81d265fb0 outdated

256+        submitres3 = node.submitpackage(package_hex3)
257+        assert_equal(sorted(submitres3["replaced-transactions"]), sorted([tx.rehash() for tx in package_txns2]))
258+        self.assert_mempool_contents(expected=package_txns3, unexpected=package_txns2)
259+
260+    def test_package_rbf_partial(self):
261+        self.log.info("Test that package RBF works when a transaction was already submitted")

ismaelsadeeq commented at 11:52 am on April 26, 2024:

The parent transaction will be detected as inmempool as such the child just get added to the mempool if it passes normal transaction consensus and policy rules, the package does not have any conflict and hence package RBF rules are not checked.

This test also passes when I removed package RBF commits

0    def test_submitpackage_with_a_mempool_parent(self):
1        self.log.info("Test that submitpackage works when parent transactions was already submitted")

instagibbs commented at 4:37 pm on April 26, 2024:

Hmm, yeah I don’t think this test is meaningful. Removed.

in test/functional/mempool_package_rbf.py:244 in a81d265fb0 outdated

270+        node.sendrawtransaction(package_hex2[0])
271+        node.submitpackage(package_hex2)
272+        self.assert_mempool_contents(expected=package_txns2, unexpected=package_txns1)
273+        self.generate(node, 1)
274+
275+    def test_too_numerous_ancestors(self):

ismaelsadeeq commented at 12:17 pm on April 26, 2024:

nit:

0    def test_package_rbf_with_numerous_ancestors(self):

instagibbs commented at 4:37 pm on April 26, 2024:

it’s a package rbf test, I think name is fine

in test/functional/mempool_package_rbf.py:485 in a81d265fb0 outdated

480+
481+        # Check that replacements were actually rejected
482+        self.assert_mempool_contents(expected=[], unexpected=package_txns1 + package_txns2 + package_txns3)
483+        self.generate(node, 1)
484+
485+    def test_too_numerous_pkg(self):

ismaelsadeeq commented at 12:39 pm on April 26, 2024:

nit:

0    def test_package_rbf_with_wrong_pkg_size(self):

instagibbs commented at 4:37 pm on April 26, 2024:

taken

in test/functional/mempool_package_rbf.py:548 in a81d265fb0 outdated

543+
544+        package_hex1, package_txns1 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE + fee_delta, child_fee=DEFAULT_FEE - fee_delta)
545+        node.submitpackage(package_hex1)
546+        self.assert_mempool_contents(expected=package_txns1)
547+
548+        # Package 2 feerate is below the rate of directly conflicted parent, even though

ismaelsadeeq commented at 12:41 pm on April 26, 2024:

0        # Package 2 feerate is below the feerate of directly conflicted parent, even though

instagibbs commented at 4:37 pm on April 26, 2024:

taken

in test/functional/mempool_package_rbf.py:565 in a81d265fb0 outdated

560+
561+        self.log.info("Test that package RBF doesn't have issues with mempool<->package conflicts via inconsistency")
562+        node = self.nodes[0]
563+        coin = self.coins.pop()
564+
565+        # Put simple tx in mempool to chain off of

ismaelsadeeq commented at 12:45 pm on April 26, 2024:

Incomplete sentence?

instagibbs commented at 4:37 pm on April 26, 2024:

complete sentence, but not grammatically correct or useful, removed

in src/validation.cpp:1297 in ce04c35217 outdated

1270@@ -1271,13 +1271,15 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
1271     // Remove conflicting transactions from the mempool
1272     for (CTxMemPool::txiter it : m_all_conflicts)
1273     {
1274-        LogPrint(BCLog::MEMPOOL, "replacing tx %s (wtxid=%s) with %s (wtxid=%s) for %s additional fees, %d delta bytes\n",
1275+        LogPrint(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n",

ismaelsadeeq commented at 2:05 pm on April 26, 2024:

nice this will be helpful for collecting data, previously we only know that a transaction is replaced now it indicates which tx replaced it!

E.g two transaction package evicting another two

 02024-04-26T14:36:53.889903Z [httpworker.3] [validation.cpp:1282] [Finalize] [mempool] replacing mempool tx 1ae8118ceb71249b2af793f671a1293a4c999e89f5091f52c42a3cf1e08c6840 (wtxid=97fc8654dcbd025b4165c56179d124ee4adf2d65acf8a66f8f5464dab7d07089, fees=10000, vsize=104). New tx a95629c7f4690199fce1ffba70b8fa9ce854d74da76e9e88fd0d4676211f10d5 (wtxid=009f7beb2df5b699bea4233a5b9b6fab2a80d42639b19291d4909a63f8739c47, fees=10000, vsize=104)
 12024-04-26T14:36:53.889910Z [httpworker.3] [validation.cpp:1282] [Finalize] [mempool] replacing mempool tx 9bd6895590404d3b04a208bbebf48a0397cb43b4df7ad349427be5e7dc1a2bbb (wtxid=e26f5339fcf7d3232c9bd58a99e6e2cb9c2369d70e7f8fed7296221ee8667119, fees=10000, vsize=104). New tx a95629c7f4690199fce1ffba70b8fa9ce854d74da76e9e88fd0d4676211f10d5 (wtxid=009f7beb2df5b699bea4233a5b9b6fab2a80d42639b19291d4909a63f8739c47, fees=10000, vsize=104)
 22024-04-26T14:36:53.889920Z [httpworker.3] [validationinterface.cpp:203] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=1ae8118ceb71249b2af793f671a1293a4c999e89f5091f52c42a3cf1e08c6840 wtxid=97fc8654dcbd025b4165c56179d124ee4adf2d65acf8a66f8f5464dab7d07089 reason=replaced
 32024-04-26T14:36:53.889929Z [scheduler] [validationinterface.cpp:203] [operator()] [validation] TransactionRemovedFromMempool: txid=1ae8118ceb71249b2af793f671a1293a4c999e89f5091f52c42a3cf1e08c6840 wtxid=97fc8654dcbd025b4165c56179d124ee4adf2d65acf8a66f8f5464dab7d07089 reason=replaced
 42024-04-26T14:36:53.889938Z [httpworker.3] [validationinterface.cpp:203] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=9bd6895590404d3b04a208bbebf48a0397cb43b4df7ad349427be5e7dc1a2bbb wtxid=e26f5339fcf7d3232c9bd58a99e6e2cb9c2369d70e7f8fed7296221ee8667119 reason=replaced
 52024-04-26T14:36:53.889946Z [scheduler] [validationinterface.cpp:203] [operator()] [validation] TransactionRemovedFromMempool: txid=9bd6895590404d3b04a208bbebf48a0397cb43b4df7ad349427be5e7dc1a2bbb wtxid=e26f5339fcf7d3232c9bd58a99e6e2cb9c2369d70e7f8fed7296221ee8667119 reason=replaced
 62024-04-26T14:36:53.889974Z [httpworker.3] [validation.cpp:1378] [SubmitPackage] [mempool] replaced 2 mempool transactions with 2 new one(s) for 40000 additional fees, 0 delta bytes
 72024-04-26T14:36:53.889981Z [httpworker.3] [validationinterface.cpp:193] [TransactionAddedToMempool] [validation] Enqueuing TransactionAddedToMempool: txid=a95629c7f4690199fce1ffba70b8fa9ce854d74da76e9e88fd0d4676211f10d5 wtxid=009f7beb2df5b699bea4233a5b9b6fab2a80d42639b19291d4909a63f8739c47
 82024-04-26T14:36:53.889988Z [httpworker.3] [validationinterface.cpp:193] [TransactionAddedToMempool] [validation] Enqueuing TransactionAddedToMempool: txid=69e3f27c8097c2f76b32c3f6fb31cc11d89b674bed800cf61236e921c8c9d5c3 wtxid=ed7b933757b73937bf454af80e60cbd9b4fb24d7ce95d297519f5327a0470333
 92024-04-26T14:36:53.889995Z [scheduler] [validationinterface.cpp:193] [operator()] [validation] TransactionAddedToMempool: txid=a95629c7f4690199fce1ffba70b8fa9ce854d74da76e9e88fd0d4676211f10d5 wtxid=009f7beb2df5b699bea4233a5b9b6fab2a80d42639b19291d4909a63f8739c47
102024-04-26T14:36:53.890001Z [scheduler] [validationinterface.cpp:193] [operator()] [validation] TransactionAddedToMempool: txid=69e3f27c8097c2f76b32c3f6fb31cc11d89b674bed800cf61236e921c8c9d5c3 wtxid=ed7b933757b73937bf454af80e60cbd9b4fb24d7ce95d297519f5327a0470333

in src/validation.cpp:1463 in ce04c35217 outdated

1458@@ -1450,6 +1459,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
1459         m_pool.m_signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence());
1460     }
1461 
1462+    if (!m_replaced_transactions.empty()) {
1463+        LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with 1 new one for %s additional fees, %d delta bytes\n",

ismaelsadeeq commented at 2:49 pm on April 26, 2024:

nit:

0        LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with 1 new transaction for %s additional fees, %d delta bytes\n",

instagibbs commented at 4:36 pm on April 26, 2024:

taken

in src/validation.cpp:1466 in ce04c35217 outdated

1458@@ -1450,6 +1459,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
1459         m_pool.m_signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence());
1460     }
1461 
1462+    if (!m_replaced_transactions.empty()) {
1463+        LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with 1 new one for %s additional fees, %d delta bytes\n",
1464+                 m_replaced_transactions.size(),
1465+                 ws.m_modified_fees - (int)m_conflicting_fees,
1466+                 ws.m_vsize - (int)m_conflicting_size);

ismaelsadeeq commented at 2:50 pm on April 26, 2024:

nit: use static cast?

instagibbs commented at 4:36 pm on April 26, 2024:

done

instagibbs force-pushed on Apr 26, 2024

DrahtBot commented at 4:41 pm on April 26, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24308043248

DrahtBot added the label CI failed on Apr 26, 2024

instagibbs commented at 4:46 pm on April 26, 2024: member

all feedback should be addressed now

instagibbs force-pushed on Apr 26, 2024

DrahtBot removed the label CI failed on Apr 26, 2024

ismaelsadeeq approved

ismaelsadeeq commented at 1:48 pm on April 29, 2024: member

Code Review ACK 7cede4caa0a6b3dd57397d96ee98239fb890ca32

My comments were addressed and I have reviewed all the commits and test this locally on regtest.

I’ve fuzz this PR for a while locally and no any crash.

DrahtBot requested review from glozow on Apr 29, 2024

DrahtBot requested review from murchandamus on Apr 29, 2024

instagibbs force-pushed on Apr 30, 2024

instagibbs commented at 5:45 pm on April 30, 2024: member

rebased on master due to conflict from https://github.com/bitcoin/bitcoin/pull/29906

ismaelsadeeq commented at 8:48 am on May 1, 2024: member

reACK 69deec6fd074e8524dd11103834739b02f50e814

instagibbs force-pushed on May 1, 2024

in src/validation.cpp:1134 in ec85c4b712 outdated

1134+    // No conflicts means we're finished. Further checks are all RBF-only.
1135+    if (!m_rbf) return true;
1136+
1137+    // We're in package RBF context; replacement proposal must be size 2
1138+    if (workspaces.size() != 2) {
1139+        return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: replacing cluster not size two");

instagibbs commented at 12:43 pm on May 1, 2024:

Should all these failures be PCKG_POLICY? We have individual errors which are reported as well, and now with 1P1C relay, we probably want to act on failures at p2p layer?

instagibbs commented at 2:58 pm on May 8, 2024:

update: in master we’re now processing these PCKG_POLICY cases, so should be ok? @glozow

glozow commented at 9:00 am on May 9, 2024:

yeah, I think PCKG_POLICY is fine given https://github.com/bitcoin/bitcoin/pull/30012/commits/6119f76ef72c1adc55c1a384c20f8ba9e1a01206. Just need to make sure the MempoolAcceptResults are correct so that we are attributing the failures correctly

instagibbs commented at 2:11 pm on May 9, 2024:

IIUC It will leave the “individual failures” in place which should be: parent: TX_RECONSIDERABLE child: TX_MISSING_INPUTS

instagibbs force-pushed on May 1, 2024

instagibbs commented at 12:45 pm on May 1, 2024: member

rebased to pick up 1P1C relay

in src/validation.cpp:1150 in d08e7a9ab6 outdated

1145@@ -1146,6 +1146,12 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
1146         }
1147     }
1148 
1149+    // Should be parent->child topology
1150+    const auto parent_result{FindInPackageParents(txns, txns.back())};

glozow commented at 3:31 pm on May 2, 2024:

in d08e7a9ab6b353f415ca30f7714b12f39f84fe48, instead of FindInPackageParents, maybe just call IsChildWithParents()? Or even better, Assume(IsChildWithParents())?

instagibbs commented at 4:13 pm on May 2, 2024:

Oh even easier than what I replaced it with, duh

instagibbs commented at 4:21 pm on May 2, 2024:

pushed with the Assume() check inline with the “making sure package is cluster size two” check

instagibbs force-pushed on May 2, 2024

DrahtBot added the label Needs rebase on May 3, 2024

instagibbs force-pushed on May 3, 2024

instagibbs commented at 2:11 pm on May 3, 2024: member

rebased due to conflict on master

DrahtBot removed the label Needs rebase on May 3, 2024

instagibbs force-pushed on May 3, 2024

DrahtBot added the label CI failed on May 3, 2024

DrahtBot commented at 3:10 pm on May 3, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24559142372

DrahtBot removed the label CI failed on May 3, 2024

instagibbs force-pushed on May 7, 2024

instagibbs force-pushed on May 8, 2024

in src/validation.cpp:1205 in b86487ccf2 outdated

1166+    // to be only paying anti-DoS fees
1167+    if (CFeeRate(parent_ws.m_modified_fees, parent_ws.m_vsize) >=
1168+            CFeeRate(parent_ws.m_modified_fees + child_ws.m_modified_fees, parent_ws.m_vsize + child_ws.m_vsize)) {
1169+        return package_state.Invalid(PackageValidationResult::PCKG_POLICY,
1170+                                     "package RBF failed: parent paying for child anti-DoS", "");
1171+    }

glozow commented at 1:43 pm on May 9, 2024:

in b86487ccf2e3d43da3d3ce8c3d2e2510677eb9ec

The comments + error string are a bit contradictory (“child only paying anti-DoS” and “parent paying for child anti-DoS”) unless maybe I’m misunderstanding?

Also maybe error str “package RBF failed: parent paying for child replacement” and debug str “package feerate is X while parent feerate is Y” ?

instagibbs commented at 6:48 pm on May 9, 2024:

done

in src/validation.cpp:1212 in b86487ccf2 outdated

1172+
1173+    // Check if it's economically rational to mine this package rather than the ones it replaces.
1174+    if (const auto err_tup{ImprovesFeerateDiagram(m_pool, direct_conflict_iters, m_all_conflicts, m_total_modified_fees, m_total_vsize)}) {
1175+        return package_state.Invalid(PackageValidationResult::PCKG_POLICY,
1176+                                     "package RBF failed: " + err_tup.value().second, "");
1177+    }

glozow commented at 1:49 pm on May 9, 2024:

b86487ccf2e3d43da3d3ce8c3d2e2510677eb9ec

I wonder if PaysForRBF and ImprovesFeerateDiagram should be swapped in the order of checks?

ImprovesFeerateDiagram is a more expensive, is a superset of the rules, and a less intuitive error (I’d have an easier time guessing what went wrong with “pays less fees” vs “does not improve feerate diagram”. Especially looking at the functional tests which switched out the error strings due to this check being earlier.

instagibbs commented at 2:09 pm on May 9, 2024:

is a superset of the rules

PaysForRBF has incremental rate on top, so ImprovesFeerateDiagram is not a strict superset fwiw.

That said, I don’t think swapping the order is a problem and would indeed be cheaper.

glozow commented at 3:59 pm on May 9, 2024:

True, it’s only a superset of Rule 3 👍

instagibbs commented at 6:48 pm on May 9, 2024:

swapped them and updated tests accordingly

in src/validation.cpp:1141 in b86487ccf2 outdated

1137+
1138+    // If the package has in-mempool ancestors, we won't consider a package RBF
1139+    // since it would result in a cluster larger than 2
1140+    for (const auto& ws : workspaces) {
1141+        if (!ws.m_ancestors.empty()) {
1142+            return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: replacing cluster with ancestors not size two");

glozow commented at 1:52 pm on May 9, 2024:

0            return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: new transaction cannot have mempool ancestors");

instagibbs commented at 6:48 pm on May 9, 2024:

done

in src/validation.cpp:1207 in b86487ccf2 outdated

1168+            CFeeRate(parent_ws.m_modified_fees + child_ws.m_modified_fees, parent_ws.m_vsize + child_ws.m_vsize)) {
1169+        return package_state.Invalid(PackageValidationResult::PCKG_POLICY,
1170+                                     "package RBF failed: parent paying for child anti-DoS", "");
1171+    }
1172+
1173+    // Check if it's economically rational to mine this package rather than the ones it replaces.

glozow commented at 1:54 pm on May 9, 2024:

Maybe you can mention that this replaces PaysMoreForConflicts function from ReplacementChecks?

instagibbs commented at 6:48 pm on May 9, 2024:

done

instagibbs force-pushed on May 9, 2024

in src/validation.cpp:1134 in 53ed8fdb67 outdated

1130+    // No conflicts means we're finished. Further checks are all RBF-only.
1131+    if (!m_rbf) return true;
1132+
1133+    // We're in package RBF context; replacement proposal must be size 2
1134+    if (workspaces.size() != 2 || !Assume(IsChildWithParents(txns))) {
1135+        return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: replacing cluster not size two");

glozow commented at 1:57 pm on May 9, 2024:

I don’t think it’s very clear which cluster isn’t size two in this str

0        return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: package must be 1-parent-1-child");

instagibbs commented at 6:48 pm on May 9, 2024:

done

in doc/policy/packages.md:41 in 53ed8fdb67 outdated

35@@ -36,10 +36,9 @@ The following rules are enforced for all packages:
36 * Packages cannot have conflicting transactions, i.e. no two transactions in a package can spend
37    the same inputs. Packages cannot have duplicate transactions. (#20833)
38 
39-* No transaction in a package can conflict with a mempool transaction. Replace By Fee is
40-  currently disabled for packages. (#20833)
41+* Only limited package replacements are currently considered. (#28984)
42 
43-   - Package RBF may be enabled in the future.
44+   - More general package RBF may be enabled in the future.

glozow commented at 2:01 pm on May 9, 2024:

Could add more detailed docs for what the rules are in package RBF?

all to-be-replaced signal (bip125 or v3)
package is 1p1c, only in clusters up to size 2
to-be-replaced all in clusters up to size 2
no more than 100 total to-be-replaced
total fees of package > all fees being replaced, at incremental relay feerate
must improve feerate diagram
parent feerate must be < package feerate

instagibbs commented at 6:48 pm on May 9, 2024:

@glozow pushed some more detailed docs, thanks!

instagibbs force-pushed on May 9, 2024

instagibbs commented at 6:49 pm on May 9, 2024: member

@glozow thanks for the review, all comments addressed

in test/functional/mempool_package_rbf.py:238 in 99c0f92015 outdated

236+        self.assert_mempool_contents(expected=expected_txns)
237+
238+        # Finally, evict MAX_REPLACEMENT_CANDIDATES
239+        package_parent = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_coins[:-1], fee_per_output=10000)
240+        package_child = self.wallet.create_self_transfer(fee_rate=10000*DEFAULT_FEE, utxo_to_spend=package_parent["new_utxos"][0])
241+        pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]], maxfeerate=0)

glozow commented at 2:32 pm on May 10, 2024:

nit: you change the fee in the last successful one, which is a little bit sus since fee shouldn’t matter in this test

Maybe create 2 constants for the parent fee_per_output and child fee/feerate, and use them for each round?

instagibbs commented at 3:59 pm on May 13, 2024:

I turned off maxfeerate, jacked up the child feerates, and made constants for both

in test/functional/mempool_package_rbf.py:257 in 99c0f92015 outdated

252+        package_hex2, package_txns2 = self.create_simple_package(coin, Decimal("0.00009"), DEFAULT_FEE * 2)
253+        package_hex3, package_txns3 = self.create_simple_package(coin, DEFAULT_FEE, DEFAULT_FEE * 5)
254+        node.submitpackage(package_hex1)
255+        self.assert_mempool_contents(expected=package_txns1)
256+        # The first two transactions have the same conflicts
257+        package_duplicate_conflicts_hex = [package_hex2[0]] + package_hex3

glozow commented at 2:38 pm on May 10, 2024:

I don’t really understand what’s being tested here - this doesn’t have anything to do with package RBF? This isn’t a topologically valid package since the transactions aren’t related to each other in addition to being double spends. #30066 seems closer to what this is going for?

instagibbs commented at 3:59 pm on May 13, 2024:

Agreed, it’s perhaps an older badly done version of #30066, removed

in test/functional/mempool_package_rbf.py:256 in 99c0f92015 outdated

279+
280+        # Double-spends the original package
281+        self.ctr += 1
282+        parent_result1 = self.wallet.create_self_transfer(
283+            fee_rate=0,
284+            fee=DEFAULT_FEE,

glozow commented at 2:41 pm on May 10, 2024:

These 2 things are contradictory. AFAICT fee_rate is ignored, delete?

instagibbs commented at 3:59 pm on May 13, 2024:

removed a series of these ignored args

in test/functional/mempool_package_rbf.py:465 in 99c0f92015 outdated

460+        for tx in expected_txns:
461+            node.sendrawtransaction(tx.serialize().hex())
462+        self.assert_mempool_contents(expected=expected_txns)
463+
464+        # Now make conflicting packages for each coin
465+        package_hex1, package_txns1 = self.create_simple_package(coin1, DEFAULT_FEE, DEFAULT_FEE * 4)

glozow commented at 2:54 pm on May 10, 2024:

nit: more varied fees for the packages for the wrong_conflict_cluster_size_* tests, could also use constant

instagibbs commented at 3:59 pm on May 13, 2024:

picked a constant and used it

DrahtBot added the label CI failed on May 12, 2024

DrahtBot commented at 12:10 pm on May 12, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24790654138

instagibbs commented at 12:30 pm on May 12, 2024: member

silent merge conflict with https://github.com/bitcoin/bitcoin/pull/29939

in test/functional/mempool_package_rbf.py:17 in 497aed95d3 outdated

12+from test_framework.test_framework import BitcoinTestFramework
13+from test_framework.util import (
14+    assert_greater_than_or_equal,
15+    assert_raises_rpc_error,
16+    assert_equal,
17+    fill_mempool,

glozow commented at 9:12 am on May 13, 2024:

This is now in mempool_util (= linter complaint)

instagibbs commented at 3:59 pm on May 13, 2024:

done

in src/validation.cpp:1182 in 99c0f92015 outdated

1178+    }
1179+
1180+    // Ensure this two transaction package is a "chunk" on its own; we don't want the child
1181+    // to be only paying anti-DoS fees
1182+    const CFeeRate parent_feerate(parent_ws.m_modified_fees, parent_ws.m_vsize);
1183+    const CFeeRate package_feerate(parent_ws.m_modified_fees + child_ws.m_modified_fees, parent_ws.m_vsize + child_ws.m_vsize);

glozow commented at 9:21 am on May 13, 2024:

nit: this would be easier to read and more extensible in the future

0    const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize);

instagibbs commented at 3:58 pm on May 13, 2024:

taken

in src/validation.cpp:1172 in 99c0f92015 outdated

1168+
1169+    const auto& parent_ws = workspaces[0];
1170+    const auto& child_ws = workspaces[1];
1171+
1172+    // Use the child as the transaction for attributing errors to.
1173+    const Txid child_hash = child_ws.m_ptx->GetHash();

glozow commented at 9:24 am on May 13, 2024:

nit; make reference not copy

0    const Txid& child_hash = child_ws.m_ptx->GetHash();

instagibbs commented at 3:58 pm on May 13, 2024:

done

in src/validation.cpp:1186 in 99c0f92015 outdated

1182+    const CFeeRate parent_feerate(parent_ws.m_modified_fees, parent_ws.m_vsize);
1183+    const CFeeRate package_feerate(parent_ws.m_modified_fees + child_ws.m_modified_fees, parent_ws.m_vsize + child_ws.m_vsize);
1184+    if (parent_feerate >= package_feerate) {
1185+        return package_state.Invalid(PackageValidationResult::PCKG_POLICY,
1186+                                     "package RBF failed: parent paying for child replacement",
1187+                                     strprintf("package feerate is %s while parent feerate is %s", package_feerate.ToString(), parent_feerate.ToString()));

glozow commented at 9:35 am on May 13, 2024:

Sorry to nit my own previous suggestion, but I think “parent paying for child replacement” might still be too specific, since it could be that the child has nothing to replace but is lower feerate (like package_txns6 in the functional test).

Maybe

0                                     "package RBF failed: package feerate is less than parent feerate",
1                                     strprintf("package feerate %s <= parent feerate %s", package_feerate.ToString(), parent_feerate.ToString()));

instagibbs commented at 3:58 pm on May 13, 2024:

taken

in test/functional/mempool_package_rbf.py:192 in 497aed95d3 outdated

187+        self.log.info("Check Package RBF must have strict cpfp structure")
188+        coin = self.coins.pop()
189+        package_hex5, package_txns5 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_FEE)
190+        node.submitpackage(package_hex5)
191+        self.assert_mempool_contents(expected=package_txns5)
192+        package_hex6, package_txns6 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE + fee_delta, child_fee=DEFAULT_FEE - (fee_delta / 2))

glozow commented at 10:01 am on May 13, 2024:

Given that fee_delta is a multiple of DEFAULT_FEE and you’re using fee_delta / 2 here… I think the general readability of this test would be improved if you created a constant base unit FEE which was equal to, say, DEFAULT_FEE / 10 (or 104 * 10 which would give most of the transactions whole number feerates), and then used FEE, 2*FEE, 8*FEE, etc. in all the tests.

instagibbs commented at 3:58 pm on May 13, 2024:

I created DEFAULT_CHILD_FEE and used that pretty much everywhere it made sense

in test/functional/mempool_package_rbf.py:552 in 99c0f92015 outdated

547+        assert_equal(pkg_results2["package_msg"], 'package RBF failed: insufficient feerate: does not improve feerate diagram')
548+        self.assert_mempool_contents(expected=package_txns1)
549+        self.generate(node, 1)
550+
551+    def test_child_conflicts_parent_mempool_ancestor(self):
552+        fill_mempool(self, self.nodes[0], self.wallet)

glozow commented at 10:29 am on May 13, 2024:

with rebase

0        fill_mempool(self, self.nodes[0])

instagibbs commented at 3:58 pm on May 13, 2024:

done

in doc/policy/packages.md:59 in 99c0f92015 outdated

55+
56+   - Must improve feerate diagram. (#29242)
57+
58+   - *Rationale*: Basic support for package RBF can be used by wallets
59+     by making chains of no longer than two, then directly conflicting
60+     those chains when needed. Combined with V3 transactions this can

glozow commented at 10:31 am on May 13, 2024:

Maybe there should be some v3 test coverage (including nice 0fee parent package RBF)?

 0diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
 1index 8285b82c19..990d36359e 100755
 2--- a/test/functional/mempool_accept_v3.py
 3+++ b/test/functional/mempool_accept_v3.py
 4@@ -579,6 +579,32 @@ class MempoolAcceptV3(BitcoinTestFramework):
 5         )
 6         self.check_mempool([tx_with_multi_children["txid"], tx_with_sibling3_rbf["txid"], tx_with_sibling2["txid"]])
 7 
 8+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)(extra_args=["-acceptnonstdtxn=1"])
 9+    def test_package_rbf(self):
10+        self.log.info("Test package RBF: v3 0-fee parent + high-fee child replaces parent's conflicts")
11+        node = self.nodes[0]
12+        # Reuse the same coins so that the transactions conflict with one another.
13+        parent_coin = self.wallet.get_utxo(confirmed_only=True)
14+
15+        # package1 pays default fee on both transactions
16+        parent1 = self.wallet.create_self_transfer(utxo_to_spend=parent_coin, version=3)
17+        child1 = self.wallet.create_self_transfer(utxo_to_spend=parent1["new_utxo"], version=3)
18+        package_hex1 = [parent1["hex"], child1["hex"]]
19+        fees_package1 = parent1["fee"] + child1["fee"]
20+        submitres1 = node.submitpackage(package_hex1)
21+        assert_equal(submitres1["package_msg"], "success")
22+        self.check_mempool([parent1["txid"], child1["txid"]])
23+
24+        # package2 has a 0-fee parent (conflicting with package1) and very high fee child
25+        parent2 = self.wallet.create_self_transfer(utxo_to_spend=parent_coin, fee=0, fee_rate=0, version=3)
26+        child2 = self.wallet.create_self_transfer(utxo_to_spend=parent2["new_utxo"], fee=fees_package1*10, version=3)
27+        package_hex2 = [parent2["hex"], child2["hex"]]
28+
29+        submitres2 = node.submitpackage(package_hex2)
30+        assert_equal(submitres2["package_msg"], "success")
31+        assert_equal(set(submitres2["replaced-transactions"]), set([parent1["txid"], child1["txid"]]))
32+        self.check_mempool([parent2["txid"], child2["txid"]])
33+
34 
35     def run_test(self):
36         self.log.info("Generate blocks to create UTXOs")
37@@ -598,6 +624,7 @@ class MempoolAcceptV3(BitcoinTestFramework):
38         self.test_reorg_2child_rbf()
39         self.test_v3_sibling_eviction()
40         self.test_reorg_sibling_eviction_1p2c()
41+        self.test_package_rbf()
42 
43 
44 if __name__ == "__main__":

instagibbs commented at 3:58 pm on May 13, 2024:

was wondering about this case, added with some modifications for test simplicity

sdaftuar commented at 5:42 pm on May 24, 2024:

s/V3 transactions/TRUC transactions/, perhaps?

instagibbs commented at 6:42 pm on May 24, 2024:

removed references to v3, and added a note that non-standard relay args should be removed once they’re made standard

murchandamus commented at 3:43 pm on June 17, 2024:

I understood above comments to mean that this reference to v3 was replaced by a reference to TRUC transactions, but it seems to still mention v3.

instagibbs commented at 6:36 pm on June 17, 2024:

done in follow-up, I had removed V3 in test, not the actual doc

glozow commented at 10:32 am on May 13, 2024: member

some test review

instagibbs force-pushed on May 13, 2024

DrahtBot removed the label CI failed on May 14, 2024

DrahtBot added the label Needs rebase on May 15, 2024

instagibbs force-pushed on May 15, 2024

instagibbs commented at 12:13 pm on May 15, 2024: member

rebased due to conflict

DrahtBot removed the label Needs rebase on May 15, 2024

t-bast commented at 1:47 pm on May 15, 2024: contributor

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 commented at 3:14 pm on May 16, 2024: member

sorry forgot to link #30072 here, added to OP

instagibbs force-pushed on May 20, 2024

instagibbs commented at 2:36 pm on May 20, 2024: member

rebased on latest #30072

DrahtBot added the label Needs rebase on May 23, 2024

glozow referenced this in commit 4c387cb64f on May 24, 2024

instagibbs force-pushed on May 24, 2024

DrahtBot removed the label Needs rebase on May 24, 2024

instagibbs commented at 1:57 pm on May 24, 2024: member

rebased on master :rocket:

in src/policy/v3_policy.cpp:94 in 3f46eccbbd outdated

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

sdaftuar commented at 5:25 pm on May 24, 2024:

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/ ?

instagibbs commented at 6:42 pm on May 24, 2024:

fixed

in src/validation.cpp:1167 in 5a5a96833b outdated

1163+    for (Workspace& ws : workspaces) {
1164+        // Aggregate all conflicts into one set.
1165+        direct_conflict_iters.merge(ws.m_iters_conflicting);
1166+    }
1167+
1168+    for (const auto& ws : workspaces) {

sdaftuar commented at 5:32 pm on May 24, 2024:

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?

instagibbs commented at 6:42 pm on May 24, 2024:

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

in src/validation.cpp:1153 in 5a5a96833b outdated

1149+    if (workspaces.size() != 2 || !Assume(IsChildWithParents(txns))) {
1150+        return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: package must be 1-parent-1-child");
1151+    }
1152+
1153+    // If the package has in-mempool ancestors, we won't consider a package RBF
1154+    // since it would result in a cluster larger than 2

sdaftuar commented at 5:39 pm on May 24, 2024:

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?

instagibbs commented at 6:42 pm on May 24, 2024:

made an attempt at a grepp-able cautionary note

instagibbs force-pushed on May 24, 2024

DrahtBot added the label CI failed on May 24, 2024

DrahtBot removed the label CI failed on May 26, 2024

in src/policy/v3_policy.cpp:137 in 848c4e55da outdated

133@@ -135,10 +134,7 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
134                 }
135             }
136 
137-            // It shouldn't be possible to have any mempool siblings at this point. SingleV3Checks
138-            // catches mempool siblings and sibling eviction is not extended to packages. Also, if the package consists of connected transactions,
139-            // any tx having a mempool ancestor would mean the package exceeds ancestor limits.
140-            if (!Assume(!parent_info.m_has_mempool_descendant)) {
141+            if (parent_info.m_has_mempool_descendant) {

theStack commented at 5:07 pm on May 29, 2024:

Basic question: is this commit (848c4e55da85ec9776ce1c16ca51ea370502125b, “PackageV3Checks: Relax assumptions”) strictly needed for this PR or already preparation for future work? The tests pass even if the Assumes are kept, and I haven’t been able to create a scenario where this condition is true, as the scenario described in the commit message:

0Consider:
1
2TxA (in mempool) <- TxB (in mempool)
3
4TxA (in mempool) <- TxB' (in package, conflicts with TxB) <- TxC (in package)

isn’t possible due to V3 ancestor limits.

instagibbs commented at 6:56 pm on May 29, 2024:

is this commit (848c4e5, “PackageV3Checks: Relax assumptions”) strictly needed for this PR or already preparation for future work?

If it’s not removed the next commit will cause failures.

It hits in the fuzzing package_eval fuzzing target. I could add a unit test for it, but it doesn’t really test much, except that we don’t crash right before removing the soon to be erroneous assertion. Here’s an input you can run to verify:

0EQQAAAgAAAAAAAAAAAAAAAUAAAAAAAAAAAAAAAA6AAAACAAADQAAAAAAAAAAAAAAAAAAaQFVKGEAawAAAAAABQAAAAAAAAAAAAAAAAA6AAAACAAADQAAAAAAAAAAAAAAAAAAaQFVKGF0YWNhcnJpZXJzaXplVVUAAAEAAAAAAFURVQEuaQEA/wAAAigBbwAAOzsBswBiaQFpAWR0YWNhcnJpZXJzaXplVVUAAAAAAAAAAFURVQFkaQEA/wAAAigBbwAAOzsBswBiaQFVZCgFaWkBAQA=

isn’t possible due to V3 ancestor limits.

It’s only impossible due to PackageV3Checks, since TxC has no in-mempool ancestors at PreChecks/SingleV3Checks time.

theStack commented at 1:27 am on May 30, 2024:

Thanks, that was helpful, I could reproduce the assertion crash with this input. Ended up putting some debug outputs into the fuzz test to see what txs are created to trigger it, and it matches the scenario described in the commit message via two submitted packages [TxA <- TxB] and [TxB’ <- TxC].

isn’t possible due to V3 ancestor limits.

It’s only impossible due to PackageV3Checks, since TxC has no in-mempool ancestors at PreChecks/SingleV3Checks time.

Oh right, that’s the part I was missing, forgotting to submit TxB first in my try to reproduce the crash in the functional test.

I think the specific condition if (parent_info.m_has_mempool_descendant) still can’t be hit though with current limits at this position, as scenarios causing this are already failing a few lines above (“This tx can’t have both a parent and an in-package child.”), throwing an “would have too many ancestors” error. (I guess it’s just fine to keep it, as both error reasons are legit, and it’s just a question of which one to throw first?)

ismaelsadeeq commented at 1:08 pm on June 11, 2024:

It hits in the fuzzing package_eval fuzzing target.

I was able to reproduce this locally.

in src/validation.cpp:1172 in 0044c1dad9 outdated

1168+    }
1169+
1170+    const auto& parent_ws = workspaces[0];
1171+    const auto& child_ws = workspaces[1];
1172+
1173+    // The aggregated set of conflicts cannot exceed 100.

glozow commented at 9:50 am on June 10, 2024:

nit 0044c1dad9ffc3d58afd06d7533f16beb0d0a829:

Would prefer to not have magic number comments

0 // Don't consider replacements that would cause us to remove a large number of mempool entries.
1 // This limit is not increased in a package RBF. Use the aggregate number of transactions.

instagibbs commented at 7:10 pm on June 10, 2024:

taken

in src/test/txpackage_tests.cpp:966 in 7d895935ae outdated

961+        Package package2;
962+        // Package1 and package2 have the same parent. The children conflict.
963+        auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0,
964+                                                        /*input_height=*/0, /*input_signing_key=*/coinbaseKey,
965+                                                        /*output_destination=*/parent_spk,
966+                                                        /*output_amount=*/coinbase_value - 200, /*submit=*/false);

glozow commented at 9:55 am on June 10, 2024:

nit: 7d895935aef63b4aa728310b580f418ce35fc234

we started using low_fee_amt instead of 200 in this test

instagibbs commented at 7:10 pm on June 10, 2024:

adapted more of the test to use this value

in src/test/txpackage_tests.cpp:988 in 7d895935ae outdated

983+        BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
984+        BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
985+        expected_pool_size += 2;
986+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
987+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash())));
988+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));

glozow commented at 10:03 am on June 10, 2024:

Should use CheckPackageMempoolAcceptResult to check that results are populated as expected, which also means you can remove a few of these checks:

0        if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
1            BOOST_ERROR(err_1.value());
2        }
3        auto it_parent_1 = submit1.m_tx_results.find(tx_parent->GetWitnessHash());
4        auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
5
6        BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
7        BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
8        expected_pool_size += 2;
9        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);

instagibbs commented at 7:09 pm on June 10, 2024:

great, added a little commentary on top and repeated this for 2nd package

in src/test/txpackage_tests.cpp:1074 in 7d895935ae outdated

1069+        BOOST_CHECK(it_child_3 != submit3.m_tx_results.end());
1070+        BOOST_CHECK_EQUAL(it_parent_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
1071+        BOOST_CHECK_EQUAL(it_child_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
1072+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
1073+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_3->GetHash())));
1074+        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_3->GetHash())));

glozow commented at 10:32 am on June 10, 2024:

Can check m_wtxids_fee_calculations and m_effective_feerate.

Wrote up the changes for this + using CheckPackageMempoolAcceptResult:

  0index cd69fce2f5..b68686cfcb 100644
  1--- a/src/test/txpackage_tests.cpp
  2+++ b/src/test/txpackage_tests.cpp
  3@@ -975,28 +975,22 @@ BOOST_FIXTURE_TEST_CASE(package_rbf_tests, TestChain100Setup)
  4 
  5         LOCK(m_node.mempool->cs);
  6         const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, /*test_accept=*/false, std::nullopt);
  7-        BOOST_CHECK_MESSAGE(submit1.m_state.IsValid(), "Package validation unexpectedly failed: " << submit1.m_state.GetRejectReason());
  8-        auto it_parent_1 = submit1.m_tx_results.find(tx_parent->GetWitnessHash());
  9-        auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
 10-        BOOST_CHECK(it_parent_1 != submit1.m_tx_results.end());
 11-        BOOST_CHECK(it_child_1 != submit1.m_tx_results.end());
 12-        BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
 13-        BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
 14+        if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
 15+            BOOST_ERROR(err_1.value());
 16+        }
 17         expected_pool_size += 2;
 18-        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
 19-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash())));
 20-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));
 21 
 22         const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, /*test_accept=*/false, std::nullopt);
 23+        if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) {
 24+            BOOST_ERROR(err_2.value());
 25+        }
 26         auto it_parent_2 = submit2.m_tx_results.find(tx_parent->GetWitnessHash());
 27         auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash());
 28-        BOOST_CHECK(it_parent_2 != submit2.m_tx_results.end());
 29-        BOOST_CHECK(it_child_2 != submit2.m_tx_results.end());
 30-        BOOST_CHECK_MESSAGE(submit2.m_state.IsValid(), "Package validation unexpectedly failed" << submit2.m_state.GetRejectReason());
 31         BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
 32         BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
 33         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
 34-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_2->GetHash())));
 35+
 36+        // child1 has been replaced
 37         BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));
 38     }
 39 
 40@@ -1034,44 +1028,60 @@ BOOST_FIXTURE_TEST_CASE(package_rbf_tests, TestChain100Setup)
 41         Package package3{tx_parent_3, tx_child_3};
 42 
 43         const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt);
 44-        BOOST_CHECK_MESSAGE(submit1.m_state.IsValid(), "Package validation unexpectedly failed" << submit1.m_state.GetRejectReason());
 45+        if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
 46+            BOOST_ERROR(err_1.value());
 47+        }
 48         auto it_parent_1 = submit1.m_tx_results.find(tx_parent_1->GetWitnessHash());
 49         auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
 50-        BOOST_CHECK(it_parent_1 != submit1.m_tx_results.end());
 51-        BOOST_CHECK(it_child_1 != submit1.m_tx_results.end());
 52         BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
 53         BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
 54+        BOOST_CHECK_EQUAL(it_parent_1->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_parent_1)), 200);
 55+        BOOST_CHECK_EQUAL(it_child_1->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child_1)), 200);
 56+        BOOST_CHECK(it_parent_1->second.m_replaced_transactions.empty());
 57+        BOOST_CHECK(it_child_1->second.m_replaced_transactions.empty());
 58         expected_pool_size += 2;
 59         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
 60-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_1->GetHash())));
 61-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));
 62 
 63         // This replacement is actually not package rbf; the parent carries enough fees
 64         // to replace the entire package on its own.
 65         const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, false, std::nullopt);
 66-        BOOST_CHECK_MESSAGE(submit2.m_state.IsValid(), "Package validation unexpectedly failed" << submit2.m_state.GetRejectReason());
 67+        if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) {
 68+            BOOST_ERROR(err_2.value());
 69+        }
 70         auto it_parent_2 = submit2.m_tx_results.find(tx_parent_2->GetWitnessHash());
 71         auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash());
 72-        BOOST_CHECK(it_parent_2 != submit2.m_tx_results.end());
 73-        BOOST_CHECK(it_child_2 != submit2.m_tx_results.end());
 74         BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
 75         BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
 76+        BOOST_CHECK_EQUAL(it_parent_2->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_parent_2)), 800);
 77+        BOOST_CHECK_EQUAL(it_child_2->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child_2)), 200);
 78+
 79+        // parent2 replaced both transactions
 80+        BOOST_CHECK(it_parent_2->second.m_replaced_transactions.size() == 2);
 81+        BOOST_CHECK(it_child_2->second.m_replaced_transactions.empty());
 82         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
 83-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_2->GetHash())));
 84-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_2->GetHash())));
 85 
 86         // Package RBF, in which the replacement transaction's child sponsors the fees to meet RBF feerate rules
 87         const auto submit3 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package3, false, std::nullopt);
 88-        BOOST_CHECK_MESSAGE(submit3.m_state.IsValid(), "Package validation unexpectedly failed" << submit3.m_state.GetRejectReason());
 89+        if (auto err_3{CheckPackageMempoolAcceptResult(package3, submit3, /*expect_valid=*/true, m_node.mempool.get())}) {
 90+            BOOST_ERROR(err_3.value());
 91+        }
 92         auto it_parent_3 = submit3.m_tx_results.find(tx_parent_3->GetWitnessHash());
 93         auto it_child_3 = submit3.m_tx_results.find(tx_child_3->GetWitnessHash());
 94-        BOOST_CHECK(it_parent_3 != submit3.m_tx_results.end());
 95-        BOOST_CHECK(it_child_3 != submit3.m_tx_results.end());
 96         BOOST_CHECK_EQUAL(it_parent_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
 97         BOOST_CHECK_EQUAL(it_child_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
 98+
 99+        // package3 was considered as a package to replace both package2 transactions
100+        BOOST_CHECK(it_parent_3->second.m_replaced_transactions.size() == 2);
101+        BOOST_CHECK(it_child_3->second.m_replaced_transactions.empty());
102+
103+        std::vector<Wtxid> expected_package3_wtxids({tx_parent_3->GetWitnessHash(), tx_child_3->GetWitnessHash()});
104+        const auto package3_total_vsize{GetVirtualTransactionSize(*tx_parent_3) + GetVirtualTransactionSize(*tx_child_3)};
105+        BOOST_CHECK(it_parent_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids);
106+        BOOST_CHECK(it_child_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids);
107+        BOOST_CHECK_EQUAL(it_parent_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 1499);
108+        BOOST_CHECK_EQUAL(it_child_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 1499);
109+
110         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
111-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_3->GetHash())));
112-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_3->GetHash())));
113     }
114 
115 }

instagibbs commented at 7:09 pm on June 10, 2024:

taken with only light modifications

in test/functional/mempool_package_rbf.py:37 in 7d895935ae outdated

32+        self.setup_clean_chain = True
33+        # Required for fill_mempool()
34+        self.extra_args = [[
35+            "-datacarriersize=100000",
36+            "-maxmempool=5",
37+            "-acceptnonstdtxn=1", # TRUC test; remove once standard

glozow commented at 10:41 am on June 10, 2024:

Needs rebase for #29496?

instagibbs commented at 7:09 pm on June 10, 2024:

yep, rebased + removed

in src/validation.cpp:1403 in aa3a4da248 outdated

1392@@ -1391,6 +1393,13 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
1393     std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids),
1394                    [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); });
1395 
1396+    if (!m_subpackage.m_replaced_transactions.empty()) {
1397+        LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with %u new one(s) for %s additional fees, %d delta bytes\n",
1398+                 m_subpackage.m_replaced_transactions.size(), workspaces.size(),
1399+                 m_subpackage.m_total_modified_fees - static_cast<int>(m_subpackage.m_conflicting_fees),
1400+                 m_subpackage.m_total_vsize - static_cast<int>(m_subpackage.m_conflicting_size));

glozow commented at 10:44 am on June 10, 2024:

aa3a4da248fe9d4a765fa4c80797ca333ee37f07: is the static cast necessary?

instagibbs commented at 7:09 pm on June 10, 2024:

removed the static cast for m_conflicting_fees since that’s the same type as m_total_modified_fees, but left the other static casts in place since explicit is better than implicit

in src/test/fuzz/package_eval.cpp:310 in 5dd74f9c6d outdated

306@@ -307,7 +307,7 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
307             // just use result_package.m_state here. This makes the expect_valid check meaningless, but
308             // we can still verify that the contents of m_tx_results are consistent with m_state.
309             const bool expect_valid{result_package.m_state.IsValid()};
310-            Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, nullptr));
311+            Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, &tx_pool));

glozow commented at 10:50 am on June 10, 2024:

5dd74f9c6ddc218417bda841f493ca2ed3f2b327

I don’t 100% remember why we did this, but I think it was so we don’t check mempool contents when test_accept=true?

instagibbs commented at 7:09 pm on June 10, 2024:

This should only be called when not test_accept, right?

glozow commented at 3:41 pm on June 11, 2024:

Aha yes. Can resolve.

in src/test/util/txmempool.cpp:93 in 91a7e8ba76 outdated

88+            if (atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() == 2) {
89+                for (const auto& fee_calc_wtxid : atmp_result.m_wtxids_fee_calculations.value()) {
90+                    const auto& entry = *Assert(mempool->GetEntry(fee_calc_wtxid));
91+                    if (entry.GetCountWithDescendants() + entry.GetCountWithAncestors() != 3) {
92+                        return strprintf("tx %s has too many ancestors or descendants for a package rbf", fee_calc_wtxid.ToString());
93+                    }

glozow commented at 10:53 am on June 10, 2024:

91a7e8ba7606b68401b29d2609c240a604ab6fe1: this isn’t a perfect test for cluster size 2… why not call CheckConflictTopology?

instagibbs commented at 7:09 pm on June 10, 2024:

Directly checked cluster size instead.

glozow commented at 3:38 pm on June 11, 2024:

Nice, agree that’s better. However e4e14fedc9622ca7cfc40af4f2aa70ed4bb7daa6 is now unnecessary and can be dropped.

instagibbs commented at 3:45 pm on June 11, 2024:

even better. dropped.

in doc/policy/packages.md:49 in 7b74cbf6ed outdated

47+
48+   - All conflicting clusters must be clusters of up to size 2.
49+
50+   - No more than MAX_REPLACEMENT_CANDIDATES transactions can be replaced.
51+
52+   - Total fee plus incremental relay fee must be paid (ala bip125 rules 3 and 4).

glozow commented at 10:57 am on June 10, 2024:

No mentions of BIP125 outside of signaling pls

0   - Replacements must pay more total total fees at the incremental relay fee (analogous to regular [replacement rules](./mempool-replacements.md) 3 and 4).

instagibbs commented at 7:10 pm on June 10, 2024:

sure, taken

glozow commented at 10:58 am on June 10, 2024: member

looks pretty good 7b74cbf6ed7cafdbd458471ae89fa88c52256278

PackageV3Checks: Relax assumptions

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.

5da3967815

instagibbs force-pushed on Jun 10, 2024

in src/validation.cpp:524 in d3d653dce3 outdated

523@@ -524,7 +524,7 @@ class MemPoolAccept
524                             /* m_bypass_limits */ false,

ismaelsadeeq commented at 1:07 pm on June 11, 2024:

In d3d653dce34ad1f3235869b8a54f9572e89e5b68 [policy] package rbf

Update places where it is claimed that package RBF is disabled?

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 8372fcadca1..d18fc1c0f2c 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -602,8 +602,8 @@ public:
 5     /**
 6      * Submission of a subpackage.
 7      * If subpackage size == 1, calls AcceptSingleTransaction() with adjusted ATMPArgs to avoid
 8-     * package policy restrictions like no CPFP carve out (PackageMempoolChecks) and disabled RBF
 9-     * (m_allow_replacement), and creates a PackageMempoolAcceptResult wrapping the result.
10+     * package policy restrictions like no CPFP carve out (PackageMempoolChecks)
11+     * and creates a PackageMempoolAcceptResult wrapping the result.
12      *
13      * If subpackage size > 1, calls AcceptMultipleTransactions() with the provided ATMPArgs.
14      *
15@@ -950,7 +950,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
16     ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts);
17 
18     // Note that these modifications are only applicable to single transaction scenarios;
19-    // carve-outs and package RBF are disabled for multi-transaction evaluations.
20+    // carve-outs are disabled for multi-transaction evaluations.
21     CTxMemPool::Limits maybe_rbf_limits = m_pool.m_opts.limits;
22 
23     // Calculate in-mempool ancestors, up to a limit.

instagibbs commented at 1:46 pm on June 11, 2024:

done, and found one more place with out of date comment

in src/test/txpackage_tests.cpp:1036 in 49b0f706fd outdated

1031+
1032+        // 1 parent paying 200sat, 1 child paying 200sat.
1033+        Package package1{tx_parent_1, tx_child_1};
1034+        // 1 parent paying 800sat, 1 child paying 200sat.
1035+        Package package2{tx_parent_2, tx_child_2};
1036+        // 1 parent paying 199sat, 1 child paying 1300.

ismaelsadeeq commented at 1:24 pm on June 11, 2024:

nit:

0        // 1 parent paying 199sat, 1 child paying 1300sat.

instagibbs commented at 1:46 pm on June 11, 2024:

done

instagibbs force-pushed on Jun 11, 2024

in src/test/util/txmempool.cpp:85 in 5bb449cf91 outdated

81@@ -82,6 +82,15 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,
82                                 wtxid.ToString());
83         }
84 
85+        if (!atmp_result.m_replaced_transactions.empty() && mempool) {

glozow commented at 3:43 pm on June 11, 2024:

nit: 5bb449cf91907f613dc4d7e1114be3befaf7b2ff commit message should be test util instead of fuzz, since this is used in unit tests too

glozow commented at 4:26 pm on June 11, 2024:

I got f25c15383ff051effd0add25352fe0f0e2224d0c and 5bb449cf91907f613dc4d7e1114be3befaf7b2ff mixed up - they could be squashed.

instagibbs commented at 1:48 pm on June 12, 2024:

squashed and lightly re-ordered

instagibbs force-pushed on Jun 11, 2024

in src/validation.cpp:1537 in dd364f9a81 outdated

1537-        assert(!m_subpackage.m_rbf);
1538+        // package to spend. If there are no conflicts, no transaction can spend a coin
1539+        // needed by another transaction in the package. We also need to make sure that no package
1540+        // tx replaces (or replaces the ancestor of) the parent of another package tx. As long as we
1541+        // check these two things, we don't need to track the coins spent.
1542+        // If there are conflicts, PackageMempoolChecks() ensures later that any package RBF attempt

glozow commented at 4:21 pm on June 11, 2024:

Very different uses of “conflicts” here

0        // package to spend. If there are no conflicts within the package, no transaction can spend a coin
1        // needed by another transaction in the package. We also need to make sure that no package
2        // tx replaces (or replaces the ancestor of) the parent of another package tx. As long as we
3        // check these two things, we don't need to track the coins spent.
4        // If a package tx conflicts with a mempool tx, PackageMempoolChecks() ensures later that any package RBF attempt

instagibbs commented at 1:48 pm on June 12, 2024:

taken

in doc/policy/packages.md:45 in dd364f9a81 outdated

43-   - Package RBF may be enabled in the future.
44+   - All direct conflicts must signal replacement (or have `-mempoolfullrbf=1` set).
45+
46+   - Packages are 1-parent-1-child, with no in-mempool ancestors of the package.
47+
48+   - All conflicting clusters must be clusters of up to size 2.

glozow commented at 4:23 pm on June 11, 2024:

dd364f9a813221956ec0b19f45db231b148491c2: I suppose we should more thoroughly define what “cluster” means somewhere

instagibbs commented at 1:48 pm on June 12, 2024:

defined it inline

in doc/policy/packages.md:54 in dd364f9a81 outdated

52+   - Replacements must pay more total total fees at the incremental relay fee (analogous to
53+     regular [replacement rules](./mempool-replacements.md) 3 and 4).
54+
55+   - Parent feerate must be lower than package feerate.
56+
57+   - Must improve feerate diagram. (#29242)

glozow commented at 4:23 pm on June 11, 2024:

Also somewhat unclear… perhaps needs a link to the delving post or something

instagibbs commented at 1:48 pm on June 12, 2024:

Added delving link but for Future Work I’m wondering longer term where cluster mempool design stuff should live.

glozow commented at 10:45 am on June 12, 2024: member

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

DrahtBot requested review from ismaelsadeeq on Jun 12, 2024

instagibbs force-pushed on Jun 12, 2024

instagibbs commented at 1:53 pm on June 12, 2024: member

Added a release note

in src/validation.cpp:1206 in 612847ae1a outdated

1202+                                     "package RBF failed: package feerate is less than parent feerate",
1203+                                     strprintf("package feerate %s <= parent feerate is %s", package_feerate.ToString(), parent_feerate.ToString()));
1204+    }
1205+
1206+    // Check if it's economically rational to mine this package rather than the ones it replaces.
1207+    // This takes the place of ReplacementChecks()'s PaysMoreForConflicts() in the package RBF setting.

theStack commented at 8:32 am on June 13, 2024:

0    // This takes the place of ReplacementChecks()'s PaysMoreThanConflicts() in the package RBF setting.

instagibbs commented at 1:53 pm on June 13, 2024:

done

in src/validation.cpp:1199 in 612847ae1a outdated

1195+
1196+    // Ensure this two transaction package is a "chunk" on its own; we don't want the child
1197+    // to be only paying anti-DoS fees
1198+    const CFeeRate parent_feerate(parent_ws.m_modified_fees, parent_ws.m_vsize);
1199+    const CFeeRate package_feerate(m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize);
1200+    if (parent_feerate >= package_feerate) {

theStack commented at 8:34 am on June 13, 2024:

nit: slightly easier to reason for the reader imho

0    if (package_feerate <= parent_feerate) {

instagibbs commented at 1:53 pm on June 13, 2024:

done

in src/validation.cpp:1216 in 612847ae1a outdated

1210+                                     "package RBF failed: " + err_tup.value().second, "");
1211+    }
1212+
1213+    LogPrint(BCLog::TXPACKAGES, "package RBF checks passed: parent %s (wtxid=%s), child %s (wtxid=%s)\n",
1214+        txns.front()->GetHash().ToString(), txns.front()->GetWitnessHash().ToString(),
1215+        txns.back()->GetHash().ToString(), txns.back()->GetWitnessHash().ToString());

theStack commented at 9:03 am on June 13, 2024:

nit: it might be worth to add a belt-and-suspenders check that txns has size two (either directly or indirectly by asserting txns.size() == workspaces.size() above)

instagibbs commented at 1:53 pm on June 13, 2024:

added at beginning of function

in src/test/txpackage_tests.cpp:949 in 63307f0aa6 outdated

944+{
945+    mineBlocks(5);
946+    LOCK(::cs_main);
947+    size_t expected_pool_size = m_node.mempool->size();
948+    CKey child_key;
949+    child_key.MakeNewKey(true);

theStack commented at 9:05 am on June 13, 2024:

nit: shorter (can also be done for grandchild_key)

0    CKey child_key = GenerateRandomKey();

instagibbs commented at 1:53 pm on June 13, 2024:

done

in test/functional/mempool_package_rbf.py:139 in 63307f0aa6 outdated

134+        self.log.info("Test child can pay to replace a parent's single conflicted tx")
135+        node = self.nodes[0]
136+
137+        # Make singleton tx to conflict with in next batch
138+        singleton_coin = self.coins[-1]
139+        del self.coins[-1]

theStack commented at 9:42 am on June 13, 2024:

0        single_coin = self.coins.pop()

instagibbs commented at 1:53 pm on June 13, 2024:

done in this and one more location

in doc/policy/packages.md:47 in 8f2f64cb99 outdated

45+
46+   - Packages are 1-parent-1-child, with no in-mempool ancestors of the package.
47+
48+   - All conflicting clusters(connected components of mempool transactions) must be clusters of up to size 2.
49+
50+   - No more than MAX_REPLACEMENT_CANDIDATES transactions can be replaced.

theStack commented at 9:45 am on June 13, 2024:

could mention “analogous to regular replacement rule 5”?

instagibbs commented at 1:53 pm on June 13, 2024:

done

theStack commented at 9:59 am on June 13, 2024: contributor

Spent most time reviewing the core commit 612847ae1a55e92bb7732905a026be96a436372a 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.

[policy] package rbf

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>

dc21f61c72

[test] package rbf 4d15bcf448

Fuzz: pass mempool to CheckPackageMempoolAcceptResult 316d7b63c9

CheckPackageMempoolAcceptResult: Check package rbf invariants d3466e4cc5

mempool: Improve logging of replaced transactions 6e3c4394cf

doc: update package RBF comment afd52d8e63

Add release note for size 2 package rbf 94ed4fbf8e

instagibbs force-pushed on Jun 13, 2024

instagibbs commented at 1:54 pm on June 13, 2024: member

@theStack part of splitting out the diagram stuff to a prep PR is that you can just assume it means “economically rational to replace” :+1:

glozow commented at 1:48 pm on June 14, 2024: member

reACK 94ed4fbf8e via range-diff

ismaelsadeeq approved

ismaelsadeeq commented at 3:31 pm on June 14, 2024: member

re-ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10

theStack approved

theStack commented at 4:19 pm on June 14, 2024: contributor

Code-review ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10

glozow requested review from sdaftuar on Jun 14, 2024

in doc/policy/packages.md:41 in afd52d8e63 outdated

35@@ -36,10 +36,29 @@ The following rules are enforced for all packages:
36 * Packages cannot have conflicting transactions, i.e. no two transactions in a package can spend
37    the same inputs. Packages cannot have duplicate transactions. (#20833)
38 
39-* No transaction in a package can conflict with a mempool transaction. Replace By Fee is
40-  currently disabled for packages. (#20833)
41+* Only limited package replacements are currently considered. (#28984)
42 
43-   - Package RBF may be enabled in the future.
44+   - All direct conflicts must signal replacement (or have `-mempoolfullrbf=1` set).

murchandamus commented at 3:41 pm on June 17, 2024:

0   - All direct conflicts must signal replacement (or the node must have `-mempoolfullrbf=1` set).

instagibbs commented at 6:36 pm on June 17, 2024:

done in follow-up

in doc/policy/packages.md:45 in afd52d8e63 outdated

43-   - Package RBF may be enabled in the future.
44+   - All direct conflicts must signal replacement (or have `-mempoolfullrbf=1` set).
45+
46+   - Packages are 1-parent-1-child, with no in-mempool ancestors of the package.
47+
48+   - All conflicting clusters(connected components of mempool transactions) must be clusters of up to size 2.

murchandamus commented at 3:41 pm on June 17, 2024:

0   - All conflicting clusters (connected components of mempool transactions) must be clusters of up to size 2.

instagibbs commented at 6:36 pm on June 17, 2024:

done in follow-up

in src/test/util/txmempool.cpp:76 in d3466e4cc5 outdated

68@@ -68,6 +69,28 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,
69             return strprintf("tx %s unexpectedly failed: %s", wtxid.ToString(), atmp_result.m_state.ToString());
70         }
71 
72+        // Each subpackage is allowed MAX_REPLACEMENT_CANDIDATES replacements (only checking individually here)
73+        if (atmp_result.m_replaced_transactions.size() > MAX_REPLACEMENT_CANDIDATES) {
74+            return strprintf("tx %s result replaced too many transactions",
75+                                wtxid.ToString());
76+        }

murchandamus commented at 3:47 pm on June 17, 2024:

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

0        // Each subpackage is allowed MAX_REPLACEMENT_CANDIDATES replacements (only checking individually here)
1        if (atmp_result.m_replaced_transactions.size() > MAX_REPLACEMENT_CANDIDATES) {
2            return strprintf("tx %s would replace too many transactions",
3                                wtxid.ToString());
4        }

instagibbs commented at 6:37 pm on June 17, 2024:

done in follow-up

glozow commented at 10:55 am on July 4, 2024:

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.

in src/test/util/txmempool.cpp:82 in d3466e4cc5 outdated

77+
78+        // Replacements can't happen for subpackages larger than 2
79+        if (!atmp_result.m_replaced_transactions.empty() &&
80+            atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() > 2) {
81+             return strprintf("tx %s was part of a too-large package RBF subpackage",
82+                                wtxid.ToString());

murchandamus commented at 3:50 pm on June 17, 2024:

I think there may be extraneous words in “too-large package RBF subpackage”. How about:

0             return strprintf("RBF subpackage with tx %s was too large",
1                                wtxid.ToString());

instagibbs commented at 6:36 pm on June 17, 2024:

done in follow-up

in test/functional/mempool_package_rbf.py:176 in 4d15bcf448 outdated

171+        package_hex3, package_txns3 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_CHILD_FEE)
172+        pkg_results3 = node.submitpackage(package_hex3)
173+        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"])
174+
175+        self.assert_mempool_contents(expected=package_txns1)
176+        self.generate(node, 1)

murchandamus commented at 4:17 pm on June 17, 2024:

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

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

instagibbs commented at 6:36 pm on June 17, 2024:

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

in test/functional/mempool_package_rbf.py:188 in 4d15bcf448 outdated

183+        package_hex5, package_txns5 = self.create_simple_package(coin, parent_fee=DEFAULT_CHILD_FEE, child_fee=DEFAULT_CHILD_FEE - Decimal("0.00000001"))
184+        pkg_results5 = node.submitpackage(package_hex5)
185+        assert 'package RBF failed: package feerate is less than parent feerate' in pkg_results5["package_msg"]
186+
187+        self.assert_mempool_contents(expected=package_txns4)
188+        self.generate(node, 1)

murchandamus commented at 4:21 pm on June 17, 2024:

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

instagibbs commented at 6:36 pm on June 17, 2024:

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

murchandamus commented at 5:15 pm on June 17, 2024: contributor

utACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10

instagibbs commented at 6:37 pm on June 17, 2024: member

opened followup at https://github.com/bitcoin/bitcoin/pull/30295

achow101 commented at 9:09 pm on June 17, 2024: member

ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10

achow101 merged this on Jun 17, 2024

achow101 closed this on Jun 17, 2024

hebasto added the label Needs CMake port on Jun 30, 2024

fanquake referenced this in commit c4d45b695e on Jul 12, 2024

hebasto commented at 6:46 am on July 14, 2024: member

Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/264.

hebasto removed the label Needs CMake port on Jul 14, 2024

Cluster size 2 package rbf #28984

Code Coverage

Reviews

Conflicts