[qt] Receive: checkbox for bech32 address

Sjors commented at 2:54 pm on December 22, 2017: member

Checkbox does what you would expect. Press tab from the amount field to get there.

It’s unchecked by default.

When launched with -addresstype=bech32 it’s checked by default. When launched with -addresstype=legacy it unchecked and disabled.

The change in receivecoinsdialog.ui is smaller than it looks, due to the way git handles XML diffs. I had to add a horizontal spacer to make it look decent, see #11950 (comment). This causes column numbers to change in the rest of the grid.

I recommend testing on at least one other OS than OSX to be on the safe side.

Sjors commented at 2:55 pm on December 22, 2017: member

I haven’t tried, but this should work with or without #11937.

Sjors renamed this:
~~Segwit qt bech32 receive~~
[qt] Receive: check box for bech32 address
on Dec 22, 2017

Sjors renamed this:
~~[qt] Receive: check box for bech32 address~~
[qt] Receive: checkbox for bech32 address
on Dec 22, 2017

fanquake added the label GUI on Dec 23, 2017

laanwj added the label Wallet on Dec 23, 2017

gmaxwell commented at 0:58 am on December 24, 2017: contributor

Sounds like a good idea to me, when updating the PR you might want to fix the spelling in the commit message: reveice -> receive. utACK

Sjors force-pushed on Dec 24, 2017

sipa commented at 1:02 pm on December 24, 2017: member

Concept ACK. Needs rebase.

Sjors force-pushed on Dec 24, 2017

Sjors commented at 1:25 pm on December 24, 2017: member

Rebased on top of 5d92439 (and fixed typo in commit message).

By the way, I generally use git commit --amend --date "$(date)" to keep the commit timestamps fresh each time I rebase or change something. Any strong opinions against that?

in src/qt/walletmodel.h:221 in 2e68c57df0 outdated

216@@ -217,6 +217,8 @@ class WalletModel : public QObject
217     int getDefaultConfirmTarget() const;
218 
219     bool getDefaultWalletRbf() const;
220+    
221+    bool getDefaultBech32() const;

promag commented at 2:31 pm on December 28, 2017:

Expose address type instead getDefaultAddressType()?

Sjors commented at 10:36 am on January 2, 2018:

Done

Sjors commented at 10:49 am on January 2, 2018:

Although it introduces a compiler warning:

0qt/receivecoinsdialog.cpp:45:62: warning: comparison of constant 'OUTPUT_TYPE_BECH32' (3)
1with expression of type 'bool' is always false [-Wtautological-constant-out-of-range-compare]
2ui->bech32->setCheckState(model->getDefaultAddressType() == OUTPUT_TYPE_BECH32 ? Qt::Checked : Qt::Unchecked)

Sjors commented at 10:52 am on January 2, 2018:

Nvm, I broke it.

promag commented at 2:34 pm on December 28, 2017: member

utACK. Needs rebase.

jb55 commented at 10:48 pm on December 29, 2017: member

@Sjors what do think about aligning this next to the units dropdown instead of centered? Not sure if this is intended to look like this:

dec29-144346

Also needs rebase

Sjors commented at 10:56 pm on December 29, 2017: member

I’ll rebase after the SegWit stuff is merged. @jb55 good catch, it doesn’t look like that on OSX (see my own screenshot). It should be more or less attached to the currency dropdown, definitely not centered.

I’m guessing you’re on Ubuntu? I’ll try to reproduce. What happens when you make the window a little less wide?

I notice it shows “bits” for you, so you’ve somehow combined this with your own PR? That’s probably not related though.

jb55 commented at 0:12 am on December 30, 2017: member

I’m guessing you’re on Ubuntu? I’ll try to reproduce. What happens when you make the window a little less wide?

I’m using nixos with a tiling window manager (xmonad), allowing me to resize it to any size.

It gets closer to the dropdown when I make it shorter.

I notice it shows “bits” for you, so you’ve somehow combined this with your own PR? That’s probably not related though.

Yeah I’m just in my gui integration testing branch, but nothing else should be affecting this particular issue.

Sjors commented at 3:38 pm on December 31, 2017: member

I noticed the same behavior on Ubuntu 17.04. It’s fine with the default window size:

But misbehaves when you make it wider:

In fact, MacOS has the same issue (and the default window can be wider).

I don’t think it’s a showstopper, but I’ll try to fix it next year :-)

Sjors force-pushed on Jan 2, 2018

Sjors commented at 11:18 am on January 2, 2018: member

Rebased. Horizontally expanding the window now behaves.

jb55 commented at 3:43 pm on January 2, 2018: member

Tested ACK 4dc35c6c2677a470ee2189c11fa16aedc2b8df56

in src/qt/forms/receivecoinsdialog.ui:192 in 4dc35c6c26 outdated

187+             <string>An optional amount to request. Leave this empty or zero to not request a specific amount.</string>
188+            </property>
189+           </widget>
190+          </item>
191+          <item>
192+           <widget class="QCheckBox" name="bech32">

promag commented at 3:54 pm on January 2, 2018:

Nit, use_bech32_checkbox or similar? Just bech32 is a weak name IMO.

Sjors commented at 11:02 am on January 5, 2018:

I’ll rename to useBech32.

in src/qt/forms/receivecoinsdialog.ui:199 in 4dc35c6c26 outdated

194+             <sizepolicy hsizetype="Fixed" vsizetype="Fixed">
195+              <horstretch>0</horstretch>
196+              <verstretch>0</verstretch>
197+             </sizepolicy>
198+            </property>
199+            <property name="maximumSize">

promag commented at 3:55 pm on January 2, 2018:

Is this necessary?

Sjors commented at 4:15 pm on January 4, 2018:

The horizontal resize UI glitch I encountered above seems be caused by the input field and dropdown menu having a really large maximum width. In case of the currency dropdown, this causes the whitespace around the dropdown to stretch, which in turn causes it to be separated from the bech32 checkbox. I’m not sure if there’s another way to prevent that. I also don’t know if this max size plays well with l18n.

Sjors commented at 11:44 am on January 5, 2018:

It’s not causing any problems with German: :-)

in src/qt/addresstablemodel.cpp:344 in 4dc35c6c26 outdated

340@@ -341,7 +341,7 @@ void AddressTableModel::updateEntry(const QString &address,
341     priv->updateEntry(address, label, isMine, purpose, status);
342 }
343 
344-QString AddressTableModel::addRow(const QString &type, const QString &label, const QString &address)
345+QString AddressTableModel::addRow(const QString &type, const QString &label, const QString &address, const bool bech32)

promag commented at 3:57 pm on January 2, 2018:

Could receive OutputType type instead of bool bech32? Just pass that to AddDestinationForKey below.

Sjors commented at 11:35 am on January 5, 2018:

I think a boolean makes more sense here, because the check-box can only tell that OUTPUT_TYPE_BECH32 should be used. If it’s unchecked, it doesn’t know if the fallback is OUTPUT_TYPE_P2SH or OUTPUT_TYPE_LEGACY.

Sjors commented at 8:37 am on January 11, 2018:

Update: it now uses OutputType.

in src/qt/walletmodel.h:219 in 4dc35c6c26 outdated

215@@ -214,6 +216,9 @@ class WalletModel : public QObject
216 
217     bool hdEnabled() const;
218 
219+    // returns OUTPUT_TYPE_LEGACY, OUTPUT_TYPE_P2SH or OUTPUT_TYPE_BECH32

promag commented at 3:59 pm on January 2, 2018:

Remove comment? Return type is enough.

in src/qt/walletmodel.cpp:738 in 4dc35c6c26 outdated

732@@ -733,6 +733,11 @@ bool WalletModel::hdEnabled() const
733     return wallet->IsHDEnabled();
734 }
735 
736+OutputType WalletModel::getDefaultAddressType() const
737+{
738+    return g_address_type;

promag commented at 4:00 pm on January 2, 2018:

Nit, assert(g_address_type != OUTPUT_TYPE_NONE) ping @sipa.

Sjors commented at 8:31 pm on January 4, 2018:

This feature doesn’t really care if OUTPUT_TYPE_NONE Is returned though; the checkbox would be unchecked. So is this really the place for an assert? If so, I’m happy to add it.

sipa commented at 2:50 pm on January 16, 2018:

I think this is fine. NONE will be caught if it’s passed down and actually used somewhere.

promag commented at 4:03 pm on January 2, 2018: member

Correct me if I’m wrong but if g_address_type is OUTPUT_TYPE_BECH32 and the checkbox is unchecked, it will still use bech32 type.

Edit: in other words, maybe if g_address_type == OUTPUT_TYPE_BECH32 the checkbox should be checked and disabled.

Some review comments. Have to test though.

molxyz commented at 5:13 am on January 4, 2018: none

I compiled this PR for windows to test but the checkbox is not there. I guess right now this PR is only for linux?

Sjors commented at 4:16 pm on January 4, 2018: member

@molxyz QT is cross-platform, so if the check-box is missing on Windows that’s either a bug or something went wrong when you compiled it. I’ll try it in my own VM once I have that setup.

Sjors commented at 10:52 am on January 5, 2018: member

@promag when launched with -addresstype=bech32 unchecking the check box still produces a bech32 address. I’ll fix that.

promag commented at 11:00 am on January 5, 2018: member

@promag when launched with -addresstype=bech32 unchecking the check box still produces a bech32 address. I’ll fix that.

Right, either the checkbox should be hidden or disabled?

Sjors commented at 11:24 am on January 5, 2018: member

@promag I’ll disable it.

Sjors force-pushed on Jan 5, 2018

Sjors commented at 11:51 am on January 5, 2018: member

Rebased on top of 06cdb1d, which means I also updated the call to LearnRelatedScripts (not sure if that’s needed).

Checkbox is now checked and disabled when launched with -addresstype=bech32.

Sjors force-pushed on Jan 5, 2018

Sjors commented at 1:32 pm on January 5, 2018: member

address_types.py fails on Travis but passes on my machine.

Sjors commented at 1:26 pm on January 6, 2018: member

@molxyz it took me a bit of effort to compile QT on Windows, but the bech32 check box is there:

promag commented at 4:06 pm on January 10, 2018: member

2nd round 😄

Are we going to remove the checkbox in the future? If so I don’t think this is a great solution.

Maybe just add a new line:

0Label: [input]
1Amount: [input] [combo]
2Address type: [combo]            -- defaults to `g_address_type`
3Message: [input]

Sjors commented at 4:18 pm on January 10, 2018: member

@promag I suspect it will be long time before all wallets support bech32. Best we can hope for in the near term is to be able to check it by default.

If so I don’t think this is a great solution.

Why?

I think an address type combo is needlessly intimating, especially if we also add legacy address support. If the choice is only between bech32 and segwit-p2sh fallback, a check box seems simpler.

promag commented at 4:25 pm on January 10, 2018: member

It must be possible to request a payment for p2sh, think of older wallets.

Sjors commented at 5:31 pm on January 10, 2018: member

@promag that was my point. In other words, if at some point we make bech32 the default, we’d have the same check-box, but checked (and not disabled) by default.

promag commented at 5:44 pm on January 10, 2018: member

Currently it’s not possible to request payment to p2sh if -addresstype=bech32 or am I wrong?

Sjors commented at 7:55 am on January 11, 2018: member

Currently it’s not possible to request payment to p2sh if -addresstype=bech32 or am I wrong?

That’s correct. I haven’t bothered to solve that, because I don’t see a good reason why a user would go through the trouble of launching QT with -addresstype=bech32 and then not use bech32.

However, once we decide to make bech32 the default, then obviously the checkbox needs to be uncheckable. This PR kicks the can down the road a bit in terms of implementation details, but I think a checkbox makes sense.

Also note that when the user launches with -addresstype=legacy, the check box does work, but there’s no way to generate a segwit-p2sh address.

The underlying question is: how many address formats do we want the UI to support without using a launch param? The answer #11403 currently gives is 1. If the answer should be 3 then we need a dropdown, if the answer is 2 then a checkbox is sufficient.

Sjors force-pushed on Jan 11, 2018

Sjors commented at 8:37 am on January 11, 2018: member

Decided not to kick the can down the road and changed the checkbox behavior a bit:

disabled when launched with -addresstype=legacy
checkbox toggles between bech32 and p2sh-segwit

Also rebased.

Sjors force-pushed on Jan 11, 2018

promag commented at 10:07 am on January 11, 2018: member

It’s not intuitive it will use p2sh-segwit when running with -addresstype=bech32.

flack commented at 10:30 am on January 11, 2018: contributor

Couldn’t the checkbox be always unchecked and say “use Bech32 address” when started with -addresstype=p2sh-segwit and say “use P2SH-Segwit address” (or ideally something more comprehensible like “use backwards-compatible address”) when started with -addresstype=bech32?

promag commented at 10:58 am on January 11, 2018: member

@flack IMHO changing a UI element is not ideal. Users may expect the same thing every time? @Sjors There could be a new option in the “Expert” panel to show the address type selector?

Sjors commented at 11:16 am on January 11, 2018: member

It’s not intuitive it will use p2sh-segwit when running with -addresstype=bech32.

It doesn’t do that. When launched with -addresstype=bech32 it will use bech32. Unless the user unchecks the “bech32” box, in which case it uses p2sh-segwit. I don’t think the UI needs to account for scenarios where a user launches with a parameter; at that point I assume they know what they’re doing. I’m merely trying to not break it in that case.

I think the confusing thing here is the name of the parameter. It really should be called -defaultaddresstype=....

It’s the same in the RPC:

0bitcoind addresstype=bech32
1bitcoin-cli getnewaddress "" "p2sh-segwit"

There’s also not really a popular word for “not bech32”. This UI design is based on it being an opt-in feature, where users don’t need to know what happens when they don’t opt-in.

Renaming the check box might make sense if we change the default, but let’s wait and see how other wallets handle that by then.

promag commented at 11:21 am on January 11, 2018: member

When launched with -addresstype=bech32 it will use bech32. Unless the user unchecks the “bech32” box, in which case it uses p2sh-segwit.

That’s my point, why would the user assume it would be p2sh-segwit in that case? There is nothing indicating that.

Sjors commented at 2:30 pm on January 11, 2018: member

@promag I can change the tooltip in case the wallet is launched with -addresstype=bech32? It would explain the downgrade behavior if the user deselects it. This seems less disruptive than changing the label and more user friendly than a dropdown with two technical terms. Even if the user is still confused, the behavior would be quite obvious once they see the generated address.

promag commented at 2:43 pm on January 11, 2018: member

First and foremost, I agree that the UI must be simple and clean and avoid technical terms.

So with that in mind:

99% of the time the checkbox will be “visual trash” as it will have the correct state.
Even the combo box will be visual trash like the checkbox.
IMHO a checkbox + tooltip hammering is not the right UI solution for a simple yet technical choice.

So my suggestion is to add an option to enable the technical side of this UI and then present a combo box with explicit address types.

It’s like coin control.

flack commented at 3:37 pm on January 11, 2018: contributor

Since all the fields in the form are optional, you could almost say that they’re visual trash, too :-) At least in the sense that normally, you just want an address to paste somewhere.

But since we’re really scrutinizing this: Strictly speaking, it’s a bit strange that the checkbox is on the same line as Amount. There’s not really any logical connection between the amount you request and the address type you want. So maybe just move the thing to a new line, give it a proper “Address type” label and then simply have a dropdown with all available options (legacy/p2sh/bech32) with the -addresstype value preselected. Would be more consistent visually, and really hard to misunderstand

Sjors commented at 6:28 pm on January 11, 2018: member

@promag:

99% of the time the checkbox will be “visual trash”

I don’t think this is true. We don’t know how many users will be using bech32 and how often they’re able to use it. They might have some customers / friends / exchanges with a bech32 compatible wallet and others that don’t.

So my suggestion is to add an option to enable the technical side of this UI and then present a combo box with explicit address types. It’s like coin control.

I don’t think bech32 is as technically advanced and footgun as coin control. Do we really want to “hide” this feature behind a setting? @flack I would say moving it to a new line is something that can easily be done in a future PR. That’s a much smaller change. Let’s just roughly get in a place everyone can agree on.

laanwj added this to the milestone 0.16.0 on Jan 11, 2018

Sjors commented at 7:31 pm on January 11, 2018: member

I can’t reproduce this Travis failure on MacOS or Ubuntu 14.04 (I didn’t use the depends system).

in src/wallet/wallet.cpp:6 in 9eff99b407 outdated

2@@ -3,8 +3,6 @@
3 // Distributed under the MIT software license, see the accompanying
4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
5 
6-#include <wallet/wallet.h>

jonasschnelli commented at 7:36 pm on January 11, 2018:

Is this intentional? Could this cause the current travis issue?

Sjors commented at 7:50 pm on January 11, 2018:

@jonasschnelli I moved it to wallet.h because OutputType is needed in the header. My C++ is a bit rusty; do I need to keep it in the cpp file? My local machine doesn’t care.

jb55 commented at 7:57 pm on January 11, 2018:

Perhaps a forward declaration would suffice?

instagibbs commented at 8:26 pm on January 11, 2018:

I think this is likely the travis issue.

Has a similar error in Elements project that was exactly including wallet.h in a new file, we had to def guard it, may not be appropriate here.

Sjors commented at 4:43 pm on January 12, 2018:

@jb55 sadly ISO C++ forbids forward references to 'enum' types.

ryanofsky commented at 6:21 pm on January 12, 2018:

@jb55 sadly ISO C++ forbids forward references to ’enum’ types.

This is only true for enums that don’t specify a width. If you define an enum like enum MyEnum : int {...};, you can forward declare it with enum MyEnum : int;. So you could add : int to the OutputType definition in wallet.h in order to be able to forward declare it other places. Moving it seems fine, too, though.

jonasschnelli commented at 7:42 pm on January 11, 2018: contributor

Address is not showing correctly in the QRCode windows (macOS, QT5.10).

Sjors commented at 7:51 pm on January 11, 2018: member

@jonasschnelli what do you mean by not correctly? It’s cutting off the text below, or something else?

jonasschnelli commented at 7:52 pm on January 11, 2018: contributor

See the screenshot above,.. text is cutted off left and right.

Sjors commented at 8:09 pm on January 11, 2018: member

@jonasschnelli technically that’s a bug in #11403 (when you launch with -addresstype=bech32) :-)

I’ll try to fix it here.

in src/qt/forms/receivecoinsdialog.ui:209 in 9eff99b407 outdated

204+            </property>
205+            <property name="focusPolicy">
206+             <enum>Qt::StrongFocus</enum>
207+            </property>
208+            <property name="toolTip">
209+             <string>Bech32 addresses (BIP-173) are cheaper to spend from and offer better protection against typos. Not all wallets support this new format and your privacy may be impacted if not enough other people use it.</string>

achow101 commented at 9:16 pm on January 11, 2018:

Can this also say that not checking this will use the default address type (default or from bitcoin.conf/command line options) and what that address type is?

instagibbs commented at 9:19 pm on January 11, 2018:

@achow101 it’s going to become a bit of a mouthful. I propose taking out the language about privacy(not really actionable information imo), and more information about functional behavior as you said.

Sjors commented at 9:29 pm on January 11, 2018:

@achow101 my idea was to show change the tooltip depending on what the wallet default is: #11991 (comment) That should keep it short enough.

Can this also say that not checking this will use the default address type (default or from bitcoin.conf/command line options) and what that address type is

That’s not what it does. It’s bech32 when on, p2sh-segwit when off. When launched with legacy address type it’s disabled and unchecked, so hopefully clear enough that it won’t use p2sh-segwit in that case.

Sjors commented at 9:32 pm on January 11, 2018:

@instagibbs I can leave out the privacy remark, but I do think it’s actionable: people can google “bech32 privacy” (and find nothing for the moment…). I think it’s useful to point out that being the first to use a new address format makes you stand out.

instagibbs commented at 9:47 pm on January 11, 2018:

Probably best in release notes, imo. This information could quickly become false, depending on uptake.

achow101 commented at 9:50 pm on January 11, 2018:

@Sjors I don’t think it is clear that it will be using p2sh-segwit when unchecked. It’s trivial to just add a note to the tooltip indicating as such.

Sjors commented at 12:02 pm on January 12, 2018:

I’ll remove the privacy note and add something about p2sh-segwit.

Just bear in mind the current wallet doesn’t tell the user what p2sh-segwit is either. The use of SegWit is essentially hidden from the user. It “just works”. It’s bech32 that needs explaining, not the default behavior. I think the reason people may want to turn it on or off is because “they told me [not] to give them a bech32 address”, rather than “they told me [not] to give them p2sh-segwit address”.

Sjors force-pushed on Jan 12, 2018

Sjors commented at 5:43 pm on January 12, 2018: member

I moved enum OutputType to key.h in hopes of pleasing Travis. Let me know if that’s a bad place.

I removed the privacy remark from the tooltip and instead point out the P2SH-SegWit fallback.

bech32 address now fits in the QR code, which I made slightly wider and with a slightly larger font for improved readability. I don’t know why we want to render text inside a QR code image though, since the address is already shown in that dialog in a copyable way. But that’s for another time.

in src/qt/receiverequestdialog.cpp:187 in 90f9862b8d outdated

186-            QFont font = GUIUtil::fixedPitchFont();
187-            font.setPixelSize(12);
188+            painter.setRenderHint(QPainter::TextAntialiasing);
189+            painter.drawImage(10, 0, qrImage.scaled(QR_IMAGE_SIZE, QR_IMAGE_SIZE));
190+            QFont font;
191+            font.setPointSize(14);

jonasschnelli commented at 8:55 pm on January 12, 2018:

I think that won’t solve the problem… I’ll open a PR for a “long term” fix.

Sjors commented at 9:10 pm on January 12, 2018:

I forgot to upload the screenshot, but it worked for me on OSX.

jonasschnelli commented at 9:37 pm on January 12, 2018:

Didn’t worked for me. See #12173 which should fix it regardless of the use font / font-system.

jonasschnelli commented at 9:39 pm on January 12, 2018: contributor

One little conceptual thing. The checkbox is currently on the level where the amount and denomination is, is that ideal? I would suggest moving the checkbox down to the button row (or to an extra row?). Maybe before of after the Buttons…

Sjors force-pushed on Jan 13, 2018

Sjors commented at 2:03 pm on January 13, 2018: member

@jonasschnelli developers often use very large monitors. I don’t think it’s worth sacrificing a full row of address history. We’d also have to change the label, because “Use bech32 address” doesn’t fit in the left margin, and that row would look quite empty with only a checkbox, leading to ever more bike-shedding. I prefer to change this later if at all, preferably as part of larger UI cleanup.

I removed the font fix in favor of #12173.

in src/key.h:18 in 1379de29d4 outdated

14@@ -15,6 +15,15 @@
15 #include <stdexcept>
16 #include <vector>
17 
18+enum OutputType

laanwj commented at 8:45 am on January 15, 2018:

I don’t think this belongs in key.h. OutputType is only ever used as part of wallet code. There’s already potential confusion with txnouttype in script/standard.h, which is not your fault, but the confusion would become worse if OutputType is no longer wallet specific.

Sjors commented at 3:33 pm on January 15, 2018:

@laanwj I switched to @ryanofsky’s enum MyEnum : int approach and moved the enum back to wallet.h.

laanwj commented at 3:40 pm on January 15, 2018:

Thanks

sipa commented at 10:41 am on January 15, 2018: member

I haven’t looked at the code, but inside QR codes it’s advantageous to use uppercase Bech32 (it will result in a smaller QR encoding).

Sjors force-pushed on Jan 15, 2018

in src/qt/receivecoinsdialog.cpp:48 in 11a563ac81 outdated

42@@ -41,6 +43,17 @@ ReceiveCoinsDialog::ReceiveCoinsDialog(const PlatformStyle *_platformStyle, QWid
43         ui->removeRequestButton->setIcon(_platformStyle->SingleColorIcon(":/icons/remove"));
44     }
45 
46+    // configure bech32 checkbox, disable if launched with legacy as default:
47+    if (model->getDefaultAddressType() == OUTPUT_TYPE_BECH32) {
48+      ui->useBech32->setCheckState(Qt::Checked);

jonasschnelli commented at 7:14 am on January 16, 2018:

nit: 2 spaces rather then 4 spaces indentation? Not sure if that is after the guidelines…

Sjors commented at 10:01 am on January 16, 2018:

It’s not, it’s my editor misbehaving. Fixed (in four places).

jonasschnelli commented at 7:17 am on January 16, 2018: contributor

utACK 11a563ac81a95307c097a15a4243b079f7e45973 modulo style nits.

Sjors force-pushed on Jan 16, 2018

Sjors commented at 10:47 am on January 16, 2018: member

address_types.py failed on Travis. I doubt me adding 8 spaces caused that.

promag commented at 10:52 am on January 16, 2018: member

@Sjors restarted the failing job. Not the 1st time I see that test failing. Seems that sync_mempools sometimes doesn’t sync..

in src/qt/editaddressdialog.cpp:5 in c3752c4324 outdated

1@@ -2,6 +2,8 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5+#include <wallet/wallet.h>

ryanofsky commented at 4:23 pm on January 16, 2018:

Could declare g_address_type as extern if you wanted to remove this.

Sjors commented at 8:09 pm on January 16, 2018:

Done, but wasn’t able to do this in receivecoinsdialog (undeclared identifier 'OUTPUT_TYPE_BECH32, etc).

in src/qt/receivecoinsdialog.cpp:54 in c3752c4324 outdated

49+    } else {
50+        ui->useBech32->setCheckState(Qt::Unchecked);
51+    }
52+
53+    if (model->getDefaultAddressType() == OUTPUT_TYPE_LEGACY) {
54+        ui->useBech32->setEnabled(false);

ryanofsky commented at 4:25 pm on January 16, 2018:

Might want to call setenabled unconditionally (ui->useBech32->setEnabled(model->getDefaultAddressType() == OUTPUT_TYPE_LEGACY))) in case we want to support changing default address type in the ui in the future.

Sjors commented at 8:09 pm on January 16, 2018:

Done

in src/qt/receivecoinsdialog.cpp:149 in c3752c4324 outdated

145@@ -133,7 +146,11 @@ void ReceiveCoinsDialog::on_receiveButton_clicked()
146     QString address;
147     QString label = ui->reqLabel->text();
148     /* Generate new receiving address */
149-    address = model->getAddressTableModel()->addRow(AddressTableModel::Receive, label, "");
150+    OutputType address_type = g_address_type;

ryanofsky commented at 4:27 pm on January 16, 2018:

Maybe use model->getDefaultAddressType() here instead of g_address_type to be more consistent.

instagibbs commented at 7:04 pm on January 16, 2018:

yes then perhaps next line only use address_type for comparison.

Sjors commented at 8:09 pm on January 16, 2018:

Done

ryanofsky commented at 4:48 pm on January 16, 2018: member

Tested ACK c3752c4324ce8a4a9a615896e50f771df83f945d. Looks great!

Other feedback:

If I hover over the disabled checkbox with -addresstype=legacy, I still see tooltip suggesting that generated address will be a segwit address, which seems misleading. It would be better to hide the checkbox in this case, or to show a more accurate tooltip when the box is disabled. Hiding the checkbox doesn’t seem like a terrible thing given that the requested configuration is “legacy” and not having a checkbox was the old behavior.
It might be useful if address type were shown in requested payments details popup (displayed when you double click on a requested payment). This would make it easier for users to distinguish address types, and see effect of the new checkbox,
Would be nice to have some unit test coverage for this.
I tend to agree that -defaultaddresstype would be clearer name than -addresstype. Could open a separate PR to rename it and see what people think of the idea.
Maybe say “Generate Bech32 address” instead of “Use Bech32 address” since latter could imply that it is using something that already exists.
Maybe incorporate some help wording suggested by gmaxwell in “This will use an address that begins with BC1 instead of 1 and will result in lower transaction fees in the future. BC1 style addresses are not accepted everywhere yet. Turn off this option if the sending party says your address is invalid” or similar." https://botbot.me/freenode/bitcoin-core-dev/msg/95662952/

in src/qt/forms/receivecoinsdialog.ui:209 in c3752c4324 outdated

204+            </property>
205+            <property name="focusPolicy">
206+             <enum>Qt::StrongFocus</enum>
207+            </property>
208+            <property name="toolTip">
209+             <string>Bech32 addresses (BIP-173) are cheaper to spend from and offer better protection against typos. When unchecked a PS2SH wrapped SegWit address will be created, compatible with older wallets.</string>

instagibbs commented at 6:59 pm on January 16, 2018:

s/PS2SH /P2SH-/

Sjors commented at 8:09 pm on January 16, 2018:

Fixed

instagibbs approved

instagibbs commented at 7:13 pm on January 16, 2018: member

tACK, typo aside

Also thumbs-upped a few comments that I agree with, though non-blockers.

Sjors force-pushed on Jan 16, 2018

Sjors commented at 8:10 pm on January 16, 2018: member

hiding the checkbox if -addresstype=legacy
“Generate Bech32 address” instead of “Use Bech32 address”

Todo, though can probably be merged without:

unit test coverage
consider new tooltip wording

Leaving for future PRs:

address type were shown in requested payments details popup

-defaultaddresstype would be clearer name than -addresstype

Arguably too late if that doesn’t get into the 0.16 release.

[qt] receive tab: bech32 address opt-in checkbox

When launched with -adresstype=legacy the checkbox will be hidden.

63ac8907ce

Sjors force-pushed on Jan 16, 2018

Sjors commented at 8:25 pm on January 16, 2018: member

Pff, not sure how to avoid turning the tooltip into War & Peace. How about:

This will generate an address that begins with bc1 instead of 1 or 3 and will result in lower transaction fees in the future (BIP-173). bc1 style addresses are not accepted everywhere yet. If the sending party says your address is invalid, turn off this option to use a P2SH wrapped SegWit address.

flack commented at 8:59 pm on January 16, 2018: contributor

Here’s my stab at a slightly shorter version (270 chars insted of 359):

Generate an address that begins with bc1, which will lower your fees when you spend it (BIP173). These addresses are not accepted everywhere yet. If the sender says your address is invalid, turn off this option to generate a P2SH wrapped SegWit address beginning with 3.

ryanofsky commented at 9:03 pm on January 16, 2018: member

utACK 63ac8907ce6ab095b37858d6e96946b59ce1c13f. Changes since last review: removed wallet.h include, hiding checkbox with legacy option, using getDefaultAddressType instead of g_address_type another place, fixing PS2SH typo.

Pff, not sure how to avoid turning the tooltip into War & Peace. How about… #11991 (comment)

That seems very good. flack’s version is also ok, but I think yours is better because his kind of sounds like something magical about the letters “bc1” lowers the fees.

Arguably too late if that doesn’t get into the 0.16 release.

0.16 branch hasn’t even been created yet, so it seems not too late. Anyway I suggested creating a PR because I liked your idea, but thought it would be better to propose someplace else, rather than here.

flack commented at 9:24 pm on January 16, 2018: contributor

sounds like something magical about the letters “bc1” lowers the fees.

Well, it is Magical Internet Money after all :-)

on a slightly more serious note: I actually had to scratch my head for a minute when reading

This (…) will result in lower transaction fees in the future

because it’s not immediately obvious that the lower transaction fees only apply when you actually use the Bech32 address. If I’m not really familiar with how all this works, I might get the impression that all my future transactions will somehow get a discounted fee. Or is this meant to convey that the mempool would be less full if everybody used Bech32, thus lowering the fee for everyone?

ryanofsky commented at 9:49 pm on January 16, 2018: member

on a slightly more serious note: I actually had to scratch my head for a minute when reading This (…) will result in lower transaction fees in the future

Yes, definitely. I liked the part of your suggestion that says this “will lower your fees when you spend it.” I was going to mention this but forgot.

makomk commented at 11:44 pm on January 16, 2018: none

flack’s version is also ok, but I think yours is better because his kind of sounds like something magical about the letters “bc1” lowers the fees.

Perhaps it would be better worded as “an address that begins with bc1 and will lower your fees when you spend from it”?

in src/qt/editaddressdialog.cpp:14 in 63ac8907ce

10@@ -11,6 +11,8 @@
11 #include <QDataWidgetMapper>
12 #include <QMessageBox>
13 
14+extern OutputType g_address_type;

jonasschnelli commented at 6:42 am on January 17, 2018:

Can also be done in a follow up PR, but directly accessing the underlaying non QT system should be avoided. This should be accessed via WalletModel.

in src/qt/forms/receivecoinsdialog.ui:10 in 63ac8907ce

 6@@ -7,7 +7,7 @@
 7     <x>0</x>
 8     <y>0</y>
 9     <width>776</width>
10-    <height>364</height>
11+    <height>396</height>

jonasschnelli commented at 6:43 am on January 17, 2018:

Whats the reason for changing the height?

jonasschnelli commented at 6:44 am on January 17, 2018:

I guess it’s QT Creators magic… I guess others do usually edit those files “by hand” to avoid such “larger” unnecessary changes.

Sjors commented at 9:12 am on January 17, 2018:

It was magic. Didn’t seem worth tweaking with so many other changes in that file, but will keep in mind.

jonasschnelli approved

jonasschnelli commented at 6:48 am on January 17, 2018: contributor

Tested ACK 63ac8907ce6ab095b37858d6e96946b59ce1c13f

jonasschnelli merged this on Jan 17, 2018

jonasschnelli closed this on Jan 17, 2018

jonasschnelli referenced this in commit 062c8b69f4 on Jan 17, 2018

Sjors deleted the branch on Jan 17, 2018

jonasschnelli referenced this in commit 10d10d7fad on Jan 18, 2018

laanwj referenced this in commit 95941396ff on Jan 24, 2018

PastaPastaPasta referenced this in commit 6bfb836bf9 on Jan 17, 2020

PastaPastaPasta referenced this in commit 4d560c1f55 on Feb 12, 2020

DrahtBot locked this on Sep 8, 2021

[qt] Receive: checkbox for bech32 address #11991