New outputs argument for bumpfee/psbtbumpfee #25344

pull rodentrabies wants to merge 3 commits into bitcoin:master from rodentrabies:bumpfee-extra-outputs changing 7 files +112 −56
  1. rodentrabies commented at 11:49 pm on June 11, 2022: contributor

    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.

  2. in src/wallet/rpc/spend.cpp:989 in 94a4094ce1 outdated 
    982@@ -983,6 +983,23 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
983                              "are replaceable).\n"},
984                     {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n"
985                              "\"" + FeeModes("\"\n\"") + "\""},
986+                    {"extra_outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Additional outputs (key-value pairs) in the same format\n"
987+                             "as the \"outputs\" argument to \"send\": each address can only appear once and there can only be\n"
988+                             "one \"data\" object.\n",
989+                         {
    rodentrabies commented at 11:51 pm on June 11, 2022:
    Lines below are pretty much copied from send. Not sure how to extract it somewhere and reuse in both places.
    unknown commented at 12:29 pm on June 12, 2022:
    I think its okay because they are already used at multiple places: send(), sendall() and walletcreatefundedpsbt()
    achow101 commented at 8:32 pm on June 13, 2022:
    You can de-duplicate this in the same way that FundTxDoc works. Have a look at that and its usage in various help texts for some guidance.
    rodentrabies commented at 4:54 pm on June 14, 2022:
    @achow101 thanks! Done.
  3. rodentrabies force-pushed on Jun 11, 2022
  4. in test/functional/wallet_bumpfee.py:184 in f188181b9b outdated 
    176@@ -167,6 +177,9 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
177     if mode == "fee_rate":
178         bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"fee_rate": str(NORMAL)})
179         bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": NORMAL})
180+    elif mode == "extra_outputs":
181+        bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"extra_outputs": {dest_address: 0.003}})
182+        bumped_tx = rbf_node.bumpfee(rbfid, {"extra_outputs": {dest_address: 0.003}})
183     else:
    rodentrabies commented at 11:55 pm on June 11, 2022:
    ~Need to add a check whether the additional output is actually there.~ done
  5. DrahtBot added the label RPC/REST/ZMQ on Jun 12, 2022
  6. DrahtBot added the label Wallet on Jun 12, 2022
  7. rodentrabies force-pushed on Jun 12, 2022
  8. DrahtBot commented at 12:25 pm on June 12, 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
    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.

  9. ghost commented at 5:10 pm on June 12, 2022: none

    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?

  10. rodentrabies commented at 10:29 pm on June 12, 2022: contributor

    Without -named:

     0$ ./src/bitcoin-cli -regtest getnewaddress
 1<address1>
 2$ ./src/bitcoin-cli -regtest getnewaddress
 3<address2>
 4$ ./src/bitcoin-cli -regtest getnewaddress
 5<address3>
 6$ ./src/bitcoin-cli -regtest send '[{<address1>: 1.0}, {<address2>: 2.0}]'
 7{
 8  "txid": <txid1>,
 9  "complete": true
10}
11$ ./src/bitcoin-cli -regtest bumpfee <txid1> '{"extra_outputs": {<address3>: 3.0}}'
12{
13  "txid": <txid2>,
14  "origfee": -,
15  "fee": -,
16  "errors": [
17  ]
18}

    With -named the last two steps of the above:

    0$ ./src/bitcoin-cli -regtest -named send outputs='[{<address1>: 1.0}, {<address2>: 2.0}]'
1$ ./src/bitcoin-cli -regtest -named bumpfee txid=<txid1> options='{"extra_outputs": {<address3>: 3.0}}'
  11. achow101 commented at 8:33 pm on June 13, 2022: member
    Concept ACK
  12. ghost commented at 3:13 am on June 14, 2022: none

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

     0$ bitcoin-cli -named sendtoaddress address="tb1p8xrz8rxk0v66f5hfaaqat9qew582vyhnqqdxjvwqz28uvj403v6qx2d0vm" amount=0.0001 replaceable=true
 1
 2d0dae04b5afe8c845d12b15a9983468e7811c299f19f05599d58bb2400f05027
 3
 4$ bitcoin-cli bumpfee d0dae04b5afe8c845d12b15a9983468e7811c299f19f05599d58bb2400f05027 '{"extra_outputs": {"tb1qrxm8ujt3t62k8k62egqj0sx27k7rtazsl33qrq": 0.0001}}'
 5
 6{
 7  "txid": "cf40c3a74f1f9d756cee2c5b68d2ddcb495dcfbed75a67f78a12251749a1be23",
 8  "origfee": 0.00000154,
 9  "fee": 0.00001111,
10  "errors": [
11  ]
12}

    This adds another output in original transaction:

    image

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

  13. rodentrabies force-pushed on Jun 14, 2022
  14. achow101 commented at 7:41 pm on June 14, 2022: member
    ACK cbcc40b7b6eeba0e5ae195d513285b66da8d7d63
  15. rodentrabies commented at 9:52 pm on June 14, 2022: contributor

    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.

  16. ghost commented at 0:11 am on June 15, 2022: none

    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:

    0$ 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:

    0$ bitcoin-cli bumpfee <txid for Tx1> '{"outputs": {"C": <amt>, "D": <amt>}}'
  17. rodentrabies commented at 11:02 am on June 15, 2022: contributor

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

    0$ 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.

  18. luke-jr commented at 8:00 pm on June 18, 2022: member

    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.

  19. luke-jr commented at 8:01 pm on June 18, 2022: member

    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.

  20. rodentrabies commented at 10:59 am on June 19, 2022: contributor

    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.

  21. rodentrabies commented at 11:01 am on June 19, 2022: contributor

    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.

  22. ghost commented at 2:39 pm on June 19, 2022: none

    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.

  23. rodentrabies commented at 5:56 pm on June 19, 2022: contributor

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

    0$ 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.

  24. in src/wallet/rpc/spend.cpp:1010 in cbcc40b7b6 outdated 
    1002@@ -983,6 +1003,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1003                              "are replaceable).\n"},
1004                     {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n"
1005                              "\"" + FeeModes("\"\n\"") + "\""},
1006+                    {"extra_outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Additional outputs (key-value pairs) in the same format\n"
1007+                             "as the \"outputs\" argument to \"send\": each address can only appear once and there can only be\n"
1008+                             "one \"data\" object.\n",
1009+                         OutputsDoc(),
1010+                     },
    ishaanam commented at 6:39 pm on June 19, 2022: 
    0                    },

    nit: misaligned brackets

    rodentrabies commented at 9:56 pm on June 20, 2022:
    done
  25. ishaanam commented at 6:53 pm on June 19, 2022: contributor

    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:

     0$ ./bitcoin-cli  -regtest -rpcwallet=second_wallet getnewaddress
 1bcrt1q3yjd0arl7fskt5ynwm6njz4ez63p9c86yeffew
 2$ ./bitcoin-cli -regtest -rpcwallet=first_wallet send '{"bcrt1q3yjd0arl7fskt5ynwm6njz4ez63p9c86yeffew": 1}' null "unset" 1.1
 3{
 4  "txid": "99bfd824edbff565146b9eda6f4370e851f9579c1744b0a54090dce9db6ed393",
 5  "complete": true
 6}
 7$ ./bitcoin-cli -regtest -rpcwallet=first_wallet bumpfee "99bfd824edbff565146b9eda6f4370e851f9579c1744b0a54090dce9db6ed393" '{"extra_outputs": {"bcrt1q3yjd0arl7fskt5ynwm6njz4ez63p9c86yeffew": 30}}'
 8{
 9  "txid": "07df53d9246cfdeec83646469ab1781a73814a4b3223c22d7096d812d9d66289",
10  "origfee": 0.00000156,
11  "fee": 0.00001875,
12  "errors": [
13  ]
14}

    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.

  26. rodentrabies force-pushed on Jun 20, 2022
  27. rodentrabies commented at 10:04 pm on June 20, 2022: contributor
    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.
  28. rodentrabies force-pushed on Jun 21, 2022
  29. ghost commented at 2:49 pm on June 21, 2022: none

    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:

    0-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:

     0$ bitcoin-cli -named sendtoaddress address="tb1p8xrz8rxk0v66f5hfaaqat9qew582vyhnqqdxjvwqz28uvj403v6qx2d0vm" amount=0.0001 replaceable=true
 1
 2acc32a3b2c25b7cbf2582629be891babfcb1f8c139bd03bab075e8ec47f19bd2
 3
 4$ bitcoin-cli bumpfee acc32a3b2c25b7cbf2582629be891babfcb1f8c139bd03bab075e8ec47f19bd2 '{"extra_outputs": {"tb1qrxm8ujt3t62k8k62egqj0sx27k7rtazsl33qrq": 0.0001}}'
 5{
 6  "txid": "429924607dde3d83379fc3501adbbaa2112d4f974cb243d24863e575397171aa",
 7  "origfee": 0.00000212,
 8  "fee": 0.00001129,
 9  "errors": [
10  ]
11}

    image

    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.

  30. rodentrabies commented at 2:58 pm on June 21, 2022: contributor

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

  31. maflcko removed the label Wallet on Jul 1, 2022
  32. maflcko removed the label RPC/REST/ZMQ on Jul 1, 2022
  33. DrahtBot added the label RPC/REST/ZMQ on Jul 1, 2022
  34. DrahtBot added the label Wallet on Jul 1, 2022
  35. ishaanam commented at 2:06 pm on July 11, 2022: contributor
    I agree that the replacing outputs approach as discussed by others above would be better, as that would allow for users to send additional bitcoin to one or multiple of the same addresses as they did in the original transaction without having to create a new output, which seems like a valid use case. This would be cheaper for the user than having a transaction with two outputs that both send bitcoin to the same place as the sender would have to pay less fees.
  36. rodentrabies force-pushed on Jul 11, 2022
  37. rodentrabies renamed this:
    New `extra_outputs` argument for `bumpfee`/`psbtbumpfee`
    New `outputs` argument for `bumpfee`/`psbtbumpfee`
    on Jul 11, 2022
  38. rodentrabies commented at 5:11 pm on July 11, 2022: contributor
    Changed implementation to the outputs argument that replaces original outputs.
  39. rodentrabies force-pushed on Jul 11, 2022
  40. in src/rpc/rawtransaction_util.cpp:73 in b18ab6b9d5 outdated 
    73@@ -83,6 +74,16 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
74 
75         rawTx.vin.push_back(in);
76     }
77+}
78+
79+void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in)
    unknown commented at 5:29 pm on July 11, 2022: 
    0void ReplaceOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in)
    ishaanam commented at 5:49 pm on July 11, 2022:
    I don’t think that this change would make much sense because AddOutputs() is called in ConstructTransaction(), which is usually called in the context of constructing a transaction and adding the inputs and outputs for the first time.
    unknown commented at 6:14 pm on July 11, 2022:

    AddOutputs() is called in ConstructTransaction(), which is usually called in the context of constructing a transaction and adding the inputs and outputs for the first time

    Sorry I didn’t check this. This suggestion can be ignored.

  41. unknown approved
  43. rodentrabies commented at 6:41 pm on July 11, 2022: contributor

    Maybe makes sense to name this argument new_outputs to be as explicit as possible.

    Repeating a review comment for better visibility: maybe the final name of the new argument should be new_outputs instead of just outputs. That way its purpose is clearer, I think.

  44. DrahtBot added the label Needs rebase on Aug 1, 2022
  45. rodentrabies force-pushed on Aug 1, 2022
  46. DrahtBot removed the label Needs rebase on Aug 1, 2022
  48. in src/wallet/feebumper.cpp:157 in 891129f192 outdated 
    153@@ -155,7 +154,7 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid)
154 }
155 
156 Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<bilingual_str>& errors,
157-                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
158+                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, std::vector<CTxOut>& outputs)
    ishaanam commented at 6:34 pm on August 5, 2022:

    nit (here and the header file as well)

    0                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, const std::vector<CTxOut>& outputs)
  49. in src/wallet/feebumper.h:9 in 891129f192 outdated 
    5@@ -6,6 +6,7 @@
6 #define BITCOIN_WALLET_FEEBUMPER_H
7 
8 #include <primitives/transaction.h>
9+#include <wallet/wallet.h>
    ishaanam commented at 6:45 pm on August 5, 2022:
    Why is this moved from feebumper.cpp to feebumper.h?
  50. ishaanam commented at 11:14 pm on August 5, 2022: contributor
    Code review ACK 891129f1920faa2dc4b01b109bb1e73855ff1aea
  51. rodentrabies force-pushed on Aug 6, 2022
  52. ishaanam commented at 2:52 pm on August 6, 2022: contributor
    re-crACK 9201fda51af1b25def45a1825b46a08aa8932db9
  53. unknown approved
  55. in src/wallet/rpc/spend.cpp:991 in 9201fda51a outdated 
    987@@ -968,6 +988,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
988                              "are replaceable).\n"},
989                     {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n"
990                              "\"" + FeeModes("\"\n\"") + "\""},
991+                    {"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "New outputs (key-value pairs) that will replace\n"
    Sjors commented at 4:04 pm on August 8, 2022:
    The PR description suggests outputs are added, but here it says they are replaced. And I think that’s also what the code is doing.
    ishaanam commented at 4:08 pm on August 8, 2022:
    I think the PR description hasn’t been updated, the author changed the implementation such that the outputs are now replaced. #25344 (comment)
  56. in src/wallet/rpc/spend.cpp:993 in b2997036b8 outdated 
    987@@ -988,6 +988,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
988                              "are replaceable).\n"},
989                     {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n"
990                              "\"" + FeeModes("\"\n\"") + "\""},
991+                    {"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "New outputs (key-value pairs) that will replace\n"
992+                             "the original ones, if specified. Each address can only appear once and there can only be\n"
993+                             "one \"data\" object.\n",
    Sjors commented at 4:23 pm on August 8, 2022:
    b2997036b84ed84404601cc4f5e088a9c7a87abd : maybe clarify that the change output does not need to be specified.
  57. in src/wallet/rpc/spend.cpp:1085 in b2997036b8 outdated 
    1054@@ -1048,6 +1055,15 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1055             coin_control.m_signal_bip125_rbf = options["replaceable"].get_bool();
1056         }
1057         SetFeeEstimateMode(*pwallet, coin_control, conf_target, options["estimate_mode"], options["fee_rate"], /*override_min_fee=*/false);
1058+
1059+        // Prepare new outputs by creating a temporary tx and calling AddOutputs().
1060+        if (!options["outputs"].isNull()) {
    Sjors commented at 4:24 pm on August 8, 2022:
    b2997036b84ed84404601cc4f5e088a9c7a87abd: sorry for not testing this, but if I use an empty array for outputs, is it going to send everything to the change address? What if there was no change address? Should we require at least 1 output if this field is present?
    ishaanam commented at 2:18 am on September 1, 2022:
    I did a quick modification of the existing test to test this, and it looks like if someone were to use an empty array for outputs, then the resulting fee-bumped transaction would have the same outputs as the transaction it is replacing, which is what I would expect to happen from looking at the code. I think it would be useful to at least mention somewhere that if no outputs are provided but the field is still present, then the original outputs are used.
    achow101 commented at 8:32 pm on November 21, 2022:

    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.

    achow101 commented at 6:33 pm on January 4, 2023:

    I still think this needs to be addressed, either implementing the suggestion or a comment defending this decision to allow an empty outputs array.

    I think this is the only thing blocking me from ACKing this PR.

    unknown commented at 1:45 am on January 5, 2023:
    Throwing an error when an empty array is used with this argument make sense. Error message could be something like: "Invalid parameter, output argument must not be an empty array"
  58. Sjors commented at 4:29 pm on August 8, 2022: member

    Concept ACK. Needs release note.

    As others have noted this makes it easier to “cancel” a transaction, even though that’s not the goal or documented use case. Similarly it lets a user reduce the amount for a given output if they wanted to do so maliciously. Both these things are already possible and inherent to unconfirmed transactions, but it’s useful to consider.

  59. DrahtBot added the label Needs rebase on Aug 15, 2022
  60. Sjors commented at 1:30 pm on September 6, 2022: member
    Needs a rebase
  61. rodentrabies force-pushed on Sep 7, 2022
  62. DrahtBot removed the label Needs rebase on Sep 7, 2022
  63. Sjors commented at 11:12 am on September 9, 2022: member

    This broke a test, at least on the CI:

     0 test  2022-09-07T14:28:16.710000Z TestFramework (ERROR): Assertion failed 
 1                                   Traceback (most recent call last):
 2                                     File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/test_framework.py", line 133, in main
 3                                       self.run_test()
 4                                     File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/wallet_bumpfee.py", line 86, in run_test
 5                                       test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address)
 6                                     File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/wallet_bumpfee.py", line 186, in test_simple_bumpfee_succeeds
 7                                       assert_equal(bumped_tx["origfee"], -rbftx["fee"])
 8                                     File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/util.py", line 56, in assert_equal
 9                                       raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
10                                   AssertionError: not(0.00070000 == 0.00001000)
  64. rodentrabies commented at 1:35 pm on September 9, 2022: contributor
    I looked into it briefly and didn’t understand the reason. Will look into it more on the weekend.
  65. rodentrabies force-pushed on Sep 15, 2022
  66. rodentrabies force-pushed on Sep 15, 2022
  67. unknown approved
  69. ishaanam commented at 9:52 pm on October 5, 2022: contributor
    reACK 5068940043a4c44bb9e7d3a2d70a12c753ff82e4 I also tested this by replacing the output of a transaction with external inputs.
  70. in src/wallet/rpc/spend.cpp:962 in ab9c422533 outdated 
    930@@ -931,6 +931,26 @@ RPCHelpMan signrawtransactionwithwallet()
931     };
932 }
933 
934+// Definition of allowed formats of specifying transaction outputs in
935+// `send` and `walletcreatefundedpsbt` RPCs.
936+static std::vector<RPCArg> OutputsDoc()
    achow101 commented at 8:24 pm on November 21, 2022:

    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.

  71. in src/wallet/rpc/spend.cpp:1066 in 41464504b7 outdated 
    1061+        if (!options["outputs"].isNull()) {
1062+            CMutableTransaction tempTx;
1063+            AddOutputs(tempTx, options["outputs"]);
1064+            for (const auto& output : tempTx.vout) {
1065+                outputs.push_back(output);
1066+            }
    achow101 commented at 8:27 pm on November 21, 2022:

    In 41464504b74690dac173f0615f8fb2b444d8f0e9 “wallet: add outputs arguments to bumpfee and psbtbumpfee”

    nit: Could be simplified to copying tempTx.vout

    0            outputs = tempTx.vout
  72. in src/rpc/rawtransaction_util.cpp:24 in 41464504b7 outdated 
    20@@ -21,24 +21,15 @@
21 #include <util/strencodings.h>
22 #include <util/translation.h>
23 
24-CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf)
25+void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, const UniValue& locktime, std::optional<bool> rbf)
    achow101 commented at 8:36 pm on November 21, 2022:

    In 41464504b74690dac173f0615f8fb2b444d8f0e9 “wallet: add outputs arguments to bumpfee and psbtbumpfee”

    I don’t think it makes sense to take locktime as a parameter to AddInputs. While the locktime can be relevant to the transaction’s inputs, it doesn’t make sense to be parsing and setting that value when adding the inputs. It would be better to have ConstructTransaction parse and set the locktime, and then AddInputs can just look at rawTx.nLockTime when it sets the inputs.

  73. in test/functional/wallet_bumpfee.py:108 in 5068940043 outdated 
    104@@ -105,18 +105,18 @@ def run_test(self):
105 
106     def test_invalid_parameters(self, rbf_node, peer_node, dest_address):
107         self.log.info('Test invalid parameters')
108-        rbfid = spend_one_input(rbf_node, dest_address)
109+        rbfid = spend_one_input(rbf_node, dest_address, data='deadbeef')
    achow101 commented at 8:49 pm on November 21, 2022:

    In 5068940043a4c44bb9e7d3a2d70a12c753ff82e4 “test: add tests for outputs argument to bumpfee/psbtbumpfee”

    It’s not obvious to me why this test changed? I don’t see how adding a data carrier output to this test is related to the rest of the changes in this PR.

  74. rodentrabies force-pushed on Dec 17, 2022
  75. rodentrabies force-pushed on Jan 14, 2023
  76. rodentrabies force-pushed on Jan 14, 2023
  77. DrahtBot added the label Needs rebase on Jan 17, 2023
  78. wallet: extract and reuse RPC argument format definition for outputs a804f3cfc0
  79. wallet: add `outputs` arguments to `bumpfee` and `psbtbumpfee` c0ebb98382
  80. test: add tests for `outputs` argument to `bumpfee`/`psbtbumpfee` 4c8ecccdcd
  81. rodentrabies force-pushed on Jan 17, 2023
  82. DrahtBot removed the label Needs rebase on Jan 17, 2023
  83. unknown approved
  85. achow101 commented at 8:46 pm on January 26, 2023: member
    ACK 4c8ecccdcd813fac3a7ef6a1493ef3977220421d
  86. maflcko requested review from Sjors on Jan 27, 2023
  87. achow101 requested review from ishaanam on Feb 7, 2023
  88. in test/functional/wallet_bumpfee.py:654 in 4c8ecccdcd 
    650         sequence=MAX_BIP125_RBF_SEQUENCE, **next(u for u in node.listunspent() if u["amount"] == Decimal("0.00100000")))
651     destinations = {dest_address: Decimal("0.00050000")}
652     if change_size > 0:
653         destinations[node.getrawchangeaddress()] = change_size
654+    if data:
655+        destinations['data'] = data
    ishaanam commented at 1:17 am on February 8, 2023:
    Non-blocking: This change is never used.
  89. ishaanam commented at 1:19 am on February 8, 2023: contributor
    reACK 4c8ecccdcd813fac3a7ef6a1493ef3977220421d
  90. achow101 merged this on Feb 16, 2023
  91. achow101 closed this on Feb 16, 2023
  92. ghost commented at 6:59 pm on February 16, 2023: none
    Finally. Thanks @rodentrabies this would improve UX.
  93. rodentrabies deleted the branch on Feb 16, 2023
  94. sidhujag referenced this in commit 058efe5419 on Feb 17, 2023
  95. bitcoin deleted a comment on Feb 17, 2023
  96. bitcoin locked this on Feb 17, 2024


rodentrabies achow101 DrahtBot ghost luke-jr ishaanam Sjors


Sjors

Labels
Wallet RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me