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

greenaddress commented at 1:46 pm on July 14, 2017: contributor

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 force-pushed on Jul 14, 2017

jonasschnelli commented at 6:31 pm on July 14, 2017: contributor

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

jonasschnelli added the label TX fees and policy on Jul 14, 2017

in src/validation.cpp:512 in bea046166a outdated

509+                    const int64_t nConflictingTime = pool.info(ptxConflicting->GetHash()).nTime;
510+                    fReplacementOptOut = nAcceptTime - nConflictingTime < nReplacementTimeout;
511+                    if (fReplacementOptOut)
512                     {
513-                        if (_txin.nSequence < std::numeric_limits<unsigned int>::max()-1)
514+                        for (const CTxIn &_txin : ptxConflicting->vin)

jonasschnelli commented at 7:06 pm on July 14, 2017:

Maybe use SignalsOptInRBF()

greenaddress commented at 7:50 pm on July 14, 2017:

I only moved the code but indeed that’s a good idea (addressed in 38f4c85b47351d40700e088122317aa385c5ee65) - thanks!

TheBlueMatt commented at 7:53 pm on July 14, 2017: member

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 commented at 8:05 pm on July 14, 2017: contributor

@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 commented at 11:06 pm on July 14, 2017: contributor

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 commented at 0:31 am on July 15, 2017: member

Indeed, mere policies are not BIP material…

gmaxwell commented at 1:27 am on July 15, 2017: contributor

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 commented at 4:18 am on July 16, 2017: none

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 commented at 5:20 am on July 16, 2017: contributor

@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 commented at 9:05 pm on July 31, 2017: contributor

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 commented at 4:26 am on August 1, 2017: contributor

@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 commented at 10:05 pm on August 1, 2017: contributor

@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 commented at 10:06 pm on August 1, 2017: contributor

conflicts; should I rebase and squash while at it?

petertodd commented at 3:35 pm on August 3, 2017: contributor

@greenaddress +1

greenaddress force-pushed on Aug 3, 2017

luke-jr approved

luke-jr commented at 6:13 am on September 2, 2017: member

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 commented at 11:31 pm on September 4, 2017: contributor

@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 force-pushed on Sep 4, 2017

in src/validation.cpp:507 in 2cf7f43134 outdated

503@@ -503,17 +504,14 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
504                 // first-seen mempool behavior should be checking all
505                 // unconfirmed ancestors anyway; doing otherwise is hopelessly
506                 // insecure.
507+                //

promag commented at 0:07 am on September 5, 2017:

Remove empty comment (not found in master).

in src/wallet/feebumper.cpp:92 in 2cf7f43134 outdated

88@@ -89,7 +89,8 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin
89         return;
90     }
91 
92-    if (!SignalsOptInRBF(wtx)) {
93+    const int64_t nReplacementTimeout = gArgs.GetArg("-mempoolreplacementtimeout", DEFAULT_REPLACEMENT_TIMEOUT);

promag commented at 0:22 am on September 5, 2017:

Factor out duplicate implementation to a new function?

in test/functional/bumpfee.py:146 in 2cf7f43134 outdated

137@@ -136,6 +138,19 @@ def test_nonrbf_bumpfee_fails(peer_node, dest_address):
138     assert_raises_jsonrpc(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
139 
140 
141+def test_nonrbf_bumpfee_fails_then_succeeds(test, peer_node, dest_address):
142+    test.stop_nodes()
143+    test.start_node(0, extra_args=["-prematurewitness", "-walletprematurewitness", "-mempoolreplacementtimeout=1"])
144+    not_rbfid = peer_node.sendtoaddress(dest_address, Decimal("0.00090000"))
145+    assert_raises_jsonrpc(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
146+    time.sleep(1)

promag commented at 0:25 am on September 5, 2017:

Use setmocktime instead?

in src/validation.cpp:484 in 2cf7f43134 outdated

480@@ -481,6 +481,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
481 
482     // Check for conflicts with in-memory transactions
483     std::set<uint256> setConflicts;
484+    const int64_t nReplacementTimeout = gArgs.GetArg("-mempoolreplacementtimeout", DEFAULT_REPLACEMENT_TIMEOUT);

promag commented at 0:25 am on September 5, 2017:

Update to new style.

greenaddress force-pushed on Dec 9, 2017

greenaddress commented at 12:13 pm on December 9, 2017: contributor

@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 commented at 7:58 pm on December 9, 2017: contributor

utACK 927666539bd61da5038b752ee60d106b9577ca42

in test/functional/bumpfee.py:143 in 927666539b outdated

137@@ -136,6 +138,21 @@ def test_nonrbf_bumpfee_fails(peer_node, dest_address):
138     assert_raises_rpc_error(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
139 
140 
141+def test_nonrbf_bumpfee_fails_then_succeeds(test, dest_address):
142+    test.stop_nodes()
143+    test.start_node(0, extra_args=["-prematurewitness", "-walletprematurewitness", "-mempoolreplacementtimeout=30"])

promag commented at 1:13 am on December 10, 2017:

Why stop node[1]?

If not, removestop_nodes above and here can be:

0test.restart_node(0, ["-prematurewitness", "-walletprematurewitness", "-mempoolreplacementtimeout=30"])

Use restart_node below.

in test/functional/bumpfee.py:149 in 927666539b outdated

144+    not_rbfid = test.nodes[0].sendtoaddress(dest_address, Decimal("0.00090000"))
145+    assert_raises_rpc_error(-4, "not BIP 125 replaceable", test.nodes[0].bumpfee, not_rbfid)
146+    test.nodes[0].setmocktime(int(time.time()) + 30)
147+    bumped_tx = test.nodes[0].bumpfee(not_rbfid)
148+    assert bumped_tx["txid"] in test.nodes[0].getrawmempool()
149+    assert not_rbfid not in test.nodes[0].getrawmempool()

promag commented at 1:22 am on December 10, 2017:

Avoid 2nd call to getrawmempool.

in src/init.cpp:478 in 927666539b outdated

474@@ -475,6 +475,7 @@ std::string HelpMessage(HelpMessageMode mode)
475     strUsage += HelpMessageOpt("-datacarrier", strprintf(_("Relay and mine data carrier transactions (default: %u)"), DEFAULT_ACCEPT_DATACARRIER));
476     strUsage += HelpMessageOpt("-datacarriersize", strprintf(_("Maximum size of data in data carrier transactions we relay and mine (default: %u)"), MAX_OP_RETURN_RELAY));
477     strUsage += HelpMessageOpt("-mempoolreplacement", strprintf(_("Enable transaction replacement in the memory pool (default: %u)"), DEFAULT_ENABLE_REPLACEMENT));
478+    strUsage += HelpMessageOpt("-mempoolreplacementtimeout=<n>", strprintf(_("Number of seconds after which transactions in mempool can be replaced (default: %u)"), DEFAULT_REPLACEMENT_TIMEOUT));

promag commented at 1:26 am on December 10, 2017:

IMO there should be better parameter validation. Validate (must be number and positive integer) and add tests? See https://github.com/bitcoin/bitcoin/blob/59d3dc85b698430f71f6e242a01a25a70c9ef397/test/functional/feature_logging.py#L33-L34

in test/functional/bumpfee.py:141 in 927666539b outdated

137@@ -136,6 +138,21 @@ def test_nonrbf_bumpfee_fails(peer_node, dest_address):
138     assert_raises_rpc_error(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
139 
140 
141+def test_nonrbf_bumpfee_fails_then_succeeds(test, dest_address):

promag commented at 1:32 am on December 10, 2017:

Looks like this should be BumpFeeTest member (test = self)?

in src/validation.h:139 in 927666539b outdated

134@@ -135,6 +135,8 @@ static const unsigned int DEFAULT_BANSCORE_THRESHOLD = 100;
135 static const bool DEFAULT_PERSIST_MEMPOOL = true;
136 /** Default for -mempoolreplacement */
137 static const bool DEFAULT_ENABLE_REPLACEMENT = true;
138+/** Default for -mempoolreplacementtimeout in seconds after which transactions in mempool are replaceable (i.e. 6 hours) */
139+static const unsigned int DEFAULT_REPLACEMENT_TIMEOUT = 6 * 60 * 60;

promag commented at 1:34 am on December 10, 2017:

int64_t?

promag commented at 1:41 am on December 10, 2017: member

This test is failing https://github.com/bitcoin/bitcoin/blob/59d3dc85b698430f71f6e242a01a25a70c9ef397/test/functional/listtransactions.py#L180

greenaddress force-pushed on Dec 12, 2017

greenaddress force-pushed on Dec 13, 2017

greenaddress commented at 12:48 pm on December 13, 2017: contributor

@promag added a test for parameter validation. I also think the value should be below or equal to mempool expiry/eviction (two weeks) but very open to suggestions and thanks for catching the other issues

in src/policy/rbf.h:26 in 4fc450a4a1 outdated

18@@ -19,10 +19,13 @@ enum RBFTransactionState {
19 // opt-in to replace-by-fee, according to BIP 125
20 bool SignalsOptInRBF(const CTransaction &tx);
21 
22+// Check whether the replacement policy has expired
23+bool ExpiredOptInRBFPolicy(const int64_t now, const int64_t accepted, const int64_t timeout);
24+
25 // Determine whether an in-mempool transaction is signaling opt-in to RBF
26 // according to BIP 125

MarcoFalke commented at 4:00 pm on December 13, 2017:

nit: Comment needs update.

in src/init.cpp:1181 in 4fc450a4a1 outdated

1104@@ -1104,6 +1105,11 @@ bool AppInitParameterInteraction()
1105         boost::split(vstrReplacementModes, strReplacementModeList, boost::is_any_of(","));
1106         fEnableReplacement = (std::find(vstrReplacementModes.begin(), vstrReplacementModes.end(), "fee") != vstrReplacementModes.end());
1107     }
1108+    const int64_t replacement_timeout = gArgs.GetArg("-mempoolreplacementtimeout", DEFAULT_REPLACEMENT_TIMEOUT);
1109+    const int64_t expiry = gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60;
1110+    if (replacement_timeout < 0 || replacement_timeout > expiry) {
1111+            return InitError("mempoolreplacementtimeout has to be a non negative number below or equal to mempoolexpiry (in seconds)");
1112+    }
1113

MarcoFalke commented at 4:03 pm on December 13, 2017:

Not sure if you might also want to check if it is accidentally(?) set above 6hours and print a warning?

greenaddress commented at 4:35 pm on December 13, 2017:

It’s a good question. Would there be a good reason to set it above 6 hours if 6 hours is the default, especially long term?

MarcoFalke commented at 6:04 pm on December 13, 2017:

Probably not, since you’d be potentially censoring yourself from valid and about-to-be-mined txes.

greenaddress commented at 6:12 pm on December 13, 2017:

Maybe it can be useful in testing or to have old behavior, hence adding a log warning entry seems to me a good idea but i’ll maybe wait for others to agree as we don’t want to spam the logs either

MarcoFalke commented at 4:28 pm on December 13, 2017: member

Concept ACK. Some documentation nits inside.

I do not think this needs a BIP. To the best of my knowledge there is not a single BIP that documents local node policy. I also don’t think we should start going in that direction, as node policy is potentially subject to random changes. Whereas the BIP process is meant to document or propose changes and then more or less “freeze” the proposal after adoption.

The only BIPs related to policy are feefilter and rbf125. Both of them needed some sort of coordination across the network, i.e. new protocol message or a new way to interpret a field in a tx. Those are on-off changes, i.e. either you support the new protocol message or you don’t/either you interpret the field in this specific way or you don’t. Whereas, setting local tx pool policy does not need to be coordinated across the network.

in src/wallet/feebumper.cpp:83 in 6ff7196e5c outdated

79 bool TransactionCanBeBumped(CWallet* wallet, const uint256& txid)
80 {
81     LOCK2(cs_main, wallet->cs_wallet);
82     const CWalletTx* wtx = wallet->GetWalletTx(txid);
83-    return wtx && SignalsOptInRBF(*wtx->tx) && !wtx->mapValue.count("replaced_by_txid");
84+    return wtx && TransactionCanBeReplaced(*wtx) && !wtx->mapValue.count("replaced_by_txid");

TheBlueMatt commented at 7:09 pm on December 13, 2017:

Dunno that we want to support replacing a transaction in wallet based on this. If nothing else it needs a buffer in excess of the replacement timeout to ensure propagation works well, and probably shouldn’t let the user override the replacement timeout in wallet. May also be good to wait some time for network upgrade, ie amybe this should be a separate PR in a separate release.

greenaddress commented at 7:27 pm on December 13, 2017:

Fair enough, I’ll move the wallet part to a new PR. What sort of buffer you had in mind? a percentage of the replacement timeout, say 10%? or perhaps better to have fixed buffer of a few minutes?

TheBlueMatt commented at 7:38 pm on December 13, 2017:

Just need something to capture the time for the original transaction to propagate. A few minutes is probably fine, but no reason not to be conservative…say, 20 minutes?

greenaddress commented at 7:50 pm on December 13, 2017:

Seems reasonable to me

greenaddress commented at 10:42 pm on July 21, 2018:

I think this is now all addressed

TheBlueMatt commented at 7:09 pm on December 13, 2017: member

Concept ACK

MarcoFalke added the label Needs rebase on Jun 6, 2018

greenaddress force-pushed on Jul 21, 2018

greenaddress commented at 10:41 pm on July 21, 2018: contributor

@TheBlueMatt I rebased and added the 20 minutes buffer. I also made the wallet part and rpc part off by default such that it can be turned on once the network upgrades in a second release (and perhaps remove entirely the config option as I configured it as debug_only, won’t show up w/ -help)

This means that unless a user sets -enablewalletreplacementtimeout=1 (0 by default) they won’t see transaction rbfable via rpc/gui wallet. I left this in because is more useful for testing and because allows #reckless to try the feature in advance of a release even if the network is only partially upgraded.

greenaddress commented at 10:47 pm on July 21, 2018: contributor

@promag if you have time I’d be keen on your feedback/re-review

DrahtBot removed the label Needs rebase on Jul 22, 2018

in src/validation.h:128 in 39103725dd outdated

121@@ -122,6 +122,12 @@ static const unsigned int DEFAULT_BANSCORE_THRESHOLD = 100;
122 static const bool DEFAULT_PERSIST_MEMPOOL = true;
123 /** Default for -mempoolreplacement */
124 static const bool DEFAULT_ENABLE_REPLACEMENT = true;
125+/** Default for -mempoolreplacementtimeout in seconds after which transactions in mempool are replaceable (i.e. 6 hours) */
126+static const int64_t DEFAULT_REPLACEMENT_TIMEOUT = 6 * 60 * 60;
127+/** Default for buffer in seconds on top of the timeout after which transactions in mempool are replaceable (i.e. 6 hours) in the GUI */
128+static const int64_t REPLACEMENT_TIMEOUT_BUFFER = 20 * 60;

petertodd commented at 1:18 am on July 23, 2018:

20 minutes seems a bit long to me.

How long do transactions take to propagate to miners these days? Surely more like a minute or two? In the replacement case it’s ok if not every last miner actually gets the replacement; having the occasional one missing isn’t a big deal.

How about five minutes?

greenaddress commented at 8:18 am on July 23, 2018:

I think 5 minutes would be fine. @TheBlueMatt ?

Sjors commented at 10:04 pm on February 8, 2019:

Could also use a code comment to explain why this additional timeout is needed.

greenaddress commented at 6:48 pm on February 22, 2019:

@Sjors added a comment

practicalswift commented at 7:56 am on September 15, 2018: contributor

This PR doesn’t compile when rebased on master :-)

in src/policy/rbf.cpp:47 in 39103725dd outdated

46+    const CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
47     pool.CalculateMemPoolAncestors(entry, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
48 
49-    for (CTxMemPool::txiter it : setAncestors) {
50-        if (SignalsOptInRBF(it->GetTx())) {
51+    for (const CTxMemPool::txiter it : setAncestors) {

practicalswift commented at 7:57 am on September 15, 2018:

~~Should be const reference instead?~~

MarcoFalke commented at 1:31 pm on September 15, 2018:

txiter is already const_txiter, no need to change anything here

in src/validation.cpp:640 in 39103725dd outdated

634@@ -632,6 +635,9 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
635                             break;
636                         }
637                     }
638+                const int64_t conflicting_time = pool.info(ptxConflicting->GetHash()).nTime;
639+                const bool conflicting_pretimeout = !ExpiredOptInRBFPolicy(nAcceptTime, conflicting_time, replacement_timeout);
640+                fReplacementOptOut = conflicting_pretimeout && !SignalsOptInRBF(*ptxConflicting);

practicalswift commented at 8:34 am on September 21, 2018:

Replace with:

0bool fReplacementOptOut = conflicting_pretimeout && !SignalsOptInRBF(*ptxConflicting);

fReplacementOptOut is not needed above :-)

DrahtBot commented at 1:33 pm on September 21, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15639 (bitcoin-wallet tool: Drop libbitcoin_server.a dependency by ryanofsky)
#15638 (Move-only: Pull wallet code out of libbitcoin_server by ryanofsky)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

DrahtBot added the label Needs rebase on Sep 24, 2018

jnewbery commented at 11:22 pm on November 21, 2018: member

Definite concept ACK. I’d love to see some progress on this.

On the BIP question: I don’t think this needs a BIP. My preference would be for documentation on design or node policy to live in https://github.com/bitcoin-core/docs.

Sjors commented at 10:05 pm on February 8, 2019: member

Concept ACK

ziggamon commented at 5:50 pm on February 22, 2019: none

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 commented at 6:07 pm on February 22, 2019: contributor

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 commented at 6:33 pm on February 22, 2019: none

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 force-pushed on Feb 22, 2019

DrahtBot removed the label Needs rebase on Feb 22, 2019

abitfan commented at 8:48 pm on February 22, 2019: contributor

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

MarcoFalke deleted a comment on Feb 23, 2019

Sjors commented at 8:26 am on February 23, 2019: member

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.

in test/functional/wallet_bumpfee.py:146 in a220fcab03 outdated

141+    self.stop_node(0)
142+    err_msg = "Error: mempoolreplacementtimeout has to be a non negative number below or equal to mempoolexpiry (in seconds)"
143+    self.nodes[0].assert_start_raises_init_error(["-mempoolreplacementtimeout=-1"], err_msg)
144+    beyond_expiry = 60 * 60 + 1
145+    self.nodes[0].assert_start_raises_init_error(["-mempoolexpiry=1",
146+                                            "-mempoolreplacementtimeout=%d" % (beyond_expiry)], err_msg)

practicalswift commented at 11:40 pm on February 25, 2019:

Indentation seems a bit off here. Consider using black :-)

greenaddress commented at 7:13 pm on February 26, 2019:

Yes looks off, you are right.

What is black?

practicalswift commented at 8:04 pm on February 26, 2019:

black is the new hot Python equivalent of go fmt. It is currently taking the open source Python world by storm! :-)

Try: pip3 install black && black file_to_format.py

MarcoFalke commented at 8:13 pm on February 26, 2019:

@practicalswift black will reformat the whole file, I don’t think this is something we desire right now or should ask contributors to do

MarcoFalke commented at 8:15 pm on February 26, 2019:

Also, leaving feedback about whitespace in a review without leaving any other non-style comments about the code is distracting to the author and other reviewers

practicalswift commented at 8:23 pm on February 26, 2019:

black was suggested as a way to easily obtain a sane formatting suggestion for the block in question. Reformatting the whole file as part of this PR is obviously a very bad idea :-)

greenaddress commented at 1:52 pm on March 8, 2019:

fixed in 03fa5a1b421be6c07bc5fc33118961d6da93fb65

DrahtBot added the label Needs rebase on Mar 4, 2019

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

MarcoFalke commented at 6:46 pm on March 4, 2019: member

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.

Allow all mempool transactions to be replaced after a configurable timeout (default 6h) 03fa5a1b42

greenaddress force-pushed on Mar 8, 2019

greenaddress commented at 1:53 pm on March 8, 2019: contributor

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

DrahtBot removed the label Needs rebase on Mar 8, 2019

gmaxwell commented at 11:16 pm on March 11, 2019: contributor

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 commented at 1:21 am on March 13, 2019: contributor

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 commented at 10:45 pm on April 5, 2019: member

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 added the label Needs rebase on Apr 10, 2019

DrahtBot commented at 2:22 pm on April 10, 2019: member

Sjors commented at 5:33 pm on April 23, 2019: member

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

in src/policy/rbf.cpp:22 in 03fa5a1b42

18+bool ExpiredOptInRBFPolicy(const int64_t now, const int64_t accepted, const int64_t timeout)
19+{
20+    return now - accepted >= timeout;
21+}
22+
23+RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool, const int64_t now, const int64_t timeout, const bool enabled_replacement_timeout)

jnewbery commented at 3:18 pm on May 8, 2019:

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.

in src/validation.cpp:641 in 03fa5a1b42

644-                            break;
645-                        }
646-                    }
647+                    const int64_t conflicting_time = pool.info(ptxConflicting->GetHash()).nTime;
648+                    const bool conflicting_pretimeout = !ExpiredOptInRBFPolicy(nAcceptTime, conflicting_time, replacement_timeout);
649+                    fReplacementOptOut = conflicting_pretimeout && !SignalsOptInRBF(*ptxConflicting);

jnewbery commented at 3:25 pm on May 8, 2019:

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

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

jnewbery commented at 9:32 pm on May 8, 2019:

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

in src/validation.h:130 in 03fa5a1b42

122@@ -123,6 +123,13 @@ static const unsigned int DEFAULT_BANSCORE_THRESHOLD = 100;
123 static const bool DEFAULT_PERSIST_MEMPOOL = true;
124 /** Default for -mempoolreplacement */
125 static const bool DEFAULT_ENABLE_REPLACEMENT = true;
126+/** Default for -mempoolreplacementtimeout in seconds after which transactions in mempool are replaceable (i.e. 6 hours) */
127+static const int64_t DEFAULT_REPLACEMENT_TIMEOUT = 6 * 60 * 60;
128+/** Default for buffer in seconds on top of the timeout after which transactions in mempool are replaceable (i.e. 6 hours) in the GUI
129+ * This buffer is needed because otherwise is more likely ours peers will reject the transaction in case they received it substantial time after us */
130+static const int64_t REPLACEMENT_TIMEOUT_BUFFER = 5 * 60;

jnewbery commented at 3:28 pm on May 8, 2019:

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

in src/init.cpp:520 in 03fa5a1b42

515@@ -516,6 +516,8 @@ void SetupServerArgs()
516     gArgs.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), false, OptionsCategory::NODE_RELAY);
517     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);
518     gArgs.AddArg("-mempoolreplacement", strprintf("Enable transaction replacement in the memory pool (default: %u)", DEFAULT_ENABLE_REPLACEMENT), false, OptionsCategory::NODE_RELAY);
519+    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);
520+    gArgs.AddArg("-enablewalletreplacementtimeout", strprintf("Whether to enable wallet replacement timeout (default: %u)", DEFAULT_WALLET_REPLACEMENT_TIMEOUT), true, OptionsCategory::OPTIONS);

jnewbery commented at 7:06 pm on May 8, 2019:

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

MarcoFalke commented at 8:06 pm on July 18, 2019:

I think we can remove this option entirely and add wallet and rpc support after the p2p change is merged

jnewbery commented at 7:08 pm on May 8, 2019: member

This PR introduces two changes:

a policy change to allow mempool transactions to be replaced after a configurable time
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?

ziggamon commented at 7:42 pm on May 8, 2019: none

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 commented at 9:31 pm on May 8, 2019: member

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 commented at 11:11 pm on May 8, 2019: contributor

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 commented at 1:21 pm on May 9, 2019: none

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

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 commented at 2:32 pm on May 9, 2019: member

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 commented at 12:45 pm on May 12, 2019: member

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

in src/validation.cpp:638 in 03fa5a1b42

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

PastaPastaPasta commented at 2:52 am on June 6, 2019:

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

jnewbery commented at 4:53 am on June 6, 2019:

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

PastaPastaPasta changes_requested

MarcoFalke commented at 8:08 pm on July 18, 2019: member

@greenaddress Are you still working on this. It would help if all the wallet changes are removed and saved for a later pull request. Generally we wait one release before using new p2p features in the wallet.

MarcoFalke removed this from the milestone 0.19.0 on Sep 16, 2019

laanwj commented at 12:03 pm on September 30, 2019: member

This PR seems to have gone inactive. Let me know if there’s still interest in it and I’ll reopen.

laanwj closed this on Sep 30, 2019

MarcoFalke commented at 8:11 pm on September 30, 2019: member

An easy first step would be to make the RPC interface more flexible by reporting a string reason: #16490

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

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

Conflicts