Fix coin control input size accounting for taproot spends #766

pull theStack wants to merge 1 commits into bitcoin-core:master from theStack:202310-gui-fix_input_size_estimation_for_taproot_spends changing 1 files +13 −1
  1. 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.
  2. gui: fix coin control input size accounting for taproot spends 00a52e6394
  3. 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.

  4. 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
  5. hebasto added the label Wallet on Oct 7, 2023
  6. hebasto added this to the milestone 26.0 on Oct 7, 2023
  7. hebasto commented at 12:33 pm on October 7, 2023: member
  8. 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?
  9. 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

  10. DrahtBot added the label CI failed on Oct 13, 2023
  11. DrahtBot removed the label CI failed on Oct 16, 2023
  12. maflcko commented at 1:34 pm on October 16, 2023: contributor
    lgtm ACK 00a52e63946d5a90cdfb68204373d9c23d885161
  13. DrahtBot requested review from MarnixCroes on Oct 16, 2023
  14. 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.
  15. furszy commented at 2:10 pm on October 16, 2023: member
    utACK 00a52e6394
  16. DrahtBot removed review request from MarnixCroes on Oct 16, 2023
  17. DrahtBot requested review from MarnixCroes on Oct 16, 2023
  18. DrahtBot removed review request from MarnixCroes on Oct 16, 2023
  19. DrahtBot requested review from MarnixCroes on Oct 16, 2023
  20. DrahtBot removed review request from MarnixCroes on Oct 16, 2023
  21. DrahtBot requested review from MarnixCroes on Oct 16, 2023
  22. DrahtBot removed review request from MarnixCroes on Oct 16, 2023
  23. DrahtBot requested review from MarnixCroes on Oct 16, 2023
  24. DrahtBot removed review request from MarnixCroes on Oct 16, 2023
  25. DrahtBot requested review from MarnixCroes on Oct 16, 2023
  26. DrahtBot removed review request from MarnixCroes on Oct 16, 2023
  27. DrahtBot requested review from MarnixCroes on Oct 16, 2023
  28. hebasto merged this on Oct 17, 2023
  29. hebasto closed this on Oct 17, 2023

  30. jarolrod commented at 10:04 pm on October 17, 2023: member
    post merge ACK
  31. Frank-GER referenced this in commit f56cc94840 on Oct 21, 2023
  32. bitcoin-core locked this on Oct 16, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 23:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me