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

achow101 · 2022-07-22T21:43:03Z

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 · 2022-07-23T01:02:43Z

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.

furszy

Concept ACK, good catch.

Discussion point about the implementation:

Wouldn't be better to replace 09a0dcb for furszy/bitcoin@0fe6524?

The rationale is the same as fd98ced:
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 09a0dcb is introducing.

achow101 · 2022-07-25T15:25:16Z

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 · 2022-07-26T14:10:55Z

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 09a0dcb 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 · 2022-07-26T17:30:42Z

@furszy Implemented as suggested.

furszy

Code review ACK 65bb2fa

ishaanam

ACK 65bb2fa
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 9b4461d 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.

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

achow101 · 2022-08-17T00:35:56Z

Rebased for a silent merge conflict in the test.

ishaanam

ut-reACK 65bb2fa
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.

src/wallet/wallet.cpp

src/wallet/spend.cpp

test/functional/rpc_fundrawtransaction.py

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.

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.

instagibbs · 2022-08-18T15:12:01Z

ACK ef8e2a5

furszy

ACK ef8e2a5

~~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.

ishaanam

reACK ef8e2a5

ishaanam · 2022-08-19T01:28:17Z

src/wallet/spend.cpp

            if (!coin_control.GetExternalOutput(outpoint, txout)) {
                return std::nullopt;


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?

Suggested change

if (!coin_control.GetExternalOutput(outpoint, txout)) {

return std::nullopt;

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

…are also in the wallet

S3RK · 2022-08-23T07:05:03Z

test/functional/rpc_fundrawtransaction.py

+        ext_desc = self.nodes[0].getaddressinfo(ext_addr)["desc"]
+        fundedtx = wallet.fundrawtransaction(rawtx, {'fee_rate': 10, "change_type": "bech32", "solving_data": {"descriptors": [ext_desc]}})
+        # 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)
+        tx_size = ceil(10 + 41*3 + 31*2 + (2 + 107*2 + 108)/4)


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

b2544d1 qt: Update translation source file for string freeze (Hennadii Stepanov) Pull request description: This PR updates the `src/qt/locale/bitcoin_en.xlf` translation source file according to [Release schedule for 24.0](bitcoin/bitcoin#24987). There were some new strings added since the [recent](#654) update: - "Unable to find UTXO for external input" in bitcoin/bitcoin#25679 - "Pre-syncing Headers (%1%)…" in bitcoin/bitcoin#25717 - "Unknown. Pre-syncing Headers (%1, %2%)…" in bitcoin/bitcoin#25717 ACKs for top commit: jarolrod: ACK b2544d1 Tree-SHA512: cc3785a3279b2fae8cd477cd3a5b07f5321b25e963f4e03429538a6d43a858f6636e118eccfa64c77dc6279624937ddb6b7dd5d52021193ed93392a98662f25a

DrahtBot added the Wallet label Jul 22, 2022

neto626roman approved these changes Jul 22, 2022

View reviewed changes

bitcoin deleted a comment from neto626roman Jul 23, 2022

This was referenced Jul 23, 2022

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Merged

wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection #25685

Merged

furszy reviewed Jul 25, 2022

View reviewed changes

achow101 force-pushed the fix-external-but-have-tx branch from aeb9542 to 65bb2fa Compare July 26, 2022 17:30

furszy approved these changes Jul 26, 2022

View reviewed changes

DrahtBot mentioned this pull request Aug 9, 2022

wallet: group outputs only once, decouple it from Coin Selection #25806

Merged

ishaanam reviewed Aug 10, 2022

View reviewed changes

wallet: Add CWallet::IsMine(COutPoint)

f2d00bf

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

achow101 force-pushed the fix-external-but-have-tx branch from 65bb2fa to 4a33786 Compare August 17, 2022 00:35

ishaanam reviewed Aug 17, 2022

View reviewed changes

fanquake requested review from furszy and instagibbs August 18, 2022 01:13

instagibbs reviewed Aug 18, 2022

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

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

src/wallet/spend.cpp Show resolved Hide resolved

test/functional/rpc_fundrawtransaction.py Show resolved Hide resolved

test/functional/rpc_fundrawtransaction.py Show resolved Hide resolved

achow101 added 3 commits August 18, 2022 11:00

wallet: SelectExternal actually external inputs

a537d7a

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.

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

eb87963

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.

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

ef8e2a5

achow101 force-pushed the fix-external-but-have-tx branch from 4a33786 to ef8e2a5 Compare August 18, 2022 15:09

furszy approved these changes Aug 18, 2022

View reviewed changes

fanquake requested a review from glozow August 18, 2022 16:21

ishaanam reviewed Aug 19, 2022

View reviewed changes

fanquake merged commit 0425ce5 into bitcoin:master Aug 19, 2022

achow101 mentioned this pull request Aug 19, 2022

wallet: allow psbtbumpfee to work with txs with external inputs #23202

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 19, 2022

Merge bitcoin#25679: wallet: Correctly identify external inputs that …

e81bb43

…are also in the wallet

S3RK reviewed Aug 23, 2022

View reviewed changes

hebasto mentioned this pull request Sep 1, 2022

Update translation source file for string freeze bitcoin-core/gui#660

Merged

bitcoin locked and limited conversation to collaborators Aug 23, 2023

		if (!coin_control.GetExternalOutput(outpoint, txout)) {
		return std::nullopt;

	if (!coin_control.GetExternalOutput(outpoint, txout)) {
	return std::nullopt;
	assert(coin_control.GetExternalOutput(outpoint, txout));

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

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

Uh oh!

Conversation

achow101 commented Jul 22, 2022

Uh oh!

DrahtBot commented Jul 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 25, 2022

Uh oh!

furszy commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jul 26, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 17, 2022

Uh oh!

ishaanam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Aug 18, 2022

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

S3RK Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jul 23, 2022 •

edited

Loading

furszy left a comment •

edited

Loading

furszy commented Jul 26, 2022 •

edited

Loading

ishaanam left a comment •

edited

Loading

furszy left a comment •

edited

Loading

S3RK Aug 23, 2022 •

edited

Loading