wallet: Allow `bumpfee` for txs that don't signal BIP-125

petertodd commented at 4:12 pm on November 4, 2022: contributor

There is no technical reason to prevent this, though of course, non-signalling transactions are less likely to actually get mined. There is no consensus over the mempool, and especially in times of congestion, a higher fee transaction may be able to replace a lower fee one regardless of fee rate.

No changes to the tests were required, as this case is not tested. If -mempoolfullrbf (https://github.com/bitcoin/bitcoin/pull/25353) is enabled, the replacement is of course added to the mempool. If not, the replacement is added to the wallet and may be broadcast later.

Question: What would be the best way to force the transaction to be added to the mempool, with broadcast?

Allow `bumpfee` for txs that don't signal BIP-125

There is no technical reason to prevent this, though of course, non-signalling
transactions are less likely to actually get mined. There is no consensus over
the mempool, and especially in times of congestion, a higher fee transaction
may be able to replace a lower fee one regardless of fee rate.

a79a50dcc7

fanquake added the label Wallet on Nov 4, 2022

maflcko renamed this:
~~Allow `bumpfee` for txs that don't signal BIP-125~~
wallet: Allow `bumpfee` for txs that don't signal BIP-125
on Nov 4, 2022

maflcko added the label Brainstorming on Nov 4, 2022

ghost commented at 4:54 pm on November 4, 2022: none

Concept ACK

Will it be better if bumpfee RPC had an argument force false by default and if true would replace transaction irrespective of signalling?

Example: bitcoin-cli -named bumpfee txid=<txid> force=true

achow101 commented at 5:03 pm on November 4, 2022: member

In our wallet, we try not to make transactions that are unlikely to be propagated around the network. One of the ways that we do this is to make sure that a transaction is accepted into our own mempool. So we also (try to) adhere to mempool options as well. Thus this should respect the -mempoolfullrbf option.

Additionally, in line with the above principle, we generally don’t make changes to the wallet involving relay policy until it is clear that a new policy has been adopted by the network. So this change should wait until -mempoolfullrbf=1 is the default and has wide enough adoption that such transactions propagate well.

This also needs tests for the specific behavior.

No changes to the tests were required, as this case is not tested

CI seems to disagree.

kristapsk commented at 5:28 pm on November 4, 2022: contributor

Will it be better if bumpfee RPC had an argument force false by default and if true would replace transaction irrespective of signalling?

Sounds right thing to do here to me. Also could have extra confirmation warning dialog in Qt GUI when trying to bump non-BIP125 tx.

luke-jr commented at 6:15 pm on November 4, 2022: member

I like the idea of an explicit force parameter. But it shouldn’t be a positional argument.

petertodd commented at 6:29 pm on November 4, 2022: contributor

On November 4, 2022 6:04:03 PM GMT+01:00, Andrew Chow @.***> wrote:

In our wallet, we try not to make transactions that are unlikely to be propagated around the network. One of the ways that we do this is to make sure that a transaction is accepted into our own mempool. So we also (try to) adhere to mempool options as well. Thus this should respect the -mempoolfullrbf option.

This isn’t a matter of “respect”: BIP 125 isn’t a contract. We have a transaction that we would like to get mined that isn’t getting mined. A fee bump is improving our chances.

Additionally, in line with the above principle, we generally don’t make changes to the wallet involving relay policy until it is clear that a new policy has been adopted by the network. So this change should wait until -mempoolfullrbf=1 is the default and has wide enough adoption that such transactions propagate well.

But this isn’t really a question of relay policy: the fee bumped transaction is perfectly valid, and some nodes will propagate it.

The most likely scenario where this is relevant is congestion. In that scenario the minimum relay fee rate is not going to be the same on all nodes. So your fee bumped transaction genuinely has a higher chance of getting mined. Indeed, your own node may not be accepting the original to the mempool due to a higher than default relay fee.

This also needs tests for the specific behavior.

No changes to the tests were required, as this case is not tested

CI seems to disagree.

FWIW I ran make check as the docs suggested. I haven’t done Core development in years, so I was going by what the README said.

ghost commented at 6:57 pm on November 4, 2022: none

FWIW I ran make check as the docs suggested. I haven’t done Core development in years, so I was going by what the README said.

Functional test is failing and make check only works for unit tests.

https://github.com/bitcoin/bitcoin/blob/master/test/README.md

test/functional/test_runner.py to run the standard test suite (try test/functional/test_runner.py -j 60 or a similar high number to run the tests more quickly in parallel)
test/functional/.py to run an individual test file
test/functional/test_runner.py --extended to run the extended test suite
test/functional/test_runner.py --help to see the various options for running tests

https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests

achow101 commented at 7:12 pm on November 4, 2022: member

This isn’t a matter of “respect”: BIP 125 isn’t a contract. We have a transaction that we would like to get mined that isn’t getting mined. A fee bump is improving our chances.

It’s not going to help if our own mempool isn’t going to accept it. If that’s the case, we can’t even rebroadcast it unless we somehow store that the transaction needs to be unconditionally broadcast, or we go back unconditionally broadcasting all wallet transactions. But there’s potential for a privacy leak there.

The most likely scenario where this is relevant is congestion. In that scenario the minimum relay fee rate is not going to be the same on all nodes. So your fee bumped transaction genuinely has a higher chance of getting mined. Indeed, your own node may not be accepting the original to the mempool due to a higher than default relay fee.

In that scenario, we should be checking the conditions for whether a transaction can be abandoned before allowing fee bumping. In such a situation, the same effect can be achieved by abandoning the transaction and recreating it.

petertodd commented at 7:24 pm on November 4, 2022: contributor

On November 4, 2022 8:12:30 PM GMT+01:00, Andrew Chow @.***> wrote:

This isn’t a matter of “respect”: BIP 125 isn’t a contract. We have a transaction that we would like to get mined that isn’t getting mined. A fee bump is improving our chances.

It’s not going to help if our own mempool isn’t going to accept it. If that’s the case, we can’t even rebroadcast it unless we somehow store that the transaction needs to be unconditionally broadcast, or we go back unconditionally broadcasting all wallet transactions. But there’s potential for a privacy leak there.

… and as I said, it’d be good to have a mechanism to simply force the transaction into our mempool and broadcast it to peers for these types of rare cases.

This is likely to be a rare case, so privacy leaks aren’t relevant. And of course, privacy leaks can be mitigated by occasionally accepting full-rnf replacements.

The most likely scenario where this is relevant is congestion. In that scenario the minimum relay fee rate is not going to be the same on all nodes. So your fee bumped transaction genuinely has a higher chance of getting mined. Indeed, your own node may not be accepting the original to the mempool due to a higher than default relay fee.

In that scenario, we should be checking the conditions for whether a transaction can be abandoned before allowing fee bumping. In such a situation, the same effect can be achieved by abandoning the transaction and recreating it.

There is no safe way of recreating a transaction from the user’s perspective. The correct thing to do is to feebump to ensure that the recipient isn’t paid twice.

Congestion is a transient condition. Not permanent.

Indeed, abandontransaction is a very dangerous RPC call that can easily result in lost money. We should have added a true cancel transaction instead that double spent to avoid a double payment.

maflcko commented at 8:22 am on November 5, 2022: member

I don’t think it makes sense to show this option unless the mempool takes and broadcasts the transaction, that is -mempoolfullrbf is on. Moreover it would be good to have tests to check or describe how this interacts with -spendzeroconfchange (Spend unconfirmed change when sending transactions (default: 1))

zndtoshi commented at 6:09 am on November 11, 2022: none

Allow bumpfee in GUI and the first time a user tries to do it change the mempool policy fullrbf to 1 as that’s the implicit intention of the user. This could be an option in the settings menu as well.

Sjors commented at 11:19 am on November 11, 2022: member

I don’t think it makes sense to show this option unless the mempool takes and broadcasts the transaction, that is -mempoolfullrbf is on.

Agreed. I have some experience with how the GUI handles transactions that don’t get into our own mempool, and it’s no fun. A user might have sent a transaction without RBF, not know about the -mempoolfullrbf setting and then naively call bumpfee. The result would be thoroughly confusing.

No changes to the tests were required, as this case is not tested.

All the more reason to add test coverage for it.

0/test/functional/wallet_bumpfee.py", line 233, in test_nonrbf_bumpfee_fails
1                                       assert_raises_rpc_error(-4, "Transaction is not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
2                                     File /test/functional/test_framework/util.py", line 130, in assert_raises_rpc_error
3                                       assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"

Enabling this ability for users who manually turned on mempoolfullrbf=1 is fine with me, though the documentation should warn that the transaction will only propagate to peers that also have this enabled (i.e. it’s an advanced feature).

ghost commented at 9:32 pm on November 18, 2022: none

Agreed. I have some experience with how the GUI handles transactions that don’t get into our own mempool, and it’s no fun. A user might have sent a transaction without RBF, not know about the -mempoolfullrbf setting and then naively call bumpfee. The result would be thoroughly confusing.

I tried this today on signet. It wasn’t confusing for me because Tx1 got mined soon and Tx2 was marked as conflicted.

Enabling this ability for users who manually turned on mempoolfullrbf=1 is fine with me, though the documentation should warn that the transaction will only propagate to peers that also have this enabled (i.e. it’s an advanced feature).

Agreed. Tested this as well by running 2 signet nodes and replacing Tx1 with Tx2 on one. It was relayed to the peer I added manually but not visible in explorers as they might not be running nodes with mempoolfullrbf=1. Tx1 was mined in a few minutes so I am assuming none of my automatic outbound connections and signet miners (@ajtowns and @kallewoof) are using full rbf.

DrahtBot commented at 9:32 pm on November 18, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	1440000bytes

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

bitcoin deleted a comment on Nov 19, 2022

petertodd commented at 8:45 pm on November 20, 2022: contributor

I don’t think it makes sense to show this option unless the mempool takes and broadcasts the transaction, that is -mempoolfullrbf is on.

So, how about I add a full-rbf flag to the Chain interface then? IIUC that’s the sanest way for CWallet to get access to that setting.

petertodd commented at 8:52 pm on November 20, 2022: contributor

Allow bumpfee in GUI and the first time a user tries to do it change the mempool policy fullrbf to 1 as that’s the implicit intention of the user. This could be an option in the settings menu as well.

Well, at least I can mention mempoolfullrbf=1 in the error message.

luke-jr commented at 11:45 pm on November 20, 2022: member

Can we make the bump transaction and then abort if our own mempool rejects it?

(Further GUI UX stuff belongs in a separate PR IMO)

petertodd commented at 0:31 am on November 21, 2022: contributor

On November 20, 2022 5:45:25 PM CST, Luke Dashjr @.***> wrote:

Can we make the bump transaction and then abort if our own mempool rejects it?

(Further GUI UX stuff belongs in a separate PR IMO)

So if we do that, it’ll work regardless of the ‘mempoolfullrbf’ setting during high fee events when the 1st tx has been dropped from our mempool. That seems reasonable in principle.

Sjors commented at 12:50 pm on November 23, 2022: member

@petertodd:

So, how about I add a full-rbf flag to the Chain interface then?

That seems a reasonable place for that.

maflcko commented at 4:38 pm on January 26, 2023: member

Are you still working on this? (CI is red) Maybe close or mark as draft, if not.

bitcoin deleted a comment on Jan 26, 2023

petertodd marked this as a draft on Feb 4, 2023

petertodd commented at 4:50 pm on February 4, 2023: contributor

Converted to draft. Thanks for the reminder!

DrahtBot commented at 0:22 am on August 3, 2023: contributor

There hasn’t been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

maflcko commented at 6:53 am on August 3, 2023: member

Closing for now due to prolonged inactivity and red CI since this pull was opened. Please leave a comment in this thread, so that it can be reopened, once and if you start working on this again.

maflcko closed this on Aug 3, 2023

bitcoin locked this on Aug 2, 2024

wallet: Allow bumpfee for txs that don’t signal BIP-125 #26454

Reviews

wallet: Allow `bumpfee` for txs that don’t signal BIP-125 #26454