test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py

[sdaftuar]

In the sibling eviction test, we're currently testing that a transaction with ancestor feerate (and mining score) of 179 s/b is able to replace a transaction with ancestor feerate (and mining score) of 300 s/b, due to a shortcoming in our current RBF rules.

In preparation for fixing our RBF rules to not allow such replacements, fix the test by bumping the fee of the replacement to be a bit higher.

[DrahtBot]

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	instagibbs, glozow

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:

• #28676 ([WIP] Cluster mempool implementation by sdaftuar)

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.

[sdaftuar]

@glozow @instagibbs This was something I noticed when rebasing #28676.

[instagibbs]

Suggested change

	fee_to_beat_child2 = int(tx_v3_child_2["fee"] * COIN)
	fee_to_beat = max(int(tx_v3_child_2["fee"] * COIN), int(tx_unrelated_replacee["fee"] * COIN))

would you consider this instead? seems the more direct thing we're trying to accomplish

[glozow]

makes sense to me. Use fee_per_output=fee_to_beat_child2*2 ?

[sdaftuar]

I think that works, let me know if what I pushed matches what you're thinking!

[sdaftuar]

Oops -- I miscalculated the ancestor feerates after rewriting it this way, and the new tx still has insufficient fee. Here's what everything looks like with the change here (that is now merged, doh):

tx_unrelated_replacee: fee 31200, vsize 104
parent tx: fee 2000, vsize 147
tx_v3_child_2: fee 10400, vsize 104

tx_v3_child_3: fee 62400, vsize 157

So the total fee of the new transaction is high enough, but the ancestor feerate is still too low: (62400+2000)/304 = 211, which is less than the 300 s/b we need to beat the first tx.

[glozow]

Oof my bad for not checking more closely. How's this?
(Edited)

diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 8285b82c19..ab43ca60c3 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -533,10 +533,28 @@ class MempoolAcceptV3(BitcoinTestFramework):
         tx_unrelated_replacee = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_unrelated_conflict)
         assert tx_unrelated_replacee["txid"] in node.getrawmempool()
 
-        fee_to_beat = max(int(tx_v3_child_2["fee"] * COIN), int(tx_unrelated_replacee["fee"]*COIN))
+        fee_to_beat_absolute = int(tx_v3_child_2["fee"] * COIN) + int(tx_unrelated_replacee["fee"]*COIN)
+
+        entry_unrelated_replacee = node.getmempoolentry(tx_unrelated_replacee["txid"])
+        entry_tx_v3_child_2 = node.getmempoolentry(tx_v3_child_2["txid"])
+        entry_tx_parent = node.getmempoolentry(tx_v3_parent["txid"])
+
+        # Need to have a higher feerate than both direct conflicts
+        feerate_to_beat = max(int(entry_unrelated_replacee["fees"]["modified"] / entry_unrelated_replacee["vsize"] * COIN), \
+            int(entry_tx_v3_child_2["fees"]["modified"] / entry_tx_v3_child_2["vsize"] * COIN))
+
+        # Simulate to get tx size
+        test_tx_v3_child_3 = self.wallet.create_self_transfer_multi(
+            utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], version=3
+        )
+
+        # Include the parent size because the ancestor feerate is what matters.
+        fee_to_beat_feerate = feerate_to_beat * (test_tx_v3_child_3["tx"].get_vsize() + entry_tx_parent["vsize"])
+        fee_v3_child_3 = max(fee_to_beat_feerate, fee_to_beat_absolute) + 5
 
         tx_v3_child_3 = self.wallet.create_self_transfer_multi(
-            utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], fee_per_output=fee_to_beat*2, version=3
+            utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict],
+            fee_per_output=fee_v3_child_3, version=3
         )
         node.sendrawtransaction(tx_v3_child_3["hex"])
         self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_3["txid"]])

[glozow]

above:

• fee_to_beat_absolute is 41600 (above code uses max(tx_unrelated_replacee, tx_v3_child_2) but Rule 3 would require us to beat the sum of their fees haha)
• feerate_to_beat is 300, i.e. 31200 /104
• fee_to_beat_feerate is 300 * (147 + 154) = 90300
• fee_v3_child_3 is max(41600, 90300) + 5 = 90305
• this means the ancestor feerate is 90305 / (147 + 154) = 300.017

[sdaftuar]

Inspired by your approach, I thought it might be helpful to just add support for setting a target feerate on a transaction in miniwallet, something like this?

diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 8285b82c19..db6aa78167 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -533,13 +533,25 @@ class MempoolAcceptV3(BitcoinTestFramework):
         tx_unrelated_replacee = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_unrelated_conflict)
         assert tx_unrelated_replacee["txid"] in node.getrawmempool()
 
-        fee_to_beat = max(int(tx_v3_child_2["fee"] * COIN), int(tx_unrelated_replacee["fee"]*COIN))
-
+        def get_ancestor_feerate(txid):
+            entry = node.getmempoolentry(txid)
+            return entry["fees"]["ancestor"]*COIN // entry["ancestorsize"]
+
+        # For RBF to succeed, we should have a greater total fee and a greater
+        # feerate than what we're replacing.
+        # Use 500 vbytes as an upper bound on how big the new transaction might be.
+        fee_to_beat = int(tx_v3_child_2["fee"]*COIN) + int(tx_unrelated_replacee["fee"]*COIN) + 500
+        feerate_to_beat = max(get_ancestor_feerate(tx_v3_child_2["txid"]),
+                    get_ancestor_feerate(tx_unrelated_replacee["txid"]))
+
+        # Since this transaction has a parent, double the target feerate so
+        # that we'll pay for the parent (assuming comparable sizes).
         tx_v3_child_3 = self.wallet.create_self_transfer_multi(
-            utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], fee_per_output=fee_to_beat*2, version=3
+            utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], fee_per_output=fee_to_beat, version=3, target_feerate=2*feerate_to_beat
         )
         node.sendrawtransaction(tx_v3_child_3["hex"])
         self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_3["txid"]])
+        assert get_ancestor_feerate(tx_v3_child_3["txid"]) > feerate_to_beat
 
     @cleanup(extra_args=["-acceptnonstdtxn=1"])
     def test_reorg_sibling_eviction_1p2c(self):
diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py
index 470ed08ed4..1e31ad6139 100644
--- a/test/functional/test_framework/wallet.py
+++ b/test/functional/test_framework/wallet.py
@@ -292,6 +292,7 @@ class MiniWallet:
         fee_per_output=1000,
         target_weight=0,
         confirmed_only=False,
+        target_feerate=None
     ):
         """
         Create and return a transaction that spends the given UTXOs and creates a
@@ -322,6 +323,20 @@ class MiniWallet:
         if target_weight:
             self._bulk_tx(tx, target_weight)
 
+        # If a certain feerate is required, use the size we just calculated to
+        # adjust the outputs, so that we achieve the target feerate.
+        if target_feerate:
+            additional_fee = target_feerate * tx.get_vsize() - fee
+            if (additional_fee > 0):
+                reduce_amount = additional_fee // num_outputs
+                outputs_value_total = 0
+                for i in range(num_outputs):
+                    tx.vout[i].nValue -= int(reduce_amount)
+                    assert tx.vout[i].nValue > 0
+                    outputs_value_total += tx.vout[i].nValue
+                self.sign_tx(tx)
+                fee = Decimal(inputs_value_total - outputs_value_total) / COIN
+
         txid = tx.rehash()
         return {

[instagibbs]

ACK f8a141c

[glozow]

ACK f8a141c