Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

achow101 · 2019-10-21T21:29:04Z

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.

DrahtBot · 2019-10-21T22:25:40Z

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)
#21283 (Implement BIP 370 PSBTv2 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.

instagibbs

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.

src/wallet/wallet.cpp

src/wallet/rpcwallet.cpp

instagibbs

RPC help also needs to be updated to reflect the new functionality. For example: https://github.com/bitcoin/bitcoin/pull/17211/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR3167

achow101 · 2019-10-24T21:24:30Z

Updated the help

instagibbs · 2019-10-25T18:47:09Z

can you also reflect the updated help into walletcreatefundedpsbt verbatim? It's helpful for usage.

achow101 · 2019-10-29T19:08:13Z

Also updated the help of walletcreatefundedpsbt

instagibbs · 2019-10-31T14:23:54Z

not compiling

instagibbs · 2019-11-04T15:13:23Z

code review ACK 7f9aef6

luke-jr · 2019-11-04T16:24:24Z

src/wallet/rpcwallet.cpp

@@ -3157,6 +3206,25 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
                        "This boolean should reflect whether the transaction has inputs\n"
                        "(e.g. fully valid, or on-chain transactions), if known by the caller."
                    },
+                    {"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"?

I don't think this really belongs as part of options. It's not like a setting, just additional data needed.

luke-jr · 2019-11-13T20:39:21Z

src/wallet/wallet.cpp

@@ -1332,14 +1332,12 @@ int64_t CWalletTx::GetTxTime() const

 // Helper for producing a max-sized low-S low-R signature (eg 71 bytes)
 // or a max-sized low-S signature (e.g. 72 bytes) if use_max_sig is true
-bool CWallet::DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig) const
+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.

achow101 · 2021-10-03T16:58:02Z

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

meshcollider

Re-utACK 928af61

Only changes are those suggested above.

instagibbs

crACK 928af61

instagibbs · 2021-10-04T01:10:41Z

src/wallet/spend.cpp

+
+        CInputCoin coin(outpoint, txout, input_bytes);
+        nValueFromPresetInputs += coin.txout.nValue;
+        if (coin.m_input_bytes <= 0) {


nit: we should probably keep error checking conditions the same, input_bytes == -1 as per above

instagibbs · 2021-10-04T01:16:04Z

src/wallet/coincontrol.h

    void Select(const COutPoint& output)
    {
        setSelected.insert(output);
    }

+    void Select(const COutPoint& outpoint, const CTxOut& txout)


Would rather it be named the more obvious SelectExternal for grepping

ghost · 2021-10-04T04:55:52Z

reACK 928af61

nit: agree with #17211 (comment)

yanmaani

utACK 928af61.

What's crACK btw? (#17211 (review))

yanmaani · 2021-10-04T04:50:58Z

src/wallet/rpcwallet.cpp

@@ -3289,6 +3291,54 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
        coinControl.fAllowWatchOnly = ParseIncludeWatchonly(NullUniValue, wallet);
    }

+    if (options.exists("solving_data")) {
+        UniValue solving_data = options["solving_data"].get_obj();


Any reason for this not to be const?

yanmaani · 2021-10-04T04:51:22Z

src/wallet/rpcwallet.cpp

+                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not hex", pk_str));
+                }
+                const std::vector<unsigned char> data(ParseHex(pk_str));
+                CPubKey pubkey(data.begin(), data.end());


yanmaani · 2021-10-04T04:57:48Z

src/wallet/spend.cpp

@@ -788,10 +811,10 @@ static bool CreateTransactionInternal(
    }

    // Calculate the transaction fee
-    TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly);
+    TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control);
    int nBytes = tx_sizes.vsize;
    if (nBytes < 0) {


nit: Shouldn't that likewise be -1, then?

yanmaani · 2021-10-04T05:02:19Z

test/functional/rpc_fundrawtransaction.py

+        raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): 15})
+        assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx)
+
+        # Error conditions


I think you forgot "TX must have at least one output"?

doesn't seem related to the feature?

meshcollider · 2021-10-04T09:08:04Z

Remaining review comments are non-blocking and can be left for a followup.

…sbt to …

apoelstra · 2021-10-04T15:28:45Z

Thank you for merging this!!

t-bast · 2021-10-04T16:35:30Z

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 · 2021-10-04T16:54:05Z

@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 · 2021-10-04T16:58:56Z

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!

maflcko · 2021-10-05T09:21:52Z

test/functional/rpc_fundrawtransaction.py

+        assert_raises_rpc_error(-8, "Unable to parse descriptor 'not a descriptor'", wallet.fundrawtransaction, raw_tx, {"solving_data": {"descriptors":["not a descriptor"]}})
+
+        # But funding should work when the solving data is provided
+        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:

diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index f1215915a6..e92d5c8886 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -1009,13 +1009,13 @@ class RawTransactionsTest(BitcoinTestFramework): addr = self.nodes[0].deriveaddresses(desc)[0] addr_info = self.nodes[0].getaddressinfo(addr) - self.nodes[0].sendtoaddress(addr, 10) - self.nodes[0].sendtoaddress(wallet.getnewaddress(), 10) + self.nodes[0].sendtoaddress(addr, .10) + self.nodes[0].sendtoaddress(wallet.getnewaddress(), .10) self.nodes[0].generate(6) ext_utxo = self.nodes[0].listunspent(addresses=[addr])[0] # An external input without solving data should result in an error - raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): 15}) + raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): .15}) assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx) # Error conditions

Forced push 86c6620 (#23100) on top of 446b706, which doesn't include this change added in e39b5a5... 🤔

@MarcoFalke if I had to guess, that would fail due to paying extremely high fee?

fundrawtransaction adjusts the feerate by adjusting the change output

Forced push 86c6620 (#23100) on top of 446b706, which doesn't include this change added in e39b5a5... 🤔

Right now the only successful run of the last 15 or 20 runs is that one. 😄

ryanofsky · 2021-10-05T21:55:49Z

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

Traceback (most recent call last):
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
    self.run_test()
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\rpc_fundrawtransaction.py", line 137, in run_test
    self.test_external_inputs()
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\rpc_fundrawtransaction.py", line 1027, in test_external_inputs
    funded_tx = wallet.fundrawtransaction(raw_tx, {"solving_data": {"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"]]}})
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\coverage.py", line 49, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\authproxy.py", line 144, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Insufficient funds (-4)

…rawtransaction.py 75a9305 Fix intermittent test failures due to missing sync_all (Samuel Dobson) eb02dbb Use self.generate not node.generate throughout tests (Samuel Dobson) Pull request description: After #17211 was merged, there have been a few intermittent test failures in `wallet_send.py`, `rpc_fundrawtransaction.py`, and `rpc_psbt.py` I believe all these failures are due to these missing `sync_all()`s ACKs for top commit: achow101: ACK 75a9305 lsilva01: Tested ACK 75a9305 on Ubuntu 20.04 Tree-SHA512: 91de5763664046e5a35802eb1a9a28f69a1a27d78d26c9fa0024bcfab4ccb4b7ad4feebea7de4b19e141db9c39270c623c57c47942a48dfe679fdc8cad70df43

…pc_fundrawtransaction.py 75a9305 Fix intermittent test failures due to missing sync_all (Samuel Dobson) eb02dbb Use self.generate not node.generate throughout tests (Samuel Dobson) Pull request description: After bitcoin#17211 was merged, there have been a few intermittent test failures in `wallet_send.py`, `rpc_fundrawtransaction.py`, and `rpc_psbt.py` I believe all these failures are due to these missing `sync_all()`s ACKs for top commit: achow101: ACK 75a9305 lsilva01: Tested ACK bitcoin@75a9305 on Ubuntu 20.04 Tree-SHA512: 91de5763664046e5a35802eb1a9a28f69a1a27d78d26c9fa0024bcfab4ccb4b7ad4feebea7de4b19e141db9c39270c623c57c47942a48dfe679fdc8cad70df43

…cleanups 4356878 External input fund support cleanups (Gregory Sanders) Pull request description: Minor cleanups to bitcoin/bitcoin#17211 ACKs for top commit: achow101: ACK 4356878 meshcollider: utACK 4356878 benthecarman: ACK 4356878 Tree-SHA512: 865f8a3804f8c0027f5393a0539041158166a919378f2c3bc99b936843eee2329372bcc2af888fa62babfa5f6baf4f13d4cfef7b4e26a7265a82a908f9719ad6

Upstream bitcoin/bitcoin#17211 solved a long-standing issue for doing atomic trades with names (namely that the buyer's wallet was not able to directly fund a transaction with the non-wallet name input in it). Based on this, we can now streamline name_ant_workflow.py to make use of this new feature. This allows us to properly use fundrawtransaction to fund the final transaction including already the name input and output, and thus with correct fee estimation done. (Previously we just funded a dummy transaction with a guessed higher fee, and then added in the name input and output manually, hoping the fee would still be fine.)

fa1eef3 Update name_ant_workflow.py. (Daniel Kraft) Pull request description: Upstream bitcoin/bitcoin#17211 solved a long-standing issue for doing atomic trades with names (namely that the buyer's wallet was not able to directly fund a transaction with the non-wallet name input in it). Based on this, we can now streamline `name_ant_workflow.py` to make use of this new feature. This allows us to properly use `fundrawtransaction` to fund the final transaction including already the name input and output, and thus with correct fee estimation done. (Previously we just funded a dummy transaction with a guessed higher fee, and then added in the name input and output manually, hoping the fee would still be fine.) Top commit has no ACKs. Tree-SHA512: 0cb0eb4796bcc5a0baa3187258867b90f944548e6249110f1da03ddbde1f0ef3c06a0b4d323916d1b04753b3cd5c7823d8d9ed7fe9f2cbf762a72c091ca93817

achow101 force-pushed the fundtx-external-inputs branch from 6f632fb to fe1c549 Compare October 21, 2019 21:30

fanquake added RPC/REST/ZMQ Wallet labels Oct 21, 2019

achow101 force-pushed the fundtx-external-inputs branch 2 times, most recently from 5deea6c to 9e10531 Compare October 23, 2019 00:51

achow101 mentioned this pull request Oct 23, 2019

Be able to fund transactions with peg-ins specified ElementsProject/elements#755

Merged

instagibbs reviewed Oct 24, 2019

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

instagibbs reviewed Oct 24, 2019

View reviewed changes

achow101 force-pushed the fundtx-external-inputs branch 2 times, most recently from 7af1fcc to c63d41d Compare October 24, 2019 21:24

achow101 force-pushed the fundtx-external-inputs branch from c63d41d to e90de8e Compare October 25, 2019 16:05

DrahtBot added the Needs rebase label Oct 29, 2019

achow101 force-pushed the fundtx-external-inputs branch 2 times, most recently from e96d6e5 to a7e26d4 Compare October 29, 2019 19:07

DrahtBot removed the Needs rebase label Oct 29, 2019

achow101 force-pushed the fundtx-external-inputs branch from a7e26d4 to 7f9aef6 Compare October 31, 2019 19:06

luke-jr reviewed Nov 4, 2019

View reviewed changes

luke-jr reviewed Nov 13, 2019

View reviewed changes

achow101 force-pushed the fundtx-external-inputs branch from 7f9aef6 to 1b189f8 Compare November 18, 2019 22:56

DrahtBot added the Needs rebase label Nov 22, 2019

achow101 force-pushed the fundtx-external-inputs branch from 1b189f8 to 0073107 Compare November 22, 2019 21:51

DrahtBot added Needs rebase and removed Needs rebase labels Nov 22, 2019

achow101 force-pushed the fundtx-external-inputs branch from 0073107 to fd6ce28 Compare November 25, 2019 19:08

achow101 added 2 commits October 3, 2021 13:24

Tests for funding with external inputs

e39b5a5

allow send rpc take external inputs and solving data

928af61

achow101 force-pushed the fundtx-external-inputs branch from 0b6b098 to 928af61 Compare October 3, 2021 17:24

JeremyRand mentioned this pull request Oct 3, 2021

[Wallet] FundRawTransaction can fund a transaction with preset inputs found in the CoinView #10202

Closed

meshcollider reviewed Oct 3, 2021

View reviewed changes

instagibbs approved these changes Oct 4, 2021

View reviewed changes

yanmaani reviewed Oct 4, 2021

View reviewed changes

meshcollider merged commit 573b462 into bitcoin:master Oct 4, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 4, 2021

Merge bitcoin#17211: Allow fundrawtransaction and walletcreatefundedp…

1f3bb2e

…sbt to …

maflcko reviewed Oct 5, 2021

View reviewed changes

This was referenced Oct 5, 2021

RPC: Allow user to supply weight of external input #23187

Closed

wallet: fund transaction external input cleanups #23188

Merged

RPC: support fee bumping with external inputs #23189

Closed

meshcollider mentioned this pull request Oct 5, 2021

Fix intermittent failure in wallet_send.py and rpc_fundrawtransaction.py #23200

Merged

domob1812 mentioned this pull request Oct 11, 2021

Use fundrawtransaction more in the suggested ANT workflow namecoin/namecoin-core#473

Merged

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

Conversation

achow101 commented Oct 21, 2019

DrahtBot commented Oct 21, 2019 • edited

Conflicts

instagibbs left a comment

Choose a reason for hiding this comment

instagibbs left a comment

Choose a reason for hiding this comment

achow101 commented Oct 24, 2019

instagibbs commented Oct 25, 2019

achow101 commented Oct 29, 2019

instagibbs commented Oct 31, 2019

instagibbs commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Oct 3, 2021

meshcollider left a comment

Choose a reason for hiding this comment

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 4, 2021

yanmaani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meshcollider commented Oct 4, 2021

apoelstra commented Oct 4, 2021

t-bast commented Oct 4, 2021

achow101 commented Oct 4, 2021 • edited

t-bast commented Oct 4, 2021

Choose a reason for hiding this comment

katesalazar Oct 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katesalazar Oct 5, 2021 • edited

Choose a reason for hiding this comment

ryanofsky commented Oct 5, 2021

DrahtBot commented Oct 21, 2019 •

edited

achow101 commented Oct 4, 2021 •

edited

katesalazar Oct 5, 2021 •

edited

katesalazar Oct 5, 2021 •

edited