From efc9b85e6f4aa431d308089874a18f0bbdcdd0fd Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 17 Sep 2020 15:06:39 +0200 Subject: [PATCH 1/4] Mark send RPC experimental --- doc/release-notes-16378.md | 5 +++-- src/wallet/rpcwallet.cpp | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/release-notes-16378.md b/doc/release-notes-16378.md index b006ea1a56..958633e780 100644 --- a/doc/release-notes-16378.md +++ b/doc/release-notes-16378.md @@ -1,5 +1,6 @@ RPC --- - A new `send` RPC with similar syntax to `walletcreatefundedpsbt`, including - support for coin selection and a custom fee rate. Using the new `send` method - is encouraged: `sendmany` and `sendtoaddress` may be deprecated in a future release. + support for coin selection and a custom fee rate. The `send` RPC is experimental + and may change in subsequent releases. Using it is encouraged once it's no + longer experimental: `sendmany` and `sendtoaddress` may be deprecated in a future release. diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 62a3206802..0191f8372b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3874,6 +3874,7 @@ static UniValue listlabels(const JSONRPCRequest& request) static RPCHelpMan send() { return RPCHelpMan{"send", + "\nEXPERIMENTAL warning: this call may be changed in future releases.\n" "\nSend a transaction.\n", { {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "a json array with outputs (key-value pairs), where none of the keys are duplicated.\n" From 0fc1c685e1ca68ca8ed2b35f623bbe6a9fc36d66 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 17 Sep 2020 20:57:51 +0200 Subject: [PATCH 2/4] [rpc] send: fix parsing replaceable option --- src/wallet/rpcwallet.cpp | 2 +- test/functional/wallet_send.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 0191f8372b..1503826752 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3995,7 +3995,7 @@ static RPCHelpMan send() int change_position; bool rbf = pwallet->m_signal_rbf; if (options.exists("replaceable")) { - rbf = options["add_to_wallet"].get_bool(); + rbf = options["replaceable"].get_bool(); } CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf); CCoinControl coin_control; diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index b64d2030a4..c5b02ff32a 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -328,8 +328,12 @@ def run_test(self): self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1],add_to_wallet=False) self.log.info("Replaceable...") - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False, replaceable=True) - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=False, replaceable=False) + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=True, replaceable=True) + assert res["complete"] + assert_equal(self.nodes[0].gettransaction(res["txid"])["bip125-replaceable"], "yes") + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=True, replaceable=False) + assert res["complete"] + assert_equal(self.nodes[0].gettransaction(res["txid"])["bip125-replaceable"], "no") self.log.info("Subtract fee from output") self.test_send(from_wallet=w0, to_wallet=w1, amount=1, subtract_fee_from_outputs=[0]) From d813d26f06248aaa7be3c698c87939cc777fafd0 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 17 Sep 2020 15:28:59 +0200 Subject: [PATCH 3/4] [rpc] send: various touch-ups --- src/wallet/rpcwallet.cpp | 24 +++++++++++------------- test/functional/wallet_send.py | 23 ++++++++++++----------- 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 1503826752..e185a1ac05 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2968,7 +2968,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f {"lockUnspents", UniValueType(UniValue::VBOOL)}, {"lock_unspents", UniValueType(UniValue::VBOOL)}, {"locktime", UniValueType(UniValue::VNUM)}, - {"feeRate", UniValueType()}, // will be checked below, + {"feeRate", UniValueType()}, // will be checked below {"psbt", UniValueType(UniValue::VBOOL)}, {"subtractFeeFromOutputs", UniValueType(UniValue::VARR)}, {"subtract_fee_from_outputs", UniValueType(UniValue::VARR)}, @@ -3877,7 +3877,7 @@ static RPCHelpMan send() "\nEXPERIMENTAL warning: this call may be changed in future releases.\n" "\nSend a transaction.\n", { - {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "a json array with outputs (key-value pairs), where none of the keys are duplicated.\n" + {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "A JSON array with outputs (key-value pairs), where none of the keys are duplicated.\n" "That is, each address can only appear once and there can only be one 'data' object.\n" "For convenience, a dictionary, which holds the key-value pairs directly, is also accepted.", { @@ -3909,7 +3909,7 @@ static RPCHelpMan send() {"include_watching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only.\n" "Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n" "e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."}, - {"inputs", RPCArg::Type::ARR, /* default */ "empty array", "Specify inputs instead of adding them automatically. A json array of json objects", + {"inputs", RPCArg::Type::ARR, /* default */ "empty array", "Specify inputs instead of adding them automatically. A JSON array of JSON objects", { {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"}, {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"}, @@ -3919,7 +3919,7 @@ static RPCHelpMan send() {"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"}, {"lock_unspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"}, {"psbt", RPCArg::Type::BOOL, /* default */ "automatic", "Always return a PSBT, implies add_to_wallet=false."}, - {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "A json array of integers.\n" + {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "A JSON array of integers.\n" "The fee will be equally deducted from the amount of each specified output.\n" "Those recipients will receive less bitcoins than you enter in their corresponding amount field.\n" "If no outputs are specified here, the sender pays the fee.", @@ -3943,8 +3943,8 @@ static RPCHelpMan send() }, RPCExamples{"" "\nSend with a fee rate of 1 satoshi per byte\n" - + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.1}' 1 sat/b\n" + - "\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") + + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.1}' 1 sat/b\n") + + "\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" + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.1}' 1 economical '{\"add_to_wallet\": false, \"inputs\": [{\"txid\":\"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0\", \"vout\":1}]}'") }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue @@ -4012,7 +4012,7 @@ static RPCHelpMan send() // Make a blank psbt PartiallySignedTransaction psbtx(rawTx); - // Fill transaction with out data and sign + // Fill transaction with our data and sign bool complete = true; const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, true, false); if (err != TransactionError::OK) { @@ -4024,13 +4024,11 @@ static RPCHelpMan send() UniValue result(UniValue::VOBJ); - // Serialize the PSBT - CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); - ssTx << psbtx; - const std::string result_str = EncodeBase64(ssTx.str()); - if (psbt_opt_in || !complete || !add_to_wallet) { - result.pushKV("psbt", result_str); + // Serialize the PSBT + CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); + ssTx << psbtx; + result.pushKV("psbt", EncodeBase64(ssTx.str())); } if (complete) { diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index c5b02ff32a..4fdfad01c3 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -29,9 +29,9 @@ def skip_test_if_missing_module(self): def test_send(self, from_wallet, to_wallet=None, amount=None, data=None, arg_conf_target=None, arg_estimate_mode=None, - conf_target=None, estimate_mode=None, add_to_wallet=None,psbt=None, - inputs=None,add_inputs=None,change_address=None,change_position=None,change_type=None, - include_watching=None,locktime=None,lock_unspents=None,replaceable=None,subtract_fee_from_outputs=None, + conf_target=None, estimate_mode=None, add_to_wallet=None, psbt=None, + inputs=None, add_inputs=None, change_address=None, change_position=None, change_type=None, + include_watching=None, locktime=None, lock_unspents=None, replaceable=None, subtract_fee_from_outputs=None, expect_error=None): assert (amount is None) != (data is None) @@ -92,13 +92,13 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None, res = from_wallet.send(outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, options=options) else: try: - assert_raises_rpc_error(expect_error[0],expect_error[1],from_wallet.send, - outputs=outputs,conf_target=arg_conf_target,estimate_mode=arg_estimate_mode,options=options) + assert_raises_rpc_error(expect_error[0], expect_error[1], from_wallet.send, + outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, options=options) except AssertionError: # Provide debug info if the test fails self.log.error("Unexpected successful result:") self.log.error(options) - res = from_wallet.send(outputs=outputs,conf_target=arg_conf_target,estimate_mode=arg_estimate_mode,options=options) + res = from_wallet.send(outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, options=options) self.log.error(res) if "txid" in res and add_to_wallet: self.log.error("Transaction details:") @@ -131,7 +131,7 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None, assert tx assert_equal(tx["bip125-replaceable"], "yes" if replaceable else "no") # Ensure transaction exists in the mempool: - tx = from_wallet.getrawtransaction(res["txid"],True) + tx = from_wallet.getrawtransaction(res["txid"], True) assert tx if amount: if subtract_fee_from_outputs: @@ -164,7 +164,7 @@ def run_test(self): self.nodes[1].createwallet(wallet_name="w2") w2 = self.nodes[1].get_wallet_rpc("w2") # w3 is a watch-only wallet, based on w2 - self.nodes[1].createwallet(wallet_name="w3",disable_private_keys=True) + self.nodes[1].createwallet(wallet_name="w3", disable_private_keys=True) w3 = self.nodes[1].get_wallet_rpc("w3") for _ in range(3): a2_receive = w2.getnewaddress() @@ -188,7 +188,7 @@ def run_test(self): self.sync_blocks() # w4 has private keys enabled, but only contains watch-only keys (from w2) - self.nodes[1].createwallet(wallet_name="w4",disable_private_keys=False) + self.nodes[1].createwallet(wallet_name="w4", disable_private_keys=False) w4 = self.nodes[1].get_wallet_rpc("w4") for _ in range(3): a2_receive = w2.getnewaddress() @@ -253,7 +253,7 @@ def run_test(self): self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode="sat/b", expect_error=(-3, "Amount out of range")) # Fee rate of 0.1 satoshi per byte should throw an error - # TODO: error should say 1.000 sat/b + # TODO: error should use sat/b self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode="sat/b", expect_error=(-4, "Fee rate (0.00000100 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)")) @@ -325,7 +325,8 @@ def run_test(self): locked_coins = w0.listlockunspent() assert_equal(len(locked_coins), 1) # Locked coins are automatically unlocked when manually selected - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1],add_to_wallet=False) + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1], add_to_wallet=False) + assert res["complete"] self.log.info("Replaceable...") res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, add_to_wallet=True, replaceable=True) From f7b331ea85d45c7337e527b6e77a45da7a689b7d Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 17 Sep 2020 15:29:32 +0200 Subject: [PATCH 4/4] rpc: add brackets to ConstructTransaction --- src/rpc/rawtransaction_util.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index cfe4575090..8dfbead0e4 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -21,14 +21,16 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, bool rbf) { - if (outputs_in.isNull()) + if (outputs_in.isNull()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output argument must be non-null"); + } UniValue inputs; - if (inputs_in.isNull()) + if (inputs_in.isNull()) { inputs = UniValue::VARR; - else + } else { inputs = inputs_in.get_array(); + } const bool outputs_is_obj = outputs_in.isObject(); UniValue outputs = outputs_is_obj ? outputs_in.get_obj() : outputs_in.get_array();