diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index a961a6e5e4..e86bd8d96e 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -145,12 +145,8 @@ void CreatePegInInput(CMutableTransaction& mtx, uint32_t input_idx, Sidechain::B CreatePegInInputInner(mtx, input_idx, tx_btc, merkle_block, claim_scripts, txData, txOutProofData, active_chain_tip); } -CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional rbf, const CBlockIndex* active_chain_tip, std::map* outputs_aux, bool allow_peg_in, bool allow_issuance) +void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, std::optional rbf, const CBlockIndex* active_chain_tip, bool allow_peg_in, bool allow_issuance) { - if (outputs_in.isNull()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output argument must be non-null"); - } - UniValue inputs; if (inputs_in.isNull()) { inputs = UniValue::VARR; @@ -158,17 +154,6 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal inputs = inputs_in.get_array(); } - const UniValue& outputs = outputs_in.get_array(); - - CMutableTransaction rawTx; - - if (!locktime.isNull()) { - int64_t nLockTime = locktime.getInt(); - if (nLockTime < 0 || nLockTime > LOCKTIME_MAX) - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, locktime out of range"); - rawTx.nLockTime = nLockTime; - } - for (unsigned int idx = 0; idx < inputs.size(); idx++) { const UniValue& input = inputs[idx]; const UniValue& o = input.get_obj(); @@ -279,6 +264,16 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal } } +} + +void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in, std::map* outputs_aux) +{ + if (outputs_in.isNull()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output argument must be non-null"); + } + + const bool outputs_is_obj = outputs_in.isObject(); + UniValue outputs = outputs_is_obj ? outputs_in.get_obj() : outputs_in.get_array(); // Keep track of the fee output so we can add it in the very end of the transaction. CTxOut fee_out; @@ -383,6 +378,21 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal outputs_aux->insert(std::make_pair(rawTx.vout[i], psbt_outs[i])); } } +} + +CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional rbf, const CBlockIndex* active_chain_tip, std::map* outputs_aux, bool allow_peg_in, bool allow_issuance) +{ + CMutableTransaction rawTx; + + if (!locktime.isNull()) { + int64_t nLockTime = locktime.getInt(); + if (nLockTime < 0 || nLockTime > LOCKTIME_MAX) + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, locktime out of range"); + rawTx.nLockTime = nLockTime; + } + + AddInputs(rawTx, inputs_in, rbf, active_chain_tip, allow_peg_in, allow_issuance); + AddOutputs(rawTx, outputs_in, outputs_aux); if (rbf.has_value() && rbf.value() && rawTx.vin.size() > 0 && !SignalsOptInRBF(CTransaction(rawTx))) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter combination: Sequence number(s) contradict replaceable option"); diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h index 43fc2e0409..814df6b541 100644 --- a/src/rpc/rawtransaction_util.h +++ b/src/rpc/rawtransaction_util.h @@ -47,11 +47,19 @@ void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const */ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map& coins); +/** Normalize univalue-represented inputs and add them to the transaction */ +void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, bool rbf, const CBlockIndex* active_chain_tip, bool allow_peg_in = true, bool allow_issuance = true); + +/** Normalize univalue-represented outputs and add them to the transaction */ +void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in, std::map* outputs_aux = nullptr); + /** Create a transaction from univalue parameters. If (and only if) output_pubkeys_out is null, the "nonce hack" of storing Confidential Assets output pubkeys in nonces will be used. */ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional rbf, const CBlockIndex* active_chain_tip, std::map* outputs_aux = nullptr, bool allow_peg_in = true, bool allow_issuance = true); +// ELEMENTS + /** Create a peg-in input */ void CreatePegInInput(CMutableTransaction& mtx, uint32_t input_idx, CTransactionRef& tx_btc, CMerkleBlock& merkle_block, const std::set& claim_scripts, const std::vector& txData, const std::vector& txOutProofData, const CBlockIndex* active_chain_tip); void CreatePegInInput(CMutableTransaction& mtx, uint32_t input_idx, Sidechain::Bitcoin::CTransactionRef& tx_btc, Sidechain::Bitcoin::CMerkleBlock& merkle_block, const std::set& claim_scripts, const std::vector& txData, const std::vector& txOutProofData, const CBlockIndex* active_chain_tip); diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 56be84305e..aa02dd513b 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -156,7 +156,7 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid) } Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector& errors, - CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine) + CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector& outputs) { // We are going to modify coin control later, copy to re-use CCoinControl new_coin_control(coin_control); diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index a623377606..9c9d0b000b 100644 --- a/src/wallet/feebumper.h +++ b/src/wallet/feebumper.h @@ -51,7 +51,8 @@ Result CreateRateBumpTransaction(CWallet& wallet, CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, - bool require_mine); + bool require_mine, + const std::vector& outputs); //! Sign the new transaction, //! @return false if the tx couldn't be found or if it was diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 7e3d526680..aaa79ae9ae 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -305,7 +305,8 @@ public: CAmount& new_fee, CMutableTransaction& mtx) override { - return feebumper::CreateRateBumpTransaction(*m_wallet.get(), txid, coin_control, errors, old_fee, new_fee, mtx, /* require_mine= */ true) == feebumper::Result::OK; + std::vector outputs; // just an empty list of new recipients for now + return feebumper::CreateRateBumpTransaction(*m_wallet.get(), txid, coin_control, errors, old_fee, new_fee, mtx, /* require_mine= */ true, outputs) == feebumper::Result::OK; } bool signBumpTransaction(CMutableTransaction& mtx) override { return feebumper::SignTransaction(*m_wallet.get(), mtx); } bool commitBumpTransaction(const uint256& txid, diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index f76cd2dd0d..1a341fa871 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1060,6 +1060,26 @@ RPCHelpMan signrawtransactionwithwallet() }; } +// Definition of allowed formats of specifying transaction outputs in +// `bumpfee`, `psbtbumpfee`, `send` and `walletcreatefundedpsbt` RPCs. +static std::vector OutputsDoc() +{ + return + { + {"", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED, "", + { + {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address,\n" + "the value (float or string) is the amount in " + CURRENCY_UNIT + ""}, + }, + }, + {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", + { + {"data", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A key-value pair. The key must be \"data\", the value is hex-encoded data"}, + }, + }, + }; +} + static RPCHelpMan bumpfee_helper(std::string method_name) { const bool want_psbt = method_name == "psbtbumpfee"; @@ -1096,7 +1116,12 @@ static RPCHelpMan bumpfee_helper(std::string method_name) "still be replaceable in practice, for example if it has unconfirmed ancestors which\n" "are replaceable).\n"}, {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n" - "\"" + FeeModes("\"\n\"") + "\""}, + "\"" + FeeModes("\"\n\"") + "\""}, + {"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "New outputs (key-value pairs) which will replace\n" + "the original ones, if provided. Each address can only appear once and there can\n" + "only be one \"data\" object.\n", + OutputsDoc(), + RPCArgOptions{.skip_type_check = true}}, }, RPCArgOptions{.oneline_description="options"}}, }, @@ -1133,6 +1158,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name) coin_control.fAllowWatchOnly = pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); // optional parameters coin_control.m_signal_bip125_rbf = true; + std::vector outputs; if (!request.params[1].isNull()) { UniValue options = request.params[1]; @@ -1143,6 +1169,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name) {"fee_rate", UniValueType()}, // will be checked by AmountFromValue() in SetFeeEstimateMode() {"replaceable", UniValueType(UniValue::VBOOL)}, {"estimate_mode", UniValueType(UniValue::VSTR)}, + {"outputs", UniValueType()}, // will be checked by AddOutputs() }, true, true); @@ -1156,6 +1183,16 @@ static RPCHelpMan bumpfee_helper(std::string method_name) coin_control.m_signal_bip125_rbf = options["replaceable"].get_bool(); } SetFeeEstimateMode(*pwallet, coin_control, conf_target, options["estimate_mode"], options["fee_rate"], /*override_min_fee=*/false); + + // Prepare new outputs by creating a temporary tx and calling AddOutputs(). + if (!options["outputs"].isNull()) { + if (options["outputs"].isArray() && options["outputs"].empty()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output argument cannot be an empty array"); + } + CMutableTransaction tempTx; + AddOutputs(tempTx, options["outputs"]); + outputs = tempTx.vout; + } } // Make sure the results are valid at least up to the most recent block @@ -1173,7 +1210,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name) CMutableTransaction mtx; feebumper::Result res; // Targeting feerate bump. - res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/ !want_psbt); + res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/ !want_psbt, outputs); if (res != feebumper::Result::OK) { switch(res) { case feebumper::Result::INVALID_ADDRESS_OR_KEY: @@ -1248,18 +1285,7 @@ RPCHelpMan send() {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The 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.", - { - {"", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED, "", - { - {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the address, the value (float or string) is the amount in " + CURRENCY_UNIT + ""}, - }, - }, - {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", - { - {"data", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A key-value pair. The key must be \"data\", the value is hex-encoded data"}, - }, - }, - }, + OutputsDoc(), RPCArgOptions{.skip_type_check = true}}, {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n" diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 4bf68c571b..a5099b75ce 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -83,7 +83,7 @@ class BumpFeeTest(BitcoinTestFramework): self.log.info("Running tests") dest_address = peer_node.getnewaddress() - for mode in ["default", "fee_rate"]: + for mode in ["default", "fee_rate", "new_outputs"]: test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address) self.test_invalid_parameters(rbf_node, peer_node, dest_address) test_segwit_bumpfee_succeeds(self, rbf_node, dest_address) @@ -161,6 +161,14 @@ class BumpFeeTest(BitcoinTestFramework): assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', rbf_node.bumpfee, rbfid, {"estimate_mode": mode}) + self.log.info("Test invalid outputs values") + assert_raises_rpc_error(-8, "Invalid parameter, output argument cannot be an empty array", + rbf_node.bumpfee, rbfid, {"outputs": []}) + assert_raises_rpc_error(-8, "Invalid parameter, duplicated address: " + dest_address, + rbf_node.bumpfee, rbfid, {"outputs": [{dest_address: 0.1}, {dest_address: 0.2}]}) + assert_raises_rpc_error(-8, "Invalid parameter, duplicate key: data", + rbf_node.bumpfee, rbfid, {"outputs": [{"data": "deadbeef"}, {"data": "deadbeef"}]}) + self.clear_mempool() @@ -173,6 +181,10 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address): if mode == "fee_rate": bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"fee_rate": str(NORMAL)}) bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": NORMAL}) + elif mode == "new_outputs": + new_address = peer_node.getnewaddress() + bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"outputs": [{new_address: 0.0003}]}) + bumped_tx = rbf_node.bumpfee(rbfid, {"outputs": [{new_address: 0.0003}]}) else: bumped_psbt = rbf_node.psbtbumpfee(rbfid) bumped_tx = rbf_node.bumpfee(rbfid) @@ -196,6 +208,10 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address): bumpedwtx = rbf_node.gettransaction(bumped_tx["txid"]) assert_equal(oldwtx["replaced_by_txid"], bumped_tx["txid"]) assert_equal(bumpedwtx["replaces_txid"], rbfid) + # if this is a new_outputs test, check that outputs were indeed replaced + if mode == "new_outputs": + assert len(bumpedwtx["details"]) == 1 + # assert bumpedwtx["details"][0]["address"] == new_address # ELEMENTS FIXME: address differs self.clear_mempool() @@ -644,12 +660,14 @@ def test_change_script_match(self, rbf_node, dest_address): self.clear_mempool() -def spend_one_input(node, dest_address, change_size=Decimal("0.0004000")): +def spend_one_input(node, dest_address, change_size=Decimal("0.00040000"), 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.append({node.getrawchangeaddress(): change_size}) + if data: + destinations['data'] = data destinations.append({"fee": Decimal("0.0001")}) rawtx = node.createrawtransaction([tx_input], destinations) signedtx = node.signrawtransactionwithwallet(rawtx)