Fix coin control input size accounting for taproot spends

theStack commented at 11:22 pm on October 6, 2023: contributor

If manual coin control is used in the GUI, the input size accounting for P2TR is currently overshooting, as it still assumes P2WPKH (segwitv0) spends which have a larger witness, as ECDSA signatures are longer and the pubkey also has to be provided. Fix that by adding sizes depending on the witness version. Note that the total accounting including outputs is still off and there is some weird logic involved depending on whether SFFO is used, but it’s (hopefully) a first step into the right direction.

gui: fix coin control input size accounting for taproot spends 00a52e6394

DrahtBot commented at 11:22 pm on October 6, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, furszy
Concept ACK	MarnixCroes

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

hebasto renamed this:
~~gui: fix coin control input size accounting for taproot spends~~
Fix coin control input size accounting for taproot spends
on Oct 7, 2023

hebasto added the label Wallet on Oct 7, 2023

hebasto added this to the milestone 26.0 on Oct 7, 2023

hebasto commented at 12:33 pm on October 7, 2023: member

cc @achow101 @furszy @Sjors

in src/qt/coincontroldialog.cpp:425 in 00a52e6394

420@@ -421,7 +421,19 @@ void CoinControlDialog::updateLabels(CCoinControl& m_coin_control, WalletModel *
421         std::vector<unsigned char> witnessprogram;
422         if (out.txout.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram))
423         {
424-            nBytesInputs += (32 + 4 + 1 + (107 / WITNESS_SCALE_FACTOR) + 4);
425+            // add input skeleton bytes (outpoint, scriptSig size, nSequence)
426+            nBytesInputs += (32 + 4 + 1 + 4);

MarnixCroes commented at 11:12 am on October 10, 2023:

bit of a nit: the outpoint is 36 right? maybe good to consolidate 32 and 4 or breakdown the outpoint comment (into txid and vout index number) to prevent confusion?

MarnixCroes commented at 11:36 am on October 10, 2023: contributor

nice catch. Tested a bit, works as expected.

Note that the total accounting including outputs is still off and there is some weird logic involved depending on whether SFFO is used, but it’s (hopefully) a first step into the right direction.

Thanks for mentioning, I was a bit confused.

I’m not 100% sure about the exact number myself, so cACK

DrahtBot added the label CI failed on Oct 13, 2023

DrahtBot removed the label CI failed on Oct 16, 2023

maflcko commented at 1:34 pm on October 16, 2023: contributor

lgtm ACK 00a52e63946d5a90cdfb68204373d9c23d885161

DrahtBot requested review from MarnixCroes on Oct 16, 2023

in src/qt/coincontroldialog.cpp:436 in 00a52e6394

432+                // 1 WU (witness item count) + 65 WU (Schnorr signature with len byte)
433+                nBytesInputs += 66 / WITNESS_SCALE_FACTOR;
434+            } else {
435+                // not supported, should be unreachable
436+                throw std::runtime_error("Trying to spend future segwit version script");
437+            }

furszy commented at 2:09 pm on October 16, 2023:

I don’t think that throwing exception in the GUI for backend incompatibilities between releases is good. This will crash the app. Instead, would be better to disable the fee, bytes, etc calculations and show “no information” in the labels when the version is unknown.

maflcko commented at 2:22 pm on October 16, 2023:

I don’t think it is supported to use a gui version of a different commit than the node?

furszy commented at 2:33 pm on October 16, 2023:

I don’t think it is supported to use a gui version of a different commit than the node?

Doesn’t necessarily need to be a different commit. The supported witness version numbers are hardcoded above. If we add a new version, and for some reason the wallet opens (which is not supported atm), this will crash the app.

But, my comment was more general. Ideally, all these hardcoded size and fee calculations shouldn’t be part of the gui code. The backend should provide the information and the gui should either present it or show something else denoting the “not enough information to calculate size/fee”. Never throw an exception that crashes the app, gui developers shouldn’t be aware of the wallet internals.

maflcko commented at 2:56 pm on October 16, 2023:

I think a crash on an internal logic error is better than displaying the wrong value. If this code can be removed and the wallet be used instead, as you suggest, that seems even better.

furszy commented at 2:10 pm on October 16, 2023: member

utACK 00a52e6394

DrahtBot removed review request from MarnixCroes on Oct 16, 2023