The ultimate send RPC #16378

pull Sjors wants to merge 3 commits into bitcoin:master from Sjors:2019/07/send changing 8 files +568 −18
  1. Sjors commented at 1:56 pm on July 12, 2019: member

    walletcreatefundedpsbt has some interesting features that sendtoaddress and sendmany don’t have:

    • manual coin selection
    • outputting a PSBT (it was controversial to add this, see #18201)
    • create a transaction without adding to wallet (which leads to broadcasting, unless -walletbroadcast=0)

    At the same time walletcreatefundedpsbt can’t broadcast a transaction, which is inconvenient for simple use cases.

    This PR introduces a new send RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can’t sign all inputs, it outputs a PSBT. If add_to_wallet is set to false it will return the transaction in both PSBT and hex format.

    Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see #16546).

    For bitcoin-cli users, it tries to keep the simplest use case easy to use:

    0bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b
    

    This paves the way for deprecating sendtoaddress and sendmany though there’s no rush. The only missing feature compared to these older methods is adding labels to a destination address.

    Depends on:

    • #16377 ([rpc] don't automatically append inputs in walletcreatefundedpsbt)
    • #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option)
    • #18244 ([rpc] have lockUnspents also lock manually selected coins)
  2. Sjors force-pushed on Jul 12, 2019
  3. fanquake added the label RPC/REST/ZMQ on Jul 12, 2019
  4. in src/wallet/rpcwallet.cpp:4345 in 5136b7316e outdated
    4193+        },
    4194+        RPCResult{
    4195+                    "{\n"
    4196+                    "  \"complete\" : true|false, (boolean) If the transaction has a complete set of signatures\n"
    4197+                    "  \"txid\" :                 (string) The transaction id for the send. Only 1 transaction is created regardless of \n"
    4198+                    "                                      the number of addresses.\n",
    


    instagibbs commented at 2:15 pm on July 12, 2019:
    as noted in my other PR, I think broadcast is the wrong word here. It’s moreso that we do not commit it to the wallet, which also broadcasts it

    Sjors commented at 2:28 pm on July 12, 2019:

    I’m open to other terms, maybe commit? But that seems less clear.

    The background motivation here is that an RPC consumer may need certainty about whether the transaction failed or succeeded. For example this should even work if the RPC connection gets cut before the call returns.

    Just retrying is unsafe. So right now the only option is to check if the wallet contains transactions to the expected destination(s). With this flag, the RPC consumer gets full control and can for example commit the full transaction to some redundant database before broadcasting it.

    (and maybe in future we support wallet-less spend based on just a descriptor with private keys, where there’s no way to know if a previous attempt failed, since you can’t rely on the local mempool for that)


    instagibbs commented at 2:49 pm on July 12, 2019:
    “broadcast” already has meaning to users unfortunately e.g., walletbroadcast startup arg

    Sjors commented at 3:25 pm on July 13, 2019:
    That’s a rather scary startup option imo; forget it once and everything gets broadcast?

    laanwj commented at 12:51 pm on August 1, 2019:

    That’s a rather scary startup option imo; forget it once and everything gets broadcast?

    This is definitely an option, like the proxy configuration, one’d want to have in the bitcoin.conf and not on the command line!

    But also, not necessarily: the idea is that when you have it enabled, you broadcast wallet transactions manually (e.g. by pasting them into the transaction broadcast of a block explorer, or a script such as bitcoin-submittx over tor) so they end up in the mempool or block chain through some other route. There’s no reason for the wallet to broadcast them then.


    Sjors commented at 5:04 pm on August 1, 2019:
    The option I introduce here doesn’t even add the transaction to the wallet; it only returns the hex. So it goes beyond walletbroadcast=0 afaik.

    Sjors commented at 4:34 pm on August 2, 2019:
    I renamed broadcast to add_to_wallet and explained it a bit better in the docs.
  5. DrahtBot commented at 4:18 pm on July 12, 2019: 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:

    • #18788 (tests: Update more tests to work with descriptor wallets by achow101)

    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.

  6. laanwj added the label Wallet on Jul 15, 2019
  7. DrahtBot added the label Needs rebase on Aug 2, 2019
  8. Sjors force-pushed on Aug 2, 2019
  9. DrahtBot removed the label Needs rebase on Aug 2, 2019
  10. luke-jr commented at 0:05 am on August 20, 2019: member
    Suggest moving “inputs” to a field on “options”, and removing PSBT output from send (there is already a RPC for it, as you note).
  11. Sjors commented at 8:56 am on August 21, 2019: member

    @luke-jr agree about moving the inputs. That also makes it easier to rebase on #11413 (explicit fee rate) and have a syntax like send 0.001 BTC bc1... 1 sat/B.

    The reason I added PSBT output is to make multisig easier. When used with a descriptor wallet #15764 you simply call send and it will sign its own part and give you a PSBT to give to the other party (haven’t tested this yet though). With the existing psbt methods that’s more tedious.

  12. Sjors commented at 10:34 am on August 26, 2019: member

    Closing in favor of tweaking sendmany / sendtoaddress:

    • #11413 adds fee rate support (1 sat/b) without the need for an options JSON
    • @luke-jr suggested moving coin selection into an options JSON (it’s a more advanced feature than fee selection, so that’s fine by me)
    • PSBT output can be added when we need it, same for the option to avoid committing transaction to wallet
  13. Sjors closed this on Aug 26, 2019

  14. Sjors reopened this on Feb 24, 2020

  15. Sjors force-pushed on Feb 24, 2020
  16. Sjors commented at 9:28 pm on February 24, 2020: member

    And we’re back! I tried to make the simplest use case, sending coins to a single address with a manual fee in satoshi per byte, easy for bitcoin-cli users. This means you can set conf_target and estimate_mode either in the options dictionary or as a positional argument.

    0bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b
    

    Ideally I’d also get rid of the JSON encoding, but at least this RPC example is easy enough for a new bitcoin-cli user to copy-paste and tweak. And meanwhile GUI PSBT support has made enough progress.

    This needs some cleanup, but I suggest focussing initial review on:

    1. the dependencies: #16377 and #11413 (I’ll have to adjust some things depending on those)
    2. concept ACKs and/or suggestions for the RPC interface @luke-jr suggested:

    Suggest moving “inputs” to a field on “options”

    Done

  17. Sjors force-pushed on Feb 25, 2020
  18. Sjors force-pushed on Feb 25, 2020
  19. Sjors force-pushed on Feb 25, 2020
  20. Sjors force-pushed on Feb 25, 2020
  21. Sjors referenced this in commit aee8d81378 on Feb 25, 2020
  22. Sjors force-pushed on Mar 2, 2020
  23. Sjors force-pushed on Mar 2, 2020
  24. Sjors force-pushed on Mar 2, 2020
  25. Sjors renamed this:
    [WIP] The ultimate send RPC
    The ultimate send RPC
    on Mar 2, 2020
  26. Sjors commented at 5:37 pm on March 2, 2020: member
    This now has enough test coverage that I consider it review worthy, but note that it depends on 3 other pull requests.
  27. Sjors force-pushed on Mar 4, 2020
  28. DrahtBot added the label Needs rebase on Mar 7, 2020
  29. promag commented at 3:07 pm on March 15, 2020: member
    Concept ACK.
  30. Sjors referenced this in commit 24cba79301 on Mar 23, 2020
  31. Sjors referenced this in commit f3450a0186 on Mar 27, 2020
  32. Sjors referenced this in commit 7669405234 on Apr 1, 2020
  33. Sjors referenced this in commit 0ec75f1b32 on Apr 22, 2020
  34. Sjors referenced this in commit 192d1203f8 on Apr 22, 2020
  35. Sjors referenced this in commit 45839a57e5 on Apr 23, 2020
  36. Sjors referenced this in commit 2313f9ab09 on Apr 27, 2020
  37. Sjors referenced this in commit f94e94c37c on Apr 27, 2020
  38. Sjors referenced this in commit 372eef41f0 on Apr 27, 2020
  39. Sjors referenced this in commit 09d61da0fc on Apr 27, 2020
  40. Sjors referenced this in commit 2988c2fda1 on May 7, 2020
  41. Sjors referenced this in commit e8fc838858 on May 18, 2020
  42. Sjors referenced this in commit 87a29a1e68 on May 22, 2020
  43. Sjors referenced this in commit 4b6fb767b2 on Jun 10, 2020
  44. Sjors referenced this in commit fc46eb2202 on Jun 16, 2020
  45. Sjors referenced this in commit 4bb5361ae7 on Jun 18, 2020
  46. Sjors referenced this in commit 29f6508c28 on Jun 19, 2020
  47. Sjors referenced this in commit 42cc844f65 on Jun 19, 2020
  48. meshcollider added this to the "PRs" column in a project

  49. Sjors referenced this in commit b5e02cd2f3 on Jun 25, 2020
  50. Sjors force-pushed on Jun 25, 2020
  51. Sjors commented at 7:01 pm on June 25, 2020: member
    Rebased after #16377 and #11413, it should be easier to review now.
  52. DrahtBot removed the label Needs rebase on Jun 25, 2020
  53. Sjors referenced this in commit 53f13c8467 on Jun 29, 2020
  54. Sjors referenced this in commit 4b02413733 on Jul 2, 2020
  55. Sjors referenced this in commit 6d7475cffa on Jul 6, 2020
  56. Sjors referenced this in commit 9b8ddbcc1c on Jul 14, 2020
  57. Sjors referenced this in commit bef5f38822 on Jul 18, 2020
  58. Sjors referenced this in commit 70b728651d on Jul 30, 2020
  59. Sjors referenced this in commit 7ec42c1748 on Jul 31, 2020
  60. Sjors referenced this in commit f324534295 on Aug 6, 2020
  61. Sjors force-pushed on Aug 7, 2020
  62. Sjors force-pushed on Aug 7, 2020
  63. Sjors force-pushed on Aug 7, 2020
  64. Sjors force-pushed on Aug 7, 2020
  65. Sjors force-pushed on Aug 7, 2020
  66. Sjors commented at 3:37 pm on August 7, 2020: member

    Rebased and tests should pass now. Some things that I think are worth discussing:

    1. CommitTransaction() happily accepts transactions with nLockTime in the future. It’s not clear to me if those eventually get broadcast; in the functional test I had to re-submit it manually. This could be tackled in another PR though.
    2. We return a transaction hash or a PSBT, in part depending on magic. You can opt out of this magic by setting the psbt option to true (which implies add_to_wallet=false, but in addition to the serialised hex it returns a PSBT even for a fully signed transaction)
    3. A hex serialized transaction is only returned when setting add_to_wallet to false, so there’s no magic there. But depending on how (1) is handled, this could change, e.g. it could return a hex for transaction with (far) future nLockTime (not my preference though).
  67. in src/wallet/rpcwallet.cpp:3836 in 15ba29aafe outdated
    3832@@ -3818,6 +3833,186 @@ static UniValue listlabels(const JSONRPCRequest& request)
    3833     return ret;
    3834 }
    3835 
    3836+UniValue send(const JSONRPCRequest& request)
    


    MarcoFalke commented at 12:21 pm on August 14, 2020:
    Might use the non-deprecated constructor. E.g. #19528

    Sjors commented at 4:19 pm on August 28, 2020:
    I switched to the fancy new thing.
  68. in src/wallet/rpcwallet.cpp:3884 in 15ba29aafe outdated
    3879+                    },
    3880+                    {"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"},
    3881+                    {"lock_unspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"},
    3882+                    {"psbt", RPCArg::Type::BOOL,  /* default */ "automatic", "Always return a PSBT, implies add_to_wallet=false."},
    3883+                    {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "A json array of integers.\n"
    3884+                    "                              The fee will be equally deducted from the amount of each specified output.\n"
    


    MarcoFalke commented at 12:22 pm on August 14, 2020:
    0                    "The fee will be equally deducted from the amount of each specified output.\n"
    

    No need for the whitespace

  69. in src/wallet/rpcwallet.cpp:3924 in 15ba29aafe outdated
    3919+        }, true
    3920+    );
    3921+
    3922+    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
    3923+    if (!wallet) return NullUniValue;
    3924+    CWallet* const pwallet = wallet.get();
    


    MarcoFalke commented at 12:22 pm on August 14, 2020:

    As the previous line already checks for nullptr, this can be a reference.

    0    CWallet& wallet = pwallet.get();
    

    Sjors commented at 4:06 pm on August 28, 2020:
    Do you mean const CWallet& pwallet = wallet.get(); ? This creates a const mess when calling FundTransaction (which I’d rather not refactor in this PR).
  70. MarcoFalke commented at 12:23 pm on August 14, 2020: member
    some style-nits to consider next time you rebase
  71. Sjors force-pushed on Aug 28, 2020
  72. Sjors referenced this in commit 5dbd6d683b on Aug 28, 2020
  73. Sjors force-pushed on Aug 31, 2020
  74. Sjors commented at 11:56 am on August 31, 2020: member
    Rebased after #18244 landed.
  75. in src/wallet/rpcwallet.cpp:3915 in 27c07f0866 outdated
    3910+                        },
    3911+                    },
    3912+                },
    3913+            },
    3914+            {"conf_target", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
    3915+            {"estimate_mode", RPCArg::Type::STR, RPCArg::Optional::OMITTED, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
    


    meshcollider commented at 0:33 am on September 7, 2020:

    Because these are Optional::OMITTED, the “default” is actually the description string, so the only output is:

    02. conf_target                           (numeric) wallet default
    13. estimate_mode                         (string) unset
    

    Sjors commented at 6:57 pm on September 7, 2020:

    I dropped the Optional::OMITTED bit, not super intuitive… (cc @MarcoFalke)

    02. conf_target                           (numeric, optional, default=wallet default) Confirmation target (in blocks), or fee rate (for BTC/kB or sat/B estimate modes)
    13. estimate_mode                         (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
    2                                         "unset"
    3                                         "economical"
    4                                         "conservative"
    5                                         "BTC/kB"
    6                                         "sat/B"
    74. options                               (json object, optional)
    
  76. in src/wallet/rpcwallet.cpp:3958 in 27c07f0866 outdated
    3953+        RPCResult{
    3954+            RPCResult::Type::OBJ, "", "",
    3955+                {
    3956+                    {RPCResult::Type::BOOL, "complete", "If the transaction has a complete set of signatures"},
    3957+                    {RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of \n"
    3958+                                                       "the number of addresses.\n"},
    


    meshcollider commented at 0:36 am on September 7, 2020:
    nit: unnecessary newline
  77. meshcollider commented at 0:52 am on September 7, 2020: contributor

    utACK 27c07f08662a8044c729d0adead0511984c40cef

    Only lightly read through the test. Ran the functional tests. LGTM. Just a couple of helptext comments inline. Thanks Sjors!

  78. Sjors referenced this in commit 867727da91 on Sep 7, 2020
  79. [rpc] walletcreatefundedpsbt: allow inputs to be null
    This is of neglible use here, but it allows new RPC methods to take outputs as their first argument and make inputs optional.
    1bc8d0fd59
  80. [rpc] add snake case aliases for transaction methods 2c2a1445dc
  81. Sjors force-pushed on Sep 7, 2020
  82. in src/wallet/rpcwallet.cpp:3999 in 62d3523e21 outdated
    3994+            int change_position;
    3995+            bool rbf = pwallet->m_signal_rbf;
    3996+            const UniValue &replaceable_arg = options["replaceable"];
    3997+            if (!replaceable_arg.isNull()) {
    3998+                RPCTypeCheckArgument(replaceable_arg, UniValue::VBOOL);
    3999+                rbf = replaceable_arg.isTrue();
    


    meshcollider commented at 2:41 am on September 10, 2020:
    Why is this check so much more involved than just if exists() and get_bool() like you do with add_to_wallet below?

    Sjors commented at 11:44 am on September 10, 2020:

    Because I blindly copied it from walletcreatefundedpsbt :-)

    Replaced with simpler version.

  83. [rpc] add send method 92326d8976
  84. Sjors force-pushed on Sep 10, 2020
  85. in src/wallet/rpcwallet.cpp:3946 in 92326d8976
    3941+                }
    3942+        },
    3943+        RPCExamples{""
    3944+        "\nSend with a fee rate of 1 satoshi per byte\n"
    3945+        + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.1}' 1 sat/b\n" +
    3946+            "\nCreate a transaction that should confirm the next block, with a specific input, and return result without adding to wallet or broadcasting to the network\n")
    


    ShaunApps commented at 1:13 pm on September 10, 2020:
    nit: parenthesis ‘)’ in wrong place, enclosing HelpExampleCli and next example string

    Sjors commented at 12:58 pm on September 17, 2020:
    Thanks, adding fix to followup PR.
  86. ShaunApps commented at 1:14 pm on September 10, 2020: none

    ACK.

    Ran functional tests. Played w/ command in cli. Left a nit

  87. laanwj added this to the "Blockers" column in a project

  88. meshcollider commented at 0:40 am on September 11, 2020: contributor

    Light re-utACK 92326d89766155a792254d30a9962251b8fc7799

    (still haven’t read the test carefully)

  89. in src/wallet/rpcwallet.cpp:2971 in 92326d8976
    2967+                {"inputs", UniValueType(UniValue::VARR)},
    2968                 {"lockUnspents", UniValueType(UniValue::VBOOL)},
    2969                 {"lock_unspents", UniValueType(UniValue::VBOOL)},
    2970-                {"feeRate", UniValueType()}, // will be checked below
    2971+                {"locktime", UniValueType(UniValue::VNUM)},
    2972+                {"feeRate", UniValueType()}, // will be checked below,
    


    achow101 commented at 0:44 am on September 15, 2020:
    nit: Extra trailing comma

    Sjors commented at 12:59 pm on September 17, 2020:
    Thanks, fixing nits in followup.
  90. in src/wallet/rpcwallet.cpp:4014 in 92326d8976
    4009+            }
    4010+
    4011+            // Make a blank psbt
    4012+            PartiallySignedTransaction psbtx(rawTx);
    4013+
    4014+            // Fill transaction with out data and sign
    


    achow101 commented at 1:06 am on September 15, 2020:
    nit: s/out/our
  91. in src/wallet/rpcwallet.cpp:4027 in 92326d8976
    4022+            complete = FinalizeAndExtractPSBT(psbtx, mtx);
    4023+
    4024+            UniValue result(UniValue::VOBJ);
    4025+
    4026+            // Serialize the PSBT
    4027+            CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
    


    achow101 commented at 1:08 am on September 15, 2020:
    Seems like this serialization should only occur if we are actually going to return the psbt, so move it inside the next if block?
  92. in test/functional/wallet_send.py:33 in 92326d8976
    28+        self.skip_if_no_wallet()
    29+
    30+    def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
    31+                  arg_conf_target=None, arg_estimate_mode=None,
    32+                  conf_target=None, estimate_mode=None, add_to_wallet=None,psbt=None,
    33+                  inputs=None,add_inputs=None,change_address=None,change_position=None,change_type=None,
    


    achow101 commented at 1:10 am on September 15, 2020:
    nit: spaces after each arg, here and below

    Sjors commented at 1:13 pm on September 17, 2020:
    ,[a-zA-Z0-9]+ found a bunch more
  93. in src/wallet/rpcwallet.cpp:3901 in 92326d8976
    3896+            {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
    3897+                        "       \"" + FeeModes("\"\n\"") + "\""},
    3898+            {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
    3899+                {
    3900+                    {"add_inputs", RPCArg::Type::BOOL, /* default */ "false", "If inputs are specified, automatically include more if they are not enough."},
    3901+                    {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns a serialized transaction which will not be added to the wallet or broadcast"},
    


    achow101 commented at 1:15 am on September 15, 2020:
    Seems like it would be better to call this broadcast rather than add_to_wallet.

    Sjors commented at 1:14 pm on September 17, 2020:
    Maybe, but this also works when walletbroadcast is off
  94. in test/functional/wallet_send.py:256 in 92326d8976
    251+        fee = self.nodes[1].decodepsbt(res["psbt"])["fee"]
    252+        assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00002"))
    253+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode="sat/b",
    254+                       expect_error=(-3, "Amount out of range"))
    255+        # Fee rate of 0.1 satoshi per byte should throw an error
    256+        # TODO: error should say 1.000 sat/b
    


    achow101 commented at 1:21 am on September 15, 2020:
    I think that’s supposed to be 0.1, not 1.000
  95. achow101 commented at 1:33 am on September 15, 2020: member

    ACK 92326d89766155a792254d30a9962251b8fc7799 Reviewed code and test, ran tests.

    Just some nits and non-blocking comments.

  96. in src/rpc/rawtransaction_util.cpp:24 in 1bc8d0fd59 outdated
    20@@ -21,10 +21,15 @@
    21 
    22 CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, bool rbf)
    23 {
    24-    if (inputs_in.isNull() || outputs_in.isNull())
    25-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, arguments 1 and 2 must be non-null");
    26+    if (outputs_in.isNull())
    


    kallewoof commented at 2:19 am on September 15, 2020:

    [rpc] walletcreatefundedpsbt: allow inputs to be null:

    Style-nit: add {} while at it, since you’re touching this line.


    Sjors commented at 1:17 pm on September 17, 2020:
    Will change in the followup in a separate commit, in case people think it’s too late to touch.
  97. in src/rpc/rawtransaction_util.cpp:28 in 1bc8d0fd59 outdated
    25-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, arguments 1 and 2 must be non-null");
    26+    if (outputs_in.isNull())
    27+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output argument must be non-null");
    28+
    29+    UniValue inputs;
    30+    if (inputs_in.isNull())
    


    kallewoof commented at 2:19 am on September 15, 2020:

    [rpc] walletcreatefundedpsbt: allow inputs to be null:

    Add {}s.

  98. in src/wallet/rpcwallet.cpp:3885 in 92326d8976
    3880+                    "That is, each address can only appear once and there can only be one 'data' object.\n"
    3881+                    "For convenience, a dictionary, which holds the key-value pairs directly, is also accepted.",
    3882+                {
    3883+                    {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
    3884+                        {
    3885+                            {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT + ""},
    


    kallewoof commented at 2:36 am on September 15, 2020:
    RPCArg::Type::AMOUNT is not a key-value pair. I think you mean RPCArg::Type::OBJ.
  99. in src/wallet/rpcwallet.cpp:3890 in 92326d8976
    3885+                            {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT + ""},
    3886+                        },
    3887+                        },
    3888+                    {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
    3889+                        {
    3890+                            {"data", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A key-value pair. The key must be \"data\", the value is hex-encoded data"},
    


    kallewoof commented at 2:38 am on September 15, 2020:
    RPCArg::Type::STR_HEX is not a key-value pair either. I think you mean RPCArg::Type::OBJ here too.

    Sjors commented at 7:23 pm on September 17, 2020:
    I think the type is correct, see also walletcreatefundedpsbt, but it’s confusing (in the code) that the wrapping object is described next to the data field. cc @achow101 / @MarcoFalke thoughts?

    achow101 commented at 7:34 pm on September 17, 2020:
    The type is correct because it is describing the type of the value in this key-value pair. The weirdness is that this output specification is the almost the only place where the key is not fixed but user defined.
  100. in src/wallet/rpcwallet.cpp:3906 in 92326d8976
    3901+                    {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns a serialized transaction which will not be added to the wallet or broadcast"},
    3902+                    {"change_address", RPCArg::Type::STR_HEX, /* default */ "pool address", "The bitcoin address to receive the change"},
    3903+                    {"change_position", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"},
    3904+                    {"change_type", RPCArg::Type::STR, /* default */ "set by -changetype", "The output type to use. Only valid if change_address is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."},
    3905+                    {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
    3906+                    {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
    


    kallewoof commented at 2:40 am on September 15, 2020:
    Why are there two conf_target and estimate_mode parameters?

    Sjors commented at 1:20 pm on September 17, 2020:
    For convenience; setting the fee rate is so common I’d rather not have to use the options field.
  101. in src/wallet/rpcwallet.cpp:4009 in 92326d8976
    4004+            FundTransaction(pwallet, rawTx, fee, change_position, options, coin_control);
    4005+
    4006+            bool add_to_wallet = true;
    4007+            if (options.exists("add_to_wallet")) {
    4008+                add_to_wallet = options["add_to_wallet"].get_bool();
    4009+            }
    


    kallewoof commented at 2:44 am on September 15, 2020:

    Alternative:

    0bool add_to_wallet = !options.exists("add_to_wallet") || options["add_to_wallet"].get_bool();
    

    Sjors commented at 1:20 pm on September 17, 2020:
    I’ll leave this to your #19957

    kallewoof commented at 0:12 am on September 18, 2020:
    Feels out of scope for #19957, and not a big deal anyway so leaving it be.
  102. kallewoof commented at 2:47 am on September 15, 2020: member
    utACK 92326d89766155a792254d30a9962251b8fc7799
  103. meshcollider commented at 2:51 am on September 15, 2020: contributor
    Nits can be left for followup, let’s get this in to move #16546 forward 🚀
  104. meshcollider merged this on Sep 15, 2020
  105. meshcollider closed this on Sep 15, 2020

  106. sidhujag referenced this in commit 2d9d86f6b8 on Sep 15, 2020
  107. meshcollider moved this from the "PRs" to the "Done" column in a project

  108. meshcollider removed this from the "Blockers" column in a project

  109. in src/wallet/rpcwallet.cpp:2980 in 2c2a1445dc outdated
    2976@@ -2972,22 +2977,24 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
    2977             coinControl.m_add_inputs = options["add_inputs"].get_bool();
    2978         }
    2979 
    2980-        if (options.exists("changeAddress")) {
    2981-            CTxDestination dest = DecodeDestination(options["changeAddress"].get_str());
    2982+        if (options.exists("changeAddress") || options.exists("change_address")) {
    


    fjahr commented at 1:58 pm on September 15, 2020:
    Would be more robust to throw an error if both options are set.

    Sjors commented at 1:21 pm on September 17, 2020:
    I believe that’s covered in #19957
  110. in src/wallet/rpcwallet.cpp:3879 in 92326d8976
    3870@@ -3866,6 +3871,185 @@ static UniValue listlabels(const JSONRPCRequest& request)
    3871     return ret;
    3872 }
    3873 
    3874+static RPCHelpMan send()
    3875+{
    3876+    return RPCHelpMan{"send",
    3877+        "\nSend a transaction.\n",
    3878+        {
    3879+            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "a json array with outputs (key-value pairs), where none of the keys are duplicated.\n"
    


    fjahr commented at 2:01 pm on September 15, 2020:
    nit: JSON, Capitalize A

    Sjors commented at 1:22 pm on September 17, 2020:
    I’ll address nits in the followup PR
  111. in src/wallet/rpcwallet.cpp:3921 in 92326d8976
    3916+                        },
    3917+                    },
    3918+                    {"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"},
    3919+                    {"lock_unspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"},
    3920+                    {"psbt", RPCArg::Type::BOOL,  /* default */ "automatic", "Always return a PSBT, implies add_to_wallet=false."},
    3921+                    {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "A json array of integers.\n"
    


    fjahr commented at 2:04 pm on September 15, 2020:
    nit: JSON
  112. in src/wallet/rpcwallet.cpp:3911 in 92326d8976
    3906+                    {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
    3907+            "       \"" + FeeModes("\"\n\"") + "\""},
    3908+                    {"include_watching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only.\n"
    3909+                                          "Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n"
    3910+                                          "e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."},
    3911+                    {"inputs", RPCArg::Type::ARR, /* default */ "empty array", "Specify inputs instead of adding them automatically. A json array of json objects",
    


    fjahr commented at 2:05 pm on September 15, 2020:
    nit: JSON
  113. in src/wallet/rpcwallet.cpp:3997 in 92326d8976
    3992+
    3993+            CAmount fee;
    3994+            int change_position;
    3995+            bool rbf = pwallet->m_signal_rbf;
    3996+            if (options.exists("replaceable")) {
    3997+                rbf = options["add_to_wallet"].get_bool();
    


    fjahr commented at 2:09 pm on September 15, 2020:
    This seems like an bug? s/“add_to_wallet”/“replaceable”/

    fjahr commented at 2:12 pm on September 15, 2020:

    nit: Maybe this and comparable sections could be compressed with ternaries as well

    0const bool rbf = options.exists("replaceable") ? options["add_to_wallet"].get_bool() : pwallet->m_signal_rbf;
    

    EDIT: or see kalle’s suggestion…


    Sjors commented at 1:23 pm on September 17, 2020:
    Hopefully addressed in #19957?

    fjahr commented at 4:32 pm on September 17, 2020:

    Not that I see… I am not sure if I am missing something or if you missed the point of my initial (non-nit) comment :) But now I seem to be able to make code suggestions again (didn’t work before for some reason), this is what I meant with “this seems like a bug”:

    0                rbf = options["replaceable"].get_bool();
    

    Sjors commented at 6:42 pm on September 17, 2020:
    I think I got distracted by your second comment. Will fix in #19969
  114. in test/functional/wallet_send.py:330 in 92326d8976
    325+        locked_coins = w0.listlockunspent()
    326+        assert_equal(len(locked_coins), 1)
    327+        # Locked coins are automatically unlocked when manually selected
    328+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1],add_to_wallet=False)
    329+
    330+        self.log.info("Replaceable...")
    


    fjahr commented at 2:26 pm on September 15, 2020:
    The assert if the replaceable option was actually set is only done if the tx is added to the wallet, so I think these tests can not fail.

    Sjors commented at 1:28 pm on September 17, 2020:
    Added the check here in two other places just in case, though test_send should probably be refactored to check this unless it’s explicitly expected to fail.
  115. in test/functional/wallet_send.py:328 in 92326d8976
    323+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1], add_to_wallet=False, lock_unspents=True)
    324+        assert res["complete"]
    325+        locked_coins = w0.listlockunspent()
    326+        assert_equal(len(locked_coins), 1)
    327+        # Locked coins are automatically unlocked when manually selected
    328+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1],add_to_wallet=False)
    


    fjahr commented at 2:29 pm on September 15, 2020:
    micro-nit: spacing before add_to_wallet
  116. Sjors deleted the branch on Sep 15, 2020
  117. fjahr commented at 4:03 pm on September 15, 2020: member

    post-merge Concept ACK

    Feel free to ignore my nits, even in a follow-up. But the replaceable issue needs to be addressed afaict.

  118. Sjors commented at 1:31 pm on September 17, 2020: member
    Nits addressed in #19969, where I’m also declaring this RPC experimental.
  119. fanquake referenced this in commit e36aa351a3 on Sep 29, 2020
  120. sidhujag referenced this in commit 4793d4ff1f on Sep 29, 2020
  121. Fabcien referenced this in commit 9804792410 on Oct 7, 2021
  122. Fabcien referenced this in commit 3ed98b036f on Oct 7, 2021
  123. Fabcien referenced this in commit d044621ed4 on Oct 7, 2021
  124. DrahtBot locked this on Feb 15, 2022

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-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me