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

[petertodd]

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 (#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?

[ghost]

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]

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]

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]

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

[petertodd]

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]

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]

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]

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]

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]

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]

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.

/test/functional/wallet_bumpfee.py", line 233, in test_nonrbf_bumpfee_fails
                                       assert_raises_rpc_error(-4, "Transaction is not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
                                     File /test/functional/test_framework/util.py", line 130, in assert_raises_rpc_error
                                       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]

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]

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.

[petertodd]

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]

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]

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]

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]

@petertodd:

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

That seems a reasonable place for that.

[maflcko]

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

[petertodd]

Converted to draft. Thanks for the reminder!

[DrahtBot]

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]

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.