Re-include RBF replacement txs in fee estimation

[darosior]

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.

[ghost]

Concept ACK

[darosior]

Asking the mempool statistics reference on IRC, @0xB10C pointed out to me there are between 2000 and 6000 replacements per day at the moment.

[maflcko]

Out of interest, did anyone collect stats on how many txs are replacements? Edit: see previous comment

[ghost]

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:

1. Fee estimation is a meme IMO or maybe helps in UX for newbies. You cannot predict if a fee rate will confirm the transaction in next N blocks. Context: https://bitcoin.stackexchange.com/questions/105860/what-are-we-trying-to-predict-in-fee-estimation-and-why/106129
2. If fee estimates are used, they should be available for all type of transactions and not based on % of txs per day. Rationale in PR Exclude RBF replacement txs from fee estimation #9519 was not good enough to change something. Not sure about the ACKs in that PR.
3. If any % still matters to document in PR, it should be RBF per day because if a user did transaction with RBF enabled, fee estimate may be required by replacement tx. Total transaction per day around 200k so 20% is 40k RBF transactions. We shouldn't wait for usage or other node implementations/wallets to do better before doing things that make sense.

[0xB10C]

Out of interest, did anyone collect stats on how many txs are replacements? Edit: see previous comment

[Rspigler]

Concept ACK, due to reasons stated in OP

[benthecarman]

utACK 4abe6aa

[darosior]

For what it's worth, I've been comparing running this on 2 pretty similar nodes for the past week. This didn't affect estimates, it must be because we are currently in a pretty low fee period.

[tryphe]

untested ACK 4abe6aa (edited hash, sorry clipboard was bugged)

Concept is simple and code changes are very straightforward (only one line is effectively changed).

[theStack]

Concept and code review ACK 4abe6aa

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23075 (refactoring: Fee estimation functional test cleanups by darosior)
• #22674 (validation: mempool validation and submission for packages of 1 child + parents by glozow)

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.

[josibake]

utACK 4abe6aa

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.

[Zero-1729]

crACK 4abe6aa

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.

[glozow]

code review ACK 4abe6aa

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.

[darosior]

Rebased on master after #22796 merge and addressed @theStack 's nit.

[theStack]

re-ACK 5b29258 🌴

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

[darosior]

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

[glozow]

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.

[glozow]

ACK 3b61372

Thanks for patiently answering my questions! Verified that 053415b is move-only, and the added functional test looks correct to me. Will take my test improvement-related comments to #23075.

[darosior]

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!

[darosior]

@tryphe @theStack @josibake do you mind re-[reviewing-and-]ACKing? I think we are getting close to merge and no other conflicting PR reported by Drahbot seem closer to merge.

[theStack]

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.

Suggested change

	overhead, op, scriptsig, nseq, value, spk = 10, 36, 5, 4, 8, 24
	tx_size = overhead + op + scriptsig + nseq + value + spk
	fee = tx_size * feerate
	tx = CTransaction()
	tx.vin = [CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), SCRIPT_SIG[utxo["vout"]])]
	tx.vout = [CTxOut(int(utxo["amount"] * COIN) - fee, P2SH_1)]
	tx = CTransaction()
	tx.vin = [CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), SCRIPT_SIG[utxo["vout"]])]
	tx.vout = [CTxOut(int(utxo["amount"] * COIN), P2SH_1)]
	fee = len(tx.serialize()) * feerate
	tx.vout[0].nValue -= fee

[darosior]

Oh right since we use legacy txs in this test it's indeed much cleaner. However i'd like to not invalidate 4 ACKs on a cosmetic change. Do you mind if i make it a follow-up in #23075 ?

[theStack]

Might be a bit shorter and more pythonic without exception:

Suggested change

	while True:
	try:
	u = utxos_to_respend.pop(0)
	send_tx(node, u, high_feerate)
	except IndexError:
	break
	while utxos_to_respond:
	u = utxos_to_respend.pop(0)
	send_tx(node, u, high_feerate)

[darosior]

Indeed 🤦‍♂️ but same rationale as above, as ugly as it is i'd rather address it in #23075

[theStack]

re-ACK 3b61372 🍪

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.

[maflcko]

One risk I could see is that someone running with a full-rbf patch might pay a higher fee now. So I am wondering if this needs to be re-introduced with a full-rbf patch?

[darosior]

This can only increase the fees for anyone, but if it does it means that it was necessary (a higher feerate was needed in order to have this transaction go through). How could a full-rbf patch wrongly increase estimates?

[maflcko]

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

[darosior]

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.

[maflcko]

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.

[darosior]

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.

[maflcko]

This will show up in the section "Policy" (example: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-22.0.md#policy), but I think we never had a "notable change" policy section. Maybe "Fee estimation changes"?

[darosior]

Done here #23284

[darosior]

For what it's worth here is again another diagram for last ~2 wee

ks