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.
981@@ -982,7 +982,6 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
982 const CAmount& nModifiedFees = ws.m_modified_fees;
983 const CAmount& nConflictingFees = ws.m_conflicting_fees;
984 const size_t& nConflictingSize = ws.m_conflicting_size;
985- const bool fReplacementTransaction = ws.m_replacement_transaction;
m_replacement_transaction, now that it is unused and inline setConflicts.size() again?
Concept ACK
802@@ -805,8 +803,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
803 // If we don't hold the lock allConflicting might be incomplete; the
804 // subsequent RemoveStaged() and addUnchecked() calls don't guarantee
805 // mempool consistency for us.
806- fReplacementTransaction = setConflicts.size();
807- if (fReplacementTransaction)
808+ if (setConflicts.size())
809 {
nit: braces go on the same line
0 if (setConflicts.size()) {
Out of interest, did anyone collect stats on how many txs are replacements? Edit: see previous comment
Although 0xb10c shared there are 2000-6000 replacement tx per day , I don’t think it matters for few reasons:
Out of interest, did anyone collect stats on how many txs are replacements? Edit: see previous comment
992@@ -998,11 +993,10 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
993 m_pool.RemoveStaged(allConflicting, false, MemPoolRemovalReason::REPLACED);
994
995 // This transaction should only count for fee estimation if:
996- // - it isn't a BIP 125 replacement transaction (may not be widely supported)
High-fee non-miner-policy txs (In this case full-rbf’ed) might fill your mempool, but never get confirmed, thus raising your estimates.
This is a general problem and I was just wondering if the patch should be re-introduced for full-rbf (or other similar policy changes in the future).
Oh for people running it without knowing that some miners do as well.
I think stopping (again) to consider replacement transactions would be a step backward. However it could make sense for a full-RBF patch to stop considering replacement to not-opted-in transactions for fee estimation until the full-RBF policy is reasonably deployed on the network. Since we only implement explicit signaling it sounds like the complexity of doing so would be reasonable.
untested ACK 4abe6aab798fcb9d80228d4e49844b96690bfa2f (edited hash, sorry clipboard was bugged)
Concept is simple and code changes are very straightforward (only one line is effectively changed).
802@@ -805,9 +803,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
803 // If we don't hold the lock allConflicting might be incomplete; the
804 // subsequent RemoveStaged() and addUnchecked() calls don't guarantee
805 // mempool consistency for us.
806- fReplacementTransaction = setConflicts.size();
807- if (fReplacementTransaction)
808- {
809+ if (setConflicts.size()) {
nit: Not sure if we have this in the coding guidelines, but personally I’m not a fan of implicit int-to-bool casts. Could avoid it via e.g.
0 if (setConflicts.size() > 0) {
or
0 if (!setConflicts.empty()) {
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
utACK https://github.com/bitcoin/bitcoin/pull/22539/commits/4abe6aab798fcb9d80228d4e49844b96690bfa2f
reading through #9519 , the general consensus seemed to be “this will be trivial to and should be reverted once RBF is widely adopted by miners.”
i didn’t get the sense anyone was arguing to never include RBF in fee estimation, so given that RBF is widely adopted, this change makes sense.
crACK 4abe6aab798fcb9d80228d4e49844b96690bfa2f
Change makes sense given the wide adoption of RBF txs. Plus, as stated above, the original PR that excluded RBFs did state a potential revert in the future once there’s increased use in the wild (when widely adopted as “miner policy”).
Note: I am leaning towards the code suggestion here, specifically if (setConflicts.size() > 0) {, to make the condition more explicit about the expected value of setConflicts.size() (as was similarly done here in the same file). It’s a non-blocking nit, so feel free to ignore.
code review ACK 4abe6aab798fcb9d80228d4e49844b96690bfa2f
If we’re at the point where most of the network and miners support RBF, this makes our fee estimates more accurate. My concern correctness-wise was making sure that only the new transaction is considered in fee estimation (I don’t have much knowledge of CBlockPolicyEstimator). I’ve checked that when Finalize() calls RemoveStaged() which calls removeUnchecked() for each transaction, the mempool will call minerPolicyEstimator->removeTx() for the removed transactions.
re-ACK 5b29258076a3665100400768de8124be64eb4c6f 🌴
Verified via git range-diff 4abe6aab...5b292580 that changes since my last ACK are only rebase-related, plus the slight improvement in the if condition (avoiding implicit int-to-bool cast).
Rebased
sorry for making you rebase again, but this change is more straightforward
Sure
re-ACK fab8ce071566033316189283684340424cb2f7d0
Verified via git range-diff 5b292580...fab8ce07 that changes since my last ACK are only rebase-related.
ACK https://github.com/bitcoin/bitcoin/pull/22539/commits/fab8ce071566033316189283684340424cb2f7d0
Although I still have the same opinion: #22539 (comment)
Lets not restrict it for some transactions because of numbers.
re-ACK fab8ce071566033316189283684340424cb2f7d0
Clean rebase.
So i’ve been plotting the estimates at each block on both a node with this patch and one without. Here are the results for the past 3 days (still running)
Feerate is low at the moment so there is hardly any divergence. Data for generating this plot can be found here https://github.com/darosior/fee_estimation_hacks
This effectively reverts de1ae324bf3fb7451c1008a1a9721ff9f469533b.
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.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Let's not have run_test get into a giant function as we add more tests
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
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
In 4556406562:
A nice followup could be to use MiniWallet and get rid of feature_fee_estimation.py’s dependency on wallet in general? send_self_transfer allows specifying the utxo to spend and the feerate; you wouldn’t have to do these size calculations.
316+ txid = send_tx(node, u, low_feerate)
317+ utxos_to_respend.append(u)
318+ txids_to_replace.append(txid)
319+ # Broadcast 5 low fee transaction which don't need to
320+ for _ in range(5):
321+ send_tx(node, utxos.pop(0), low_feerate)
I’m a little confused about why there needs to be 10% of the transactions confirming with low feerate? Why can’t the test be:
(edit: ended up just writing it https://github.com/glozow/bitcoin/commit/10b89fc4bc8408a6b9341ff98fb910128f60ee6b)
I’m a little confused about why there needs to be 10% of the transactions confirming with low feerate?
There does not need to, i just wanted to simulate a more realistic scenario than just triggering the specific behaviour.
321+ send_tx(node, utxos.pop(0), low_feerate)
322+ # Mine the transactions on another node
323+ self.sync_mempools(wait=.1, nodes=[node, miner])
324+ for txid in txids_to_replace:
325+ miner.prioritisetransaction(txid=txid, fee_delta=-COIN)
326+ self.generate(miner, 1)
0@@ -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
Is this what we want?
Yes, because there is nothing we can do about that. And such a race sounds to be very improbable.. (are there still mining pools refusing RBFs for whatever reason?)
If yes, maybe document?
This is not a change in behaviour, i don’t think it should be in the changelog. Before this patch if you accept a replacement transaction in your mempool but the original tx ends up being mined you would lose the data anyways.
Thanks for adding a test! Sorry it’s taken me some time to review - also ended up with an additional question.
Other possible test extensions:
transaction is RBF’ed in our mempool, but the original transaction is included in a block
That’s a very good point. I think with this patch we would keep the replacement tx in the buckets until it expires..
I think with this patch we would keep the replacement tx in the buckets until it expires..
Actually, I think it would be removed here when removeForBlock() calls removeConflicts here since it’s conflicted out of the mempool. So that’s fine, we’d just lose a data point.
I ask because in some cases we keep the original transaction (e.g. we’ll put it in vExtraTxnForCompact for reconstructing compact blocks), so I wondered if that’s something we want here.
ACK 3b613722f6b895d7b268b3f878fddfc888381226
Thanks for patiently answering my questions! Verified that 053415b297b8665f2d2c4dce7c2c54bcc5298ef4 is move-only, and the added functional test looks correct to me. Will take my test improvement-related comments to #23075.
Actually, I think it would be removed here when removeForBlock() calls removeConflicts here since it’s conflicted out of the mempool. So that’s fine, we’d just lose a data point.
So at first i did not think it was fine. We in this case call removeTx with inBlock set to false, ie counting this removal as a failure to confirm. This could have led to wrecked estimations because we would count a higher fee tx as failed.
But actually, we’d only count it as a failure if it didn’t confirm for at least 12 blocks (for short stats, 24 and 42 for med and long). It’s plenty of time for the replacement to have propagated and makes this type of race so unlikely that i think it’s fine.
Thank you for raising this point!
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)]
To avoid manual tx size calculation with magic numbers, you could simply create the CTransaction first (without the fee), determine tx size via len(tx.serialize()) and then subtract the fee.
0 tx = CTransaction()
1 tx.vin = [CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), SCRIPT_SIG[utxo["vout"]])]
2 tx.vout = [CTxOut(int(utxo["amount"] * COIN), P2SH_1)]
3 fee = len(tx.serialize()) * feerate
4 tx.vout[0].nValue -= fee
327+ while True:
328+ try:
329+ u = utxos_to_respend.pop(0)
330+ send_tx(node, u, high_feerate)
331+ except IndexError:
332+ break
Might be a bit shorter and more pythonic without exception:
0 while utxos_to_respond:
1 u = utxos_to_respend.pop(0)
2 send_tx(node, u, high_feerate)
re-ACK 3b613722f6b895d7b268b3f878fddfc888381226 🍪
Verified that since my last ACK a rebase was done and a test was added which LGTM. Being extra-paranoid, I also checked that the test fails on the master branch on the estimated fee assertion at the end of sanity_check_rbf_estimates.
@darosior: I agree that the proposed fixups can be done in a follow-up, was not intending to block a merge (I kind of missed reading previous comments about fixups and PR #23075 that is already prepared for that purpose). BTW I also support the idea of using MiniWallet for creating txs, as suggested by gloria.
0@@ -0,0 +1,8 @@
1+Notable changes
2+===============
3+
4+P2P and network changes
Is this something that should be mentioned in the release notes at all? Given that less than 25% of txs are opt-in-rbf and way less than that are conflicts, this shouldn’t have any significant effect on real-world fee estimation. See also your findings: #22539 (comment)
Also, it doesn’t change the P2P protocol or is a network change. At most it would affect your local estimates, so if the notes are kept, the section headline could be adjusted.
See also your findings: #22539 (comment)
I expect this to be very different with a not so low fee market (replacement-aware fee estimation to constantly be slightly higher).
the section headline could be adjusted
Sure, what do you suggest? To be honest i was not sure what headline to use and since fee estimation is ubiquitous i figured it was a notable change, but happy to change it.
ks