Cluster size 2 package rbf #28984

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 )

commit message is old; will fix

Should this be "> 2"?

I have a failing unit test for this :facepalm:

pushed fix

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

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

nit: I found the "not" in this comment confusing.

reworded

Nit: Did you mean replacEable?

fixed

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

added

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

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

I'll try and rephrase soon.

rephrased into the two cases

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

But the diff is not a only a doc change.

changed wording of commit

In 228a5ed16586ef93e7ec5c5da3ef4df0d5fc322f

#include <algorithm>

replaced section of code entirely

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?

replaced entirely(I think?)

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

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

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

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

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

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

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

this all got moved internal to mempool

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.

        // Ancestor and descendant counts are inclusive of the tx itself.
        const auto ancestor_count{direct_conflict->GetCountWithAncestors()};
        const auto descendant_count{direct_conflict->GetCountWithDescendants()};
        const bool has_ancestor{ancestor_count > 1};
        const bool has_descendant{descendant_count > 1};

        const auto& txid_string{direct_conflict->GetSharedTx()->GetHash().ToString()};

        // The only allowed configurations are:
        // 1 ancestor and 0 descendant
        // 0 ancestor and 1 descendant
        // 0 ancestor and 0 descendant
        if (ancestor_count > 2) {
            return strprintf("%s has %u ancestors, max 1 allowed", ancestor_count, txid_string", txid_string);
        } else if (descendant_count > 2) {
            return strprintf("%s has %u descendants, max 1 allowed", ancestor_count, txid_string", txid_string);
        } else if (has_ancestor && has_descendant) {
            return strprintf("%s has both ancestor and descendant", txid_string);
        }

        // Additionally enforce that:
        // If we have a parent, we are its only child.
        // If we have a child,  we are its only parent.
        if (has_descendant) {
            const auto& our_child = direct_conflict->GetMemPoolChildrenConst().begin();
            if (our_child->get().GetCountWithAncestors() > 2) {
                return strprintf("%s is not the only parent of child %s",
                                 txid_string, our_child->get().GetSharedTx()->GetHash().ToString());
            }
        } else if (has_ancestor) {
            const auto& our_parent = direct_conflict->GetMemPoolParentsConst().begin();
            if (our_parent->get().GetCountWithDescendants() > 2) {
                return strprintf("%s is not the only child of parent %s",
                                 txid_string, our_parent->get().GetSharedTx()->GetHash().ToString());
            }
        }

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

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

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

taken

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

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

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

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

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

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

left a comment for future work

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:

        LogPrint(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n",
                it->GetTx().GetHash().ToString(),
                it->GetTx().GetWitnessHash().ToString(),
                it->GetFee(),
                it->GetTxSize(),
                hash.ToString(),
                tx.GetWitnessHash().ToString(),
                entry->GetFee(),
                entry->GetTxSize());

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

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

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

done

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

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?

34c513554c7be97d91446126bb3ef0de2884fc19

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

"in a package"?

pushed updated text

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

added a small check here using GetEntry that takes Wtxids.

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.

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

added an extra phrase, hope that helps?

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

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

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

taken

Maybe test that Package RBF carveout is not supported?

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

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?

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.

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)

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

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?

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?

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.

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.

added updated comment

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

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

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

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

package1 and package3 has the same parent

fixed

You moved fill_mempool to util in #29735

forgot to remove, done

I think this might be a better name

    def test_package_rbf_with_conflicting_packages(self):

done

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

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

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

nit:

    def test_package_rbf_with_numerous_ancestors(self):

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

nit:

    def test_package_rbf_with_wrong_pkg_size(self):

taken

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

taken

Incomplete sentence?

complete sentence, but not grammatically correct or useful, removed

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

2024-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)
2024-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)
2024-04-26T14:36:53.889920Z [httpworker.3] [validationinterface.cpp:203] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=1ae8118ceb71249b2af793f671a1293a4c999e89f5091f52c42a3cf1e08c6840 wtxid=97fc8654dcbd025b4165c56179d124ee4adf2d65acf8a66f8f5464dab7d07089 reason=replaced
2024-04-26T14:36:53.889929Z [scheduler] [validationinterface.cpp:203] [operator()] [validation] TransactionRemovedFromMempool: txid=1ae8118ceb71249b2af793f671a1293a4c999e89f5091f52c42a3cf1e08c6840 wtxid=97fc8654dcbd025b4165c56179d124ee4adf2d65acf8a66f8f5464dab7d07089 reason=replaced
2024-04-26T14:36:53.889938Z [httpworker.3] [validationinterface.cpp:203] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=9bd6895590404d3b04a208bbebf48a0397cb43b4df7ad349427be5e7dc1a2bbb wtxid=e26f5339fcf7d3232c9bd58a99e6e2cb9c2369d70e7f8fed7296221ee8667119 reason=replaced
2024-04-26T14:36:53.889946Z [scheduler] [validationinterface.cpp:203] [operator()] [validation] TransactionRemovedFromMempool: txid=9bd6895590404d3b04a208bbebf48a0397cb43b4df7ad349427be5e7dc1a2bbb wtxid=e26f5339fcf7d3232c9bd58a99e6e2cb9c2369d70e7f8fed7296221ee8667119 reason=replaced
2024-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
2024-04-26T14:36:53.889981Z [httpworker.3] [validationinterface.cpp:193] [TransactionAddedToMempool] [validation] Enqueuing TransactionAddedToMempool: txid=a95629c7f4690199fce1ffba70b8fa9ce854d74da76e9e88fd0d4676211f10d5 wtxid=009f7beb2df5b699bea4233a5b9b6fab2a80d42639b19291d4909a63f8739c47
2024-04-26T14:36:53.889988Z [httpworker.3] [validationinterface.cpp:193] [TransactionAddedToMempool] [validation] Enqueuing TransactionAddedToMempool: txid=69e3f27c8097c2f76b32c3f6fb31cc11d89b674bed800cf61236e921c8c9d5c3 wtxid=ed7b933757b73937bf454af80e60cbd9b4fb24d7ce95d297519f5327a0470333
2024-04-26T14:36:53.889995Z [scheduler] [validationinterface.cpp:193] [operator()] [validation] TransactionAddedToMempool: txid=a95629c7f4690199fce1ffba70b8fa9ce854d74da76e9e88fd0d4676211f10d5 wtxid=009f7beb2df5b699bea4233a5b9b6fab2a80d42639b19291d4909a63f8739c47
2024-04-26T14:36:53.890001Z [scheduler] [validationinterface.cpp:193] [operator()] [validation] TransactionAddedToMempool: txid=69e3f27c8097c2f76b32c3f6fb31cc11d89b674bed800cf61236e921c8c9d5c3 wtxid=ed7b933757b73937bf454af80e60cbd9b4fb24d7ce95d297519f5327a0470333

nit:

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

taken

nit: use static cast?

done

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?

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

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

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

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

Oh even easier than what I replaced it with, duh

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

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

done

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.

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.

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

swapped them and updated tests accordingly

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

done

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

done

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

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

done

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

@glozow pushed some more detailed docs, thanks!

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?

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

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?

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

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

removed a series of these ignored args

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

picked a constant and used it

This is now in mempool_util (= linter complaint)

done

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

    const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize);

taken

nit; make reference not copy

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

done

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

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

taken

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.

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

with rebase

        fill_mempool(self, self.nodes[0])

done

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

diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 8285b82c19..990d36359e 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -579,6 +579,32 @@ class MempoolAcceptV3(BitcoinTestFramework):
         )
         self.check_mempool([tx_with_multi_children["txid"], tx_with_sibling3_rbf["txid"], tx_with_sibling2["txid"]])
 
+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)(extra_args=["-acceptnonstdtxn=1"])
+    def test_package_rbf(self):
+        self.log.info("Test package RBF: v3 0-fee parent + high-fee child replaces parent's conflicts")
+        node = self.nodes[0]
+        # Reuse the same coins so that the transactions conflict with one another.
+        parent_coin = self.wallet.get_utxo(confirmed_only=True)
+
+        # package1 pays default fee on both transactions
+        parent1 = self.wallet.create_self_transfer(utxo_to_spend=parent_coin, version=3)
+        child1 = self.wallet.create_self_transfer(utxo_to_spend=parent1["new_utxo"], version=3)
+        package_hex1 = [parent1["hex"], child1["hex"]]
+        fees_package1 = parent1["fee"] + child1["fee"]
+        submitres1 = node.submitpackage(package_hex1)
+        assert_equal(submitres1["package_msg"], "success")
+        self.check_mempool([parent1["txid"], child1["txid"]])
+
+        # package2 has a 0-fee parent (conflicting with package1) and very high fee child
+        parent2 = self.wallet.create_self_transfer(utxo_to_spend=parent_coin, fee=0, fee_rate=0, version=3)
+        child2 = self.wallet.create_self_transfer(utxo_to_spend=parent2["new_utxo"], fee=fees_package1*10, version=3)
+        package_hex2 = [parent2["hex"], child2["hex"]]
+
+        submitres2 = node.submitpackage(package_hex2)
+        assert_equal(submitres2["package_msg"], "success")
+        assert_equal(set(submitres2["replaced-transactions"]), set([parent1["txid"], child1["txid"]]))
+        self.check_mempool([parent2["txid"], child2["txid"]])
+
 
     def run_test(self):
         self.log.info("Generate blocks to create UTXOs")
@@ -598,6 +624,7 @@ class MempoolAcceptV3(BitcoinTestFramework):
         self.test_reorg_2child_rbf()
         self.test_v3_sibling_eviction()
         self.test_reorg_sibling_eviction_1p2c()
+        self.test_package_rbf()
 
 
 if __name__ == "__main__":

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

s/V3 transactions/TRUC transactions/, perhaps?

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

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.

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

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

fixed

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

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

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

made an attempt at a grepp-able cautionary note

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:

Consider:

TxA (in mempool) <- TxB (in mempool)

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

isn't possible due to V3 ancestor limits.

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:

EQQAAAgAAAAAAAAAAAAAAAUAAAAAAAAAAAAAAAA6AAAACAAADQAAAAAAAAAAAAAAAAAAaQFVKGEAawAAAAAABQAAAAAAAAAAAAAAAAA6AAAACAAADQAAAAAAAAAAAAAAAAAAaQFVKGF0YWNhcnJpZXJzaXplVVUAAAEAAAAAAFURVQEuaQEA/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.

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

It hits in the fuzzing package_eval fuzzing target.

I was able to reproduce this locally.

nit 0044c1dad9ffc3d58afd06d7533f16beb0d0a829:

Would prefer to not have magic number comments

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

taken

nit: 7d895935aef63b4aa728310b580f418ce35fc234

we started using low_fee_amt instead of 200 in this test

adapted more of the test to use this value

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

        if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
            BOOST_ERROR(err_1.value());
        }
        auto it_parent_1 = submit1.m_tx_results.find(tx_parent->GetWitnessHash());
        auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());

        BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
        BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
        expected_pool_size += 2;
        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);

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

Can check m_wtxids_fee_calculations and m_effective_feerate.

Wrote up the changes for this + using CheckPackageMempoolAcceptResult:

index cd69fce2f5..b68686cfcb 100644
--- a/src/test/txpackage_tests.cpp
+++ b/src/test/txpackage_tests.cpp
@@ -975,28 +975,22 @@ BOOST_FIXTURE_TEST_CASE(package_rbf_tests, TestChain100Setup)
 
         LOCK(m_node.mempool->cs);
         const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, /*test_accept=*/false, std::nullopt);
-        BOOST_CHECK_MESSAGE(submit1.m_state.IsValid(), "Package validation unexpectedly failed: " << submit1.m_state.GetRejectReason());
-        auto it_parent_1 = submit1.m_tx_results.find(tx_parent->GetWitnessHash());
-        auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
-        BOOST_CHECK(it_parent_1 != submit1.m_tx_results.end());
-        BOOST_CHECK(it_child_1 != submit1.m_tx_results.end());
-        BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
-        BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_1.value());
+        }
         expected_pool_size += 2;
-        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash())));
-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));
 
         const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, /*test_accept=*/false, std::nullopt);
+        if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_2.value());
+        }
         auto it_parent_2 = submit2.m_tx_results.find(tx_parent->GetWitnessHash());
         auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash());
-        BOOST_CHECK(it_parent_2 != submit2.m_tx_results.end());
-        BOOST_CHECK(it_child_2 != submit2.m_tx_results.end());
-        BOOST_CHECK_MESSAGE(submit2.m_state.IsValid(), "Package validation unexpectedly failed" << submit2.m_state.GetRejectReason());
         BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
         BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_2->GetHash())));
+
+        // child1 has been replaced
         BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));
     }
 
@@ -1034,44 +1028,60 @@ BOOST_FIXTURE_TEST_CASE(package_rbf_tests, TestChain100Setup)
         Package package3{tx_parent_3, tx_child_3};
 
         const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt);
-        BOOST_CHECK_MESSAGE(submit1.m_state.IsValid(), "Package validation unexpectedly failed" << submit1.m_state.GetRejectReason());
+        if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_1.value());
+        }
         auto it_parent_1 = submit1.m_tx_results.find(tx_parent_1->GetWitnessHash());
         auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
-        BOOST_CHECK(it_parent_1 != submit1.m_tx_results.end());
-        BOOST_CHECK(it_child_1 != submit1.m_tx_results.end());
         BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
         BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK_EQUAL(it_parent_1->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_parent_1)), 200);
+        BOOST_CHECK_EQUAL(it_child_1->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child_1)), 200);
+        BOOST_CHECK(it_parent_1->second.m_replaced_transactions.empty());
+        BOOST_CHECK(it_child_1->second.m_replaced_transactions.empty());
         expected_pool_size += 2;
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_1->GetHash())));
-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));
 
         // This replacement is actually not package rbf; the parent carries enough fees
         // to replace the entire package on its own.
         const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, false, std::nullopt);
-        BOOST_CHECK_MESSAGE(submit2.m_state.IsValid(), "Package validation unexpectedly failed" << submit2.m_state.GetRejectReason());
+        if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_2.value());
+        }
         auto it_parent_2 = submit2.m_tx_results.find(tx_parent_2->GetWitnessHash());
         auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash());
-        BOOST_CHECK(it_parent_2 != submit2.m_tx_results.end());
-        BOOST_CHECK(it_child_2 != submit2.m_tx_results.end());
         BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
         BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK_EQUAL(it_parent_2->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_parent_2)), 800);
+        BOOST_CHECK_EQUAL(it_child_2->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child_2)), 200);
+
+        // parent2 replaced both transactions
+        BOOST_CHECK(it_parent_2->second.m_replaced_transactions.size() == 2);
+        BOOST_CHECK(it_child_2->second.m_replaced_transactions.empty());
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_2->GetHash())));
-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_2->GetHash())));
 
         // Package RBF, in which the replacement transaction's child sponsors the fees to meet RBF feerate rules
         const auto submit3 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package3, false, std::nullopt);
-        BOOST_CHECK_MESSAGE(submit3.m_state.IsValid(), "Package validation unexpectedly failed" << submit3.m_state.GetRejectReason());
+        if (auto err_3{CheckPackageMempoolAcceptResult(package3, submit3, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_3.value());
+        }
         auto it_parent_3 = submit3.m_tx_results.find(tx_parent_3->GetWitnessHash());
         auto it_child_3 = submit3.m_tx_results.find(tx_child_3->GetWitnessHash());
-        BOOST_CHECK(it_parent_3 != submit3.m_tx_results.end());
-        BOOST_CHECK(it_child_3 != submit3.m_tx_results.end());
         BOOST_CHECK_EQUAL(it_parent_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
         BOOST_CHECK_EQUAL(it_child_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+
+        // package3 was considered as a package to replace both package2 transactions
+        BOOST_CHECK(it_parent_3->second.m_replaced_transactions.size() == 2);
+        BOOST_CHECK(it_child_3->second.m_replaced_transactions.empty());
+
+        std::vector<Wtxid> expected_package3_wtxids({tx_parent_3->GetWitnessHash(), tx_child_3->GetWitnessHash()});
+        const auto package3_total_vsize{GetVirtualTransactionSize(*tx_parent_3) + GetVirtualTransactionSize(*tx_child_3)};
+        BOOST_CHECK(it_parent_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids);
+        BOOST_CHECK(it_child_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids);
+        BOOST_CHECK_EQUAL(it_parent_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 1499);
+        BOOST_CHECK_EQUAL(it_child_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 1499);
+
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_3->GetHash())));
-        BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_3->GetHash())));
     }
 
 }

taken with only light modifications

Needs rebase for #29496?

yep, rebased + removed

aa3a4da248fe9d4a765fa4c80797ca333ee37f07: is the static cast necessary?

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

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?

This should only be called when not test_accept, right?

Aha yes. Can resolve.

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

Directly checked cluster size instead.

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

even better. dropped.

No mentions of BIP125 outside of signaling pls

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

sure, taken

In d3d653dce34ad1f3235869b8a54f9572e89e5b68 [policy] package rbf

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

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

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

nit:

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

done

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

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

squashed and lightly re-ordered

Very different uses of "conflicts" here

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

taken

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

defined it inline

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

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

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

done

nit: slightly easier to reason for the reader imho

    if (package_feerate <= parent_feerate) {

done

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)

added at beginning of function

nit: shorter (can also be done for grandchild_key)

    CKey child_key = GenerateRandomKey();

done