achow101
commented at 9:29 pm on October 21, 2019:
member
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.
achow101 force-pushed
on Oct 21, 2019
fanquake added the label
RPC/REST/ZMQ
on Oct 21, 2019
fanquake added the label
Wallet
on Oct 21, 2019
DrahtBot
commented at 10:25 pm on October 21, 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:
#22019 (wallet: Introduce SelectionResult for encapsulating a coin selection solution by achow101)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
#20640 (wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack)
#19602 (wallet: Migrate legacy wallets to 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.
achow101 force-pushed
on Oct 21, 2019
achow101 force-pushed
on Oct 23, 2019
in
src/wallet/wallet.cpp:1472
in
3445b3b406outdated
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
instagibbs
commented at 2:01 pm on October 24, 2019:
“to ensure a sufficient feer is attained for the requested feerate” ?
achow101
commented at 9:22 pm on October 24, 2019:
Done
in
src/wallet/wallet.cpp:1764
in
3445b3b406outdated
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)));
instagibbs
commented at 2:03 pm on October 24, 2019:
nit: add a reference to txNew.vin[nIn] since it’s used 3 times here
instagibbs
commented at 2:03 pm on October 24, 2019:
txNew.vin[nIn] is used three times here, just set a variable to reference it?
achow101
commented at 9:22 pm on October 24, 2019:
Done
achow101
commented at 9:22 pm on October 24, 2019:
Done
in
src/wallet/rpcwallet.cpp:3132
in
0095c01ffdoutdated
3128@@ -3094,6 +3129,18 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
3129 setSubtractFeeFromOutputs.insert(pos);
3130 }
31313132+ // Fetch specified UTXOs from the UTXO set
instagibbs
commented at 2:12 pm on October 24, 2019:
Not immediately obvious to me why this change is necessary.
Probably requires more extensive comment?
instagibbs
commented at 2:13 pm on October 24, 2019:
In other words, how did selection of inputs work before this?
achow101
commented at 5:47 pm on October 24, 2019:
CTxOuts are needed for fee estimation and knowing the values of the outputs being selected. For inputs the wallet knows about, it would fetch the outputs from the wallet (obviously). Otherwise it would basically ignore them. So to support external inputs, either the user would have to enter the output info (bad UX IMO) or we can fetch it from the UTXO set.
instagibbs
commented at 5:59 pm on October 24, 2019:
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.”
achow101
commented at 9:22 pm on October 24, 2019:
Done
instagibbs
commented at 2:16 pm on October 24, 2019:
member
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.
instagibbs
commented at 6:00 pm on October 24, 2019:
member
in
src/wallet/rpcwallet.cpp:3297
in
7f9aef6e6coutdated
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",
This seems like an advanced feature, so maybe move to “options”?
achow101
commented at 10:57 pm on November 18, 2019:
I don’t think this really belongs as part of options. It’s not like a setting, just additional data needed.
in
src/wallet/wallet.cpp:1335
in
7f9aef6e6coutdated
1331@@ -1332,14 +1332,12 @@ int64_t CWalletTx::GetTxTime() const
13321333 // 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)
luke-jr
commented at 8:39 pm on November 13, 2019:
Why not a reference instead of a pointer?
Is there any reason not to move this to SigningProvider as a method?
achow101
commented at 10:58 pm on November 18, 2019:
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.
achow101 force-pushed
on Nov 18, 2019
DrahtBot added the label
Needs rebase
on Nov 22, 2019
achow101 force-pushed
on Nov 22, 2019
DrahtBot removed the label
Needs rebase
on Nov 22, 2019
DrahtBot added the label
Needs rebase
on Nov 24, 2019
achow101 force-pushed
on Nov 25, 2019
DrahtBot removed the label
Needs rebase
on Nov 25, 2019
DrahtBot added the label
Needs rebase
on Dec 1, 2019
achow101 force-pushed
on Dec 2, 2019
DrahtBot removed the label
Needs rebase
on Dec 2, 2019
instagibbs referenced this in commit
526e802d69
on Dec 4, 2019
DrahtBot added the label
Needs rebase
on Dec 10, 2019
achow101 force-pushed
on Dec 10, 2019
DrahtBot removed the label
Needs rebase
on Dec 10, 2019
DrahtBot added the label
Needs rebase
on Jan 29, 2020
achow101 force-pushed
on Jan 29, 2020
DrahtBot removed the label
Needs rebase
on Jan 29, 2020
DrahtBot added the label
Needs rebase
on Jan 30, 2020
achow101 force-pushed
on Jan 30, 2020
DrahtBot removed the label
Needs rebase
on Jan 30, 2020
DrahtBot added the label
Needs rebase
on Feb 25, 2020
achow101 force-pushed
on Feb 25, 2020
DrahtBot removed the label
Needs rebase
on Feb 25, 2020
DrahtBot added the label
Needs rebase
on Mar 9, 2020
achow101 force-pushed
on Mar 10, 2020
DrahtBot removed the label
Needs rebase
on Mar 10, 2020
DrahtBot added the label
Needs rebase
on Apr 19, 2020
achow101 force-pushed
on Apr 19, 2020
DrahtBot removed the label
Needs rebase
on Apr 19, 2020
DrahtBot added the label
Needs rebase
on May 4, 2020
achow101 force-pushed
on May 4, 2020
DrahtBot removed the label
Needs rebase
on May 4, 2020
DrahtBot added the label
Needs rebase
on Jun 21, 2020
achow101 force-pushed
on Jun 22, 2020
DrahtBot removed the label
Needs rebase
on Jun 22, 2020
DrahtBot added the label
Needs rebase
on Sep 3, 2020
achow101 force-pushed
on Sep 3, 2020
DrahtBot removed the label
Needs rebase
on Sep 3, 2020
achow101 force-pushed
on Sep 30, 2020
in
src/wallet/wallet.cpp:1549
in
1cd3840f7boutdated
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)));
meshcollider
commented at 8:55 am on October 7, 2020:
This is a bit more complicated than it needs to be, you can drop the second coin_control &&
meshcollider
commented at 7:27 pm on October 8, 2020:
contributor
Re-utACK3cdd95425123b9c59d7e5d2c18e5698f837e4220
Only changes were the two minor above.
in
test/functional/rpc_psbt.py:579
in
3cdd954251outdated
496@@ -497,5 +497,21 @@ def test_psbt_input_keys(psbt_input, keys):
497498 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==')
499500+ # 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})
adamjonas
commented at 3:39 pm on October 19, 2020:
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'
achow101
commented at 8:22 pm on November 5, 2020:
I think I’ve fixed this.
adamjonas
commented at 3:34 pm on November 6, 2020:
I ran it 20 times and could not trigger any failures. Looks good.
DrahtBot added the label
Needs rebase
on Nov 4, 2020
achow101 force-pushed
on Nov 4, 2020
DrahtBot removed the label
Needs rebase
on Nov 4, 2020
achow101 force-pushed
on Nov 5, 2020
achow101 force-pushed
on Nov 5, 2020
in
src/wallet/rpcwallet.cpp:3256
in
8674f2b6eboutdated
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"
apoelstra
commented at 5:32 pm on November 11, 2020:
This 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.
achow101
commented at 6:41 pm on November 12, 2020:
I think I’ve resolved this.
in
src/wallet/wallet.cpp:2483
in
2825a72a51outdated
apoelstra
commented at 7:47 pm on November 11, 2020:
I think you need to subtract the coin value from value_to_select. This looks like a rebasing error.
apoelstra
commented at 8:00 pm on November 11, 2020:
Would be good to add a functional test which funds a transaction using external inputs, asserting that the wallet balance would be insufficient without the external inputs. (As in https://github.com/ElementsProject/elements/pull/755/ which is a rough analogue of this PR.)
achow101
commented at 6:42 pm on November 12, 2020:
Should be fixed. Also added a test.
achow101 force-pushed
on Nov 12, 2020
in
src/wallet/coincontrol.h:15
in
cd75670167outdated
achow101
commented at 7:31 pm on January 12, 2021:
Done
in
src/wallet/wallet.cpp:1532
in
cd75670167outdated
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));
jonatack
commented at 11:27 am on November 13, 2020:
achow101
commented at 7:31 pm on January 12, 2021:
Done
in
src/wallet/wallet.cpp:1607
in
cd75670167outdated
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]);
jonatack
commented at 11:37 am on November 13, 2020:
jonatack
commented at 12:13 pm on November 13, 2020:
e2c50aa if you retouch, while here
0- for (const COutPoint& outpoint : vPresetInputs)
1- {
2+ for (const COutPoint& outpoint : vPresetInputs) {
34...
56- if (it != mapWallet.end())
7- {
8+ if (it != mapWallet.end()) {
achow101
commented at 7:31 pm on January 12, 2021:
Done
in
src/wallet/rpcwallet.cpp:4487
in
cd75670167outdated
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",
jonatack
commented at 2:39 pm on November 13, 2020:
6bb343c for each of the new help entries:
can you line break these two sentences and remove the extra line break at the end?
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
achow101
commented at 7:31 pm on January 12, 2021:
Done
in
src/wallet/rpcwallet.cpp:4418
in
cd75670167outdated
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",
jonatack
commented at 2:44 pm on November 13, 2020:
6bb343c s/Note that all/All/ and s/utxo/UTXO/
achow101
commented at 7:31 pm on January 12, 2021:
Done
in
src/wallet/rpcwallet.cpp:3254
in
cd75670167outdated
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"
jonatack
commented at 2:48 pm on November 13, 2020:
6bb343c while making changes here, can you remove this extra space? E.g. s/ or/or/
achow101
commented at 7:32 pm on January 12, 2021:
Done
in
src/wallet/rpcwallet.cpp:4199
in
cd75670167outdated
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);
jonatack
commented at 2:58 pm on November 13, 2020:
6bb343c for easier code comprehension, here and the other FundTransaction calls
jonatack
commented at 3:13 pm on November 13, 2020:
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 }
1920 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 }
2930 if (solving_data.exists("descriptors")) {
31- UniValue desc_strs = solving_data["descriptors"].get_array();
32+ const UniValue desc_strs = solving_data["descriptors"].get_array();
achow101
commented at 7:32 pm on January 12, 2021:
Done
in
src/wallet/rpcwallet.cpp:3361
in
cd75670167outdated
3202@@ -3168,6 +3203,19 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
3203 setSubtractFeeFromOutputs.insert(pos);
3204 }
32053206+ // 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;
jonatack
commented at 3:15 pm on November 13, 2020:
6bb343c add header #include <map>
achow101
commented at 7:32 pm on January 12, 2021:
Done
in
test/functional/rpc_fundrawtransaction.py:927
in
cd75670167outdated
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']]})
jonatack
commented at 3:26 pm on November 13, 2020:
cd7567016 in both rpc_fundrawtransaction.py and rpc_psbt.py, can you please add similar tests using a script and a descriptor, plus test the behavior with invalid ones of each, and a test that hits the “Missing solving data for estimating transaction size” case?
achow101
commented at 7:32 pm on January 12, 2021:
Done.
The Missing solving data for estimating transaction size probably can’t actually be reached.
jonatack
commented at 3:37 pm on November 13, 2020:
member
Approach ACKcd75670167f. Feel free to pick and choose among the comments. Running clang-format on the changes could be helpful. I’d feel more comfortable if the test coverage mentioned below was added.
in
src/wallet/wallet.cpp:2471
in
e2c50aa112outdated
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;
apoelstra
commented at 1:50 am on November 14, 2020:
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.
achow101
commented at 4:10 am on November 14, 2020:
I have some ongoing work to refactor 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.
DrahtBot added the label
Needs rebase
on Nov 17, 2020
achow101 force-pushed
on Nov 17, 2020
DrahtBot removed the label
Needs rebase
on Nov 17, 2020
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
DrahtBot added the label
Needs rebase
on May 26, 2021
achow101 force-pushed
on May 26, 2021
achow101
commented at 4:39 pm on May 26, 2021:
member
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:
DrahtBot added the label
Needs rebase
on May 30, 2021
achow101 force-pushed
on May 30, 2021
DrahtBot removed the label
Needs rebase
on May 30, 2021
DrahtBot added the label
Needs rebase
on Jun 9, 2021
achow101 force-pushed
on Jun 9, 2021
DrahtBot removed the label
Needs rebase
on Jun 9, 2021
DrahtBot added the label
Needs rebase
on Jun 22, 2021
achow101 force-pushed
on Jun 22, 2021
DrahtBot removed the label
Needs rebase
on Jun 23, 2021
t-bast
commented at 1:47 pm on August 12, 2021:
member
If a follow-up PR (or this one) can also enable external inputs in bumpfee and psbtbumpfee you would be my hero :pray:
DrahtBot added the label
Needs rebase
on Aug 16, 2021
achow101 force-pushed
on Aug 17, 2021
DrahtBot removed the label
Needs rebase
on Aug 18, 2021
DrahtBot added the label
Needs rebase
on Sep 3, 2021
achow101 force-pushed
on Sep 3, 2021
DrahtBot removed the label
Needs rebase
on Sep 3, 2021
DrahtBot added the label
Needs rebase
on Sep 28, 2021
achow101 force-pushed
on Sep 28, 2021
DrahtBot removed the label
Needs rebase
on Sep 29, 2021
Allow CInputCoin to also be constructed with COutPoint and CTxOuta00eb388e8
Allow Coin Selection be able to take external inputsd5cfb864ae
achow101 force-pushed
on Sep 29, 2021
t-bast
commented at 8:23 am on October 1, 2021:
member
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.
fanquake requested review from meshcollider
on Oct 1, 2021
fanquake requested review from instagibbs
on Oct 1, 2021
in
src/wallet/rpcwallet.cpp:3297
in
c37d244f61outdated
3289@@ -3289,6 +3290,47 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
3290 coinControl.fAllowWatchOnly = ParseIncludeWatchonly(NullUniValue, wallet);
3291 }
32923293+ 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()));
meshcollider
commented at 6:01 am on October 3, 2021:
Check IsHex before calling ParseHex or use ParseHexV? (Here and below)
in
src/wallet/rpcwallet.cpp:4645
in
c37d244f61outdated
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"},
meshcollider
commented at 6:11 am on October 3, 2021:
This isn’t STR_HEX (same in fundrawtransaction and send)
meshcollider
commented at 6:21 am on October 3, 2021:
contributor
Question: why not put solving_data in options for fundrawtransaction and walletcreatefundedpsbt like you did with send? Preferable to use that unless there’s a good reason?
in
src/wallet/rpcwallet.cpp:3297
in
37ebd04649outdated
3290@@ -3289,6 +3291,47 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
3291 coinControl.fAllowWatchOnly = ParseIncludeWatchonly(NullUniValue, wallet);
3292 }
32933294+ 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) {
yanmaani
commented at 10:00 am on October 3, 2021:
Nit: is there any reason not to use a C++-style loop here? It’s used further on on line 3359 and so on.
in
src/wallet/rpcwallet.cpp:3312
in
37ebd04649outdated
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) {
yanmaani
commented at 10:21 am on October 3, 2021:
Ditto.
in
src/wallet/rpcwallet.cpp:3321
in
37ebd04649outdated
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) {
yanmaani
commented at 10:21 am on October 3, 2021:
Ditto.
in
test/functional/rpc_fundrawtransaction.py:1019
in
37ebd04649outdated
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+
yanmaani
commented at 10:23 am on October 3, 2021:
Some error conditions are not tested, like transactions with no outputs, invalid descriptors, or invalid public keys.
yanmaani
commented at 10:24 am on October 3, 2021:
none
utACK, very good initiative. Here are some minor comments.
allow fundtx rpcs to work with external inputs38f5642ccc
achow101 force-pushed
on Oct 3, 2021
achow101
commented at 4:58 pm on October 3, 2021:
member
Question: why not put solving_data in options for fundrawtransaction and walletcreatefundedpsbt like you did with send? Preferable to use that unless there’s a good reason?
Done
Tests for funding with external inputse39b5a5e7a
allow send rpc take external inputs and solving data928af61cdb
achow101 force-pushed
on Oct 3, 2021
meshcollider
commented at 8:25 pm on October 3, 2021:
contributor
in
test/functional/rpc_fundrawtransaction.py:1020
in
928af61cdb
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
meshcollider
commented at 9:08 am on October 4, 2021:
contributor
Remaining review comments are non-blocking and can be left for a followup.
meshcollider merged this
on Oct 4, 2021
meshcollider closed this
on Oct 4, 2021
sidhujag referenced this in commit
1f3bb2e4f6
on Oct 4, 2021
apoelstra
commented at 3:28 pm on October 4, 2021:
contributor
Thank you for merging this!!
t-bast
commented at 4:35 pm on October 4, 2021:
member
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)?
achow101
commented at 4:54 pm on October 4, 2021:
member
@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?
t-bast
commented at 4:58 pm on October 4, 2021:
member
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!
in
test/functional/rpc_fundrawtransaction.py:1027
in
928af61cdb
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"]]}})
MarcoFalke
commented at 9:21 am on October 5, 2021:
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]
1415 # 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)
1920 # Error conditions
katesalazar
commented at 9:34 am on October 5, 2021:
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