wallet: Correctly identify external inputs that are also in the wallet #25679

achow101 commented at 9:43 PM on July 22, 2022: member

if a transaction is being funded that has an external input, and that input's parent is also in the wallet, we will fail to detect that and fail to fund the transaction. In order to correctly detect such inputs, we need to be doing IsMine on all specified inputs in order to use Select and SelectExternal correctly. Additionally SelectCoins needs to call CalculateMaximumSignedInputSize with the correct parameters which depends on whether the wallet is able to solve for the input. Because there are some situations where the wallet could find an external input to belong to it (e.g. watching an address - unable to solve, but will be ISMINE_WATCHONLY), instead of switching which CalculateMaximumSignedInputSize to use, we should call the one that uses the wallet, and if that fails, try again with the one that uses external solving data.

Also adds a test for this case.

DrahtBot added the label Wallet on Jul 22, 2022

neto626roman approved

DrahtBot commented at 1:02 AM on July 23, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25685 (wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection by furszy)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction 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.

bitcoin deleted a comment on Jul 23, 2022

furszy commented at 2:59 PM on July 25, 2022: member

Concept ACK, good catch.

Discussion point about the implementation:

Wouldn't be better to replace 09a0dcb8 for https://github.com/furszy/bitcoin/commit/0fe65242b4f0794650c4fdafff4686e8bc316fbd?

The rationale is the same as fd98ced3: If we have an external input that the wallet knows about, we will be able to get the parent transaction but not be able to calculate the size nor solve it locally (needing the external provider).

So, instead of continue asking if the parent tx exist in the wallet to know whether we own the output or not. We could use IsMine(outpoint) directly.

Which saves us from doing the double CalculateMaximumSignedInputSize calculation that 09a0dcb8 is introducing.

achow101 commented at 3:25 PM on July 25, 2022: member

Wouldn't be better to replace 09a0dcb for furszy/bitcoin@0fe6524? ... Which saves us from doing the double CalculateMaximumSignedInputSize calculation that 09a0dcb is introducing.

This was the original approach, but I decided it was insufficient. I can think of a case where it would fail, but the approach that I have implemented does not.

Consider a watch-only wallet where an address has been imported (either legacy and importaddress was used, or descriptor and a addr() imported). The scriptPubKey for this would be reported ISMINE_WATCHONLY or ISMINE_SPENDABLE so if we used IsMine, we would use the first invocation of CalculateMaximumSignedInputSize but it would fail because the wallet lacks solving data. Suppose the user decides to provide the solving data via the RPC - this solving data would be in coin_control.m_external_provider, but not in the wallet. With the IsMine approach, we would not try with the external signing provider and fail to estimate the size. With the approach that I have implemented, we would fail with the wallet, and immediately try again with the external signing provider and therefore succeed.

An alternative is to use IsSolvable, but that just dummy signs the utxo too, so it's not actually any better than doing CalculateMaximumSignedInputSize twice sometimes. In fact, using IsSolvable would make it so that we always dummy sign twice, whereas the implemented approach only does it twice when the first one fails.

furszy commented at 2:10 PM on July 26, 2022: member

Yeah ok, I was also flirting a bit with the IsSolvable usage and ended up removing it from the suggestion for the same reason.

The only thing that still makes some noice on me is the first call to CalculateMaximumSignedInputSize(txout, wallet) for transactions that aren't in the wallet. We know, for sure, that the wallet is not capable of solving them.

What about simplifying 09a0dcb8 by only moving the second CalculateMaximumSignedInputSize to be executed on known transactions as well (if input_bytes==-1), and not moving the first CalculateMaximumSignedInputSize call. So it would only be:

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
--- a/src/wallet/spend.cpp	(revision fd98ced3973cbdece1cb9c0bc3ff2c11b296c787)
+++ b/src/wallet/spend.cpp	(revision 13ae18bf827db6c8786d1e1da65287ab49ff6ef5)
@@ -453,8 +453,12 @@
             if (!coin_control.GetExternalOutput(outpoint, txout)) {
                 return std::nullopt;
             }
+        }
+
+        if (input_bytes == -1) {
             input_bytes = CalculateMaximumSignedInputSize(txout, outpoint, &coin_control.m_external_provider, &coin_control);
         }
+
         // If available, override calculated size with coin control specified size
         if (coin_control.HasInputWeight(outpoint)) {
             input_bytes = GetVirtualTransactionSize(coin_control.GetInputWeight(outpoint), 0, 0);

achow101 commented at 5:30 PM on July 26, 2022: member

@furszy Implemented as suggested.

achow101 force-pushed on Jul 26, 2022

furszy approved

furszy commented at 6:07 PM on July 26, 2022: member

Code review ACK 65bb2fa

ishaanam commented at 8:40 PM on August 10, 2022: contributor

ACK 65bb2fafc85fbaa3bb74405da91c60f01a84c5be I cherry-picked the commit which adds the test onto the current master and the added test failed (as expected). I think that the IsMine function which takes an outpoint that was introduced in 9b4461d3946b8892c3c3f6dd4185b6d3b9be6e21 can be called in a few other places to allow for some simplification, but that doesn't really need to be done in this PR.

wallet: Add CWallet::IsMine(COutPoint)

It is useful to have an IsMine function that can take an outpoint.

f2d00bfe1a

achow101 force-pushed on Aug 17, 2022

achow101 commented at 12:35 AM on August 17, 2022: member

Rebased for a silent merge conflict in the test.

ishaanam commented at 8:56 PM on August 17, 2022: contributor

ut-reACK 65bb2fafc85fbaa3bb74405da91c60f01a84c5be What was the cause of the silent merge conflict in the test? It seems like the test will pass both with the changes applied because of the conflict as well as without. Edit: sorry, never mind.

fanquake requested review from furszy on Aug 18, 2022

fanquake requested review from instagibbs on Aug 18, 2022

in src/wallet/wallet.cpp:1431 in f2d00bfe1a outdated

1426 | +    AssertLockHeld(cs_wallet);
1427 | +    auto wtx = GetWalletTx(outpoint.hash);
1428 | +    if (!wtx) {
1429 | +        return ISMINE_NO;
1430 | +    }
1431 | +    if (outpoint.n >= wtx->tx->vout.size()) {

instagibbs commented at 1:34 PM on August 18, 2022:

should we just assert here? Seems like this can only happen on caller error.

achow101 commented at 2:55 PM on August 18, 2022:

No, this is called with user provided input. The caller is FundTransaction which will use this IsMine with any user specified inputs. If the user specifies a txid that exists, but a vout that does not, we need to be able to handle that gracefully, so an assert is not suitable.

in src/wallet/spend.cpp:1130 in 8b4f6f1db2 outdated

1128 | @@ -1129,10 +1129,14 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
1129 |      for (const CTxIn& txin : tx.vin) {
1130 |          // if it's not in the wallet and corresponding UTXO is found than select as external output

instagibbs commented at 1:42 PM on August 18, 2022:

Change comment to something like:

// The input is in the wallet, making it an internal input, or is found in the uxto // set, making it eligible as an external input.

achow101 commented at 3:09 PM on August 18, 2022:

Updated the comments

in src/wallet/spend.cpp:1135 in 8b4f6f1db2 outdated

1128 | @@ -1129,10 +1129,14 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
1129 |      for (const CTxIn& txin : tx.vin) {
1130 |          // if it's not in the wallet and corresponding UTXO is found than select as external output
1131 |          const auto& outPoint = txin.prevout;
1132 | -        if (wallet.mapWallet.find(outPoint.hash) == wallet.mapWallet.end() && !coins[outPoint].out.IsNull()) {
1133 | -            coinControl.SelectExternal(outPoint, coins[outPoint].out);
1134 | -        } else {
1135 | +        if (wallet.IsMine(outPoint)) {

instagibbs commented at 1:42 PM on August 18, 2022:

can't we make this an if/else if/else flow? Makes the three options clearer imo

achow101 commented at 3:09 PM on August 18, 2022:

Done

in test/functional/rpc_fundrawtransaction.py:1095 in 4a33786c9f outdated

1091 |  
1092 |          rawtx = wallet.createrawtransaction([{'txid': txid, 'vout': vout}], [{self.nodes[0].getnewaddress(): 9.999}])
1093 |          fundedtx = wallet.fundrawtransaction(rawtx, {'fee_rate': 10})
1094 |          # with 71-byte signatures we should expect following tx size
1095 | -        tx_size = 10 + 41*2 + 31*2 + (2 + 107*2)/4
1096 | +        tx_size = ceil(10 + 41*2 + 31*2 + (2 + 107*2)/4)

instagibbs commented at 1:54 PM on August 18, 2022:

is there any way to remove these magic number crimes from the test? Or at least annotate what is doing what?

achow101 commented at 3:10 PM on August 18, 2022:

Annotated

in test/functional/rpc_fundrawtransaction.py:1103 in 4a33786c9f outdated

1098 | +
1099 | +        # Using the other output should have 72 byte sigs
1100 | +        rawtx = wallet.createrawtransaction([{'txid': txid, 'vout': ext_vout}], [{self.nodes[0].getnewaddress(): 13}])
1101 | +        ext_desc = self.nodes[0].getaddressinfo(ext_addr)["desc"]
1102 | +        fundedtx = wallet.fundrawtransaction(rawtx, {'fee_rate': 10, "solving_data": {"descriptors": [ext_desc]}})
1103 | +        tx_size = ceil(10 + 41*3 + 31*2 + (2 + 107*2 + 108)/4)

instagibbs commented at 1:55 PM on August 18, 2022:

is there any way to remove these magic number crimes from the test? Or at least annotate what is doing what?

achow101 commented at 3:10 PM on August 18, 2022:

Annotated

wallet: SelectExternal actually external inputs

If an external input's utxo was created by a transaction that the wallet
knows about, then it would not be selected using SelectExternal. This
results in either funding failure or incorrect weight calculation.

a537d7aaa0

wallet: Try estimating input size with external data if wallet fails

Instead of choosing whether to use the wallet or external data when
estimating the size of an input, first use the wallet, then try external
data if that failed.

eb879634db

tests: Test that external inputs of txs in wallet is handled correctly ef8e2a5b09

achow101 force-pushed on Aug 18, 2022

instagibbs commented at 3:12 PM on August 18, 2022: member

ACK https://github.com/bitcoin/bitcoin/pull/25679/commits/ef8e2a5b09dea73de8d57e6b976d77edbde5f8ff

furszy approved

furszy commented at 4:07 PM on August 18, 2022: member

ACK ef8e2a5b

~~With a note: had to re-review the test step by step once more because, at least for me, it was not so clear why we are testing this specific error inside test_weight_calculation.~~

Probably a comment saying something like "let's now select the external vout as input of a new tx, then fund the tx using the wallet's coins plus provide the external descriptor so the process is able to calculate the size of the input that doesn't belong to the wallet" in the test would had been useful.

fanquake requested review from glozow on Aug 18, 2022

in src/wallet/spend.cpp:570 in eb879634db outdated

 568 | @@ -569,8 +569,12 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
 569 |              if (!coin_control.GetExternalOutput(outpoint, txout)) {
 570 |                  return std::nullopt;

ishaanam commented at 1:28 AM on August 19, 2022:

I tested this myself (by applying this change on top of this PR and running the functional tests) and it doesn't seem like GetExternalOutput() will return false now because, if I'm understanding this correctly, then I think that we no longer Select() utxos which we can't find in the wallet or the coins map and end up throwing an error before we get to this point now instead. For this reason might it make sense for this to be an assertion instead?

            assert(coin_control.GetExternalOutput(outpoint, txout));

ishaanam commented at 2:33 AM on August 19, 2022: contributor

reACK ef8e2a5b09dea73de8d57e6b976d77edbde5f8ff

fanquake merged this on Aug 19, 2022

fanquake closed this on Aug 19, 2022

sidhujag referenced this in commit e81bb43615 on Aug 19, 2022

in test/functional/rpc_fundrawtransaction.py:1103 in ef8e2a5b09

1104 | +        # Using the other output should have 72 byte sigs
1105 | +        rawtx = wallet.createrawtransaction([{'txid': txid, 'vout': ext_vout}], [{self.nodes[0].getnewaddress(): 13}])
1106 | +        ext_desc = self.nodes[0].getaddressinfo(ext_addr)["desc"]
1107 | +        fundedtx = wallet.fundrawtransaction(rawtx, {'fee_rate': 10, "change_type": "bech32", "solving_data": {"descriptors": [ext_desc]}})
1108 | +        # tx overhead (10) + 3 inputs (41 each) + 2 p2wpkh(31 each) + (segwit marker and flag (2) + 2 p2wpkh 71 bytes sig witnesses (107 each) + p2wpkh 72 byte sig witness (108)) / witness scaling factor (4)
1109 | +        tx_size = ceil(10 + 41*3 + 31*2 + (2 + 107*2 + 108)/4)

S3RK commented at 7:05 AM on August 23, 2022:

I'm not sure this test have enough sensitivity to detect 72 vs 71 bytes sig. ceil(10 + 41*3 + 31*2 + (2 + 107*2 + 108)/4) == ceil(10 + 41*3 + 31*2 + (2 + 107*2 + 107)/4) Probably we need more than one external input or to actually detect larger sig size.

UPD: maybe using just two inputs would work ceil(10 + 41*2 + 31*2 + (2 + 107 + 108)/4) = 209 but ceil(10 + 41*2 + 31*2 + (2 + 107 + 107)/4) = 208

hebasto referenced this in commit f79d612fba on Sep 1, 2022

bitcoin locked this on Aug 23, 2023