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

hsjoberg · 2017-12-18T15:27:22Z

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.

Sjors · 2017-12-18T16:18:56Z

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.

hsjoberg · 2017-12-18T16:30:42Z

@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.

Sjors · 2017-12-18T17:24:11Z

No problem, it's easy to work around:

Sjors

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.

Sjors · 2017-12-18T17:28:36Z

src/qt/forms/optionsdialog.ui

+                </sizepolicy>
+               </property>
+               <property name="text">
+                <string>BIP173 (bech32)</string>


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

Sure, I'll look into that.
Any suggestions of exactly what the tooltip(s) should say?

Sjors · 2017-12-18T17:29:37Z

src/qt/optionsdialog.cpp

@@ -180,6 +181,10 @@ void OptionsDialog::setMapper()
    mapper->addMapping(ui->spendZeroConfChange, OptionsModel::SpendZeroConfChange);
    mapper->addMapping(ui->coinControlFeatures, OptionsModel::CoinControlFeatures);

+    mapper->addMapping(ui->addressTypeLegacy, OptionsModel::AddressTypeLegacy);


Do we really want to maintain UI support for legacy addresses? Maybe only show this option if the app was launched with -addresstype=legacy?

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?

Nvm, it's fine for a setting, especially combined with the per address checkbox in #11991.

promag · 2017-12-18T21:19:10Z

src/qt/optionsmodel.cpp

@@ -110,6 +110,11 @@ void OptionsModel::Init(bool resetSettings)
        settings.setValue("bSpendZeroConfChange", true);
    if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
        addOverriddenOption("-spendzeroconfchange");
+
+    if(!settings.contains("strAddressType"))


Nit, missing space after if, missing {}.

Thanks, fixing missing space. :)

promag · 2017-12-18T21:19:50Z

src/qt/optionsmodel.cpp

+
+    if(!settings.contains("strAddressType"))
+      settings.setValue("strAddressType", QString::fromStdString(FormatOutputType(OUTPUT_TYPE_DEFAULT)));
+    if (!gArgs.SoftSetArg("-addresstype", settings.value("strAddressType").toString().toStdString()))


Nit, missing {}.

promag · 2017-12-18T21:23:00Z

src/qt/optionsdialog.cpp

+        break;
+    case OUTPUT_TYPE_NONE:
+    default:
+        break;


If this is not hit then assert?

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.

I think an assert would be appropriate here (stop the app if address type is unknown to the GUI).

@hsjoberg ping.

promag · 2017-12-18T22:04:30Z

src/qt/forms/optionsdialog.ui

@@ -163,6 +163,57 @@
            </property>
           </widget>
          </item>
+          <item>
+           <widget class="QGroupBox" name="groupBoxAddressType">


This is inside Export group box, should be on the outside instead?

hsjoberg · 2017-12-18T23:37:29Z

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.

hsjoberg · 2017-12-19T00:03:43Z

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.

hsjoberg · 2017-12-19T00:11:19Z

Rebased to fix some of the nits.

luke-jr · 2017-12-19T03:04:07Z

GUI settings probably shouldn't influence RPC like this, and IMO it should really be exposed per-receive.

Sjors · 2017-12-19T09:12:39Z

@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.

hsjoberg · 2017-12-19T17:51:07Z

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.

hsjoberg · 2017-12-19T17:56:33Z

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.

Sjors · 2017-12-22T14:57:20Z

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.

jb55 · 2017-12-30T22:33:12Z

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.

sipa · 2017-12-31T04:07:34Z

@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.

hsjoberg · 2017-12-31T13:35:33Z

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.

molxyz · 2018-01-02T18:59:00Z

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

jonasschnelli · 2018-01-04T08:08:49Z

src/qt/optionsmodel.cpp

+        case AddressTypeLegacy:
+            if (value.toBool()) {
+                settings.setValue("strAddressType", QString::fromStdString(FormatOutputType(OUTPUT_TYPE_LEGACY)));
+                g_address_type = OUTPUT_TYPE_LEGACY;


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).

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 :/

Guard with cs_main? Most accesses already held the lock. Fix the remaining.

jonasschnelli · 2018-01-04T08:09:22Z

Nice!
Concept ACK

jonasschnelli · 2018-01-11T07:23:03Z

Needs rebase

hsjoberg · 2018-01-11T08:30:33Z

@jonasschnelli I'll rebase to master as soon as I have time.

hsjoberg · 2018-01-16T23:17:31Z

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.

jonasschnelli · 2018-01-17T06:58:52Z

src/qt/optionsmodel.cpp

@@ -110,6 +110,11 @@ void OptionsModel::Init(bool resetSettings)
        settings.setValue("bSpendZeroConfChange", true);
    if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
        addOverriddenOption("-spendzeroconfchange");
+
+    if (!settings.contains("strAddressType"))
+        settings.setValue("strAddressType", QString::fromStdString(FormatOutputType(OUTPUT_TYPE_DEFAULT)));


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...

We can fix this in a follow up PR (that also fixes the other issues with defaults).

jonasschnelli · 2018-01-17T07:09:13Z

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.

gmaxwell · 2018-01-17T11:23:12Z

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.

jonasschnelli · 2018-01-18T19:48:58Z

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).

… P2WPKH or P2WSH 596c446 [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH (Sjors Provoost) Pull request description: If `-changetype` is not explicitly set, then regardless of `-addresstype`, the wallet will use a ~`bech32` change address~ `P2WPKH` change output if any destination is `P2WPKH` or `P2WSH`. This seems more intuitive to me and more in line with the spirit of [BIP-69](https://github.com/bitcoin/bips/blob/master/bip-0069.mediawiki). When combined with #11991 a QT user could opt to use `bech32` exclusively without having to figure out how to launch with `-changetype=bech32`, although so would #11937. Tree-SHA512: 9238d3ccd1f3be8dfdd43444ccf45d6bdc6584ced3172a3045f3ecfec4a7cc8999db0cdb76ae49236492a84e6dbf3a1fdf18544d3eaf6d518e1f8bd241db33e7

molxyz · 2018-04-25T16:51:12Z

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!

… legacy address default 82dda6b GUI: Allow generating Bech32 addresses with a legacy-address default (Luke Dashjr) 7ab1c6f GUI: Rephrase Bech32 checkbox text/tooltip (Luke Dashjr) Pull request description: - "Bech32" isn't very user-friendly; used "native segwit" as in #11937. - You don't spend from addresses. - No reason to block off Bech32 access with legacy address default. Rebased from #12208 Tree-SHA512: c82dd20d967a7f47bcc75b25be0d3a8cf00cfccc1cd14916b87d70b9c56fd53e366b456348b173f36c89b145b76624413780abaed4cea82117a9ecd47dd8fb99

maflcko · 2018-05-23T15:27:45Z

Needs rebase if still relevant. Otherwise I suggest to close.

hsjoberg · 2018-05-23T15:52:16Z

@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?)

maflcko · 2018-05-23T15:59:03Z

@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.

maflcko · 2018-05-23T15:59:30Z

If it is still relevant, please rebase so that it compiles at a minimum. (green travis)

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.

DrahtBot · 2018-07-29T15:17:38Z

The last travis run for this pull request was 55 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

maflcko · 2018-07-29T16:10:57Z

src/qt/optionsmodel.h

@@ -6,6 +6,7 @@
 #define BITCOIN_QT_OPTIONSMODEL_H

 #include <amount.h>
+#include <wallet/wallet.h>


Why this include?

maflcko · 2018-07-29T16:13:04Z

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.

practicalswift · 2018-09-15T07:55:37Z

This PR doesn't compile when rebased on master :-)

maflcko · 2018-09-15T13:31:46Z

Closing for now

…it with legacy address default 82dda6b GUI: Allow generating Bech32 addresses with a legacy-address default (Luke Dashjr) 7ab1c6f GUI: Rephrase Bech32 checkbox text/tooltip (Luke Dashjr) Pull request description: - "Bech32" isn't very user-friendly; used "native segwit" as in bitcoin#11937. - You don't spend from addresses. - No reason to block off Bech32 access with legacy address default. Rebased from bitcoin#12208 Tree-SHA512: c82dd20d967a7f47bcc75b25be0d3a8cf00cfccc1cd14916b87d70b9c56fd53e366b456348b173f36c89b145b76624413780abaed4cea82117a9ecd47dd8fb99

hsjoberg changed the title ~~GUI: Setting for deciding address type (legacy, p2sh or bech32)~~ Qt: Setting for deciding address type (legacy, p2sh or bech32) Dec 18, 2017

Sjors reviewed Dec 18, 2017

View reviewed changes

promag reviewed Dec 18, 2017

View reviewed changes

hsjoberg force-pushed the segwitwalletguioption branch from 13adc63 to 62fe665 Compare December 19, 2017 00:09

hsjoberg force-pushed the segwitwalletguioption branch from 62fe665 to 3962c69 Compare December 19, 2017 17:47

hsjoberg force-pushed the segwitwalletguioption branch from 3962c69 to 054d5fd Compare December 19, 2017 18:12

Sjors mentioned this pull request Dec 22, 2017

[qt] Receive: checkbox for bech32 address #11991

Merged

jonasschnelli reviewed Jan 4, 2018

View reviewed changes

Sjors mentioned this pull request Jan 8, 2018

[wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH #12119

Merged

jonasschnelli mentioned this pull request Jan 11, 2018

Generate segwit address in receive payment tab? #11124

Closed

laanwj added this to the 0.16.0 milestone Jan 11, 2018

hsjoberg force-pushed the segwitwalletguioption branch from 054d5fd to 450206b Compare January 11, 2018 22:19

hsjoberg force-pushed the segwitwalletguioption branch from 450206b to ff6abca Compare January 16, 2018 23:13

jonasschnelli added the GUI label Jan 17, 2018

jonasschnelli reviewed Jan 17, 2018

View reviewed changes

luke-jr mentioned this pull request Jan 17, 2018

GUI: Rephrase Bech32 checkbox texts, and enable it with legacy address default #12208

Closed

jonasschnelli removed this from the 0.16.0 milestone Jan 18, 2018

promag mentioned this pull request Feb 23, 2018

GUI: file->receiving addresses->New only generates non-bech32 addresses, even if the box "generate bech32" is ticked in the main window #12520

Closed

fanquake mentioned this pull request May 17, 2018

GUI: Rephrase Bech32 checkbox texts, and enable it with legacy address default #13251

Merged

Qt: Adding address type GUI options

872f281

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.

hsjoberg force-pushed the segwitwalletguioption branch from ff6abca to 872f281 Compare June 3, 2018 21:47

DrahtBot closed this Jul 29, 2018

DrahtBot reopened this Jul 29, 2018

maflcko reviewed Jul 29, 2018

View reviewed changes

maflcko closed this Sep 15, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Conversation

hsjoberg commented Dec 18, 2017 • edited by DrahtBot Loading

Sjors commented Dec 18, 2017

hsjoberg commented Dec 18, 2017 • edited Loading

Sjors commented Dec 18, 2017

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsjoberg commented Dec 18, 2017

hsjoberg commented Dec 19, 2017

hsjoberg commented Dec 19, 2017 • edited Loading

luke-jr commented Dec 19, 2017

Sjors commented Dec 19, 2017

hsjoberg commented Dec 19, 2017

hsjoberg commented Dec 19, 2017 • edited Loading

Sjors commented Dec 22, 2017

jb55 commented Dec 30, 2017

sipa commented Dec 31, 2017

hsjoberg commented Dec 31, 2017 • edited Loading

molxyz commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Jan 4, 2018

jonasschnelli commented Jan 11, 2018

hsjoberg commented Jan 11, 2018

hsjoberg commented Jan 16, 2018

jonasschnelli Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Jan 17, 2018

gmaxwell commented Jan 17, 2018

jonasschnelli commented Jan 18, 2018

molxyz commented Apr 25, 2018 • edited Loading

maflcko commented May 23, 2018

hsjoberg commented May 23, 2018

maflcko commented May 23, 2018

maflcko commented May 23, 2018

DrahtBot commented Jul 29, 2018

Choose a reason for hiding this comment

maflcko commented Jul 29, 2018

practicalswift commented Sep 15, 2018

maflcko commented Sep 15, 2018

hsjoberg commented Dec 18, 2017 •

edited by DrahtBot

Loading

hsjoberg commented Dec 18, 2017 •

edited

Loading

hsjoberg commented Dec 19, 2017 •

edited

Loading

hsjoberg commented Dec 19, 2017 •

edited

Loading

hsjoberg commented Dec 31, 2017 •

edited

Loading

jonasschnelli Jan 17, 2018 •

edited

Loading

molxyz commented Apr 25, 2018 •

edited

Loading