New `outputs` argument for `bumpfee`/`psbtbumpfee` #25344

rodentrabies · 2022-06-11T23:49:29Z

This implements a modification of the proposal in #22007: instead of adding outputs to the set of outputs in the original transaction, the outputs given by outputs argument completely replace the outputs in the original transaction.

As noted below, this makes it easier to "cancel" a transaction or to reduce the amounts in the outputs, which is not the case with the original proposal in #22007, but it seems from the discussion in this PR that the replace behavior is more desirable than add one.

src/wallet/rpc/spend.cpp

test/functional/wallet_bumpfee.py

DrahtBot · 2022-06-12T12:25:16Z

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
ACK	1440000bytes, achow101, ishaanam
Concept ACK	Sjors

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/700 (Dialog for allowing the user to choose the change output when bumping a tx by achow101)
#26485 (RPC: Accept options as named-only parameters by ryanofsky)
#26467 (bumpfee: Allow the user to choose which output is change by achow101)
#25979 ([WIP] wallet: standardize change output detection process by furszy)

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.

ghost · 2022-06-12T17:10:42Z

Concept ACK

This implements a proposal in #22007, using approach 1.b, that is the fee for additional arguments is paid from change, which is simpler and cleaner than 1.a.

Will the user be able to remove old outputs and add new outputs with 1a? If yes, why not both? and change the argument name to outputs from extra_outputs

Can you share example bitcoin-cli command with and without -named arguments for this pull request to test?

rodentrabies · 2022-06-12T22:29:00Z

Without -named:

$ ./src/bitcoin-cli -regtest getnewaddress
<address1>
$ ./src/bitcoin-cli -regtest getnewaddress
<address2>
$ ./src/bitcoin-cli -regtest getnewaddress
<address3>
$ ./src/bitcoin-cli -regtest send '[{<address1>: 1.0}, {<address2>: 2.0}]'
{
  "txid": <txid1>,
  "complete": true
}
$ ./src/bitcoin-cli -regtest bumpfee <txid1> '{"extra_outputs": {<address3>: 3.0}}'
{
  "txid": <txid2>,
  "origfee": -,
  "fee": -,
  "errors": [
  ]
}

With -named the last two steps of the above:

$ ./src/bitcoin-cli -regtest -named send outputs='[{<address1>: 1.0}, {<address2>: 2.0}]'
$ ./src/bitcoin-cli -regtest -named bumpfee txid=<txid1> options='{"extra_outputs": {<address3>: 3.0}}'

achow101 · 2022-06-13T20:33:12Z

Concept ACK

ghost · 2022-06-14T03:13:21Z

@rodentrabies Thanks. I had to add double quotes in address and the commands worked.

I tested the pull request by replacing a transaction on signet and add 1 output in replacement transaction:

$ bitcoin-cli -named sendtoaddress address="tb1p8xrz8rxk0v66f5hfaaqat9qew582vyhnqqdxjvwqz28uvj403v6qx2d0vm" amount=0.0001 replaceable=true

d0dae04b5afe8c845d12b15a9983468e7811c299f19f05599d58bb2400f05027

$ bitcoin-cli bumpfee d0dae04b5afe8c845d12b15a9983468e7811c299f19f05599d58bb2400f05027 '{"extra_outputs": {"tb1qrxm8ujt3t62k8k62egqj0sx27k7rtazsl33qrq": 0.0001}}'

{
  "txid": "cf40c3a74f1f9d756cee2c5b68d2ddcb495dcfbed75a67f78a12251749a1be23",
  "origfee": 0.00000154,
  "fee": 0.00001111,
  "errors": [
  ]
}

This adds another output in original transaction:

It is better than nothing although I would prefer an argument that can be used to replace all outputs with new outputs.

achow101 · 2022-06-14T19:41:48Z

ACK cbcc40b7b6eeba0e5ae195d513285b66da8d7d63

rodentrabies · 2022-06-14T21:52:45Z

Will the user be able to remove old outputs and add new outputs with 1a? If yes, why not both? and change the argument name to outputs from extra_outputs
It is better than nothing although I would prefer an argument that can be used to replace all outputs with new outputs.

Maybe have one argument outputs to specify a set of outputs and another argument like output_mode=add/output_mode=replace (choose better names though) to switch between different ways to handle them. The add behavior adds outputs as extra ones, replace - completely replaces the outputs of the original transaction.

Let's see what the other reviewers have to say about this. Maybe it's just a bit too complex.

ghost · 2022-06-15T00:11:11Z

Maybe have one argument outputs to specify a set of outputs and another argument like output_mode=add/output_mode=replace (choose better names though) to switch between different ways to handle them. The add behavior adds outputs as extra ones, replace - completely replaces the outputs of the original transaction.

Only outputs can also work like this:

Tx1 has A and B outputs. If user wants to add C output tin the replacement transaction Tx2:

$ bitcoin-cli bumpfee <txid for Tx1> '{"outputs": {"A": <amt>, "B": <amt>, "C": <amt>}}'

If user wants to have C and D as outputs for replacement transaction Tx2:

$ bitcoin-cli bumpfee <txid for Tx1> '{"outputs": {"C": <amt>, "D": <amt>}}'

rodentrabies · 2022-06-15T11:02:37Z

Tx1 has A and B outputs. If user wants to add C output tin the replacement transaction Tx2:
$ bitcoin-cli bumpfee <txid for Tx1> '{"outputs": {"A": <amt>, "B": <amt>, "C": <amt>}}'

What if user wants to add another A and B outputs (that is to have two A outputs and two B outputs for whatever reason)? Should the amounts be different? This "automagical" approach will quickly get confusing, I think.

luke-jr · 2022-06-18T20:00:13Z

What if user wants to add another A and B outputs (that is to have two A outputs and two B outputs for whatever reason)?

$ bitcoin-cli bumpfee <txid for Tx1> '{"outputs": {"A": <amt>, "A": <amt>, "B": <amt>, "B": <amt>, "C": <amt>}}' ?

But I'm not sure we should actively support that. Addresses are only supposed to be used once, after all.

luke-jr · 2022-06-18T20:01:21Z

Maybe have one argument outputs to specify a set of outputs and another argument like output_mode=add/output_mode=replace (choose better names though) to switch between different ways to handle them. The add behavior adds outputs as extra ones, replace - completely replaces the outputs of the original transaction.

Rather not have options that have fundamentally different behaviours/meanings based on others.

rodentrabies · 2022-06-19T10:59:32Z

What if user wants to add another A and B outputs (that is to have two A outputs and two B outputs for whatever reason)?

$ bitcoin-cli bumpfee <txid for Tx1> '{"outputs": {"A": <amt>, "A": <amt>, "B": <amt>, "B": <amt>, "C": <amt>}}' ?

But I'm not sure we should actively support that. Addresses are only supposed to be used once, after all.

@luke-jr agreed, that was an unfortunate example. I still think that the behavior proposed by @1440000bytes (adding new outputs if the addresses don't match and replacing outputs, if the addresses do match) is just a bit too confusing and if we end up having outputs argument instead of `extra, it should simply replace all arguments.

rodentrabies · 2022-06-19T11:01:02Z

Maybe have one argument outputs to specify a set of outputs and another argument like output_mode=add/output_mode=replace (choose better names though) to switch between different ways to handle them. The add behavior adds outputs as extra ones, replace - completely replaces the outputs of the original transaction.

Rather not have options that have fundamentally different behaviours/meanings based on others.

I don't like it either, just stated it for the record, I guess.

ghost · 2022-06-19T14:39:44Z

and if we end up having outputs argument instead of `extra, it should simply replace all arguments.

This is exactly what I was trying to express in comments above. An argument that accepts outputs and replaces all used in original transaction.

rodentrabies · 2022-06-19T17:56:39Z

If user wants to have C and D as outputs for replacement transaction Tx2:
$ bitcoin-cli bumpfee <txid for Tx1> '{"outputs": {"C": <amt>, "D": <amt>}}'

Ah, sorry, I for some reason assumed that you meant we add outputs C and D here (that is new transaction has A, B, C and D outputs).

So bottom line: we either have extra_outputs that adds to the outputs (as currently implemented in this PR), or outputs that replaces the outputs.

EDIT: or, we can have both and make them exclusive - either use extra_outputs to add arguments or use outputs to replace them, since both use cases are desired, I think.

ishaanam

Concept ACK

I'm curious as to the desired behavior when extra_outputs is used to add an output to the same address as an output of the original transaction. I tried doing this like so:

$ ./bitcoin-cli  -regtest -rpcwallet=second_wallet getnewaddress
bcrt1q3yjd0arl7fskt5ynwm6njz4ez63p9c86yeffew
$ ./bitcoin-cli -regtest -rpcwallet=first_wallet send '{"bcrt1q3yjd0arl7fskt5ynwm6njz4ez63p9c86yeffew": 1}' null "unset" 1.1
{
  "txid": "99bfd824edbff565146b9eda6f4370e851f9579c1744b0a54090dce9db6ed393",
  "complete": true
}
$ ./bitcoin-cli -regtest -rpcwallet=first_wallet bumpfee "99bfd824edbff565146b9eda6f4370e851f9579c1744b0a54090dce9db6ed393" '{"extra_outputs": {"bcrt1q3yjd0arl7fskt5ynwm6njz4ez63p9c86yeffew": 30}}'
{
  "txid": "07df53d9246cfdeec83646469ab1781a73814a4b3223c22d7096d812d9d66289",
  "origfee": 0.00000156,
  "fee": 0.00001875,
  "errors": [
  ]
}

It appears that the transaction is fee bumped just fine and there are two outputs to the same address in the same transaction. I was wondering if this is what we want to happen or if an error should be thrown in this scenario. I think that the reason an error isn't thrown currently is because when AddOutputs is called, only extra_outputs is passed and not all of the recipients, and I think that AddOutputs is where the check for duplicate addresses takes place.
One way to deal with this, if both the extra_outputs and outputs options are added as suggested above by @rodentrabies is that in this scenario the user can be told to use outputs instead if they want to increase the amount sent to an address, which I think should avoid the creation of two outputs here.

src/wallet/rpc/spend.cpp

rodentrabies · 2022-06-20T22:04:45Z

The most recent push (~~54c924d~~ 8c0027e) adds another duplicate checking pass in the CreateRateBumpTransaction function so that we can detect repeated use of addresses that were used in the original transaction (this also applies to the data output), as suggested by @ishaanam. This code will be unnecessary though, if we decide to go with the output replacement approach suggested by @1440000bytes.

ghost · 2022-06-21T14:49:38Z

This code will be unnecessary though, if we decide to go with the output replacement approach suggested by @1440000bytes.

And its easy to implement that by just removing below line from feebumper.cpp:

-all_outputs.insert(all_outputs.end(), wtx.tx->vout.begin(), wtx.tx->vout.end());

I tested it to replace output used in Tx1 with new output in replacement transaction Tx2:

$ bitcoin-cli -named sendtoaddress address="tb1p8xrz8rxk0v66f5hfaaqat9qew582vyhnqqdxjvwqz28uvj403v6qx2d0vm" amount=0.0001 replaceable=true

acc32a3b2c25b7cbf2582629be891babfcb1f8c139bd03bab075e8ec47f19bd2

$ bitcoin-cli bumpfee acc32a3b2c25b7cbf2582629be891babfcb1f8c139bd03bab075e8ec47f19bd2 '{"extra_outputs": {"tb1qrxm8ujt3t62k8k62egqj0sx27k7rtazsl33qrq": 0.0001}}'
{
  "txid": "429924607dde3d83379fc3501adbbaa2112d4f974cb243d24863e575397171aa",
  "origfee": 0.00000212,
  "fee": 0.00001129,
  "errors": [
  ]
}

Tx1: acc32a3b2c25b7cbf2582629be891babfcb1f8c139bd03bab075e8ec47f19bd2

Replaced by Tx2: 429924607dde3d83379fc3501adbbaa2112d4f974cb243d24863e575397171aa

It could be used to cancel a transaction similar to electrum, bluewallet etc. and better for privacy in some cases.

rodentrabies · 2022-06-21T14:58:36Z

And its easy to implement that by just removing below link from feebumper.cpp:

That and also remove the duplicate checking code in the body of the for-loop as I mentioned above - it will be redundant in that case because the duplicate check already happened in AddOutputs().

ghost

reACK 5068940

ishaanam

reACK 5068940043a4c44bb9e7d3a2d70a12c753ff82e4
I also tested this by replacing the output of a transaction with external inputs.

achow101 · 2022-11-21T20:24:08Z

src/wallet/rpc/spend.cpp

In ab9c4225332c028a7c84260d5bf634c428698662 "wallet: extract and reuse RPC argument format definition for outputs"

for a followup: This can be used in src/rpc/rawtransaction.cpp as well as the same lines are copied for createrawtransaction and createpsbt.

src/wallet/rpc/spend.cpp

achow101 · 2022-11-21T20:32:13Z

src/wallet/rpc/spend.cpp

This was mentioned in the RPC doc above.

However I think it is a bit confusing to have an empty array of outputs be synonymous with not providing the option at all. I think it would be better to just throw an error in that case. Checking for an empty outputs array could be done in AddOutputs as well so that check will also be done for createpsbt and createrawtransaction as it doesn't make sense to make a transaction with no outputs.

src/rpc/rawtransaction_util.cpp

test/functional/wallet_bumpfee.py

ghost

Code Review ACK 4c8eccc

achow101 · 2023-01-26T20:46:44Z

ACK 4c8eccc

ishaanam

reACK 4c8eccc

ishaanam · 2023-02-08T01:17:09Z

test/functional/wallet_bumpfee.py

+def spend_one_input(node, dest_address, change_size=Decimal("0.00049000"), data=None):
    tx_input = dict(
        sequence=MAX_BIP125_RBF_SEQUENCE, **next(u for u in node.listunspent() if u["amount"] == Decimal("0.00100000")))
    destinations = {dest_address: Decimal("0.00050000")}
    if change_size > 0:
        destinations[node.getrawchangeaddress()] = change_size
+    if data:
+        destinations['data'] = data


Non-blocking: This change is never used.

ghost · 2023-02-16T18:59:06Z

Finally. Thanks @rodentrabies this would improve UX.

rodentrabies commented Jun 11, 2022

View reviewed changes

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

rodentrabies force-pushed the bumpfee-extra-outputs branch from 94a4094 to f188181 Compare June 11, 2022 23:53

rodentrabies commented Jun 11, 2022

View reviewed changes

test/functional/wallet_bumpfee.py Outdated Show resolved Hide resolved

DrahtBot added RPC/REST/ZMQ Wallet labels Jun 12, 2022

rodentrabies force-pushed the bumpfee-extra-outputs branch from f188181 to 814c3b2 Compare June 12, 2022 11:17

DrahtBot mentioned this pull request Jun 12, 2022

wallet: allow psbtbumpfee to work with txs with external inputs #23202

Merged

rodentrabies force-pushed the bumpfee-extra-outputs branch from 814c3b2 to cbcc40b Compare June 14, 2022 16:53

ishaanam reviewed Jun 19, 2022

View reviewed changes

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

rodentrabies force-pushed the bumpfee-extra-outputs branch from cbcc40b to 54c924d Compare June 20, 2022 21:56

rodentrabies force-pushed the bumpfee-extra-outputs branch from 54c924d to 8c0027e Compare June 21, 2022 10:31

maflcko removed the Wallet label Jul 1, 2022

rodentrabies force-pushed the bumpfee-extra-outputs branch 2 times, most recently from fbed850 to 5068940 Compare September 15, 2022 10:44

ghost approved these changes Sep 15, 2022

View reviewed changes

DrahtBot mentioned this pull request Oct 3, 2022

clang-tidy: fixup named argument comments #26238

Merged

ishaanam reviewed Oct 5, 2022

View reviewed changes

This was referenced Nov 9, 2022

bumpfee: Allow the user to choose which output is change #26467

Merged

RPC: Accept options as named-only parameters #26485

Merged

achow101 reviewed Nov 21, 2022

View reviewed changes

rodentrabies force-pushed the bumpfee-extra-outputs branch from 5068940 to 7e2a5c7 Compare December 17, 2022 20:05

DrahtBot mentioned this pull request Dec 22, 2022

refactor: Avoid UniValue copies #25429

Closed

rodentrabies force-pushed the bumpfee-extra-outputs branch 2 times, most recently from ff0369b to 571394a Compare January 14, 2023 12:11

DrahtBot added the Needs rebase label Jan 17, 2023

rodentrabies added 3 commits January 17, 2023 13:28

wallet: extract and reuse RPC argument format definition for outputs

a804f3c

wallet: add outputs arguments to bumpfee and psbtbumpfee

c0ebb98

test: add tests for outputs argument to bumpfee/psbtbumpfee

4c8eccc

rodentrabies force-pushed the bumpfee-extra-outputs branch from 571394a to 4c8eccc Compare January 17, 2023 12:08

DrahtBot removed the Needs rebase label Jan 17, 2023

ghost approved these changes Jan 17, 2023

View reviewed changes

DrahtBot mentioned this pull request Jan 24, 2023

Dialog for allowing the user to choose the change output when bumping a tx bitcoin-core/gui#700

Open

maflcko requested a review from Sjors January 27, 2023 08:05

achow101 requested a review from ishaanam February 7, 2023 22:20

ishaanam reviewed Feb 8, 2023

View reviewed changes

achow101 merged commit 73966f7 into bitcoin:master Feb 16, 2023

rodentrabies deleted the bumpfee-extra-outputs branch February 16, 2023 20:15

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 17, 2023

Merge bitcoin#25344: New outputs argument for bumpfee/psbtbumpfee

058efe5

bitcoin deleted a comment Feb 17, 2023

bitcoin locked and limited conversation to collaborators Feb 17, 2024

New outputs argument for bumpfee/psbtbumpfee #25344

New outputs argument for bumpfee/psbtbumpfee #25344

Uh oh!

Conversation

rodentrabies commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jun 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

ghost commented Jun 12, 2022

Uh oh!

rodentrabies commented Jun 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jun 13, 2022

Uh oh!

ghost commented Jun 14, 2022

Uh oh!

achow101 commented Jun 14, 2022

Uh oh!

rodentrabies commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 15, 2022

Uh oh!

rodentrabies commented Jun 15, 2022

Uh oh!

luke-jr commented Jun 18, 2022

Uh oh!

luke-jr commented Jun 18, 2022

Uh oh!

rodentrabies commented Jun 19, 2022

Uh oh!

rodentrabies commented Jun 19, 2022

Uh oh!

ghost commented Jun 19, 2022

Uh oh!

rodentrabies commented Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ishaanam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rodentrabies commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 21, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodentrabies commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 26, 2023

Uh oh!

ishaanam left a comment

New `outputs` argument for `bumpfee`/`psbtbumpfee` #25344

New `outputs` argument for `bumpfee`/`psbtbumpfee` #25344

rodentrabies commented Jun 11, 2022 •

edited

Loading

DrahtBot commented Jun 12, 2022 •

edited

Loading

rodentrabies commented Jun 12, 2022 •

edited

Loading

rodentrabies commented Jun 14, 2022 •

edited

Loading

rodentrabies commented Jun 19, 2022 •

edited

Loading

ishaanam left a comment •

edited

Loading

rodentrabies commented Jun 20, 2022 •

edited

Loading

ghost commented Jun 21, 2022 •

edited by ghost

Loading

rodentrabies commented Jun 21, 2022 •

edited

Loading