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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
Reviewers, this pull request conflicts with the following ones:
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.
535@@ -536,7 +536,7 @@ def test_v3_sibling_eviction(self):
536 fee_to_beat_child2 = int(tx_v3_child_2["fee"] * COIN)
0 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
fee_per_output=fee_to_beat_child2*2 ?
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.
Oof my bad for not checking more closely. How’s this? (Edited)
0diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
1index 8285b82c19..ab43ca60c3 100755
2--- a/test/functional/mempool_accept_v3.py
3+++ b/test/functional/mempool_accept_v3.py
4@@ -533,10 +533,28 @@ class MempoolAcceptV3(BitcoinTestFramework):
5 tx_unrelated_replacee = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_unrelated_conflict)
6 assert tx_unrelated_replacee["txid"] in node.getrawmempool()
7
8- fee_to_beat = max(int(tx_v3_child_2["fee"] * COIN), int(tx_unrelated_replacee["fee"]*COIN))
9+ fee_to_beat_absolute = int(tx_v3_child_2["fee"] * COIN) + int(tx_unrelated_replacee["fee"]*COIN)
10+
11+ entry_unrelated_replacee = node.getmempoolentry(tx_unrelated_replacee["txid"])
12+ entry_tx_v3_child_2 = node.getmempoolentry(tx_v3_child_2["txid"])
13+ entry_tx_parent = node.getmempoolentry(tx_v3_parent["txid"])
14+
15+ # Need to have a higher feerate than both direct conflicts
16+ feerate_to_beat = max(int(entry_unrelated_replacee["fees"]["modified"] / entry_unrelated_replacee["vsize"] * COIN), \
17+ int(entry_tx_v3_child_2["fees"]["modified"] / entry_tx_v3_child_2["vsize"] * COIN))
18+
19+ # Simulate to get tx size
20+ test_tx_v3_child_3 = self.wallet.create_self_transfer_multi(
21+ utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], version=3
22+ )
23+
24+ # Include the parent size because the ancestor feerate is what matters.
25+ fee_to_beat_feerate = feerate_to_beat * (test_tx_v3_child_3["tx"].get_vsize() + entry_tx_parent["vsize"])
26+ fee_v3_child_3 = max(fee_to_beat_feerate, fee_to_beat_absolute) + 5
27
28 tx_v3_child_3 = self.wallet.create_self_transfer_multi(
29- utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], fee_per_output=fee_to_beat*2, version=3
30+ utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict],
31+ fee_per_output=fee_v3_child_3, version=3
32 )
33 node.sendrawtransaction(tx_v3_child_3["hex"])
34 self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_3["txid"]])
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 /104fee_to_beat_feerate is 300 * (147 + 154) = 90300fee_v3_child_3 is max(41600, 90300) + 5 = 90305
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?
0diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
1index 8285b82c19..db6aa78167 100755
2--- a/test/functional/mempool_accept_v3.py
3+++ b/test/functional/mempool_accept_v3.py
4@@ -533,13 +533,25 @@ class MempoolAcceptV3(BitcoinTestFramework):
5 tx_unrelated_replacee = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_unrelated_conflict)
6 assert tx_unrelated_replacee["txid"] in node.getrawmempool()
7
8- fee_to_beat = max(int(tx_v3_child_2["fee"] * COIN), int(tx_unrelated_replacee["fee"]*COIN))
9-
10+ def get_ancestor_feerate(txid):
11+ entry = node.getmempoolentry(txid)
12+ return entry["fees"]["ancestor"]*COIN // entry["ancestorsize"]
13+
14+ # For RBF to succeed, we should have a greater total fee and a greater
15+ # feerate than what we're replacing.
16+ # Use 500 vbytes as an upper bound on how big the new transaction might be.
17+ fee_to_beat = int(tx_v3_child_2["fee"]*COIN) + int(tx_unrelated_replacee["fee"]*COIN) + 500
18+ feerate_to_beat = max(get_ancestor_feerate(tx_v3_child_2["txid"]),
19+ get_ancestor_feerate(tx_unrelated_replacee["txid"]))
20+
21+ # Since this transaction has a parent, double the target feerate so
22+ # that we'll pay for the parent (assuming comparable sizes).
23 tx_v3_child_3 = self.wallet.create_self_transfer_multi(
24- utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], fee_per_output=fee_to_beat*2, version=3
25+ 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
26 )
27 node.sendrawtransaction(tx_v3_child_3["hex"])
28 self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_3["txid"]])
29+ assert get_ancestor_feerate(tx_v3_child_3["txid"]) > feerate_to_beat [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)(extra_args=["-acceptnonstdtxn=1"])
30 def test_reorg_sibling_eviction_1p2c(self):
31diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py
32index 470ed08ed4..1e31ad6139 100644
33--- a/test/functional/test_framework/wallet.py
34+++ b/test/functional/test_framework/wallet.py
35@@ -292,6 +292,7 @@ class MiniWallet:
36 fee_per_output=1000,
37 target_weight=0,
38 confirmed_only=False,
39+ target_feerate=None
40 ):
41 """
42 Create and return a transaction that spends the given UTXOs and creates a
43@@ -322,6 +323,20 @@ class MiniWallet:
44 if target_weight:
45 self._bulk_tx(tx, target_weight)
46
47+ # If a certain feerate is required, use the size we just calculated to
48+ # adjust the outputs, so that we achieve the target feerate.
49+ if target_feerate:
50+ additional_fee = target_feerate * tx.get_vsize() - fee
51+ if (additional_fee > 0):
52+ reduce_amount = additional_fee // num_outputs
53+ outputs_value_total = 0
54+ for i in range(num_outputs):
55+ tx.vout[i].nValue -= int(reduce_amount)
56+ assert tx.vout[i].nValue > 0
57+ outputs_value_total += tx.vout[i].nValue
58+ self.sign_tx(tx)
59+ fee = Decimal(inputs_value_total - outputs_value_total) / COIN
60+
61 txid = tx.rehash()
62 return {
Labels
Tests