Address type dropdown, add Taproot (Receive tab)

Sjors commented at 12:11 pm on October 29, 2021: member

This PR replaces the Bech32 checkbox with an address type dropdown It “Use Taproot” checkbox to the receive screen for any wallet with a Taproot descriptor. The Bech32m option is hidden for wallets that don’t have taproot descriptors.

Bech32 is kept as the default even for Taproot enabled wallets until it’s more widely supported.

(an earlier attempt of this PR added a second checkbox)

Suggested testing

notice that the Bech32m entry only appears for wallet with a taproot descriptor
verify that it generates a bc1p instead of bc1q address

Legacy wallet
Default descriptor wallet (current does not have taproot descriptor)
Wallet with taproot descriptor:
- just create a new descriptor wallet

Replaces https://github.com/bitcoin/bitcoin/pull/22260

Sjors renamed this:
~~Use Taproot checkbox for receive tab~~
Add Taproot checkbox to receive tab
on Oct 29, 2021

in src/wallet/interfaces.cpp:463 in 943fb5e34a outdated

457@@ -458,6 +458,11 @@ class WalletImpl : public Wallet
458     bool canGetAddresses() override { return m_wallet->CanGetAddresses(); }
459     bool hasExternalSigner() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); }
460     bool privateKeysDisabled() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); }
461+    bool taprootEnabled() override {
462+        if (m_wallet->IsLegacy()) return false;
463+        auto spk_man = m_wallet->GetScriptPubKeyMan(OutputType::BECH32M, false);

MarcoFalke commented at 12:34 pm on October 29, 2021:

0        auto spk_man = m_wallet->GetScriptPubKeyMan(OutputType::BECH32M, /*internal=*/false);

nit: might be nice to use named args

Sjors force-pushed on Oct 29, 2021

jarolrod added the label Feature on Oct 29, 2021

luke-jr changes_requested

luke-jr commented at 5:30 pm on October 29, 2021: member

Kinda ugly to have two mutually-exclusive (logically) checkboxes between Legacy/Bech32 and ???/Bech32m…

Probably should just make the existing checkbox a combobox or something?

Sjors commented at 7:37 pm on October 29, 2021: member

They’re not mutually exclusive. I’m intentionally trying not to bother the user with the distinction between bech32 and bech32m (except in the tooltip). The bech32 button covers both. The new button is about using Taproot, as opposed to v0 SegWit. If you uncheck bech32 it will disable the Taproot checkbox, because there is no p2sh version of taproot.

luke-jr commented at 10:52 pm on October 29, 2021: member

IMO having 2 checkboxes like this is even more confusing.

Sjors commented at 12:26 pm on October 30, 2021: member

We could show all address types: legacy, p2sh-segwit, SegWit v0 and Taproot. Perhaps the least confusing way to do that would be a dropdown that defaults to SegWit v0.

ghost commented at 6:36 am on October 31, 2021: none

We could show all address types: legacy, p2sh-segwit, SegWit v0 and Taproot. Perhaps the least confusing way to do that would be a dropdown that defaults to SegWit v0.

This makes sense

sipa commented at 5:20 pm on October 31, 2021: contributor

I think it’s wrong to have it list “taproot” explicitly (or witness v0, for that matter); in descriptor wallets that’s an implementation aspect the user shouldn’t care about. What they care is legacy(base58) vs. bech32 vs bech32m, as those correspond to what senders supports. If a witness v2 softfork were to occur, hopefully bech32m can be reused unmodified for its addresses.

The OutputType isn’t called TAPROOT for a reason; it’s BECH32M because that’s what senders care about. In a descriptor wallet, the wallet configuration determines which addresses are used for which outputtype.

FWIW, I think we should get rid of the LEGACY/P2SH_SEGWIT OutputTypes, and merge them into a BASE58 one or so.

in src/wallet/interfaces.cpp:465 in f04337ebae outdated

457@@ -458,6 +458,11 @@ class WalletImpl : public Wallet
458     bool canGetAddresses() override { return m_wallet->CanGetAddresses(); }
459     bool hasExternalSigner() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); }
460     bool privateKeysDisabled() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); }
461+    bool taprootEnabled() override {
462+        if (m_wallet->IsLegacy()) return false;

MarcoFalke commented at 1:57 pm on November 1, 2021:

Is this special case needed? Wouldn’t it return nullptr below?

Sjors commented at 2:09 pm on November 1, 2021:

Maybe not, but since legacy wallets don’t have descriptors, it does seem cleaner.

MarcoFalke commented at 2:17 pm on November 1, 2021: contributor

FWIW, I think we should get rid of the LEGACY/P2SH_SEGWIT OutputTypes, and merge them into a BASE58 one or so.

In that case we should also never ever send change to LEGACY again, unless manually done so by the user?

luke-jr commented at 4:49 pm on November 1, 2021: member

Then how do users continue avoiding Segwit? (Perhaps there’s no reason to avoid Taproot, but there isn’t a Base58 option for that, and it requires a new wallet)

Sjors commented at 7:42 pm on November 1, 2021: member

Based on the above discussion, I’m starting to like my own approach more :-)

In the long run, e.g. by the time there’s a SegWit v2 softfork, I’m hoping the distinction between bech32 and bech32m is no longer relevant because the ecosystem supports both. But my guess is that we’ll have a transitional period between when that soft-fork activates and when we feel comfortable using it by default.

On the way to that future we have this Taproot checkbox, presenting it as a new feature. In a later release we make it the default.

Then, unless there is strong opposition to using Taproot, we drop the checkbox altogether. At that point the interface is back where it was before the PR. “bech32” would then mean “bech32m” and we could even rename it. Unchecking it would produce a p2sh-segwit address which should only be needed when dealing with dinosaur exchanges.

The story then repeats for a future SegWit v2 soft fork: we first add a check-box to opt-in, make it the default and then later drop the checkbox.

sipa commented at 8:19 pm on November 1, 2021: contributor

@luke-jr I just think the choice of what script construction is used should be a wallet-level configuration option, not something selected at address creation time. In descriptor wallets the answer would be: if you want P2PKH, you use a pkh descriptor for the base58 address descriptor. For legacy wallets it could be a config option or so.

ajtowns commented at 6:38 am on November 2, 2021: contributor

@sipa That sounds to me like there should be some extra wallet-level configuration option that says “for base58 addresses, I know I can sign for both pkh(X) and sh(wpkh(X)) for both receiving(non-internal) and change(internal), but I prefer pkh(X)”. eg if you upgrade to segwit, receive some funds via sh(wpkh(X)) then decide you don’t like segwit and want to keep using pkh(X) by preference, not having that option would mean you’d have to move the sh(wpkh(X)) descriptor to a different wallet, which would probably be pretty annoying. And I think you’d get similar scenarios if you come up with new descriptors (eg switching between 4/5 and 3/5 to sign, or a musig threshold vs a checkmultisig threshold) then decided you didn’t really like them. @Sjors Having an “address type” drop down of:

base58 (legacy)
bech32 (segwit)
bech32m (taproot)

(matching -addresstype) seems like it would be better to me? Implying that taproot uses bech32 is a bit misleading and seems likely to cause confusion even with a tool tip…

With the “I prefer descriptor X for bech32 addresses” option above, you don’t need to do anything when “segwit v2” comes along – that just adds a new descriptor that you won’t generate addresses for until you update your config to say you prefer it.

(I don’t think the gui really needs a “use this specific descriptor to generate this address”; if you want to be that fine-grained, use the deriveaddresses RPC?)

Maybe also upgrade -addresstype to be something settable in the gui as well?

achow101 commented at 4:46 pm on November 2, 2021: member

I would also prefer that this use a drop down rather than adding more boolean options.

Rspigler commented at 9:28 am on November 4, 2021: contributor

tACK

v22.0 Legacy: Address option is Native Segwit/Bech32 checkbox (defaults to enabled). p2sh-segwit address generated if unchecked.
v22.0 Descriptor Wallet. Same options/addresses, but with external/internal pkh, sh-wpkh, and wpkh descriptors.
PR https://github.com/bitcoin/bitcoin/pull/22364: Again same options, but generates external/internal pkh, sh-wpkh, wpkh, and now tr descriptors (if descriptor wallet created).
PR #459 + https://github.com/bitcoin/bitcoin/pull/22364: Checkboxes for Native Segwit/Bech32 and Taproot (as pictured in OP). When both enabled, generates bech32m address from tr descriptor. With just Native Segwit/Bech32 enabled, bech32 address generated from wpkh descriptor, as expected. Unchecking Native Segwit/Bech32 disables ‘Use Taproot’, and generates p2sh-segwit address from sh-wpkh descriptor.

I also agree that a dropdown would be better. While Taproot is a segwit version, I don’t think the end user needs to be burdened with this fact by how it is currently set up. I like @ajtowns suggestion for Address_Format (Soft Fork Name). But maybe it’d be better as:

base58 (Segwit) bech32 (Native Segwit) bech32m (Taproot)

Unless you wanted to include a legacy as well as p2sh-segwit address option.

This labeling format also allows the distinction @Sjors was looking for in a future Segwit v2 softfork, if bech32m format is reused:

… bech32m (taproot) bech32m (future_fork)

If @sipa wants this to be a wallet level config option, maybe the user’s preference can be set there for the default, but then can still be changed at address creation time if necessary.

Sjors commented at 11:07 am on November 4, 2021: member

How about:

0base58 (legacy)
1base58 (p2sh-Segwit)
2bech32 (Segwit)
3bech32m (Taproot)

Rspigler commented at 6:54 pm on November 4, 2021: contributor

Should we include a legacy address option if it has already been deprecated in the GUI?

luke-jr commented at 7:02 pm on November 4, 2021: member

It’s curious that people are treating “base58” as if it’s a single address type. The difference between single-key base58 and P2SH is similar to the difference between Bech32 and Bech32m ;)

sipa commented at 2:27 am on November 9, 2021: contributor

It’s curious that people are treating “base58” as if it’s a single address type. The difference between single-key base58 and P2SH is similar to the difference between Bech32 and Bech32m ;)

You’re right; they’re different formats for the purpose of senders. What unites them is the fact that both have been around for nearly 10 years.

Sjors force-pushed on Nov 14, 2021

hebasto added the label UX on Nov 21, 2021

hebasto added the label Wallet on Nov 21, 2021

Sjors force-pushed on Dec 8, 2021

Sjors renamed this:
~~Add Taproot checkbox to receive tab~~
Address type dropdown, add Taproot (Receive tab)
on Dec 8, 2021

Sjors force-pushed on Dec 8, 2021

kristapsk approved

kristapsk commented at 11:55 pm on December 8, 2021: contributor

ACK 4c87ff08632a89437d97f8c0546bc1997cfbf435 (tested on signet with both descriptors and legacy wallets).

Sjors commented at 2:42 am on December 9, 2021: member

(it’s now a dropdown)

Sjors force-pushed on Dec 9, 2021

in src/qt/receivecoinsdialog.cpp:92 in f5ba3d9394 outdated

91-            ui->useBech32->setCheckState(Qt::Checked);
92-        } else {
93-            ui->useBech32->setCheckState(Qt::Unchecked);
94+        // Populate address type dropdown and select default
95+        ui->addressType->insertItem((int)OutputType::LEGACY, "Base58 (Legacy)");
96+        ui->addressType->setItemData((int)OutputType::LEGACY, "Not recommended due to higher fees and less protection against typos.", Qt::ToolTipRole);

MarcoFalke commented at 4:00 pm on December 9, 2021:

Sorry to bike-shed, but this wasn’t exposed previously through the gui. Adding Taproot addresses seems an odd reason to start offering this again for the average user. Experts that really want this can still use the gui rpc console.

For our GUI users, I expect that less options here is beneficial.

Sjors commented at 2:17 am on December 10, 2021:

It becomes a problem when the user sets -addresstype=legacy, because then nothing would be selected by default. Previously the checkbox would toggle between legacy and bech32 in that case. This was by design, though not well documented: https://github.com/bitcoin/bitcoin/pull/11991#issue-284181828

MarcoFalke commented at 8:11 am on December 10, 2021:

When unchecked, -addresstype=legacy would map to p2sh-witness in the GUI, not legacy. So I think we should preserve that, no?

kristapsk commented at 8:20 am on December 10, 2021:

I think Bitcoin Core users are already above average Bitcoin users and mostly know what they are doing, don’t think we should limit people from creating legacy P2PKH addresses, if they want to.

MarcoFalke commented at 8:27 am on December 10, 2021:

No one has been using this for the past 4 years, and I am not aware of anyone complaining about the lack of it? So it seems odd to add an option that has no use case.

kristapsk commented at 8:31 am on December 10, 2021:

I have used this by using RPC. Would have used in GUI if it would have been available.

Sjors commented at 12:51 pm on December 10, 2021:

Correction, the checkbox is not disabled on master when using -addresstype=legacy. The original PR description was wrong (or the behavior was changed later).

On master you can test that, when launched with -addresstype=legacy, it would produce a P2PKH address, not wrapped segwit. This happened in the code below:

Notice also that this logic leads to a bug for anyone who sets -addresstype=bech32m; it would then fall back to P2PKH instead of wrapped SegWit when unchecked. So introducing this dropdown is probably a good idea in any case (or just add || address_type == OutputType::BECH32M).

IIRC @luke-jr at the time requested the ability to opt out of SegWit entirely. But I don’t know how many people actually knew of this behaviour.

bitcoin-core deleted a comment on Dec 9, 2021

MarcoFalke approved

MarcoFalke commented at 12:55 pm on December 10, 2021: contributor

ACK

kristapsk approved

kristapsk commented at 1:50 pm on December 10, 2021: contributor

re-ACK f5ba3d9394e47fa737429ecbb43a91bb767e8c59

in doc/release-notes-gui-459.md:4 in f5ba3d9394 outdated

0@@ -0,0 +1,4 @@
1+GUI changes
2+-----------
3+
4+- The Bech32 checkbox has been replaced with a dropdown for all address types, including the new Bech32m (BIP-350) standard for Taproot enabled wallets. The default remains Bech32 (SegWit).

MarcoFalke commented at 10:50 am on December 13, 2021:

0- The Bech32 checkbox has been replaced with a dropdown for all address types, including the new Bech32m (BIP-350) standard for Taproot enabled wallets.

nit: I don’t think we need to mention stuff that doesn’t change. Also, this is wrong? The default is whatever -addresstype is set to?

Sjors commented at 11:54 am on December 13, 2021:

Fixed

Sjors force-pushed on Dec 13, 2021

promag commented at 12:04 pm on December 13, 2021: contributor

Tested ACK 79580c09ff9d3c9c845e02fbcf98f8187e7e9fea.

My only comment is that combo box entries are mapped to OutputType values - meaning we can’t reorder. The following commit uses UserRole to store the type instead of combo box index https://github.com/promag/gui/commit/b164039aa2b6462b6b1eac6bc79b40cd211b41b4 (https://github.com/promag/gui/tree/pr/459).

Sjors commented at 12:44 pm on December 13, 2021: member

@promag thanks! Let’s keep that commit in mind in case we actually want to reorder, which I don’t think is very likely.

Sjors force-pushed on Dec 13, 2021

Sjors commented at 12:48 pm on December 13, 2021: member

OTOH it actually looks a bit cleaner too, so I squashed it in

Rspigler commented at 0:08 am on December 15, 2021: contributor

Will re-test

Rspigler commented at 4:19 am on December 21, 2021: contributor

I like the way this looks, but there is a typo in 1 tooltip, and I think other tooltips can be improved.

Base58 (Legacy): “Not recommended due to higher fees and less protection against typos.” -> “Not recommended.” It is also not recommended b/c of security issues with multisig, and scaling issues. No reason to list them all. If an advanced user wants to use an old address, they will know why they need it.

Base58 (P2SH-SegWit): “Generates an address compatible with older wallets.” :heavy_check_mark:

Bech32 (SegWit): “Generates a native segwit addresses (BIP-173), which reduces your transaction fees later on and offers better protection against typos, but some old wallets don’t support it.” There is a typo: “Generates a native segwit addresses”->“Generates a native segwit address”. This is also a very long tooltip and looks bad IMO. Perhaps: “Generates a native segwit address (BIP-173) which offers various improvements.”

Bech32m (Taproot): “Although Bech32m (BIP-350) looks similar to Bech32, there is a slight difference and only some newer wallets support it.” I don’t see a reason to take up space discussing how it looks, but I can’t think of any improvement to offer right now that sounds good, other than basically what is said for SegWit :grimacing:

wallet: add taprootEnabled() to interface 56113daef4

gui: address type dropdown, add bech32m

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

c9a77e227e

Sjors commented at 4:45 am on December 21, 2021: member

I fixed the typos and shortened the tooltips a bit.

security issues with multisig

That’s not an issue for PKH (1…) addresses.

Scaling is a problem for the ecosystem, but the two issues I put in the tooltip (fees and typo protection) are relevant to individual users. If they encounter a wallet or service insisting on this, they should be appropriately annoyed :-)

Bech32 (SegWit) This is also a very long tooltip and looks bad IMO

I agree it looks ugly, but that’s mainly because I wasn’t able to add line breaks. That might be fixable though.

It’s a bit shorter now.

I don’t see a reason to take up space discussing how it looks

I reworded it a bit. The problem is that a user will put the bech32m address into a different wallet / exchange and get an error message like “Invalid (bech32) address”.

Sjors force-pushed on Dec 21, 2021

Rspigler commented at 1:35 pm on December 21, 2021: contributor

tACK c9a77e227eecf357c7dd550efb8c1827bb99a5de. I like the changes made now, thanks!

kristapsk approved

kristapsk commented at 10:42 am on December 22, 2021: contributor

re-ACK c9a77e227eecf357c7dd550efb8c1827bb99a5de

MarcoFalke merged this on Dec 22, 2021

MarcoFalke closed this on Dec 22, 2021

in src/qt/receivecoinsdialog.cpp:97 in c9a77e227e

 96+            const auto index = ui->addressType->count();
 97+            ui->addressType->addItem(text, (int) type);
 98+            ui->addressType->setItemData(index, tooltip, Qt::ToolTipRole);
 99+            if (model->wallet().getDefaultAddressType() == type) ui->addressType->setCurrentIndex(index);
100+        };
101+        add_address_type(OutputType::LEGACY, "Base58 (Legacy)", "Not recommended due to higher fees and less protection against typos.");

MarcoFalke commented at 11:28 am on December 22, 2021:

This is missing a check (like the one for taproot)?

Locally, I am getting:

Screenshot from 2021-12-22 12-26-46

Sjors commented at 2:31 am on December 23, 2021:

I assume you manually created a wallet without a pkh() descriptor? We could add checks for the existence of each output type to the interface and then remove them from the dropdown. However, if someone manually creates a wallet, they would also understand the error message, and it might even better than a missing item?

in src/qt/receivecoinsdialog.cpp:99 in c9a77e227e

 98+            ui->addressType->setItemData(index, tooltip, Qt::ToolTipRole);
 99+            if (model->wallet().getDefaultAddressType() == type) ui->addressType->setCurrentIndex(index);
100+        };
101+        add_address_type(OutputType::LEGACY, "Base58 (Legacy)", "Not recommended due to higher fees and less protection against typos.");
102+        add_address_type(OutputType::P2SH_SEGWIT, "Base58 (P2SH-SegWit)", "Generates an address compatible with older wallets.");
103+        add_address_type(OutputType::BECH32, "Bech32 (SegWit)", "Generates a native segwit address (BIP-173). Some old wallets don't support it.");

MarcoFalke commented at 11:52 am on December 22, 2021:

nit: maybe replace “wallets” with “services” or “third-party services” or “third-party wallets”.

Otherwise the user might get confused into thinking that an old wallet.dat can’t generate this, which is not true?

Sjors commented at 2:34 am on December 23, 2021:

I’m not sure if they’re more likely to think of an upgrade scenario than about receiving coins from another wallet they have? “third-party” is a bit of an tech industry term, and probably doesn’t translate well. Paging @jonatack.

(“other, old, wallets” is more clear perhaps, though maybe not pretty)

sidhujag referenced this in commit b960097cf4 on Dec 22, 2021

Sjors deleted the branch on Dec 23, 2021

RandyMcMillan referenced this in commit aaa89c92e4 on Jan 27, 2022

bitcoin-core locked this on Dec 23, 2022

Address type dropdown, add Taproot (Receive tab) #459