[qt] Receive: checkbox for bech32 address #11991

Sjors · 2017-12-22T14:54:32Z

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 · 2017-12-22T14:55:58Z

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

gmaxwell · 2017-12-24T00:58:25Z

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

sipa · 2017-12-24T13:02:40Z

Concept ACK. Needs rebase.

Sjors · 2017-12-24T13:25:43Z

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?

promag

utACK. Needs rebase.

promag · 2017-12-28T14:31:12Z

src/qt/walletmodel.h

@@ -217,6 +217,8 @@ class WalletModel : public QObject
    int getDefaultConfirmTarget() const;

    bool getDefaultWalletRbf() const;
+
+    bool getDefaultBech32() const;


Expose address type instead getDefaultAddressType()?

Although it introduces a compiler warning:

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

Nvm, I broke it.

jb55 · 2017-12-29T22:48:49Z

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

Also needs rebase

Sjors · 2017-12-29T22:56:34Z

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 · 2017-12-30T00:12:32Z

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 · 2017-12-31T15:38:15Z

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 · 2018-01-02T11:18:45Z

Rebased. Horizontally expanding the window now behaves.

jb55 · 2018-01-02T15:43:34Z

Tested ACK 4dc35c6c2677a470ee2189c11fa16aedc2b8df56

promag

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.

promag · 2018-01-02T15:54:11Z

src/qt/forms/receivecoinsdialog.ui

+           </widget>
+          </item>
+          <item>
+           <widget class="QCheckBox" name="bech32">


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

I'll rename to useBech32.

promag · 2018-01-02T15:55:46Z

src/qt/forms/receivecoinsdialog.ui

+              <verstretch>0</verstretch>
+             </sizepolicy>
+            </property>
+            <property name="maximumSize">


Is this necessary?

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.

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

promag · 2018-01-02T15:57:44Z

src/qt/addresstablemodel.cpp

@@ -341,7 +341,7 @@ void AddressTableModel::updateEntry(const QString &address,
    priv->updateEntry(address, label, isMine, purpose, status);
 }

-QString AddressTableModel::addRow(const QString &type, const QString &label, const QString &address)
+QString AddressTableModel::addRow(const QString &type, const QString &label, const QString &address, const bool bech32)


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

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.

Update: it now uses OutputType.

promag · 2018-01-02T15:59:25Z

src/qt/walletmodel.h

@@ -214,6 +216,9 @@ class WalletModel : public QObject

    bool hdEnabled() const;

+    // returns OUTPUT_TYPE_LEGACY, OUTPUT_TYPE_P2SH or OUTPUT_TYPE_BECH32


Remove comment? Return type is enough.

promag · 2018-01-02T16:00:33Z

src/qt/walletmodel.cpp

@@ -733,6 +733,11 @@ bool WalletModel::hdEnabled() const
    return wallet->IsHDEnabled();
 }

+OutputType WalletModel::getDefaultAddressType() const
+{
+    return g_address_type;


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

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.

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

molxyz · 2018-01-04T05:13:05Z

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 · 2018-01-04T16:16:01Z

@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 · 2018-01-05T10:52:47Z

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

promag · 2018-01-05T11:00:40Z

@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 · 2018-01-05T11:24:05Z

@promag I'll disable it.

Sjors · 2018-01-16T10:47:05Z

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

promag · 2018-01-16T10:52:33Z

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

ryanofsky

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/

ryanofsky · 2018-01-16T16:23:16Z

src/qt/editaddressdialog.cpp

@@ -2,6 +2,8 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.

+#include <wallet/wallet.h>


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

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

ryanofsky · 2018-01-16T16:25:56Z

src/qt/receivecoinsdialog.cpp

+    }
+
+    if (model->getDefaultAddressType() == OUTPUT_TYPE_LEGACY) {
+        ui->useBech32->setEnabled(false);


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.

ryanofsky · 2018-01-16T16:27:42Z

src/qt/receivecoinsdialog.cpp

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


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

yes then perhaps next line only use address_type for comparison.

instagibbs

tACK, typo aside

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

instagibbs · 2018-01-16T18:59:16Z

src/qt/forms/receivecoinsdialog.ui

+             <enum>Qt::StrongFocus</enum>
+            </property>
+            <property name="toolTip">
+             <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>


s/PS2SH /P2SH-/

instagibbs · 2018-01-16T19:04:09Z

src/qt/receivecoinsdialog.cpp

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


yes then perhaps next line only use address_type for comparison.

Sjors · 2018-01-16T20:10:45Z

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.

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

Sjors · 2018-01-16T20:25:06Z

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 · 2018-01-16T20:59:19Z

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

utACK 63ac890. 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 · 2018-01-16T21:24:04Z

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 · 2018-01-16T21:49:52Z

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 · 2018-01-16T23:44:26Z

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"?

jonasschnelli · 2018-01-17T06:42:50Z

src/qt/editaddressdialog.cpp

@@ -11,6 +11,8 @@
 #include <QDataWidgetMapper>
 #include <QMessageBox>

+extern OutputType g_address_type;


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.

jonasschnelli · 2018-01-17T06:43:13Z

src/qt/forms/receivecoinsdialog.ui

@@ -7,7 +7,7 @@
    <x>0</x>
    <y>0</y>
    <width>776</width>
-    <height>364</height>
+    <height>396</height>


Whats the reason for changing the height?

I guess it's QT Creators magic... I guess others do usually edit those files "by hand" to avoid such "larger" unnecessary changes.

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

jonasschnelli

Tested ACK 63ac890

63ac890 [qt] receive tab: bech32 address opt-in checkbox (Sjors Provoost) Pull request description: <img width="647" alt="schermafbeelding 2018-01-12 om 18 34 48" src="https://user-images.githubusercontent.com/10217/34887691-a6a796fe-f7c7-11e7-8b89-87ce07c61ce3.png"> 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. Tree-SHA512: ec4b733b796d9a94278a5d8040a69d9574ef50021e68f94f61f2da75d1bb57f39272cbc9f1f7d34f733a19640daf666a23844fcd132f83bfdaf327d9d1d6f105

…constructor cc90a4f Avoid potential null dereference in ReceiveCoinsDialog constructor (Russell Yanofsky) Pull request description: Not a bug in practice because current `WalletModel::getDefaultAddressType()` implementation does not dereference its `this` pointer. Encountered issue while rebasing #10244 after #11991 was merged. Tree-SHA512: d76afc410d4a436ec62936196fdac1af89c221d8c0d6e73349024afe55bbf8820f843177a8fe8210aa8021d45a17a0ecd9b6f693381e3edb234d9897cece29d7

… 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

…Dialog constructor cc90a4f Avoid potential null dereference in ReceiveCoinsDialog constructor (Russell Yanofsky) Pull request description: Not a bug in practice because current `WalletModel::getDefaultAddressType()` implementation does not dereference its `this` pointer. Encountered issue while rebasing bitcoin#10244 after bitcoin#11991 was merged. Tree-SHA512: d76afc410d4a436ec62936196fdac1af89c221d8c0d6e73349024afe55bbf8820f843177a8fe8210aa8021d45a17a0ecd9b6f693381e3edb234d9897cece29d7

Sjors mentioned this pull request Dec 22, 2017

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

Closed

Sjors changed the title ~~Segwit qt bech32 receive~~ [qt] Receive: check box for bech32 address Dec 22, 2017

Sjors changed the title ~~[qt] Receive: check box for bech32 address~~ [qt] Receive: checkbox for bech32 address Dec 22, 2017

fanquake added the GUI label Dec 23, 2017

laanwj added the Wallet label Dec 23, 2017

Sjors force-pushed the segwit-qt-bech32-receive branch from fd2d57a to 92fca0e Compare December 24, 2017 10:22

Sjors force-pushed the segwit-qt-bech32-receive branch 2 times, most recently from 94c218e to 2e68c57 Compare December 24, 2017 13:23

promag reviewed Dec 28, 2017

View reviewed changes

Sjors force-pushed the segwit-qt-bech32-receive branch 4 times, most recently from f58e356 to 4dc35c6 Compare January 2, 2018 11:16

promag reviewed Jan 2, 2018

View reviewed changes

Sjors force-pushed the segwit-qt-bech32-receive branch from 4dc35c6 to 9ee4b06 Compare January 5, 2018 11:45

ryanofsky reviewed Jan 16, 2018

View reviewed changes

instagibbs approved these changes Jan 16, 2018

View reviewed changes

Sjors force-pushed the segwit-qt-bech32-receive branch from c3752c4 to aac36a5 Compare January 16, 2018 20:10

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

63ac890

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

Sjors force-pushed the segwit-qt-bech32-receive branch from aac36a5 to 63ac890 Compare January 16, 2018 20:11

ryanofsky mentioned this pull request Jan 16, 2018

Intermittent address_types.py test failures #12205

Closed

ryanofsky reviewed Jan 16, 2018

View reviewed changes

jonasschnelli reviewed Jan 17, 2018

View reviewed changes

jonasschnelli approved these changes Jan 17, 2018

View reviewed changes

jonasschnelli merged commit 63ac890 into bitcoin:master Jan 17, 2018

Sjors deleted the segwit-qt-bech32-receive branch January 17, 2018 09:15

flack mentioned this pull request Jan 17, 2018

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

Closed

ryanofsky mentioned this pull request Jan 17, 2018

Avoid potential null dereference in ReceiveCoinsDialog constructor #12211

Merged

laanwj mentioned this pull request Jan 18, 2018

Generate segwit address in receive payment tab? #11124

Closed

meyer9 mentioned this pull request Apr 29, 2018

Should be able to generate bech32 addresses from QT GUI phoreproject/Phore#37

Closed

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

		@@ -214,6 +216,9 @@ class WalletModel : public QObject

		bool hdEnabled() const;

		// returns OUTPUT_TYPE_LEGACY, OUTPUT_TYPE_P2SH or OUTPUT_TYPE_BECH32

[qt] Receive: checkbox for bech32 address #11991

[qt] Receive: checkbox for bech32 address #11991

Conversation

Sjors commented Dec 22, 2017 • edited

Sjors commented Dec 22, 2017

gmaxwell commented Dec 24, 2017

sipa commented Dec 24, 2017

Sjors commented Dec 24, 2017

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors Jan 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jb55 commented Dec 29, 2017

Sjors commented Dec 29, 2017

jb55 commented Dec 30, 2017 via email

Sjors commented Dec 31, 2017

Sjors commented Jan 2, 2018

jb55 commented Jan 2, 2018 via email

promag left a comment • edited

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

Sjors Jan 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molxyz commented Jan 4, 2018 • edited

Sjors commented Jan 4, 2018

Sjors commented Jan 5, 2018

promag commented Jan 5, 2018

Sjors commented Jan 5, 2018

Sjors commented Jan 16, 2018

promag commented Jan 16, 2018

ryanofsky left a comment • edited

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

instagibbs 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

Sjors commented Jan 16, 2018

Sjors commented Jan 16, 2018 • edited

flack commented Jan 16, 2018

ryanofsky left a comment • edited

Choose a reason for hiding this comment

flack commented Jan 16, 2018

ryanofsky commented Jan 16, 2018

makomk commented Jan 16, 2018

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

jonasschnelli left a comment

Choose a reason for hiding this comment

Sjors commented Dec 22, 2017 •

edited

Sjors Jan 2, 2018 •

edited

promag left a comment •

edited

Sjors Jan 4, 2018 •

edited

molxyz commented Jan 4, 2018 •

edited

ryanofsky left a comment •

edited

Sjors commented Jan 16, 2018 •

edited

ryanofsky left a comment •

edited