Currently fundrawtransaction and walletcreatefundedpsbt both do not allow external inputs as the wallet does not have the information necessary to estimate their fees.
This PR adds an additional argument to both those RPCs which allows the user to specify solving data. This way, the wallet can use that solving data to estimate the size of those inputs. The solving data can be public keys, scripts, or descriptors.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
1763 int nIn = 0;
1764 for (const auto& txout : txouts)
1765 {
1766- if (!DummySignInput(txNew.vin[nIn], txout, use_max_sig)) {
1767- return false;
1768+ // Use max sig if watch only inputs were used or if this particular input is an external input
1764 for (const auto& txout : txouts)
1765 {
1766- if (!DummySignInput(txNew.vin[nIn], txout, use_max_sig)) {
1767- return false;
1768+ // Use max sig if watch only inputs were used or if this particular input is an external input
1769+ bool use_max_sig = coin_control && (coin_control->fAllowWatchOnly || (coin_control && coin_control->IsExternalSelected(txNew.vin[nIn].prevout)));
txNew.vin[nIn] since it’s used 3 times here
txNew.vin[nIn] is used three times here, just set a variable to reference it?
3128@@ -3094,6 +3129,18 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
3129 setSubtractFeeFromOutputs.insert(pos);
3130 }
3131
3132+ // Fetch specified UTXOs from the UTXO set
Not immediately obvious to me why this change is necessary.
Probably requires more extensive comment?
oh, duh pubkeys/descriptors aren’t enough :)
Suggested text: “// Fetch specified UTXOs from the UTXO set for knowing the values of the outputs being selected // and to match with the given solving_data. These are only // used when the outputs are not stored in the wallet.”
This needs tests for the scripts, descriptors options in RPC.
Also a simple check that wrong pubkeys don’t also cause things to seem complete.
walletcreatefundedpsbt
3205@@ -3157,6 +3206,25 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
3206 "This boolean should reflect whether the transaction has inputs\n"
3207 "(e.g. fully valid, or on-chain transactions), if known by the caller."
3208 },
3209+ {"solving_data", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "Keys and scripts needed for producing a final transaction with a dummy signature. Used for fee estimation during coin selection.\n",
1331@@ -1332,14 +1332,12 @@ int64_t CWalletTx::GetTxTime() const
1332
1333 // Helper for producing a max-sized low-S low-R signature (eg 71 bytes)
1334 // or a max-sized low-S signature (e.g. 72 bytes) if use_max_sig is true
1335-bool CWallet::DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig) const
1336+static bool DummySignInput(const SigningProvider* provider, CTxIn &tx_in, const CTxOut &txout, bool use_max_sig)
Why not a reference instead of a pointer?
Is there any reason not to move this to SigningProvider as a method?
I changed this to a reference.
I don’t think this really makes sense as part of SigningProvider. SigningProvider is to provide data for signing and is agnostic of what is being signed and how it is being signed. To add this to SigningProvider would violate that. Also, that would imply adding every other signing function to SigningProvider as well, and I don’t think we should do that.
1555- if (!DummySignInput(txNew.vin[nIn], txout, use_max_sig)) {
1556- return false;
1557+ CTxIn& txin = txNew.vin[nIn];
1558+ // Use max sig if watch only inputs were used or if this particular input is an external input
1559+ // to ensure a sufficient fee is attained for the requested feerate.
1560+ bool use_max_sig = coin_control && (coin_control->fAllowWatchOnly || (coin_control && coin_control->IsExternalSelected(txin.prevout)));
coin_control &&
3153+ if (!pubkey.IsFullyValid()) {
3154+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("%s is not a valid public key", pubkey_strs[i].get_str()));
3155+ }
3156+ coinControl.m_external_provider.pubkeys.emplace(pubkey.GetID(), pubkey);
3157+ // Add witnes script for pubkeys
3158+ CScript wit_script = GetScriptForDestination(WitnessV0KeyHash(pubkey.GetID()));
.getID()
Re-utACK 3cdd95425123b9c59d7e5d2c18e5698f837e4220
Only changes were the two minor above.
496@@ -497,5 +497,21 @@ def test_psbt_input_keys(psbt_input, keys):
497
498 assert_raises_rpc_error(-25, 'Inputs missing or spent', self.nodes[0].walletprocesspsbt, 'cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')
499
500+ # Test that we can fund psbts with external inputs specified
501+ ext_utxo = self.nodes[0].listunspent()[0]
502+ addr_info = self.nodes[0].getaddressinfo(ext_utxo['address'])
503+
504+ # An external input without solving data should result in an error
505+ assert_raises_rpc_error(-4, "Missing solving data for estimating transaction size", self.nodes[1].walletcreatefundedpsbt, [{"txid": ext_utxo['txid'], "vout": ext_utxo['vout']}], {self.nodes[0].getnewaddress(): 10 + ext_utxo['amount']}, 0, {'add_inputs': True})
This is causing intermittent failures for me. It’s failing about 20%. Here’s the traceback:
0File "/bitcoin/test/functional/rpc_psbt.py", line 505, in run_test
1 assert_raises_rpc_error(-4, "Missing solving data for estimating transaction size", self.nodes[1].walletcreatefundedpsbt, [{"txid": ext_utxo['txid'], "vout": ext_utxo['vout']}], {self.nodes[0].getnewaddress(): 10 + ext_utxo['amount']}, 0, {'add_inputs': True})
2File "/bitcoin/test/functional/test_framework/util.py", line 124, in assert_raises_rpc_error
3 assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
4File "/bitcoin/test/functional/test_framework/util.py", line 139, in try_rpc
5 raise AssertionError(
6AssertionError: Expected substring not found in error message:
7substring: 'Missing solving data for estimating transaction size'
8error message: 'Insufficient funds'
3231@@ -3184,7 +3232,8 @@ static RPCHelpMan fundrawtransaction()
3232 "Note that inputs which were signed may need to be resigned after completion since in/outputs have been added.\n"
3233 "The inputs added will not be signed, use signrawtransactionwithkey\n"
3234 " or signrawtransactionwithwallet for that.\n"
3235- "Note that all existing inputs must have their previous output transaction be in the wallet.\n"
3236+ "Note that all existing inputs must either have their previous output transaction be in the wallet\n"
3237+ "or be in the utxo set. Solving data must be provided for non-wallet inputs.\n"
signrawtransaction* RPCs.
6bb343c
s/Note that all/All/ -> the very next sentence also begins with “Note that all” (both uses of “Note that” could be omitted)
s/utxo/UTXO/
3045@@ -3046,7 +3046,7 @@ static RPCHelpMan listunspent()
3046 };
3047 }
3048
3049-void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl)
3050+void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue options, CCoinControl& coinControl, const UniValue& solving_data)
options losing its explicit ref typing?
2476@@ -2467,7 +2477,13 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2477 }
2478 setPresetCoins.insert(coin);
2479 } else {
else is for an if(txid is in wallet) … but what we actually want to check is if(outpoint is in wallet). If you try to spend an output you don’t know from a wallet transaction (e.g. by sending coins to a peer then trying to spend their output), this logic will simply fail – it will return false; a few lines above when trying to compute wtx.GetSpendSize on the outpoint.
2476@@ -2467,7 +2477,13 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2477 }
2478 setPresetCoins.insert(coin);
2479 } else {
2480- return false; // TODO: Allow non-wallet inputs
2481+ CTxOut txout;
2482+ if (coin_control.GetExternalOutput(outpoint, txout)) {
2483+ nValueFromPresetInputs += txout.nValue;
2484+ setPresetCoins.insert(CInputCoin(outpoint, txout));
value_to_select. This looks like a rebasing error.
9@@ -10,6 +10,8 @@
10 #include <policy/feerate.h>
11 #include <policy/fees.h>
12 #include <primitives/transaction.h>
13+#include <script/keyorigin.h>
14+#include <script/signingprovider.h>
15 #include <script/standard.h>
16
e2c50aa11 missing headers?
0 #include <script/signingprovider.h>
1 #include <script/standard.h>
2
3+#include <algorithm>
4+#include <map>
5+#include <set>
6+
1558- if (!DummySignInput(txNew.vin[nIn], txout, use_max_sig)) {
1559- return false;
1560+ CTxIn& txin = txNew.vin[nIn];
1561+ // Use max sig if watch only inputs were used or if this particular input is an external input
1562+ // to ensure a sufficient fee is attained for the requested feerate.
1563+ bool use_max_sig = coin_control && (coin_control->fAllowWatchOnly || coin_control->IsExternalSelected(txin.prevout));
e2c50aa
0 const bool use_max_sig = coin_control && (coin_control->fAllowWatchOnly || coin_control->IsExternalSelected(txin.prevout));
1624 const auto mi = wallet->mapWallet.find(input.prevout.hash);
1625 // Can not estimate size without knowing the input details
1626- if (mi == wallet->mapWallet.end()) {
1627+ if (mi != wallet->mapWallet.end()) {
1628+ assert(input.prevout.n < mi->second.tx->vout.size());
1629+ txouts.emplace_back(mi->second.tx->vout[input.prevout.n]);
e2c50aa do you want this to be lookup only?
0 txouts.emplace_back(mi->second.tx->vout.at(input.prevout.n));
2480- if (coin_selection_params.use_bnb) {
2481- value_to_select -= coin.effective_value;
2482- } else {
2483- value_to_select -= coin.txout.nValue;
2484+ input_bytes = wtx.GetSpendSize(outpoint.n, false);
2485+ txout = wtx.tx->vout[outpoint.n];
e2c50aa do we want lookup only or also insertion?
0 txout = wtx.tx->vout.at(outpoint.n);
2459@@ -2445,6 +2460,8 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2460 coin_control.ListSelected(vPresetInputs);
2461 for (const COutPoint& outpoint : vPresetInputs)
2462 {
2463+ int input_bytes = -1;
2464+ CTxOut txout;
2465 std::map<uint256, CWalletTx>::const_iterator it = mapWallet.find(outpoint.hash);
2466 if (it != mapWallet.end())
2467 {
e2c50aa if you retouch, while here
0- for (const COutPoint& outpoint : vPresetInputs)
1- {
2+ for (const COutPoint& outpoint : vPresetInputs) {
3
4...
5
6- if (it != mapWallet.end())
7- {
8+ if (it != mapWallet.end()) {
4454+ {"scripts", RPCArg::Type::ARR, /* default */ "empty array", "A json array of scripts.\n",
4455+ {
4456+ {"script", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "A script"},
4457+ },
4458+ },
4459+ {"descriptors", RPCArg::Type::ARR, /* default */ "empty array", "A json array of descriptors.\n",
6bb343c for each of the new help entries:
0 {"solving_data", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "Keys and scripts needed for producing a final transaction with a dummy signature.\n"
1 "Used for fee estimation during coin selection.",
could you omit all 6 of the “A json array of " text descriptions–the type “json array” is already displayed by the type declaration–and maybe use the gained space to better describe the entry, e.g. a recent help improvement in this file changed a json array of “Outputs” to “Outputs to subtract the fee from, specified as integer indices.”
please remove the “\n” from the end of each entry; this should only be added to intermediary lines, otherwise it causes extra line breaks in the output to be printed
4384@@ -4317,7 +4385,9 @@ static RPCHelpMan walletcreatefundedpsbt()
4385 {
4386 return RPCHelpMan{"walletcreatefundedpsbt",
4387 "\nCreates and funds a transaction in the Partially Signed Transaction format.\n"
4388- "Implements the Creator and Updater roles.\n",
4389+ "Implements the Creator and Updater roles.\n"
4390+ "Note that all existing inputs must either have their previous output transaction be in the wallet\n"
4391+ "or be in the utxo set. Solving data must be provided for non-wallet inputs.\n",
3231@@ -3184,7 +3232,8 @@ static RPCHelpMan fundrawtransaction()
3232 "Note that inputs which were signed may need to be resigned after completion since in/outputs have been added.\n"
3233 "The inputs added will not be signed, use signrawtransactionwithkey\n"
3234 " or signrawtransactionwithwallet for that.\n"
4195@@ -4128,7 +4196,7 @@ static RPCHelpMan send()
4196 // Automatically select coins, unless at least one is manually selected. Can
4197 // be overridden by options.add_inputs.
4198 coin_control.m_add_inputs = rawTx.vin.size() == 0;
4199- FundTransaction(pwallet, rawTx, fee, change_position, options, coin_control);
4200+ FundTransaction(pwallet, rawTx, fee, change_position, options, coin_control, NullUniValue);
6bb343c for easier code comprehension, here and the other FundTransaction calls
0 FundTransaction(pwallet, rawTx, fee, change_position, options, coin_control, /* solving_data */ NullUniValue);
3178+ if (solving_data.exists("descriptors")) {
3179+ UniValue desc_strs = solving_data["descriptors"].get_array();
3180+ for (unsigned int i = 0; i < desc_strs.size(); ++i) {
3181+ FlatSigningProvider desc_out;
3182+ std::string error;
3183+ std::unique_ptr<Descriptor> desc = Parse(desc_strs[i].get_str(), desc_out, error, true);
desc used?
desc
3182+ std::string error;
3183+ std::unique_ptr<Descriptor> desc = Parse(desc_strs[i].get_str(), desc_out, error, true);
3184+ coinControl.m_external_provider = Merge(coinControl.m_external_provider, desc_out);
3185+ }
3186+ }
3187+ }
6bb343c
0 if (!solving_data.isNull()) {
1 if (solving_data.exists("pubkeys")) {
2- UniValue pubkey_strs = solving_data["pubkeys"].get_array();
3+ const UniValue pubkey_strs = solving_data["pubkeys"].get_array();
4 for (unsigned int i = 0; i < pubkey_strs.size(); ++i) {
5- std::vector<unsigned char> data(ParseHex(pubkey_strs[i].get_str()));
6+ const std::vector<unsigned char> data(ParseHex(pubkey_strs[i].get_str()));
7 CPubKey pubkey(data.begin(), data.end());
8 if (!pubkey.IsFullyValid()) {
9 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("%s is not a valid public key", pubkey_strs[i].get_str()));
10 }
11 coinControl.m_external_provider.pubkeys.emplace(pubkey.GetID(), pubkey);
12- // Add witnes script for pubkeys
13- CScript wit_script = GetScriptForDestination(WitnessV0KeyHash(pubkey));
14+ // Add witness script for pubkeys
15+ const CScript wit_script = GetScriptForDestination(WitnessV0KeyHash(pubkey));
16 coinControl.m_external_provider.scripts.emplace(CScriptID(wit_script), wit_script);
17 }
18 }
19
20 if (solving_data.exists("scripts")) {
21- UniValue script_strs = solving_data["scripts"].get_array();
22+ const UniValue script_strs = solving_data["scripts"].get_array();
23 for (unsigned int i = 0; i < script_strs.size(); ++i) {
24- CScript script = ParseScript(script_strs[i].get_str());
25+ const CScript script = ParseScript(script_strs[i].get_str());
26 coinControl.m_external_provider.scripts.emplace(CScriptID(script), script);
27 }
28 }
29
30 if (solving_data.exists("descriptors")) {
31- UniValue desc_strs = solving_data["descriptors"].get_array();
32+ const UniValue desc_strs = solving_data["descriptors"].get_array();
3202@@ -3168,6 +3203,19 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
3203 setSubtractFeeFromOutputs.insert(pos);
3204 }
3205
3206+ // Fetch specified UTXOs from the UTXO set to get the scriptPubKeys and values of the outputs being selected
3207+ // and to match with the given solving_data. Only used for non-wallet outputs.
3208+ std::map<COutPoint, Coin> coins;
#include <map>
891+ # An external input without solving data should result in an error
892+ raw_tx = wallet.createrawtransaction([{"txid": ext_utxo['txid'], "vout": ext_utxo['vout']}], {self.nodes[0].getnewaddress(): 15})
893+ assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx)
894+
895+ # But funding should work when the solving data is provided
896+ funded_tx = wallet.fundrawtransaction(raw_tx, {}, False, {"pubkeys": [addr_info['pubkey']]})
Done.
The Missing solving data for estimating transaction size probably can’t actually be reached.
2486+ }
2487+ if (input_bytes == -1) {
2488+ // The input is external. We either did not find the tx in mapWallet, or we did but couldn't compute the input size with wallet data
2489+ if (!coin_control.GetExternalOutput(outpoint, txout)) {
2490+ // Not ours, and we don't have solving data.
2491+ return false;
This line will prevent you from adding the “Missing solving data for estimating transaction size” test that @jonatack suggests, since SelectCoins fails and the user will instead see “Insufficient funds”.
One hacky fix may be to just return true here without selecting any coins, trusting that CreateTransaction will fail shortly thereafter. Proper fixes might be to have SelectCoins return a richer value (gross) or to use #20040 approach which moves this effective-value computation out of SelectCoins entirely.
CreateTransaction and SelectCoins which will change some order here as well as the return values, so it might be better to push this (entire PR?) back to after that.
107@@ -108,11 +108,13 @@ static const CRPCConvertParam vRPCConvertParams[] =
108 { "combinerawtransaction", 0, "txs" },
109 { "fundrawtransaction", 1, "options" },
110 { "fundrawtransaction", 2, "iswitness" },
111+ { "fundrawtransaction", 3, "solving_data" },
solving_data to send via options.
4705@@ -4587,7 +4706,7 @@ static const CRPCCommand commands[] =
4706 { "wallet", "signrawtransactionwithwallet", &signrawtransactionwithwallet, {"hexstring","prevtxs","sighashtype"} },
4707 { "wallet", "unloadwallet", &unloadwallet, {"wallet_name", "load_on_startup"} },
4708 { "wallet", "upgradewallet", &upgradewallet, {"version"} },
4709- { "wallet", "walletcreatefundedpsbt", &walletcreatefundedpsbt, {"inputs","outputs","locktime","options","bip32derivs"} },
4710+ { "wallet", "walletcreatefundedpsbt", &walletcreatefundedpsbt, {"inputs","outputs","locktime","options","bip32derivs","solving_data"} },
commands, and then rebase
Concept ACK
Compiled on Ubuntu. Not able to test because using wrong syntax or missing something else:
0bitcoin-cli fundrawtransaction "0200000001f7739b1278e8e0df75a1f692fbface31b09d4dc7b4ddb2f20dec698e7617cd310000000000ffffffff0150c30000000000001600147c5ac840b455af9f9b3da2c327275c0b0e985e7600000000" {} false '{"pubkeys" : "02b1f9f65bdf8e686333be0ac8979908ae8aeeee33e396c446ba2b17348a32335c"}'
1error code: -1
2error message:
3JSON value is not an array as expected
Concept ACK
Compiled on Ubuntu. Not able to test because using wrong syntax or missing something else:
0bitcoin-cli fundrawtransaction "0200000001f7739b1278e8e0df75a1f692fbface31b09d4dc7b4ddb2f20dec698e7617cd310000000000ffffffff0150c30000000000001600147c5ac840b455af9f9b3da2c327275c0b0e985e7600000000" {} false '{"pubkeys" : "02b1f9f65bdf8e686333be0ac8979908ae8aeeee33e396c446ba2b17348a32335c"}' 1error code: -1 2error message: 3JSON value is not an array as expected
pubkeys is an array, not a string. The correct command would be:
bitcoin-cli fundrawtransaction "0200000001f7739b1278e8e0df75a1f692fbface31b09d4dc7b4ddb2f20dec698e7617cd310000000000ffffffff0150c30000000000001600147c5ac840b455af9f9b3da2c327275c0b0e985e7600000000" {} false '{"pubkeys" : ["02b1f9f65bdf8e686333be0ac8979908ae8aeeee33e396c446ba2b17348a32335c"]}'
pubkeysis an array, not a string. The correct command would be:0bitcoin-cli fundrawtransaction "0200000001f7739b1278e8e0df75a1f692fbface31b09d4dc7b4ddb2f20dec698e7617cd310000000000ffffffff0150c30000000000001600147c5ac840b455af9f9b3da2c327275c0b0e985e7600000000" {} false '{"pubkeys" : ["02b1f9f65bdf8e686333be0ac8979908ae8aeeee33e396c446ba2b17348a32335c"]}'
Thanks. That worked.
0{
1 "hex": "0200000001f7739b1278e8e0df75a1f692fbface31b09d4dc7b4ddb2f20dec698e7617cd310000000000ffffffff02637e0e000000000016001461c2e2d7dbcfcaa92b686fe90d0dd91870f6e0ce50c30000000000001600147c5ac840b455af9f9b3da2c327275c0b0e985e7600000000",
2 "fee": 0.00000141,
3 "changepos": 0
4}
bumpfee and psbtbumpfee you would be my hero :pray:
Could you please clarify if something is blocking that PR from being merged? Does it depend on a separate feature or refactoring to be accepted? Or is it just a lack of reviews?
This PR would be invaluable for L2 protocols (at least lightning and vaults), as they are currently forced to re-implement a bitcoin wallet internally to work around this limitation (which is quite a waste since bitcoin core already does it well!). This is particularly true for RBF, where L2 protocols currently must re-implement all the RBF logic outside of bitcoin core because transactions have external inputs, which is very sub-optimal.
It would be very helpful to have this available in bitcoin core someday (and similar arguments for the bumpfee RPCs).
I don’t have a good enough knowledge of the wallet code to provide a meaningful review, but I can help test this E2E from a lightning implementation when you feel it’s ready for that step.
3289@@ -3289,6 +3290,47 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
3290 coinControl.fAllowWatchOnly = ParseIncludeWatchonly(NullUniValue, wallet);
3291 }
3292
3293+ if (!solving_data.isNull()) {
3294+ if (solving_data.exists("pubkeys")) {
3295+ const UniValue pubkey_strs = solving_data["pubkeys"].get_array();
3296+ for (unsigned int i = 0; i < pubkey_strs.size(); ++i) {
3297+ const std::vector<unsigned char> data(ParseHex(pubkey_strs[i].get_str()));
IsHex before calling ParseHex or use ParseHexV? (Here and below)
4366@@ -4291,7 +4367,7 @@ static RPCHelpMan send()
4367 // Automatically select coins, unless at least one is manually selected. Can
4368 // be overridden by options.add_inputs.
4369 coin_control.m_add_inputs = rawTx.vin.size() == 0;
4370- FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /* override_min_fee */ false);
4371+ FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /* override_min_fee */ false, NullUniValue /* solving_data */);
4640+ {"script", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "A script"},
4641+ },
4642+ },
4643+ {"descriptors", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Descriptors that provide solving data for this transaction.",
4644+ {
4645+ {"descriptor", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "A descriptor"},
STR_HEX (same in fundrawtransaction and send)
solving_data in options for fundrawtransaction and walletcreatefundedpsbt like you did with send? Preferable to use that unless there’s a good reason?
3290@@ -3289,6 +3291,47 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
3291 coinControl.fAllowWatchOnly = ParseIncludeWatchonly(NullUniValue, wallet);
3292 }
3293
3294+ if (!solving_data.isNull()) {
3295+ if (solving_data.exists("pubkeys")) {
3296+ const UniValue pubkey_strs = solving_data["pubkeys"].get_array();
3297+ for (unsigned int i = 0; i < pubkey_strs.size(); ++i) {
3307+ }
3308+ }
3309+
3310+ if (solving_data.exists("scripts")) {
3311+ const UniValue script_strs = solving_data["scripts"].get_array();
3312+ for (unsigned int i = 0; i < script_strs.size(); ++i) {
3316+ }
3317+ }
3318+
3319+ if (solving_data.exists("descriptors")) {
3320+ const UniValue desc_strs = solving_data["descriptors"].get_array();
3321+ for (unsigned int i = 0; i < desc_strs.size(); ++i) {
1014+ ext_utxo = self.nodes[0].listunspent(addresses=[addr])[0]
1015+
1016+ # An external input without solving data should result in an error
1017+ raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): 15})
1018+ assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx)
1019+
Question: why not put
solving_datainoptionsforfundrawtransactionandwalletcreatefundedpsbtlike you did withsend? Preferable to use that unless there’s a good reason?
Done
Re-utACK 928af61cdb2c4de1c3d10e6fda13bbba5ca0bba9
Only changes are those suggested above.
488+ input_bytes = CalculateMaximumSignedInputSize(txout, &coin_control.m_external_provider, /* use_max_sig */ true);
489+ }
490+
491+ CInputCoin coin(outpoint, txout, input_bytes);
492+ nValueFromPresetInputs += coin.txout.nValue;
493+ if (coin.m_input_bytes <= 0) {
input_bytes == -1 as per above
91 void Select(const COutPoint& output)
92 {
93 setSelected.insert(output);
94 }
95
96+ void Select(const COutPoint& outpoint, const CTxOut& txout)
SelectExternal for grepping
3290@@ -3289,6 +3291,54 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
3291 coinControl.fAllowWatchOnly = ParseIncludeWatchonly(NullUniValue, wallet);
3292 }
3293
3294+ if (options.exists("solving_data")) {
3295+ UniValue solving_data = options["solving_data"].get_obj();
const?
3298+ const std::string& pk_str = pk_univ.get_str();
3299+ if (!IsHex(pk_str)) {
3300+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not hex", pk_str));
3301+ }
3302+ const std::vector<unsigned char> data(ParseHex(pk_str));
3303+ CPubKey pubkey(data.begin(), data.end());
810@@ -788,10 +811,10 @@ static bool CreateTransactionInternal(
811 }
812
813 // Calculate the transaction fee
814- TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly);
815+ TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control);
816 int nBytes = tx_sizes.vsize;
817 if (nBytes < 0) {
-1, then?
1015+
1016+ # An external input without solving data should result in an error
1017+ raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): 15})
1018+ assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx)
1019+
1020+ # Error conditions
utACK 928af61.
What’s crACK btw? (https://github.com/bitcoin/bitcoin/pull/17211#pullrequestreview-769780594)
Thanks for the quick reviews and integration guys! I’m trying to test this E2E and I’m having issues though…
I’m trying to call fundrawtransaction with a tx that spends an external p2wsh input.
I’m providing the witness script in solving_data.scripts.
But I just get an Insufficient funds (code: -4) error returned.
Reading the code, it feels like I didn’t provide the right solving data, but what else should I provide then?
Do I need to provide a complete witness stack with dummy signatures?
Also, for my understanding, how does the size evaluation work at a high-level for p2wsh inputs?
If I provide the witness script in solving_data.scripts that lets bitcoind know this part of the witness stack.
But depending on the actual witness script, bitcoind cannot know how many other items will be pushed to the witness stack to satisfy the script, does it? Does bitcoind simply ignore that (which I can easily take into account by asking for a slightly bigger feerate)?
@t-bast This feature currently only works for scripts that Bitcoin Core would be able to sign too, so you are limited to the set of standard scripts. You also need to provide pubkeys.
We would need to be able to understand arbitrary scripts in order for this to work with any script. In order for that to happen, I think we will need Miniscript to be implemented.
I suppose an alternative would be to allow the user to supply their own input size?
Damn, ok, thanks for the clarification, that makes sense.
I suppose an alternative would be to allow the user to supply their own input size?
That would be perfect for apps that rely on more complex scripts (LN / vaults).
As an external application, I know the size of my final witness stack, so if I could simply provide this size in the solving_data and have bitcoind ignore validation of my inputs, that would be great!
1022+ assert_raises_rpc_error(-5, "'01234567890a0b0c0d0e0f' is not a valid public key", wallet.fundrawtransaction, raw_tx, {"solving_data": {"pubkeys":["01234567890a0b0c0d0e0f"]}})
1023+ assert_raises_rpc_error(-5, "'not a script' is not hex", wallet.fundrawtransaction, raw_tx, {"solving_data": {"scripts":["not a script"]}})
1024+ assert_raises_rpc_error(-8, "Unable to parse descriptor 'not a descriptor'", wallet.fundrawtransaction, raw_tx, {"solving_data": {"descriptors":["not a descriptor"]}})
1025+
1026+ # But funding should work when the solving data is provided
1027+ funded_tx = wallet.fundrawtransaction(raw_tx, {"solving_data": {"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"]]}})
This sometimes fails with “insufficient funds”. I wonder if the following diff fixes it:
0diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
1index f1215915a6..e92d5c8886 100755
2--- a/test/functional/rpc_fundrawtransaction.py
3+++ b/test/functional/rpc_fundrawtransaction.py
4@@ -1009,13 +1009,13 @@ class RawTransactionsTest(BitcoinTestFramework):
5 addr = self.nodes[0].deriveaddresses(desc)[0]
6 addr_info = self.nodes[0].getaddressinfo(addr)
7
8- self.nodes[0].sendtoaddress(addr, 10)
9- self.nodes[0].sendtoaddress(wallet.getnewaddress(), 10)
10+ self.nodes[0].sendtoaddress(addr, .10)
11+ self.nodes[0].sendtoaddress(wallet.getnewaddress(), .10)
12 self.nodes[0].generate(6)
13 ext_utxo = self.nodes[0].listunspent(addresses=[addr])[0]
14
15 # An external input without solving data should result in an error
16- raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): 15})
17+ raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): .15})
18 assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx)
19
20 # Error conditions
I’m seeing this new test_external_inputs test fail on windows in 3 different PRs with an “Insufficient funds” error. Maybe a bug in the new test?
https://cirrus-ci.com/task/5958524342632448?logs=functional_tests#L2395 https://cirrus-ci.com/task/6233803560583168?logs=functional_tests#L2255 https://cirrus-ci.com/task/6423598886813696?logs=functional_tests#L2533
0Traceback (most recent call last):
1 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
2 self.run_test()
3 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\rpc_fundrawtransaction.py", line 137, in run_test
4 self.test_external_inputs()
5 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\rpc_fundrawtransaction.py", line 1027, in test_external_inputs
6 funded_tx = wallet.fundrawtransaction(raw_tx, {"solving_data": {"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"]]}})
7 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\coverage.py", line 49, in __call__
8 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
9 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\authproxy.py", line 144, in __call__
10 raise JSONRPCException(response['error'], status)
11test_framework.authproxy.JSONRPCException: Insufficient funds (-4)