Qt: Setting for deciding address type (legacy, p2sh or bech32) #11937

pull hsjoberg wants to merge 1 commits into bitcoin:master from hsjoberg:segwitwalletguioption changing 5 files +104 −0
  1. hsjoberg commented at 3:27 pm on December 18, 2017: contributor

    Related PR #11403, needs to be merged first. I’ll rebase this PR to be up to date with #11403, and rebase it to master once it has been merged.

    Exposes the addresstype parameter through the GUI. Currently directly changes the g_address_type global variable instead of requiring an application restart, I’m not sure if this is good practice, but could easily be changed if needed.

    Radio buttons seem to be acting a bit different compared to other GUI elements, so that’s why the code is somewhat different than for other settings.

  2. hsjoberg renamed this:
    GUI: Setting for deciding address type (legacy, p2sh or bech32)
    Qt: Setting for deciding address type (legacy, p2sh or bech32)
    on Dec 18, 2017
  3. Sjors commented at 4:18 pm on December 18, 2017: member
    If you edit the pull request description, you can change the base branch from bitcoin:master to sipa:201709_segwitwallet2. That should make review here much easier. Although it might cause Github to move the whole pull request to @sipa’s account, which would hurt visibility.
  4. hsjoberg commented at 4:30 pm on December 18, 2017: contributor

    @Sjors Yes, I tried to do that but it doesn’t seem to be possible.

    But opening a new PR to Wuille’s repository should be possible yes.

  5. Sjors commented at 5:24 pm on December 18, 2017: member
    No problem, it’s easy to work around:
  6. in src/qt/forms/optionsdialog.ui:210 in 13adc63255 outdated 
    205+                 <horstretch>0</horstretch>
206+                 <verstretch>0</verstretch>
207+                </sizepolicy>
208+               </property>
209+               <property name="text">
210+                <string>BIP173 (bech32)</string>
    Sjors commented at 5:28 pm on December 18, 2017:

    Maybe call this “Native SegWit (bech32, BIP173)”?

    Can you also add a description (could be a popover) to explain the difference, explain:

    • not all wallets support this format
    • there are privacy implications for using bech32 while adoption is low, as well as for mixing bech32 and P2SH
    hsjoberg commented at 11:40 pm on December 18, 2017:
    Sure, I’ll look into that. Any suggestions of exactly what the tooltip(s) should say?
  7. in src/qt/optionsdialog.cpp:185 in 13adc63255 outdated 
    180@@ -180,6 +181,10 @@ void OptionsDialog::setMapper()
181     mapper->addMapping(ui->spendZeroConfChange, OptionsModel::SpendZeroConfChange);
182     mapper->addMapping(ui->coinControlFeatures, OptionsModel::CoinControlFeatures);
183 
184+    mapper->addMapping(ui->addressTypeLegacy, OptionsModel::AddressTypeLegacy);
    Sjors commented at 5:29 pm on December 18, 2017:
    Do we really want to maintain UI support for legacy addresses? Maybe only show this option if the app was launched with -addresstype=legacy?
    hsjoberg commented at 11:39 pm on December 18, 2017:
    Hmm, I think it makes more sense to have the GUI options mapped 1:1 to the addresstype parameter. Perhaps this could be something to look into later?
    Sjors commented at 7:03 pm on January 12, 2018:
    Nvm, it’s fine for a setting, especially combined with the per address checkbox in #11991.
  8. Sjors commented at 6:11 pm on December 18, 2017: member

    Concept ACK. It’s great to offer users the ability to use bech32 addresses.

    Receive tab works as expected.

    Send tab has a bug: without a restart, the change address will not use newly selected address type (maybe explicitly set -changetype?).

    The nested boxes and alignment is not very pretty:

    I suggest moving this the same level as Expert. Alternatively, use a dropdown.

    Arguably this isn’t really an expert feature anyway, especially if you remove the Legacy option. Users can’t really shoot themselves in the foot: either they get paid, or the creditor tells them they don’t understand the address, in which case the user creates a P2SH address.

    I’d like to have a way to toggle this in the address generation screen too, but that can be done independently of this PR.

  9. in src/qt/optionsmodel.cpp:114 in 13adc63255 outdated 
    109@@ -110,6 +110,11 @@ void OptionsModel::Init(bool resetSettings)
110         settings.setValue("bSpendZeroConfChange", true);
111     if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
112         addOverriddenOption("-spendzeroconfchange");
113+
114+    if(!settings.contains("strAddressType"))
    promag commented at 9:19 pm on December 18, 2017:
    Nit, missing space after if, missing {}.
    hsjoberg commented at 11:55 pm on December 18, 2017:
    Thanks, fixing missing space. :)
  10. in src/qt/optionsmodel.cpp:113 in 13adc63255 outdated 
    109@@ -110,6 +110,11 @@ void OptionsModel::Init(bool resetSettings)
110         settings.setValue("bSpendZeroConfChange", true);
111     if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
112         addOverriddenOption("-spendzeroconfchange");
113+
114+    if(!settings.contains("strAddressType"))
115+      settings.setValue("strAddressType", QString::fromStdString(FormatOutputType(OUTPUT_TYPE_DEFAULT)));
116+    if (!gArgs.SoftSetArg("-addresstype", settings.value("strAddressType").toString().toStdString()))
    promag commented at 9:19 pm on December 18, 2017:
    Nit, missing {}.
  11. in src/qt/optionsdialog.cpp:351 in 13adc63255 outdated 
    345+    case OUTPUT_TYPE_BECH32:
346+        ui->addressTypeBech32->setChecked(true);
347+        break;
348+    case OUTPUT_TYPE_NONE:
349+    default:
350+        break;
    promag commented at 9:23 pm on December 18, 2017:
    If this is not hit then assert?
    hsjoberg commented at 11:54 pm on December 18, 2017:

    Hmm I’m not sure if we really need to kill the application, if we cannot resolve an address type, it doesn’t necessarily need to be fatal.

    I’m not sure what the best practice is here.

    jonasschnelli commented at 8:05 am on January 4, 2018:
    I think an assert would be appropriate here (stop the app if address type is unknown to the GUI).
    promag commented at 2:09 am on January 16, 2018:
    @hsjoberg ping.
  12. in src/qt/forms/optionsdialog.ui:167 in 13adc63255 outdated 
    162@@ -163,6 +163,57 @@
163             </property>
164            </widget>
165           </item>
166+          <item>
167+           <widget class="QGroupBox" name="groupBoxAddressType">
    promag commented at 10:04 pm on December 18, 2017:
    This is inside Export group box, should be on the outside instead?
    hsjoberg commented at 11:56 pm on December 18, 2017:
    Fixing.
  13. hsjoberg commented at 11:37 pm on December 18, 2017: contributor

    I suggest moving this the same level as Expert. Alternatively, use a dropdown. @Sjors @promag The Address type was actually intentionally in the Expert group, but I guess you are right, it makes more sense to have it separately. I will move it. We need good tooltips and description though.

  14. hsjoberg commented at 0:03 am on December 19, 2017: contributor

    Send tab has a bug: without a restart, the change address will not use newly selected address type (maybe explicitly set -changetype?). @Sjors Very true, I’ll fix this.

  15. hsjoberg force-pushed on Dec 19, 2017
  16. hsjoberg commented at 0:11 am on December 19, 2017: contributor
    Rebased to fix some of the nits.
  17. luke-jr commented at 3:04 am on December 19, 2017: member
    GUI settings probably shouldn’t influence RPC like this, and IMO it should really be exposed per-receive.
  18. Sjors commented at 9:12 am on December 19, 2017: member

    @luke-jr wrote:

    IMO it should really be exposed per-receive

    Do you mean that it should only be exposed per receive (i.e. don’t add this setting)?

    That would not be ideal, because the wallet wouldn’t use bech32 for change in that case.

    It reveals another problem with this setting: if the user changes the setting and then runs bitcoind instead of bitcoin-qt, they might expect the RPC to use bech32 as well, just like it would in the debug console inside of QT.

    One counter argument to that is that we expect bitcoind users to know what they’re doing and to aware of the fact that GUI settings, like “spend unconfirmed change”, don’t apply to bitcoind. This would be worth pointing out in the release notes and such.

    There’s something to be said for being able to persist wallet settings, so that they apply to both bitcoin-qt and bitcoind if no flag overrides it.

    Writing the setting into bitcoin.conf is one way, but at least one potential problem with that, is that we can’t change the default later, because all users have it set in stone in their bitcoin.conf file. That’s probably an issue with the PR as well; once there is a setting, it’s weird to just change it.

    So I might agree with @luke-jr here in that this should be a per-receive thing, and not a setting. But then I would suggest that we make the change address behavior more intelligent (not in this PR): its address type should match the destination. Also making it a per receive checkbox still makes it difficult to change, as the user might grow accustomed to the check-box being checked or unchecked.

    So I’m not really sure. We should think about how to encourage users to try out bech32 without making it more difficult to make this the default in the future.

  19. hsjoberg force-pushed on Dec 19, 2017
  20. hsjoberg commented at 5:51 pm on December 19, 2017: contributor

    Send tab has a bug: without a restart, the change address will not use newly selected address type (maybe explicitly set -changetype?). @Sjors Rebased and fixed this. Right now, changetype isn’t changed if you explicitly set a type it in the configuration file, otherwise it defaults to whatever addresstype option is chosen.

  21. hsjoberg commented at 5:56 pm on December 19, 2017: contributor

    GUI settings probably shouldn’t influence RPC like this @luke-jr Isn’t this what GUI settings basically are used for? It seems like most of the settings in the GUI expose different configuration parameters.

    IMO it should really be exposed per-receive.

    Yes, I guess that makes sense too.

  22. hsjoberg force-pushed on Dec 19, 2017
  23. Sjors commented at 2:57 pm on December 22, 2017: member
    For a per receive address toggle, see #11991. These PR’s are compatible (aside from the usual potential for merge conflicts), so we can use both, either or none.
  24. jb55 commented at 10:33 pm on December 30, 2017: member
    Hmm after testing this patch over #11991, I think I prefer this one. The toggle seems a bit weird. I don’t see why I would ever want to selectively toggle a bech32 address, vs just eventually having it my default when I feel comfortable doing that.
  25. sipa commented at 4:07 am on December 31, 2017: member
    @jb55 The only reason for bech32 or not is whether the sender supports bech32 - which I think is something you may want to decide per address.
  26. hsjoberg commented at 1:35 pm on December 31, 2017: contributor
    Perhaps it makes sense to have both options available. Per receive level and as a global setting. If not, I think @Sjors PR works very well.
  27. molxyz commented at 6:59 pm on January 2, 2018: none

    ACK! I just compiled this PR for Windows. It is beautiful. Really love it. Will test.

    walletgui-by-coco

  28. in src/qt/optionsmodel.cpp:394 in 054d5fdd98 outdated 
    385@@ -381,6 +386,27 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
386                 setRestartRequired(true);
387             }
388             break;
389+        case AddressTypeLegacy:
390+            if (value.toBool()) {
391+                settings.setValue("strAddressType", QString::fromStdString(FormatOutputType(OUTPUT_TYPE_LEGACY)));
392+                g_address_type = OUTPUT_TYPE_LEGACY;
    jonasschnelli commented at 8:08 am on January 4, 2018:
    Not sure, but I guess changing g_address_type and g_change_type needs locking (concurrency), otherwise a race would be possible (unlikely, though possible).
    TheBlueMatt commented at 5:01 pm on January 12, 2018:
    Yes, probably, in current code I think its safe just because they’re only set at init, setting at runtime is probably a lock bug, but none of the option are good fixes :/
    promag commented at 2:16 am on January 16, 2018:
    Guard with cs_main? Most accesses already held the lock. Fix the remaining.
  29. jonasschnelli commented at 8:09 am on January 4, 2018: contributor
    Nice! Concept ACK
  30. jonasschnelli commented at 7:23 am on January 11, 2018: contributor
    Needs rebase
  31. hsjoberg commented at 8:30 am on January 11, 2018: contributor
    @jonasschnelli I’ll rebase to master as soon as I have time.
  32. laanwj added this to the milestone 0.16.0 on Jan 11, 2018
  33. hsjoberg force-pushed on Jan 11, 2018
  34. hsjoberg commented at 10:22 pm on January 11, 2018: contributor
    Rebased to master. I intend to look at and fix the things that have been pointed out soon.
  35. in src/qt/optionsdialog.cpp:342 in 450206b88c outdated 
    333@@ -329,6 +334,23 @@ void OptionsDialog::updateDefaultProxyNets()
334     (strProxy == strDefaultProxyGUI.toStdString()) ? ui->proxyReachTor->setChecked(true) : ui->proxyReachTor->setChecked(false);
335 }
336 
337+void OptionsDialog::setActiveAddressType() {
338+    switch(g_address_type) {
339+    case OUTPUT_TYPE_LEGACY:
340+        ui->addressTypeLegacy->setChecked(true);
341+        break;
342+    case OUTPUT_TYPE_P2SH:
    jonasschnelli commented at 6:33 am on January 12, 2018:
    Rebase issue, .. should this be OUTPUT_TYPE_P2SH_SEGWIT?
    Sjors commented at 7:13 pm on January 12, 2018:
    Yes, it won’t compile otherwise.
    hsjoberg commented at 11:13 pm on January 16, 2018:
    @jonasschnelli Fixed, thanks!
  36. in src/qt/optionsmodel.h:53 in 450206b88c outdated 
    48@@ -48,6 +49,9 @@ class OptionsModel : public QAbstractListModel
49         ThreadsScriptVerif,     // int
50         DatabaseCache,          // int
51         SpendZeroConfChange,    // bool
52+        AddressTypeLegacy,      // bool
53+        AddressTypeP2SH,        // bool
    jonasschnelli commented at 6:34 am on January 12, 2018:
    Maybe for constancy reason, change that to AddressTypeP2SHSegWit?
    promag commented at 2:17 am on January 16, 2018:
    Agree.
    hsjoberg commented at 11:13 pm on January 16, 2018:
    Fixed.
  37. in src/qt/optionsmodel.cpp:114 in 450206b88c outdated 
    109@@ -110,6 +110,11 @@ void OptionsModel::Init(bool resetSettings)
110         settings.setValue("bSpendZeroConfChange", true);
111     if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
112         addOverriddenOption("-spendzeroconfchange");
113+
114+    if (!settings.contains("strAddressType"))
115+      settings.setValue("strAddressType", QString::fromStdString(FormatOutputType(OUTPUT_TYPE_DEFAULT)));
116+    if (!gArgs.SoftSetArg("-addresstype", settings.value("strAddressType").toString().toStdString()))
117+        addOverriddenOption("-addresstype");
    Sjors commented at 7:31 pm on January 12, 2018:

    Actually this is not true. It will happily ignore -addresstype if you change the setting.

    Instead, you should add addOverriddenOption("-changetype");, since this setting is honored.

  38. in src/qt/optionsmodel.h:54 in 450206b88c outdated 
    48@@ -48,6 +49,9 @@ class OptionsModel : public QAbstractListModel
49         ThreadsScriptVerif,     // int
50         DatabaseCache,          // int
51         SpendZeroConfChange,    // bool
52+        AddressTypeLegacy,      // bool
53+        AddressTypeP2SH,        // bool
54+        AddressTypeBIP173,      // bool
    Sjors commented at 7:37 pm on January 12, 2018:
    AddressTypeBech32 would be more consistent with enum OutputType.
    promag commented at 2:17 am on January 16, 2018:
    Agree.
    hsjoberg commented at 11:13 pm on January 16, 2018:
    Fixed.
  39. Sjors changes_requested
  40. in src/qt/optionsmodel.cpp:392 in 450206b88c outdated 
    385@@ -381,6 +386,27 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
386                 setRestartRequired(true);
387             }
388             break;
389+        case AddressTypeLegacy:
390+            if (value.toBool()) {
    promag commented at 2:12 am on January 16, 2018:
    Looks like this code could be factored out to a function? (repeated 3 times)
  41. in src/qt/forms/optionsdialog.ui:200 in 450206b88c outdated 
    195+              <horstretch>0</horstretch>
196+              <verstretch>0</verstretch>
197+             </sizepolicy>
198+            </property>
199+            <property name="text">
200+             <string>P2SH</string>
    instagibbs commented at 6:41 pm on January 16, 2018:
    prefered(merged) nomenclature is “P2SH-Segwit”
    hsjoberg commented at 11:14 pm on January 16, 2018:
    @instagibbs Fixed.
  42. hsjoberg force-pushed on Jan 16, 2018
  43. hsjoberg commented at 11:17 pm on January 16, 2018: contributor

    Addressed some of the issues that have been pointed out as well as the compiling error, sorry for that.

    There are still some outstanding things that need to be addressed, I’ll look into them.

  44. jonasschnelli added the label GUI on Jan 17, 2018
  45. in src/qt/optionsmodel.cpp:112 in ff6abca252 outdated 
    109@@ -110,6 +110,11 @@ void OptionsModel::Init(bool resetSettings)
110         settings.setValue("bSpendZeroConfChange", true);
111     if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
112         addOverriddenOption("-spendzeroconfchange");
113+
114+    if (!settings.contains("strAddressType"))
115+        settings.setValue("strAddressType", QString::fromStdString(FormatOutputType(OUTPUT_TYPE_DEFAULT)));
    jonasschnelli commented at 6:58 am on January 17, 2018:
    We do that also at different places… but this makes changing defaults in the future really hard. If we the default for -addresstype, QT remains at the old state even if the user hasn’t changed the value there…
    jonasschnelli commented at 7:00 am on January 17, 2018:
    We can fix this in a follow up PR (that also fixes the other issues with defaults).
  46. jonasschnelli commented at 7:09 am on January 17, 2018: contributor

    Tested a bit and seems to work. What worries me is that the code has not the right concurrency protections to have those globals change during run-time. There would be some potential races (rare and often only possible when mixing QT with RPC).

    We can leave this for an follow up PR but I’d prefer the have the right instruments in place before adding runtime “sets” on those globals.

  47. gmaxwell commented at 11:23 am on January 17, 2018: contributor

    I don’t see any use case for the three way selection– it just confronts users with more technical details (complete with gobbltygopy technical terms like P2SH and BIP173 ) which many won’t know how to select among.

    The purpose of having the legacy setting is maintaining wallet backwards compatibility. .. but that’s blown out the moment you select either P2SH or BC1 addresses. What is the envisioned use-case for someone to set legacy but then sometimes pick P2SH? Without understanding that I am a Concept NAK.

    Being able to switch between BC1 and 3xxx addresses at receive time would very useful, but that should be a per-receive selection set when the address is requested.

  48. jonasschnelli commented at 7:48 pm on January 18, 2018: contributor
    Removing 0.16 milestone since there are some concerns with this PR. Also, seems not to be a blocker for 0.16 (especially since #11991).
  49. jonasschnelli removed this from the milestone 0.16.0 on Jan 18, 2018
  50. laanwj referenced this in commit 95941396ff on Jan 24, 2018
  51. molxyz commented at 4:51 pm on April 25, 2018: none
    I’m not sure why this PR isn’t merged? It makes much more sense to use this so I have been using it as my wallet. Please consider to merge this PR. Thank you!
  52. MarcoFalke referenced this in commit ef0e5cd517 on May 17, 2018
  53. MarcoFalke commented at 3:27 pm on May 23, 2018: member
    Needs rebase if still relevant. Otherwise I suggest to close.
  54. hsjoberg commented at 3:52 pm on May 23, 2018: contributor

    @molxyz @MarcoFalke Sorry for the delay.

    Yes, If there is interest I could continue work on this PR.

    There are some minor outstanding issues (albeit minor) regarding thread safety and other things that I’ll look into before merge.

    There doesn’t seem to be a clear consensus regarding if we should merge this. Personally I think it makes sense considering we already provide these options through the configuration file and as a command-line argument. I do understand the argument that it could be perceived as too technical though (not sure on how to improve on this?)

  55. MarcoFalke commented at 3:59 pm on May 23, 2018: member
    @hsjoberg have you seen #13251? I haven’t looked closely at you pull request, but they seem strongly related. And yours might not be relevant after that one has been merged.
  56. MarcoFalke commented at 3:59 pm on May 23, 2018: member
    If it is still relevant, please rebase so that it compiles at a minimum. (green travis)
  57. Qt: Adding address type GUI options 
    The Options-dialog now contains radio buttons for deciding which address
type to use (legacy, p2sh or BIP173 (bech32)).

This setting is connected to the `addresstype` parameter.
    872f281b69
  58. hsjoberg force-pushed on Jun 3, 2018
  59. DrahtBot closed this on Jul 29, 2018
  60. DrahtBot commented at 3:17 pm on July 29, 2018: member
  61. DrahtBot reopened this on Jul 29, 2018
  62. in src/qt/optionsmodel.h:9 in 872f281b69 
    5@@ -6,6 +6,7 @@
6 #define BITCOIN_QT_OPTIONSMODEL_H
7 
8 #include <amount.h>
9+#include <wallet/wallet.h>
    MarcoFalke commented at 4:10 pm on July 29, 2018:
    Why this include?
  63. MarcoFalke commented at 4:13 pm on July 29, 2018: member
    Hmm, I don’t think we should expose a “legacy” checkbox through the gui. I am missing the use case for that, given that “bech32” can already be unchecked.
  64. practicalswift commented at 7:55 am on September 15, 2018: contributor
    This PR doesn’t compile when rebased on master :-)
  65. MarcoFalke commented at 1:31 pm on September 15, 2018: member
    Closing for now
  66. MarcoFalke closed this on Sep 15, 2018
  67. TheArbitrator referenced this in commit 47013b051d on Jun 21, 2021
  68. MarcoFalke locked this on Sep 8, 2021


hsjoberg Sjors promag jonasschnelli luke-jr jb55 sipa molxyz TheBlueMatt instagibbs gmaxwell MarcoFalke DrahtBot practicalswift

Labels
GUI

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-29 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me