wallet: do not count wallet utxos as external #24649

S3RK · 2022-03-23T08:56:36Z

Correctly differentiating between external vs non-external utxos in coin control produces more accurate weight and fee estimations.

Weight for external utxos is estimated based on the maximum signature size, while for the wallet utxos we expect minimal signature due to signature grinding.

achow101 · 2022-03-23T16:55:55Z

I'm not entirely sure that this is what we want to do. When I implemented that external input weight feature, I specifically decided to allow user provided weight to override what the wallet does. This is because if a user were using walletcreatefundedpsbt or fundrawtransaction, it is possible that they are going to use a different signer and are merely using the wallet as a watchonly wallet. We can't know how the other signer is going to behave. Furthermore, with taproot descriptors, and with miniscript coming up soon, the wallet does not necessarily know which script branch is going to be taken and may end up using the wrong script branch when estimating size. By allowing the user to override the size estimation, the user can effectively tell provide a more accurate size estimation depending on the branch taken.

uvhw

#14461 @uvhw

uvhw

#23480

S3RK · 2022-03-24T07:30:39Z

I believe input_weights option always takes precedences and this PR doesn't change it. Rather it improves precision when input_weights option is not provided.

achow101 · 2022-03-24T14:27:27Z

Oh indeed. Looking at SelectCoins again, we only check to see if an output is external after we determine it is not in the wallet. I'm not sure that this PR actually has an effect. Can you provide a test case where the weights end up calculated differently before and after this PR?

S3RK · 2022-03-29T07:33:26Z

Yes, but it doesn't do so in CWallet::DummySignTx, so CalculateMaximumSignedTxSize does produce less precise result, which might cause a lower change than needed. I'll see if I can create a test exposing this issue.

S3RK · 2022-03-30T07:21:40Z

@achow101 added a test case

DrahtBot · 2022-03-31T05:12:49Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25291 (test: Refactor rpc_fundrawtransaction.py by akankshakashyap)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
#25218 (refactor: introduce generic 'Result' classes and connect them to CreateTransaction and GetNewDestination by furszy)
#25118 (wallet: unify “allow/block other inputs“ concept by furszy)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation 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 · 2022-04-01T18:22:25Z

ACK 3b83b8a

Thanks for the test case, now I see what the problem is.

murchandamus · 2022-05-17T15:16:52Z

ACK 3b83b8a

Github-Pull: bitcoin#24649 Rebased-From: b9a2da7

Github-Pull: bitcoin#24649 Rebased-From: 3b83b8a

murchandamus

re-ACK 7832e94

murchandamus · 2022-05-31T18:42:26Z

src/wallet/spend.cpp

+    wallet.chain().findCoins(coins);
+
+    for (const CTxIn& txin : tx.vin) {
+        // if it's not in the wallet and corresponding UTXO is found than select as external output


Nit, if you have to rebase or touch something else:

Suggested change

// if it's not in the wallet and corresponding UTXO is found than select as external output

// if it's not in the wallet and corresponding UTXO is found, then select as external output

achow101 · 2022-06-03T21:22:50Z

re-ACK 7832e94

furszy

Code reviewed 7832e94

furszy · 2022-06-04T21:11:49Z

src/wallet/spend.cpp

+    for (const CTxIn& txin : tx.vin) {
+        coins[txin.prevout]; // Create empty map entry keyed by prevout.
+    }
+    wallet.chain().findCoins(coins);


At this point, we have cs_wallet locked (lock is few lines above) and inside findCoins we lock cs_main

Wonder if it's a potential deadlock cause or not. Because, cs_main should be taking precedence over cs_wallet in other processes.

Agree it could be a problem in theory, but I'd like to understand the problem better before making a change.

Right now I couldn't find any places in our code that acquire cs_wallet while holding cs_main. To experiment I added LOCK(cs_main) to wallet/rpc/coins.cpp:197 and immediately this triggered our deadlock detection mechanism because it turns out that loadwallet rpc acquires lock in a different order (first cs_wallet and then cs_main).

What is our long-term direction with regards to using cs_main and cs_wallet together?
@achow101 any thoughts?

I'm also curious how we should think of these locks in light of multiprocess (cc @ryanofsky )

cs_main is really for things in node operation and so there should never be a scenario within the wallet where we lock cs_main before cs_wallet. That would imply that the node is doing something to the wallet, but the model we (want to) have is the wallet asks for things from the node and otherwise operates asynchronously from the node. So it should always be cs_wallet before cs_main.

@furszy Looks like the current lock order is fine. Is there anything left before you can ACK this?

Was thinking on moving the "fetch coins" block of code above the cs_wallet lock, so both mutexes are locked independently from each other.

But, at this point, it's merely a nit.

furszy · 2022-06-04T21:40:35Z

src/wallet/spend.cpp

+    for (const CTxIn& txin : tx.vin) {
+        // if it's not in the wallet and corresponding UTXO is found than select as external output
+        const auto& outPoint = txin.prevout;
+        if (wallet.mapWallet.find(outPoint.hash) == wallet.mapWallet.end() && !coins[outPoint].out.IsNull()) {


nit: would be better to use wallet.GetWalletTx(hash) instead of accessing the wallet's map directly.

will do if I have to retouch

furszy

ACK 7832e94

(Thanks for the reminder)

furszy · 2022-06-15T12:39:22Z

src/wallet/spend.cpp

+    for (const CTxIn& txin : tx.vin) {
+        coins[txin.prevout]; // Create empty map entry keyed by prevout.
+    }
+    wallet.chain().findCoins(coins);


Was thinking on moving the "fetch coins" block of code above the cs_wallet lock, so both mutexes are locked independently from each other.

But, at this point, it's merely a nit.

fanquake added the Wallet label Mar 23, 2022

uvhw suggested changes Mar 24, 2022

View reviewed changes

uvhw reviewed Mar 24, 2022

View reviewed changes

S3RK mentioned this pull request Mar 24, 2022

test: coinselection edge cases #24580

Closed

DrahtBot mentioned this pull request Apr 1, 2022

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

DrahtBot mentioned this pull request Apr 2, 2022

wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

Merged

DrahtBot mentioned this pull request Apr 14, 2022

wallet: return error msg for "too-long-mempool-chain" #24845

Merged

DrahtBot mentioned this pull request May 13, 2022

wallet: unify “allow/block other inputs“ concept #25118

Merged

DrahtBot added the Needs rebase label May 17, 2022

S3RK added 2 commits May 18, 2022 08:25

wallet: do not count wallet utxos as external

c3981e3

test: fundrawtransaction preset input weight calculation

7832e94

S3RK force-pushed the wallet_correct_external_utxo branch from 3b83b8a to 7832e94 Compare May 18, 2022 06:25

DrahtBot removed the Needs rebase label May 18, 2022

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

wallet: do not count wallet utxos as external

317b893

Github-Pull: bitcoin#24649 Rebased-From: b9a2da7

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

test: fundrawtransaction preset input weight calculation

bd48d07

Github-Pull: bitcoin#24649 Rebased-From: 3b83b8a

DrahtBot mentioned this pull request May 26, 2022

refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

Merged

murchandamus reviewed May 31, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 4, 2022

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

Merged

furszy reviewed Jun 4, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 8, 2022

test: Refactor rpc_fundrawtransaction.py #25291

Closed

furszy approved these changes Jun 15, 2022

View reviewed changes

achow101 merged commit b0c8306 into bitcoin:master Jun 16, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 17, 2022

Merge bitcoin#24649: wallet: do not count wallet utxos as external

a4ea401

achow101 mentioned this pull request Jul 13, 2022

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

Merged

bitcoin locked and limited conversation to collaborators Jun 16, 2023

	// if it's not in the wallet and corresponding UTXO is found than select as external output
	// if it's not in the wallet and corresponding UTXO is found, then select as external output

wallet: do not count wallet utxos as external #24649

wallet: do not count wallet utxos as external #24649

Uh oh!

Conversation

S3RK commented Mar 23, 2022

Uh oh!

achow101 commented Mar 23, 2022

Uh oh!

uvhw left a comment

Choose a reason for hiding this comment

Uh oh!

uvhw left a comment

Choose a reason for hiding this comment

Uh oh!

S3RK commented Mar 24, 2022

Uh oh!

achow101 commented Mar 24, 2022

Uh oh!

S3RK commented Mar 29, 2022

Uh oh!

S3RK commented Mar 30, 2022

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

achow101 commented Apr 1, 2022

Uh oh!

murchandamus commented May 17, 2022

Uh oh!

murchandamus 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!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 3, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Mar 31, 2022 •

edited

Loading

murchandamus left a comment •

edited

Loading

furszy left a comment •

edited

Loading