wallet: do not count wallet utxos as external

S3RK commented at 8:56 am on March 23, 2022: member

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.

fanquake added the label Wallet on Mar 23, 2022

achow101 commented at 4:55 pm on March 23, 2022: member

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 changes_requested

uvhw commented at 1:42 am on March 24, 2022: none

#14461 @uvhw

uvhw commented at 1:43 am on March 24, 2022: none

#23480

S3RK commented at 7:30 am on March 24, 2022: member

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 commented at 2:27 pm on March 24, 2022: member

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 commented at 7:33 am on March 29, 2022: member

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 commented at 7:21 am on March 30, 2022: member

@achow101 added a test case

DrahtBot commented at 5:12 am on March 31, 2022: 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:

#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 commented at 6:22 pm on April 1, 2022: member

ACK 3b83b8a3b032ee2d730948c538ea66e81875abbb

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

DrahtBot added the label Needs rebase on May 17, 2022

Xekyo commented at 3:16 pm on May 17, 2022: member

ACK 3b83b8a3b032ee2d730948c538ea66e81875abbb

wallet: do not count wallet utxos as external c3981e379f

test: fundrawtransaction preset input weight calculation 7832e9438f

S3RK force-pushed on May 18, 2022

DrahtBot removed the label Needs rebase on May 18, 2022

luke-jr referenced this in commit 317b8939de on May 21, 2022

luke-jr referenced this in commit bd48d075fd on May 21, 2022

in src/wallet/spend.cpp:1034 in 7832e9438f

1033+        coins[txin.prevout]; // Create empty map entry keyed by prevout.
1034+    }
1035+    wallet.chain().findCoins(coins);
1036+
1037+    for (const CTxIn& txin : tx.vin) {
1038+        // if it's not in the wallet and corresponding UTXO is found than select as external output

Xekyo commented at 6:42 pm on May 31, 2022:

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

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

Xekyo commented at 6:46 pm on May 31, 2022: member

re-ACK 7832e9438f5c66b88f60676d14e1e11d669eb109

achow101 commented at 9:22 pm on June 3, 2022: member

re-ACK 7832e9438f5c66b88f60676d14e1e11d669eb109

in src/wallet/spend.cpp:1031 in c3981e379f outdated

1030+    // and to match with the given solving_data. Only used for non-wallet outputs.
1031+    std::map<COutPoint, Coin> coins;
1032+    for (const CTxIn& txin : tx.vin) {
1033+        coins[txin.prevout]; // Create empty map entry keyed by prevout.
1034+    }
1035+    wallet.chain().findCoins(coins);

furszy commented at 9:11 pm on June 4, 2022:

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.

S3RK commented at 7:14 am on June 9, 2022:

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 )

achow101 commented at 3:48 pm on June 9, 2022:

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.

S3RK commented at 7:24 am on June 15, 2022:

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

furszy commented at 12:39 pm on June 15, 2022:

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.

in src/wallet/spend.cpp:1036 in c3981e379f outdated

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

furszy commented at 9:40 pm on June 4, 2022:

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

S3RK commented at 7:15 am on June 9, 2022:

will do if I have to retouch

furszy commented at 9:42 pm on June 4, 2022: member

Code reviewed 7832e943

furszy approved

furszy commented at 12:40 pm on June 15, 2022: member

ACK 7832e943

(Thanks for the reminder)

achow101 merged this on Jun 16, 2022

achow101 closed this on Jun 16, 2022

sidhujag referenced this in commit a4ea401996 on Jun 17, 2022

DrahtBot locked this on Jun 16, 2023

wallet: do not count wallet utxos as external #24649

Conflicts