Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable importing joinmarket wallet as watch-only to mobile wallets #1267

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BitcoinWukong
Copy link
Contributor

@BitcoinWukong BitcoinWukong commented May 11, 2022

Based on #1266 and #1265.

What changed in this PR

Added a config POLICY.display_ypub_zpub to allow showing xpub in ypub or zpub format.

Both ypub and zpub are BIP standards and are used by many mobile wallets: BlueWallet, Sentinel, etc. We have to show the pub key in ypub or zpub format when we import joinmarket wallet to these mobile wallets in order for them to figure out the correct script to use: p2sh-p2wpkh and p2wpkh respectively.

https://en.bitcoin.it/wiki/BIP_0049

Extended public keys use 0x049d7cb2 to produce a "ypub" prefix, and private keys use 0x049d7878 to produce a "yprv" prefix. Testnet uses 0x044a5262 "upub" and 0x044a4e28 "uprv."

https://en.bitcoin.it/wiki/BIP_0084

Extended public keys use 0x04b24746 to produce a "zpub" prefix, and private keys use 0x04b2430c to produce a "zprv" prefix. Testnet uses 0x045f1cf6 "vpub" and 0x045f18bc "vprv."

Motivation

With the combination of #1265 #1266 and this PR, we can now easily import one mixdepth of the joinmarket wallet as a watch-only wallet to our mobile wallets. (I tested the flow using Sentinel https://play.google.com/store/apps/details?id=com.samourai.sentinel&gl=US)

This way, whenever we want to receive a payment while traveling, we can generate a new wallet address using our mobile wallet. Later on, when we get back to our laptop, we can then start a tumbler process directly without worrying about transferring Bitcoin from other wallets to the joinmakert wallet.

(Eventually, we're going to replace xpub/ypub/zpub with output descriptors, but we're not quite there yet.)

@BitcoinWukong BitcoinWukong changed the title Enable importing joinmarket wallet as watch-only Enable importing joinmarket wallet to other wallets May 11, 2022
@BitcoinWukong BitcoinWukong changed the title Enable importing joinmarket wallet to other wallets Enable importing watch-only joinmarket wallet to other wallets May 11, 2022
@BitcoinWukong BitcoinWukong changed the title Enable importing watch-only joinmarket wallet to other wallets Enable importing joinmarket wallet as watch-only to mobile wallets May 11, 2022
@kristapsk
Copy link
Member

Concept ACK. Will review / test this and related PRs shortly.

@AdamISZ
Copy link
Member

AdamISZ commented May 11, 2022

I agree, could be a very useful addition.

I guess descriptors could also be seen as a big priority.

(To be clear I don't have any plans to work on any of this, short term)

@kristapsk
Copy link
Member

kristapsk commented May 11, 2022

I guess descriptors could also be seen as a big priority.

I already have some code for generating output descriptors in one unfinished PR. That's relatively simple compared to descriptor parsing. So we could add that too. I think here we want to support both simple xpub and descriptors, as different mobile wallets will support different things, with only descriptors this functionality will be limited currently.

@kristapsk
Copy link
Member

Needs rebase.

(Eventually, we're going to replace xpub/ypub/zpub with output descriptors, but we're not quite there yet.)

Now that #1270 is merged, we can use output descriptors here, but not sure about wallet support, likely we need to support multiple ways for now - xpub, zpub/ypub and descriptors.

jmclient/setup.py Outdated Show resolved Hide resolved
jmclient/jmclient/configure.py Outdated Show resolved Hide resolved
@BitcoinWukong
Copy link
Contributor Author

@AdamISZ @kristapsk , all comments addressed, PTAL, thanks!

@BitcoinWukong BitcoinWukong force-pushed the wukong--display-account-xpub-qr branch from 18be935 to 3703857 Compare June 7, 2022 18:55
jmclient/jmclient/configure.py Outdated Show resolved Hide resolved
jmclient/jmclient/cryptoengine.py Outdated Show resolved Hide resolved
jmclient/jmclient/configure.py Outdated Show resolved Hide resolved
@BitcoinWukong BitcoinWukong force-pushed the wukong--display-account-xpub-qr branch from 804c7dd to f58e74f Compare June 7, 2022 19:21

# Convert xpub to ypub or zpub for display
if is_segwit_mode():
if is_native_segwit_mode():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, should look at the wallet type not joinmarket.cfg settings. Noticed by opening native segwit JM wallet with native = false in joinmarket.cfg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

# non-native segwit wallet.
# If set to false, the extended public key will always display in the xpub
# format.
qt_display_ypub_zpub = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure default should be true, but probably. What do others think? @AdamISZ ? @chris-belcher ?

@BitcoinWukong BitcoinWukong force-pushed the wukong--display-account-xpub-qr branch from f58e74f to 3900038 Compare June 20, 2022 20:14
@BitcoinWukong BitcoinWukong force-pushed the wukong--display-account-xpub-qr branch from 3900038 to deb38ab Compare June 21, 2022 16:59
@BitcoinWukong BitcoinWukong marked this pull request as draft June 26, 2022 21:19
@BitcoinWukong BitcoinWukong marked this pull request as draft June 26, 2022 21:19
@BitcoinWukong BitcoinWukong marked this pull request as draft June 26, 2022 21:19
@BitcoinWukong
Copy link
Contributor Author

I've converted this PR back to Draft.

I'm going to enable both options instead: xpub and zpub. So that the user can have the option to either one.

@kristapsk
Copy link
Member

likely we need to support multiple ways for now - xpub, zpub/ypub and descriptors.

Rethinking this - what is bad from UX perspective is multiple choices for GUI user, when he just wants to import pubkey in watchonly wallet. BlueWallet's wallet import screen says "Please enter your seed words, public key, WIF, or anything else you've got", nothing about xpub vs zpub. When provided with xpub, BlueWallet will assume BIP44 and user sees no balance. Maybe just displaying single option and having zpub as default is the right thing to do? Anyone has experinece with importing in various different popular wallets?

@BitcoinWukong
Copy link
Contributor Author

When provided with xpub, BlueWallet will assume BIP44 and user sees no balance.

My personal experience is that most mobile wallet does the same thing, and that's my motivation of this PR initially.

Maybe just displaying single option and having zpub as default is the right thing to do? Anyone has experinece with importing in various different popular wallets?

This is my preferred option, but it seems that not everyone agrees with this approach though.

@theborakompanioni
Copy link
Contributor

Any chance this will find its way into rpc api?

There seems to be "problems" with the xpubs contained in the /display response. I did not verify it yet, but from reports it seems they represent e.g. m / purpose' / coin_type' / account' / change instead of m / purpose' / coin_type' / account'. Might be done intentionally, so do not see this as a bug report.
However, having the xpub/zpub for each mixdepth as distinct property in a response from the api would be awesome. 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants