Skip to content

Commit 6f0cbc7

Browse files
committed
Merge bitcoin/bitcoin#22539: Re-include RBF replacement txs in fee estimation
3b61372 Add release notes for fee est with replacement txs (Antoine Poinsot) 4556406 qa: test fee estimation with replacement transactions (Antoine Poinsot) 053415b qa: split run_test into smaller parts (Antoine Poinsot) 06c5ce9 Re-include RBF replacement txs in fee estimation (Antoine Poinsot) Pull request description: This effectively reverts #9519. RBF is now largely in use on the network (signaled for by around 20% of all transactions on average) and replacement logic is implemented in most end-user wallets. The rate of replaced transactions is also expected to rise as fee-bumping techniques are being developed for pre-signed transaction ("L2") protocols. ACKs for top commit: prayank23: reACK bitcoin/bitcoin@3b61372 Zero-1729: re-ACK 3b61372 benthecarman: reACK 3b61372 glozow: ACK 3b61372 theStack: re-ACK 3b61372 🍪 Tree-SHA512: a6146d15c80ff4ba9249314b0ef953a66a15673e61b8f98979642814f1b169b5695e330e3ee069fa9a7e4d1f8aa10e1dcb7f9aa79181cea5a4c4dbcaf5483023
2 parents c0b6c96 + 3b61372 commit 6f0cbc7

File tree

3 files changed

+131
-35
lines changed

3 files changed

+131
-35
lines changed

doc/release-notes-22539.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
Notable changes
2+
===============
3+
4+
P2P and network changes
5+
-----------------------
6+
7+
- Fee estimation now takes the feerate of replacement (RBF) transactions into
8+
account.

src/validation.cpp

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,6 @@ class MemPoolAccept
474474
std::unique_ptr<CTxMemPoolEntry> m_entry;
475475
std::list<CTransactionRef> m_replaced_transactions;
476476

477-
bool m_replacement_transaction;
478477
CAmount m_base_fees;
479478
CAmount m_modified_fees;
480479
/** Total modified fees of all transactions being replaced. */
@@ -556,7 +555,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
556555
CTxMemPool::setEntries& allConflicting = ws.m_all_conflicting;
557556
CTxMemPool::setEntries& setAncestors = ws.m_ancestors;
558557
std::unique_ptr<CTxMemPoolEntry>& entry = ws.m_entry;
559-
bool& fReplacementTransaction = ws.m_replacement_transaction;
560558
CAmount& nModifiedFees = ws.m_modified_fees;
561559
CAmount& nConflictingFees = ws.m_conflicting_fees;
562560
size_t& nConflictingSize = ws.m_conflicting_size;
@@ -779,8 +777,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
779777
}
780778

781779

782-
fReplacementTransaction = setConflicts.size();
783-
if (fReplacementTransaction) {
780+
if (!setConflicts.empty()) {
784781
CFeeRate newFeeRate(nModifiedFees, nSize);
785782
// It's possible that the replacement pays more fees than its direct conflicts but not more
786783
// than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the
@@ -885,7 +882,6 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
885882
const CAmount& nModifiedFees = ws.m_modified_fees;
886883
const CAmount& nConflictingFees = ws.m_conflicting_fees;
887884
const size_t& nConflictingSize = ws.m_conflicting_size;
888-
const bool fReplacementTransaction = ws.m_replacement_transaction;
889885
std::unique_ptr<CTxMemPoolEntry>& entry = ws.m_entry;
890886

891887
// Remove conflicting transactions from the mempool
@@ -901,11 +897,10 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
901897
m_pool.RemoveStaged(allConflicting, false, MemPoolRemovalReason::REPLACED);
902898

903899
// This transaction should only count for fee estimation if:
904-
// - it isn't a BIP 125 replacement transaction (may not be widely supported)
905900
// - it's not being re-added during a reorg which bypasses typical mempool fee limits
906901
// - the node is not behind
907902
// - the transaction is not dependent on any other transactions in the mempool
908-
bool validForFeeEstimation = !fReplacementTransaction && !bypass_limits && IsCurrentForFeeEstimation(m_active_chainstate) && m_pool.HasNoInputsOf(tx);
903+
bool validForFeeEstimation = !bypass_limits && IsCurrentForFeeEstimation(m_active_chainstate) && m_pool.HasNoInputsOf(tx);
909904

910905
// Store transaction in memory
911906
m_pool.addUnchecked(*entry, setAncestors, validForFeeEstimation);

test/functional/feature_fee_estimation.py

Lines changed: 121 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
55
"""Test fee estimation code."""
66
from decimal import Decimal
7+
import os
78
import random
89

910
from test_framework.messages import (
@@ -155,6 +156,21 @@ def check_estimates(node, fees_seen):
155156
check_raw_estimates(node, fees_seen)
156157
check_smart_estimates(node, fees_seen)
157158

159+
160+
def send_tx(node, utxo, feerate):
161+
"""Broadcast a 1in-1out transaction with a specific input and feerate (sat/vb)."""
162+
overhead, op, scriptsig, nseq, value, spk = 10, 36, 5, 4, 8, 24
163+
tx_size = overhead + op + scriptsig + nseq + value + spk
164+
fee = tx_size * feerate
165+
166+
tx = CTransaction()
167+
tx.vin = [CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), SCRIPT_SIG[utxo["vout"]])]
168+
tx.vout = [CTxOut(int(utxo["amount"] * COIN) - fee, P2SH_1)]
169+
txid = node.sendrawtransaction(tx.serialize().hex())
170+
171+
return txid
172+
173+
158174
class EstimateFeeTest(BitcoinTestFramework):
159175
def set_test_params(self):
160176
self.num_nodes = 3
@@ -212,48 +228,36 @@ def transact_and_mine(self, numblocks, mining_node):
212228
newmem.append(utx)
213229
self.memutxo = newmem
214230

215-
def run_test(self):
216-
self.log.info("This test is time consuming, please be patient")
217-
self.log.info("Splitting inputs so we can generate tx's")
218-
219-
# Start node0
220-
self.start_node(0)
231+
def initial_split(self, node):
232+
"""Split two coinbase UTxOs into many small coins"""
221233
self.txouts = []
222234
self.txouts2 = []
223235
# Split a coinbase into two transaction puzzle outputs
224-
split_inputs(self.nodes[0], self.nodes[0].listunspent(0), self.txouts, True)
236+
split_inputs(node, node.listunspent(0), self.txouts, True)
225237

226238
# Mine
227-
while len(self.nodes[0].getrawmempool()) > 0:
228-
self.generate(self.nodes[0], 1)
239+
while len(node.getrawmempool()) > 0:
240+
self.generate(node, 1)
229241

230242
# Repeatedly split those 2 outputs, doubling twice for each rep
231243
# Use txouts to monitor the available utxo, since these won't be tracked in wallet
232244
reps = 0
233245
while reps < 5:
234246
# Double txouts to txouts2
235247
while len(self.txouts) > 0:
236-
split_inputs(self.nodes[0], self.txouts, self.txouts2)
237-
while len(self.nodes[0].getrawmempool()) > 0:
238-
self.generate(self.nodes[0], 1)
248+
split_inputs(node, self.txouts, self.txouts2)
249+
while len(node.getrawmempool()) > 0:
250+
self.generate(node, 1)
239251
# Double txouts2 to txouts
240252
while len(self.txouts2) > 0:
241-
split_inputs(self.nodes[0], self.txouts2, self.txouts)
242-
while len(self.nodes[0].getrawmempool()) > 0:
243-
self.generate(self.nodes[0], 1)
253+
split_inputs(node, self.txouts2, self.txouts)
254+
while len(node.getrawmempool()) > 0:
255+
self.generate(node, 1)
244256
reps += 1
245-
self.log.info("Finished splitting")
246-
247-
# Now we can connect the other nodes, didn't want to connect them earlier
248-
# so the estimates would not be affected by the splitting transactions
249-
self.start_node(1)
250-
self.start_node(2)
251-
self.connect_nodes(1, 0)
252-
self.connect_nodes(0, 2)
253-
self.connect_nodes(2, 1)
254-
255-
self.sync_all()
256257

258+
def sanity_check_estimates_range(self):
259+
"""Populate estimation buckets, assert estimates are in a sane range and
260+
are strictly increasing as the target decreases."""
257261
self.fees_per_kb = []
258262
self.memutxo = []
259263
self.confutxo = self.txouts # Start with the set of confirmed txouts after splitting
@@ -279,11 +283,100 @@ def run_test(self):
279283
self.log.info("Final estimates after emptying mempools")
280284
check_estimates(self.nodes[1], self.fees_per_kb)
281285

282-
# check that the effective feerate is greater than or equal to the mempoolminfee even for high mempoolminfee
283-
self.log.info("Test fee rate estimation after restarting node with high MempoolMinFee")
286+
def test_feerate_mempoolminfee(self):
284287
high_val = 3*self.nodes[1].estimatesmartfee(1)['feerate']
285288
self.restart_node(1, extra_args=[f'-minrelaytxfee={high_val}'])
286289
check_estimates(self.nodes[1], self.fees_per_kb)
290+
self.restart_node(1)
291+
292+
def sanity_check_rbf_estimates(self, utxos):
293+
"""During 5 blocks, broadcast low fee transactions. Only 10% of them get
294+
confirmed and the remaining ones get RBF'd with a high fee transaction at
295+
the next block.
296+
The block policy estimator should return the high feerate.
297+
"""
298+
# The broadcaster and block producer
299+
node = self.nodes[0]
300+
miner = self.nodes[1]
301+
# In sat/vb
302+
low_feerate = 1
303+
high_feerate = 10
304+
# Cache the utxos of which to replace the spender after it failed to get
305+
# confirmed
306+
utxos_to_respend = []
307+
txids_to_replace = []
308+
309+
assert len(utxos) >= 250
310+
for _ in range(5):
311+
# Broadcast 45 low fee transactions that will need to be RBF'd
312+
for _ in range(45):
313+
u = utxos.pop(0)
314+
txid = send_tx(node, u, low_feerate)
315+
utxos_to_respend.append(u)
316+
txids_to_replace.append(txid)
317+
# Broadcast 5 low fee transaction which don't need to
318+
for _ in range(5):
319+
send_tx(node, utxos.pop(0), low_feerate)
320+
# Mine the transactions on another node
321+
self.sync_mempools(wait=.1, nodes=[node, miner])
322+
for txid in txids_to_replace:
323+
miner.prioritisetransaction(txid=txid, fee_delta=-COIN)
324+
self.generate(miner, 1)
325+
self.sync_blocks(wait=.1, nodes=[node, miner])
326+
# RBF the low-fee transactions
327+
while True:
328+
try:
329+
u = utxos_to_respend.pop(0)
330+
send_tx(node, u, high_feerate)
331+
except IndexError:
332+
break
333+
334+
# Mine the last replacement txs
335+
self.sync_mempools(wait=.1, nodes=[node, miner])
336+
self.generate(miner, 1)
337+
self.sync_blocks(wait=.1, nodes=[node, miner])
338+
339+
# Only 10% of the transactions were really confirmed with a low feerate,
340+
# the rest needed to be RBF'd. We must return the 90% conf rate feerate.
341+
high_feerate_kvb = Decimal(high_feerate) / COIN * 10**3
342+
est_feerate = node.estimatesmartfee(2)["feerate"]
343+
assert est_feerate == high_feerate_kvb
344+
345+
def run_test(self):
346+
self.log.info("This test is time consuming, please be patient")
347+
self.log.info("Splitting inputs so we can generate tx's")
348+
349+
# Split two coinbases into many small utxos
350+
self.start_node(0)
351+
self.initial_split(self.nodes[0])
352+
self.log.info("Finished splitting")
353+
354+
# Now we can connect the other nodes, didn't want to connect them earlier
355+
# so the estimates would not be affected by the splitting transactions
356+
self.start_node(1)
357+
self.start_node(2)
358+
self.connect_nodes(1, 0)
359+
self.connect_nodes(0, 2)
360+
self.connect_nodes(2, 1)
361+
self.sync_all()
362+
363+
self.log.info("Testing estimates with single transactions.")
364+
self.sanity_check_estimates_range()
365+
366+
# check that the effective feerate is greater than or equal to the mempoolminfee even for high mempoolminfee
367+
self.log.info("Test fee rate estimation after restarting node with high MempoolMinFee")
368+
self.test_feerate_mempoolminfee()
369+
370+
self.log.info("Restarting node with fresh estimation")
371+
self.stop_node(0)
372+
fee_dat = os.path.join(self.nodes[0].datadir, self.chain, "fee_estimates.dat")
373+
os.remove(fee_dat)
374+
self.start_node(0)
375+
self.connect_nodes(0, 1)
376+
self.connect_nodes(0, 2)
377+
378+
self.log.info("Testing estimates with RBF.")
379+
self.sanity_check_rbf_estimates(self.confutxo + self.memutxo)
287380

288381
self.log.info("Testing that fee estimation is disabled in blocksonly.")
289382
self.restart_node(0, ["-blocksonly"])

0 commit comments

Comments
 (0)