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

Allow all mempool txs to be replaced after a configurable timeout (default 6h) #10823

Open
wants to merge 1 commit into
base: master
from

Conversation

@greenaddress
Copy link
Contributor

commented Jul 14, 2017

This PR' aim is to improve user experience around stuck transactions without
affecting users of zero conf transactions.

tldr: Allow transaction replacement for transactions sitting in mempool for
longer than timeout (default 6h configurable) regardless of opt-in replacement
flag.

This PR affects policy/relay only.

Stuck transactions have been a problem for users recently. While wallets
are improving (opt in replacement, Child Pays For Parent, etc) there are some
cases which find users with transactions stuck for days that can't be solved
easily/reliably by wallet developers, especially when the user creates the
stuck transaction with old software or for some reason disabled available
features countering stuck transactions.

For the purpose of the below I will ignore transactions created by the core
wallet when talking about transaction expiration/eviction and focus on policy.

Bitcoin 0.12 introduced (or in a way re-introduced) opt-in transaction
replacement (BIP125), allowing people to more explicitly flag that their
transaction can be replaced (such that users of zero conf transactions can
immediately recognize them).

At the same time mempool limiting (configurable) was introduced, making the
individual mempool drop transactions at the bottom (low fee) when full.

Both before and after these changes any transaction in mempool would be
automatically evicted after 72 hours (configurable).

Recently 0.14.0 increased the eviction from 72 hours to 2 weeks. These changes
allows users of the system to aim for lower fees but at the same time makes it
frustrating for users that disable opt-in transaction replacement or that use
software that doesn't support it in first place to bump the fee at a later time
or to revert the payment as they have to wait for a while or use ad-hoc
software.

A number of miners will mine transactions regardless of opt-in flags (5-10%
maybe) and while core nodes won't propagate those transactions, a well
connected user can generally get replacement transactions mined within a
reasonable amount of time without opt-in transaction replacement flags set.

This may be convenient for attackers or ad-hoc expert use but
not ideal for wallet developers, or at least until core merges full transaction
replacement because using this functionality would requires wallets to use
preferential peering and/or forks of bitcoin core.

Until then a compromise solution that doesn't impact zero conf use and that
improves user experience would be to allow transactions to be replaced after
sitting for a timeout in mempool (thus unconfirmed).

The timeout should be high enough that allows current use of zero conf and at
the same time allows same working day solution for users.
I suggest a 6 hours timeout and to have it configurable for testing and ability
for user to change.

The changes continue to support disabling entirely transaction replacement
(-mempoolreplacement) and introduces a new command line parameter
(-mempoolreplacementtimeout) which allows to pass the number of seconds after
which a transaction can be unconditionally replaced and setting this parameter
to two weeks will keep the original behavior.

If you want to test the changes using @petertodd Replace-by-Fee tools build
core with this PR applied and wallet enabled and run with
-mempoolreplacementtimeout=10 and use doublespend.py (with and without
-b 1) from https://github.com/petertodd/replace-by-fee-tools

@greenaddress greenaddress force-pushed the greenaddress:replace-by-fee-old-transactions branch Jul 14, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

I think such policy changes should first be discussed on the bitcoin-dev mailing list and eventually deserve a BIP.

Show resolved Hide resolved src/validation.cpp Outdated
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

Yea, agreed that this should get a BIP (sadly probably means endlessly trolled), but does seem awesome to me. Does this need a new option? We dont currently have an option for opt-in-rbf, why not just leave this as hardcoded policy?

@greenaddress

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2017

@TheBlueMatt replacement can be disabled (as a node policy afaik rather than wallet) and the new option i added allows for easier testing and for people to run values that they like different from the default (either to be same as previous behavior or to get the other end without running some fork/patched core).

@TheBlueMatt @jonasschnelli I am happy to do a BIP and discuss in the dev mailing list as needed. I thought the changes may be borderline like the change from 72h to 2 weeks for mempool expiry but happy (minus potential trolling) to learn otherwise and or try to reduce the impact of the changes if this is an option.

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

I disagree that this needs a BIP.

Opt-in RBF added a new way to interpret a transaction, which just barely qualified as something you might want to do a BIP for.

This however makes an existing behavior - transactions being replaceable in spite of them not signalling opt-in RBF - happen a little sooner in some circumstances, just like adding expiration did in the first place. We didn't create a BIP for expiry, so why does this need a BIP?

Secondly, writing a BIP for such a trivial change gives the misleading impression that you could rely on the opposite behavior. We've had continual problems with people misunderstanding the security properties of zeroconf; there's no reason to add to that problem.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

Indeed, mere policies are not BIP material...

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2017

People might want to know about it... and a BIP might be a good way to communicate it... but we certainly didn't write a BIP about the expiration time changes over time, and this is strictly a narrower change.

Dunno how ideal 6 hours is though.

@EagleTM

This comment has been minimized.

Copy link

commented Jul 16, 2017

I'd suggest to put the timeout at 72h - at least when introducing the functionality. This way the behviour is similar to pre 0.14.x code. It's less likely to create havoc / confusion for operators who are still used to regularly see 0-conf transactions being re-spent after that time-frame (but not earlier).

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2017

@gmaxwell I suggested six hours because it's more than long enough (36 blocks) that if you wanted that tx mined relatively quickly, you're probably getting annoyed that it isn't.

@EagleTM Try actually doublespending some time - it's a lot easier than you think it is.

@rubensayshi

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

I think 6hrs is very short, I've seen many TXs confirmed after 24hrs+, there's plenty of people who, during bigger mempool periods, even try to aim for that...

though it might feel a bit insignificant for a BIP, this PR impacts a lot of assumptions people have about 0conf txs, it would get more significant exposure and discussion if proposed as a BIP.

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2017

@rubensayshi You seem to be arguing that the time interval should be longer for security reasons. For that argument to be valid, you'd have to substantiate the claim that a transaction with a fee so low that it fails to confirm after 36 blocks - 10 blocks more than the fee estimator even supports - is still difficult to double spend.

@greenaddress

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2017

@rubensayshi this doesn't stop those transactions from being confirmed if that's what the user wanted anyway it just allows people to replace transactions after a 6 hours long period - replacing them is already very easy to do ad-hoc even without the replacement flag but that full replacement it is currently painful to do for wallet developers without having to use different peering policies/full nodes with full replacement enabled.

I'd argue for full replacement at all times given the farce that it is doing replacement even without flags but if people want to at least try to keep the illusion of zero conf then I think 6 hours gives plenty of time for people that use zero conf to keep using it at the same ~ [in]security as today inclusive of illusion while giving users the opportunity to not have txs stuck for 2 weeks and solve stuck transactions problems within a business day.

@greenaddress

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2017

conflicts; should I rebase and squash while at it?

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2017

@greenaddress greenaddress force-pushed the greenaddress:replace-by-fee-old-transactions branch Aug 3, 2017

@luke-jr

luke-jr approved these changes Sep 2, 2017

Copy link
Member

left a comment

utACK, although it may be desirable to have some way to explicitly set the "opt-in only" policy.

Would also prefer -mempoolreplacement=fee,timeout=N syntax for the config

@greenaddress

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2017

@luke-jr not sure i understand your first point

I rebased as there was a conflict (new gArgs) and made sure feebumper.cpp handles the timeout too and added a test.

@greenaddress greenaddress force-pushed the greenaddress:replace-by-fee-old-transactions branch Sep 4, 2017

Show resolved Hide resolved src/validation.cpp Outdated
Show resolved Hide resolved src/wallet/feebumper.cpp Outdated
Show resolved Hide resolved test/functional/bumpfee.py Outdated
Show resolved Hide resolved src/validation.cpp Outdated
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 10, 2017

Needs rebase

@greenaddress greenaddress force-pushed the greenaddress:replace-by-fee-old-transactions branch 3 times, most recently Dec 9, 2017

@greenaddress

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2017

@MarcoFalke rebased

@promag moved to setmocktime, factored out the function, removed the white space and updated to new style - thanks!

More feedback welcome. I think it would be good to have more tests (ideas?) and I tried to be the least intrusive here but perhaps people have some refactoring suggestions.

edit: there's a bit more work needed as something broke with the rebase around list transactions/mempool sync

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2017

utACK 927666539bd61da5038b752ee60d106b9577ca42

Show resolved Hide resolved test/functional/bumpfee.py Outdated
Show resolved Hide resolved test/functional/bumpfee.py Outdated
Show resolved Hide resolved src/init.cpp Outdated
Show resolved Hide resolved test/functional/bumpfee.py Outdated
Show resolved Hide resolved src/validation.h Outdated
@promag

This comment has been minimized.

Copy link
Member

commented Dec 10, 2017

This test is failing

assert_array_result(self.nodes[0].listtransactions(), {"txid": txid_4}, {"bip125-replaceable":"unknown"})

@greenaddress greenaddress force-pushed the greenaddress:replace-by-fee-old-transactions branch Dec 12, 2017

@ziggamon

This comment has been minimized.

Copy link

commented Feb 22, 2019

Adding 2 cents:

The 6h limit seems arbitrary. For transactions that come in the hours after 9AM ET when Bitmex does their dump the network is vastly different than at other times.

Current state of things is that the mempool clears out overnight almost every night. Hasn't been for very long that it hasn't cleared out after two nights.

We accept 0-conf at Bitrefill (with certain checks of course), for several years, very successfully, and despite not advertising that we do, it the expectation of users is that 0-conf should work. In times when we've had to raise the minimum fee on 0-conf transactions we get support tickets from confused users. Quite a few (50%?) of users still pay from custodial services where they don't control the settings of the transactions being sent. We frequently have transactions lying around until they clear out overnight with no issue.

A limit of 72h would maybe be acceptable in this regard, but would need to be weighed against other considerations, from a utilitiarian perspective the question is weighing how many transactions this improves the experience for and how many it makes things worse.

We also need to remember that we currently have opt-in RBF, so wallets and services that choose to support replacing can simply just set that flag. I don't want to reignite the big RBF and 0-conf debate, but it seems like we have a reasonable compromise that there is clear opt-in RBF that is used by some, who then clearly declare that they don't want their transactions to be accepted unconfirmed, and other transactions where it is up to the recipient of the transactions to chose whether to accept those transactions.
So for this feature it would require wallets that can deal with replacement in a good way, yet don't have the foresight of setting the RBF flag. My gut is that the number of those transactions is much lower than the number of transactions impacted by the complexity introduced by us needing to deal with this.

@greenaddress

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

I'm in the process of rebasing and addressing nits/making sure the test pass.

@ziggamon I prefer 6h but if it there's consensus for 72h I am open to make the change.

@ziggamon

This comment has been minimized.

Copy link

commented Feb 22, 2019

This PR' aim is to improve user experience around stuck transactions without
affecting users of zero conf transactions.

Reminding about this stated goal :)

72h would def break things much less than 6h, but still makes me wonder how many considerations one would need to be keeping in mind for what the rules are.

I know this is just setting default behavior and it's all miner configurable, but my current understanding is that defaults often simply become the de facto consensus, such as with the mintxrelayfee.

Another question: How does this deal with child transactions made for CPFP? Perhaps it would be meaningful to not replace transactions that have been CPFP'd. But that too adds more complexity.

@greenaddress greenaddress force-pushed the greenaddress:replace-by-fee-old-transactions branch from 3910372 to a220fca Feb 22, 2019

@DrahtBot DrahtBot removed the Needs rebase label Feb 22, 2019

@abitfan

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

NACK for 6h,
this pr benefits mostly newcomers and user experience is the last on the priorities list.

@bitcoin bitcoin deleted a comment from DrahtBot Feb 23, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

Another argument in favor of 6 hours is that if even 3% of miners support full RBF then 6 hours is plenty of time for even a fairly lazy attacker to double spend. Although this pull request could impact that likeliness, because it's easier to reach those miners.

It may be that, as @ziggamon's points out, it's a bit contrived to build a feature that's essentially for wallets that support RBF, but leave it off by default. That scenario seems more likely than one where a user decides to upgrade to a better wallet while experiencing a stuck transaction.

Although it doesn't solve the same use case, I do like the idea of a 24-72 hour delay, because transactions that are so far down the queue tend to leave the mempool anyway. That can cause people to incorrectly believe their transaction is "cancelled". Or conversely their wallet will hold on to it forever, but never propagating, so the recipient can't even CPFP it. Making it easier to really cancel such a transaction, by double spending yourself back to your own wallet, could be useful even for wallets that don't support RBF.

@MarcoFalke MarcoFalke added this to the 0.19.0 milestone Mar 4, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Adding 0.19.0 milestone. If the 6h is too controversial even though it probably represents a sane default per @Sjors, it could be bumped higher to avoid the controversy.

@greenaddress greenaddress force-pushed the greenaddress:replace-by-fee-old-transactions branch from a220fca to 03fa5a1 Mar 8, 2019

@greenaddress

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

@MarcoFalke rebased and addressed some formatting improvement suggestions on the test

@DrahtBot DrahtBot removed the Needs rebase label Mar 8, 2019

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

Subsequently to my earlier comments I now think this is kinda pointless: Testing without RBF set gave me 100% confirmation or replacement rate for very low fee transactions within 20 blocks without the low fee txn rising to being minable by feerate presumably due to restarting nodes, and miners that replace anyways. Is this just motivated by either speculation on actual behaviour without measuring it or a distributed systems misunderstanding that causes people to not just set replacement in the first place?

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Gotta agree with @gmaxwell here.

Also, the myth that zeroconf transactions have any security recently lead to $195,000 CAD of publicly reported losses: https://globalnews.ca/news/5047918/calgary-police-nationwide-bitcoin-fraud/

Full-RBF-by-default will help mitigate this misunderstanding. I also have personal interest in stopping this misunderstanding as other reporting on the subject has inaccurately implicated me as part of this crime.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I disagree.

It is highly unlikely that regular users will use bumpfee (or other use cases of tx replacements) merely because observations suggest that miners are accepting full-rbf. This pull request would make it also mempool policy, which is useful when a wallet is not directly connected to miners, but maybe needs to do a few hops through network nodes.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Needs rebase
@Sjors

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

I did an n=1 experiment today where, trying to replace a single-input non-RBF transaction that pays 15 sat / vbyte with one that pays 50 sat / vbyte.

The new transaction doesn't show up on any explorer, suggesting none of my peers is relaying it despite the huge fee bump. It's not like there's a list of known full-RBF peers to manually connect to either.

This is potentially different from the very low fee scenario Maxwell described, because my initial transaction was very well propagated.

The procedure is slightly easier than it used to be, because there's no need to zap the wallet. Just unload the wallet and delete mempool.dat before calling sendrawtransaction.

That said, I don't think my example is a good use case. The reason the initial transaction didn't use RBF is because it was created with RPC and I forgot to set walletrbf=1. I also just noticed walletcreatefundedpsbt default replaceable argument ignores this setting (#15878).

@jnewbery
Copy link
Member

left a comment

This PR introduces two changes:

  1. a policy change to allow mempool transactions to be replaced after a configurable time
  2. allow wallets to create replacement transactions after a configurable time

I think those changes should be separated, at least into two separate commits.

My preference for (2) would be for the behavior to be controlled by a flag in the bumpfee RPC to force replacement rather than a flag enablewalletreplacementtimeout which in your current implementation is shared between node/wallet.

I'm fine having the default set to 72 hours to address the concerns of @ziggamon and others.

I think it would be good to have more tests (ideas?)

The current test is for rejection/acceptance by the wallet. Could you add a test that tries to submit a replacement tx over P2P which is rejected, and then setmocktime in the future and have the tx accepted over P2P?

return now - accepted >= timeout;
}

RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool, const int64_t now, const int64_t timeout, const bool enabled_replacement_timeout)

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 8, 2019

Member

I'm not a fan of changing the semantics of this function. Previously it meant "does this transaction (or one of its unconfirmed parents) signal opt-in RBF?". Now it means "does my mempool configuration allow this tx to be replaced?"

That means that the bip125-replaceable field in getmempoolentry would now show true for transactions that aren't replaceable under bip125 rules for example.

}
const int64_t conflicting_time = pool.info(ptxConflicting->GetHash()).nTime;
const bool conflicting_pretimeout = !ExpiredOptInRBFPolicy(nAcceptTime, conflicting_time, replacement_timeout);
fReplacementOptOut = conflicting_pretimeout && !SignalsOptInRBF(*ptxConflicting);

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 8, 2019

Member

Can remove the local fReplacementOptOut variable and test directly on the condition here:

if (!ExpiredOptInRBFPolicy(nAcceptTime, conflicting_time, replacement_timeout) &&
    !SignalsOptInRBF(*ptxConflicting)) {
    return state.Invalid(false, REJECT_DUPLICATE, "txn-mempool-conflict");
}

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 8, 2019

Member

oops, ignore this. I missed that fReplacementOptOut was initialized to true.

static const int64_t DEFAULT_REPLACEMENT_TIMEOUT = 6 * 60 * 60;
/** Default for buffer in seconds on top of the timeout after which transactions in mempool are replaceable (i.e. 6 hours) in the GUI
* This buffer is needed because otherwise is more likely ours peers will reject the transaction in case they received it substantial time after us */
static const int64_t REPLACEMENT_TIMEOUT_BUFFER = 5 * 60;

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 8, 2019

Member

Move this constant to feebumper.cpp since it's only used there.

@@ -516,6 +516,8 @@ void SetupServerArgs()
gArgs.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (default: %u)", MAX_OP_RETURN_RELAY), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-mempoolreplacement", strprintf("Enable transaction replacement in the memory pool (default: %u)", DEFAULT_ENABLE_REPLACEMENT), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-mempoolreplacementtimeout=<n>", strprintf("Number of seconds after which transactions in mempool can be replaced (default: %u)", DEFAULT_REPLACEMENT_TIMEOUT), false, OptionsCategory::NODE_RELAY);
gArgs.AddArg("-enablewalletreplacementtimeout", strprintf("Whether to enable wallet replacement timeout (default: %u)", DEFAULT_WALLET_REPLACEMENT_TIMEOUT), true, OptionsCategory::OPTIONS);

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 8, 2019

Member

I think we can remove this option entirely and replace it with an argument to bumpfee

@ziggamon

This comment has been minimized.

Copy link

commented May 8, 2019

Before going through the code, Is there any consensus on whether this is a good idea at all? Reminding of the debate around opt-in RBF. The opponents' argument was that it would lead to a slippery slope and removing 0-conf entirely. This seems like a step in this direction. Like it or not it is successfully used to a large extent, not just by our users.

Let's evaluate some situations:
a) In status quo (i.e. mempool clears out almost every night) there is no need for this.

b) In a state where the mempool is non-empty for long periods of time this creates a situation that potentially worsens a fee crisis by making it so that merchants must CPFP transactions to fit within the 72h (or whichever window), which potentially can lead to trampling for the exits at the exact wrong time, causing fees to spike further. Large amounts of automated code outbidding eachother for limited space is part of the reason for why we had the fee problem in 2017 in the first place.

CPFP here also becomes a vector for abuse per the following: Anyone can make a large consolidation transaction that includes a send to a party that does CPFP on incoming tx's within 72h => Externalize the costs of confirming this consolidation to someone else, which in this scenario is expensive.

  • Question here: Would a child transaction prevent this from running or would it run regardless? Or is there some other mechanism by which a recipient could disable this functionality?

Can somebody describe a specific scenario where a user would benefit from this change in network policy? Is this a user that uses an RBF wallet, but chooses to have RBF disabled but then changes his mind? Or is it a user that upgrades wallets in order to access this feature post-fact? Can we estimate the potential benefit and weigh against the potential damage?

I agree with John that we should separate policy aspects here from wallet aspects. Bitcoind policy for propagation is not consensus but largely goes unchanged so usually becomes a sort of de facto consensus.

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Is there any consensus on whether this is a good idea at all?

No, I don't think consensus has been reached. The fact that you have legitimate concerns about it shows that we don't have consensus!

In status quo (i.e. mempool clears out almost every night) there is no need for this.

Correct - if the mempool clears out almost every night then there is no need for this.

Would a child transaction prevent this from running or would it run regardless? Or is there some other mechanism by which a recipient could disable this functionality?

With the code in this PR, no. Descendants are ignored when considering whether a transaction is eligible for replacement.

Can somebody describe a specific scenario where a user would benefit from this change in network policy? Is this a user that uses an RBF wallet, but chooses to have RBF disabled but then changes his mind?

Currently, there are wallets that can create replacement transactions but don't signal RBF by default (I believe this is the case with Bitcoin Core and Electrum wallet). If a user of one of those wallets sends a transaction using the defaults, finds that it's stuck and later wishes to bump the fee, they are unable to. If the defaults for policy were changed according to this PR, that user would be able to bump their transaction after 6 hours (or 72 hours if we changed the default time).

My main argument for this change is that it makes expectations clearer. As @greenaddress, @petertodd and @gmaxwell have pointed out, if a transaction is unconfirmed after 6 hours, a malicious actor could easily have double-spent it. Any recipient of a transaction that hasn't been confirmed after that much time should therefore not have any confidence that the tx will ever confirm.

@SomberNight

This comment has been minimized.

Copy link

commented May 8, 2019

Can somebody describe a specific scenario where a user would benefit from this change in network policy?

Near the end of 2017, "how much time do I need to wait until my transaction gets cancelled" was a frequent question on IRC. The usual scenario was that the user had used either old software, or just a shitty wallet, that did not set RBF, and had set a low fee. This tx would then never get mined. In some unlucky cases, there was not even a change output, so CPFP could not be used.

Not sure how much wallets improved since then. I guess we will only really know when the mempool gets consistently non-empty again.

This PR, being able to RBF transactions without opt-in after some time, would be useful so that at least users could be given a precise answer to the original question: "after x hours, the tx can be changed". Then again, I am skeptical about light wallets implementing this... they would need to keep track of the time they broadcast the tx, assume default node behaviour, and only after timeout, offer an option to bump the fee...? Well I guess the option could always be there, along with some disclaimer (I think this is what we might do in Electrum).

I am in favour of this, hugely because I consider it valuable to have a precise answer (even if it only reflects default behaviour).
I guess @jnewbery meant something else (0-conf being unsecure) when saying "My main argument for this change is that it makes expectations clearer"; still, I feel the same.

Currently, there are wallets that can create replacement transactions but don't signal RBF by default (I believe this is the case with Bitcoin Core and Electrum wallet).

In Electrum, on desktop, all transactions are RBF by default since 3.1 (March 2018);
and low fee transactions have RBF enabled by default since 2.8 (March 2017),
where low fee was defined as feerate < (estimatefee(10)+estimatefee(25))/2
The Android app still prompts the user on every tx whether to make it RBF or not. This is mainly because there are still apps out there that do not display incoming RBF transactions at all until they confirm, which is a horrible experience at a PoS.

@ziggamon

This comment has been minimized.

Copy link

commented May 9, 2019

@jnewbery

Currently, there are wallets that can create replacement transactions but don't signal RBF by default (I believe this is the case with Bitcoin Core and Electrum wallet). If a user of one of those wallets sends a transaction using the defaults, finds that it's stuck and later wishes to bump the fee, they are unable to.

Right. So we could estimate the usage of these two wallets (Electrum and Bitcoin Core GUI) to what, 10% of payments in a day link, generously? And this error case is a single digit percentage of that, and something that the wallet could prevent. On the other side of the scale we have the 50%+ of payments that are being sent from custodial services or exchanges link. Or the 60% of payments that use something that doesn't even do Segwit yet link. These users will not helped by this, in fact the bitcoin network for them becomes strictly worse. I will elaborate further down.

My main argument for this change is that it makes expectations clearer.

So, Signalpolitik. If that'st he goal then that should be discussed not in a github ticket or IRC but in more broad social angles.

if a transaction is unconfirmed after 6 hours, a malicious actor could easily have double-spent it. Any recipient of a transaction that hasn't been confirmed after that much time should therefore not have any confidence that the tx will ever confirm.

If this were really the case then we wouldn't need this PR, would we? But regardless, this is an assumption that is easily testable. Let's do some tests before we roll out such a big policy change? To my knowledge most tests that are referenced happened in 2012, would be good to have some new data. Happy to let you use Bitrefill to test against, you can buy Lightning channels and just not redeem them and have my word that there will be no trouble. Our findings is that it's reasonably secure for smaller transactions and empirically there hasn't been any systematic abuse of this feature (we've had plenty of attempts to abuse other things)

@SomberNight

which is a horrible experience at a PoS

Earnestly glad that you also value the benefit of still having PoS payments with onchain bitcoin. Not sure what thoughts on this are among the maintainers of this repo. While I too think that all such payments will migrate to Lightning with some time, there is no need to break current onchain policy, Lightning is attractive enough to get people to upgrade without the stick.

By PoS here I mean the classic "scan qr code to send X BTC to Y address" mode that i think we all recognize.
Screenshot 2019-05-09 at 15 09 29

RBF breaks this model for any purchase that is denominated in USD as it allows the user to game the system by waiting indefinitely to see if the Bitcoin prices move. If Bitcoin price goes up while transaction is still unconfirmed - simply cancel it and make a new one for cheaper. If the merchant decides to change the offered price while the payment is in the mempool that opens up for a world of hurt that none of us want.

With RBF this exposure grows beyond the 15 minutes or so that the merchant decides, but it expands to the entire period until the transaction confirms, which in this case can be a very long time (hence the reason for this ticket). This isn't a huge deal when individual wallets have this behavior because you can inform the user about it and have them either disable that feature for next time, or even change wallets to one that will have a better experience. Currently RBF usage is around 7% of transactions link, probably lower of batched payments, so not a huge deal. But if we break the behavior on a policy level it means that services that have a PoS display like the above will need to move to a custodial model (deposit and confirm first, buy stuff later, keep some change on the account). One might make an argument that this mode is better and this is how bitcoin should be working and so on, but that's a bigger discussion than reviewing code in a PR. My experience with bitcoin core is that there is a desire to maintain backwards compatibility as much as possible and not introduce contentious changes.

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 9, 2019

So, Signalpolitik. If that'st he goal then that should be discussed not in a github ticket or IRC but in more broad social angles.

That's not really what I meant. After 6/72/whatever hours, if my node receives a transaction that replaces a tx in my mempool, but with a larger fee, then I can be reasonably sure that the creator of that transaction could get it confirmed ahead of the original transaction. My mempool pretending that's not the case and rejecting the replacement is not helpful to me.

I agree though, this does deserve wider discussion and a github issue isn't really the place for it (and I thank you for taking part in the discussion here!)

Let's do some tests before we roll out such a big policy change? Happy to let you use bitrefill to test against, can buy Lightning channels and just not redeem them.

That sounds fun! It'll be more difficult to double-spend you while blocks aren't consistently full. As you've observed above, this change makes sense in a world where blocks are consistently full. The reason to make this change now is that node policy should be changed in advance of expected changes in the network (see #15846 for example).

Thanks again for your robust input in this PR. It really is very useful to hear from merchants and high-frequency users.

@Sjors

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Bitcoin Core GUI has been using RBF by default for a while. Bitcoin Core RPC doesn't, but I think we should assume RPC users know what they're doing (modulo bugs like #15878). I don't think this change is useful for Bitcoin Core wallet users.

The wallet change does make sense conditional on the node policy change, but because the node only knows it's own policy, it could be source of confusion (especially for non-default settings).

@@ -631,17 +632,13 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
// first-seen mempool behavior should be checking all
// unconfirmed ancestors anyway; doing otherwise is hopelessly
// insecure.
// All transactions in mempool become replaceable after the timeout.
bool fReplacementOptOut = true;
if (fEnableReplacement)
{

This comment has been minimized.

Copy link
@PastaPastaPasta

PastaPastaPasta Jun 6, 2019

could you fix these brackets while here? ie same line?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jun 6, 2019

Member

@PastaPastaPasta - PRs shouldn't 'fix' style for code they're not otherwise touching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.