Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude RBF replacement txs from fee estimation #9519

Merged
merged 1 commit into from Jan 26, 2017

Conversation

@morcos
Copy link
Member

commented Jan 11, 2017

This has little effect now as less than 0.1% of transactions are replacement transactions, but until we're confident that opt-in-RBF is widely accepted as miner policy, it's probably better not to consider replacement transactions for fee estimation.

@@ -788,13 +788,15 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
size_t nConflictingSize = 0;
uint64_t nConflictingCount = 0;
CTxMemPool::setEntries allConflicting;
bool fReplacementTransaction = false;

This comment has been minimized.

Copy link
@JeremyRubin

JeremyRubin Jan 11, 2017

Contributor

nit: you may as well make this const bool fReplacementTransaction = setConflicts.size() under the lock & make the if below check fReplacementTransaction.

// the node is not behind, it is not dependent on any other
// transactions in the mempool, and it isn't a BIP 125
// replacement transaction (may not be widely supported).
bool validForFeeEstimation = IsCurrentForFeeEstimation() && pool.HasNoInputsOf(tx) && !fReplacementTransaction;

This comment has been minimized.

Copy link
@JeremyRubin

JeremyRubin Jan 11, 2017

Contributor

nit: !fReplacementTransaction is a lot cheaper of a check than the others, so may as well make it the first one in the group of &s. I don't think the optimizer would do that automatically?

This comment has been minimized.

Copy link
@jtimon

jtimon Jan 19, 2017

Member

No, the optimizer shouldn't do that automatically, because it could be done on purpose (ie to guarantee IsCurrentForFeeEstimation() is always called, even if fReplacementTransaction, assuming IsCurrentForFeeEstimation() changes some state). So, yeah, +1 on the nit.

@JeremyRubin
Copy link
Contributor

left a comment

My conceptual worry is this makes possible some kind of attack where all clients are now incentivized to do RBF for transactions they want to go through quickly because it will not affect network fee estimates. E.g.: I want to pay 2x normal fee for a faster transaction. First, I broadcast at 0.5 normal fee and wait a few seconds for it to propagate. Next, I broadcast at the 2x normal fee (which I was always willing to pay), but now this code does not include my fee in the estimate.

One way to address this vector would be to not mark it as invalid for fee estimate if the conflicting transaction comes in within a certain time window of the original. This way, the user derives minimal benefit from the above.

If the above is not actually an issue, this is a utACK from me (+ 2 minor code style nits).

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

Too much special-casing here... Estimates shouldn't break when 100% of the network adopts RBF support. But even more importantly, estimates shouldn't break when miners use unknown policies. There needs to be some tolerance of transactions being left unmined for reasons beyond the estimator's awareness.

@morcos

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2017

@JeremyRubin

  • re: nits, seems kind of unnecessary optimization if you ask me..
  • re: concern, I won't go so far as to say that's not a concern at all, but the debug information for fee estimates already lets you know how many of the transactions in blocks you're tracking. So if that number drops significantly it'll be worth revisiting. Beyond that, it shouldn't matter if a bunch of high fee txs aren't tracked as long as enough still are to give you the data you need.

@luke-jr sigh... sure, estimates should be magical and give you perfect answers. But whether this is the optimal algorithm is not the question here. The question is whether this is a reasonable improvement and I believe it is.

@fanquake fanquake added the Validation label Jan 11, 2017

@laanwj laanwj added this to the 0.14.0 milestone Jan 12, 2017

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

utACK

@dcousens

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2017

it's probably better not to

Why not consider them?

@morcos

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

@dcousens The issue is that if some significant fraction of miners don't accept RBF replacements but your node does, then you would expect them to get mined quickly based on their fee rate but in reality it will take longer. In the worst case situation this can lead fee estimation to think that all transactions of that fee rate would take longer to be confirmed.

If you know that some miners are not considering a certain subset of transactions for block inclusion, its better to eliminate those as potential data points for fee estimation. There are two opposing arguments to this:

  1. You've eliminated so many transactions that you don't have a big enough sample of data points. (Not a concern here)
  2. What if the particular transaction you are placing is of the type that is chosen to be excluded for fee estimation because its disadvantaged. This is a reason to not overly narrow the policy profile required for fee estimation. But in the case of replacement transactions, I think its relatively well known that not all miners might accept your replacement, and in the case they don't, the alternative is your original transaction is still mined, so its probably ok if fee estimates are maybe not perfectly tailored to your particular transaction.
@JeremyRubin

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2017

I am convinced now that the issue I brought up is unlikely to be workable, full utACK!

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

Concept ACK

@dcousens

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2017

Thanks for the explanation @morcos.

I still think given the fact this has near zero effect (0.1%), and any increased usage of RBF would also incite miner acceptance... I don't think this is necessary, but, if it was an issue impacting estimates substantially (and negatively), I would agree this is a good temporary fix.

Weak NACK

edit: I also think, if #9527 is to be enabled... these numbers could change dramatically. If the belief is that miners aren't accepting RBF... then yes, concept ACK.

@dcousens

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2017

Do we have any data/estimates on widespread RBF policy usage [by miners]?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 14, 2017

utACK 20418bd

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

I have the same worry as @luke-jr here, the special-casing seems surprising and unexpected and will distort estimates once RBF is more widely adopted.

Once they are in the block chain, RBF transactions are the same as non-RBF transactions.
Also the fee from a RBF transaction is worth just as much as from a non-RBF transaction.
So I'm not sure why a miner would adopt a anti-RBF policy in the first place. But if they do, I don't think we should special-case our code in it.

Especially if we want to generate RBF transactions ourselves later it looks like something that will come back to bite us.

Do we have any data/estimates on widespread RBF policy usage [by miners]?

I'd like to see this too.

@morcos

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

To address the question about distorting estimates, I don't really believe that is a concern.

Fundamentally if RBF replacements have the same confirmation profile as regular transactions, then none of this matters as long as we have enough total data points, which I think we are nowhere near worrying about even if all txs are default replaceable (only actual replacements are excluded).

If they have a different confirmation profile than regular transactions, then you have to look at the two sets of data separately or you can only give an answer for the subset of data that you look at. In our case there certainly isn't enough data now to give an answer for replacements on their own, so we might as well exclude replacements from the regular transaction data so we are giving an unpolluted answer for regular transactions at least.

In other words its no harm done if they aren't really subject to different policy. If they are, then this is playing it safe to the best degree we are able.

@btcdrak

This comment has been minimized.

Copy link
Member

commented Jan 18, 2017

Current usage stats of RBF transaction produced is about 0.2%-0.3%

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2017

I dont buy the "miners will be more incentivized to run RBF" argument. While this is true, miners (in this case pools) have shown both a) long lags from incentivization to when they start adopting things which pay them more (eg many miners are still leaving lots on the table by not using 0.13 to do CPFP), and b) willingness to ignore incentivization when there is community drama - its not their income that they're losing out on, its their clients.

On the flip side, I do buy that opt-in RBF will continue to get some additional adoption because of the big UX win for some wallets by using it, and while its useable today because there is some adoption from miners, the inconsistency of said adoption makes including it for fee esimation very painful.

Finally, the way this is written, it is trivial to revert even in a point release if we decide things have upgraded faster than we thought.

Strong Concept ACK.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2017

utACK 20418bd

@btcdrak

This comment has been minimized.

Copy link
Member

commented Jan 19, 2017

utACK 20418bd

@jtimon

This comment has been minimized.

Copy link
Member

commented Jan 19, 2017

utACK 20418bd reverting this when we feel it makes sense should be equally simple anyway.
I don't see the reason not to fix @JeremyRubin 's nit. It's trivial to fix, and if anything, it will prevent someone else from bringing up the same micro-optimization again, or just think about it when reading the code.

@morcos

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2017

OK added @JeremyRubin's optimization suggestions

@morcos morcos force-pushed the morcos:excludeRBF branch from bd1f849 to de1ae32 Jan 20, 2017

@morcos

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2017

Squashed bd1f849 -> de1ae32

@jtimon

This comment has been minimized.

Copy link
Member

commented Jan 20, 2017

re utACK de1ae32

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2017

If you want a source of opt-in RBF confirmation data, my OpenTimestamps calendars use opt-in RBF for all transactions; the fee strategy there is to start with the lowest possible fee, and then keep incrementing the fees with replacements over time. So any OpenTimestamps transaction that is mined is pretty much guaranteed to have been due to a replacement on default Bitcoin Core mempool settings.

Here's a sample tx: 6c13d8bcd5d4e397300302e635e3e13a1b7af929fb00ce6c7b71645e95b7b548

All prior OP_RETURN txs in that chain will have been from OpenTimestamps. I took a quick look myself, and it looks like those transactions are getting mined by a large percentage of hashing power, including large pools like F2Pool, AntPool, BitFury, etc.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2017

@petertodd I'm not sure that is required? Because blocks are not always full, if you get a transaction propagated its virtually guaranteed to get mined eventually still. What is your exact policy - is it possible that you're simply seeing the first relayed transaction get confirmed (keeping in mind that relay of things with low fee can be super unreliable)?

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Jan 23, 2017

re-utACK

@dcousens

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2017

Could we just compare the median "time to confirm" for RBF v non-RBF to compare probability of miner acceptance?

@btcdrak

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

re-utACK de1ae32

@jtimon
jtimon approved these changes Jan 24, 2017
@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2017

ACK. The only downside of this is that if replacements become common, we may not end up with enough data. But if replacements were common, we could assume miners were mining them, and it would be harmless to remove this. We know that right now that they do have a different confirmation profile, so I think this is the correct change to make.

@dcousens

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2017

We know that right now that they do have a different confirmation profile, so I think this is the correct change to make.

How do we know this though? I've been recording data for the last week in regards to this so I can verify myself, but I haven't seen any data presented in this thread.

edit:

We know they constitute ~0.2%: http://p2sh.info/dashboard/db/replace-by-fee
But their confirmation profile?

@laanwj laanwj merged commit de1ae32 into bitcoin:master Jan 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jan 26, 2017
Merge #9519: Exclude RBF replacement txs from fee estimation
de1ae32 Exclude RBF txs from fee estimation (Alex Morcos)
@dcousens

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

I collected 5181335 transactions between blocks 448207 and 451298, observing the txId and the height of the blockchain when first seen, later taking confirmed_height - seen_height to determine how many blocks each transaction waited until confirmation.

In that time, 14101 transactions were seen signalling RBF (~0.27%), with a median wait of 1 block until confirmation and 78 bytes satoshis/byte.
This matches the confirmation profile of non-RBF transactions, which had a median fee of 80 satoshis/byte.

The mean wait time for standard transactions [seen in this time period] was 6.87137 blocks, with RBF transactions on average confirming within 5.38064 blocks.

Data available if needed, happy to query in any way necessary.

edit: after discussion with @gmaxwell, it appears the suggested problematic confirmation profile isn't in regards to RBF signalling transactions, but their actual replacements. The above analysis isn't presently accounting for that, so I'll have to look into it further 👍 .

codablock added a commit to codablock/dash that referenced this pull request Jan 21, 2018
Merge bitcoin#9519: Exclude RBF replacement txs from fee estimation
de1ae32 Exclude RBF txs from fee estimation (Alex Morcos)
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
Merge bitcoin#9519: Exclude RBF replacement txs from fee estimation
de1ae32 Exclude RBF txs from fee estimation (Alex Morcos)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
Merge bitcoin#9519: Exclude RBF replacement txs from fee estimation
de1ae32 Exclude RBF txs from fee estimation (Alex Morcos)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.