Implement general purpose watch only wallet. #989

BitcoinWukong · 2021-08-12T23:08:29Z

Prepare for #663 and #792

Renamed the original createwatchonly command to createfbwatchonly, and repurposed the createwatchonly command to instead create a general purpose watch only wallet.

To create a new watch only wallet:
python wallet-tool.py createwatchonly [xpub/ypub/zpub]
A rescan is needed after the wallet creation:
bitcoin-cli -rpcwallet=[jm_wallet] rescanblockchain [starting_block_height]
The joinmarket commands can then be used directly on this watch only wallet:

python wallet-tool.py watchonly.jmdat
python wallet-tool.py watchonly.jmdat history
python wallet-tool.py watchonly.jmdat displayall

undeath · 2021-08-13T12:16:29Z

Thanks for this!

Right now this watch-only wallet does not support joinmarket's concept of mixdepths. I expect the wallet will mainly be used for joinmarket wallets, so this is important to have. Both, for the case described in #663 (move to a new wallet) and #792 (joinmarket with hardware wallet) the mixdepths need to be preserved.

The easiest way would be to always assume the imported key is the master key and force joinmarket's derivation path from there on. A more complex solution would be a dynamic approach, allowing to import keys at specified derivation paths and only do partial key derivation from there on. (eg import at path m/84'/0'/0' and then derive external and internal account and address index from there on to get m/84'/0'/0'/0/0 and others). That could cause problems if there is not a key for every mixdepth used by the main wallet.

BitcoinWukong · 2021-08-13T18:42:10Z

Both, for the case described in #663 (move to a new wallet) and #792 (joinmarket with hardware wallet) the mixdepths need to be preserved.

I'm not sure about #663 (hardware wallet support). But for #792 (YG coinjoin out to xpub), in the second use case (moving coins into cold storage, and an option to use the xpub address if the cj-out is larger than X BTC), the mixdepths is not needed.

The easiest way would be to always assume the imported key is the master key and force joinmarket's derivation path from there on.

One of my motivation, is that we should be able to import the xpub/ypub/zpub from other wallets like Electrum directly as a watch only wallet into jm. And the xpub/ypub/zpub from these wallets, Electrum for example, does not support the concept of mixdepth.

Given a zpub of an Electrum wallet, the addresses of that wallet would be m/reciving_or_change/address_index. If we use m/mixdepth/reciving_or_change/address_index instead, then the address we calculate would not be the addresses of that Electrum wallet.

That could cause problems if there is not a key for every mixdepth used by the main wallet.

I don't think these watch-only wallets can be used as main wallet at all. They'll be used as a secondary wallet that is loaded alongside the main wallet. For example, we can run things like yg-privacyenhanced.py wallet.jmdat --cj-extwallet=watchonly.jmdat --cj-extwallet-min-size=100000000.

BitcoinWukong · 2021-08-13T18:51:30Z

In fact, this approach can also work for the first use case (moving from sw to native sw yg wallet) of #792 (YG coinjoin out to xpub).

The user would get the xpub of m/84'/0'/0' of his new native sw yg wallet, and create a watch-only wallet in jm based on that. He can then run cj on his original sw wallet, by a command like yg-privacyenhanced.py p2sh_p2wpkh.jmdat --cj-extwallet=watchonly_p2wpkh.jmdat.

After running as maker for some while, all his coins will be moved to the new jm wallet in the external addresses of mixdepth 0.

kristapsk · 2021-08-16T20:49:37Z

Renamed the original createwatchonly command to createfbwatchonly, and repurposed the createwatchonly command to instead create a general purpose watch only wallet.

I'm generally against breaking backwards compatibility, as people might have created scripts around JM commands, but I think it's ok here as fidelity bonds are very recent addition to JM.

kristapsk · 2021-08-16T20:53:01Z

python wallet-tool.py createwatchonly [xpub/ypub/zpub]

What I don't like here is that we don't support ypub/zpub in other places, like wallet-tool.py display, see #558, @chris-belcher had some objections against it.

BitcoinWukong · 2021-08-19T00:11:34Z

What I don't like here is that we don't support ypub/zpub in other places, like wallet-tool.py display, see #558, @chris-belcher had some objections against it.

I think these 2 cases are different. In #558, it's about displaying ypub/zpub instead of a xpub, it would change the behavior of JoinMarket.

While in this case, if the user prefer, they can continue to use xpub to import their watch only wallet. And the ypub/zpub are additional options supported that users can also use. So if the user already has a ypub/zpub, they don't have to convert ypub/zpub to xpub and can import them directly into JoinMarket. But if they have a xpub, they can import the xpub directly into JoinMarket as well to create a watch only wallet.

kristapsk · 2021-08-19T21:21:34Z

@wukong1971, ok, good point! Will try to test / review this more carefully in coming days.

jmclient/jmclient/wallet_utils.py

kristapsk · 2021-08-25T19:51:39Z

Did basic testing, by imporing single mixdepth of different JM wallet, so far works as expected.

About multiple mixdepth support - don't see other way to add some option to also specify derivation paths. But that could be added afterwards, if choose so. People will definitely want to use it currently supported way to add xpub's of non-JM wallets. One example - this already would help adding JM coinjoin support to Wasabi.

BitcoinWukong · 2021-09-06T03:52:30Z

Rebased to latest master, did not make any change.

AdamISZ · 2021-09-06T13:42:25Z

Thanks for this!

Right now this watch-only wallet does not support joinmarket's concept of mixdepths. I expect the wallet will mainly be used for joinmarket wallets, so this is important to have. Both, for the case described in #663 (move to a new wallet) and #792 (joinmarket with hardware wallet) the mixdepths need to be preserved.

The easiest way would be to always assume the imported key is the master key and force joinmarket's derivation path from there on. A more complex solution would be a dynamic approach, allowing to import keys at specified derivation paths and only do partial key derivation from there on. (eg import at path m/84'/0'/0' and then derive external and internal account and address index from there on to get m/84'/0'/0'/0/0 and others). That could cause problems if there is not a key for every mixdepth used by the main wallet.

Considerations like this, and several follow up comments in this thread, are a very good argument for implementing wallet descriptor support (https://outputdescriptors.org/) - probably before adding this change?

Well, either way: this whole idea of watch only plus #792 makes a lot of sense and is very much in tune with what Joinmarket users are mostly trying to do, I think. @chris-belcher when you get a chance could you take a look at some of the ideas here, in particular around the watch only wallet?

BitcoinWukong · 2021-09-06T23:14:23Z

Considerations like this, and several follow up comments in this thread, are a very good argument for implementing wallet descriptor support (https://outputdescriptors.org/) - probably before adding this change?

I think when it comes to importing a wallet, it doesn't hurt for us to support both the currently wildly used xpub/ypub/zpub, and wallet descriptor at the same time.

In fact, I advocate that we support as many formats as possible when it comes to importing, we don't want to force the user to go through the hurdle of converting whatever they had into the "wallet descriptor", it adds friction to the user experience, and increases the risk of them exposing their public key to 3rd party websites.

That being said, I do agree that when we display the extended public key information in joinmarket, like in the wallet-tool.display function, it is a good idea for us to show a wallet descriptor instead of an "xpub".

kristapsk · 2021-09-08T05:37:59Z

I also think that output descriptor support could be added after this, watch only wallet creation from simple xpub/ypub/zpub already is useful and should be supported even after switching to wallet descriptors.

BitcoinWukong · 2021-09-09T02:56:01Z

Rebase

jmclient/jmclient/cryptoengine.py

BitcoinWukong · 2021-09-09T20:44:05Z

Updated _get_key_ident() in jmclient/jmclient/wallet.py per CR suggestion above.

kristapsk · 2021-09-10T19:24:50Z

Noticed another issue with createwatchonly - when you use generate, if you don't enter passphrase, it will output Failed, with createwatchonly it will fail silently, without outputing any error, but wallet file will not be created.

kristapsk · 2021-09-10T19:32:14Z

With "don't enter passphrase" I meant enter blank passphrase twice.

BitcoinWukong · 2021-09-14T21:15:14Z

with createwatchonly it will fail silently, without outputing any error, but wallet file will not be created.

This was the previous behavior from the original wallet_createwatchonly function, this PR did not change the logic.

That being said, I just pushed a new commit that prints out an error message in this case, I do agree that an error message should be printed out in this case.

kristapsk · 2021-09-29T08:14:55Z

I'm wondering shouldn't we print a warning if ypub is given and native is set to true in joinmarket.cfg...

kristapsk · 2021-11-02T10:06:22Z

Needs rebase.

BitcoinWukong · 2021-12-10T00:28:01Z

Rebased to latest master.

BitcoinWukong · 2021-12-10T01:31:49Z

I'm wondering shouldn't we print a warning if ypub is given and native is set to true in joinmarket.cfg...

Implemented a new feature, if ypub is given, we'll create a BTC_Watchonly_P2SH_P2WPKH wallet instead and generate 3xxx addresses for it regardless of the config in joinmarket.cfg

jmclient/jmclient/wallet_utils.py

BitcoinWukong · 2022-05-11T08:36:02Z

@kristapsk PR updated, PTAL, thanks!

kristapsk · 2022-05-11T12:17:05Z

docs/fidelity-bonds.md needs to be updated (s/createwatchonly/createfbwatchonly/).

Renamed the original createwatchonly command to createfbwatchonly, and repurposed the createwatchonly command to instead create a general purpose watch only wallet.

BitcoinWukong · 2022-05-13T01:23:44Z

docs/fidelity-bonds.md needs to be updated (s/createwatchonly/createfbwatchonly/).

Done. @kristapsk , please let me know if you have any other questions / concerns, thanks!

BitcoinWukong · 2022-05-13T01:25:16Z

I'm wondering shouldn't we print a warning if ypub is given and native is set to true in joinmarket.cfg...

I think we should allow it. Say the user has native == true and the user is running a native-segwit joinmarket wallet as their main wallet. They should be allowed to import a non-native-segwit wallet by using a ypub in this case.

kristapsk · 2022-05-26T08:01:41Z

Tried testing, but doesn't work.

On signet:

 ./scripts/wallet-tool.py createwatchonly tpubDE5rkpFtF6dEb9CVp6VFAnJuxuq1eLe6EH1w5nm6tQC4asR78hRuA9iGYiBGNk9X2CUY7A4GM39LfG1MWEarqg38q35XLSR7uE6faM1oPpQ
User data location: /home/user/.joinmarket/
2022-05-26 10:30:20,591 [DEBUG]  rpc: getblockchaininfo []
2022-05-26 10:30:20,593 [DEBUG]  rpc: listwallets []
2022-05-26 10:30:20,594 [DEBUG]  rpc: getwalletinfo []
Input wallet file name (default: watchonly.jmdat):  
Enter new passphrase to encrypt wallet: 
Reenter new passphrase to encrypt wallet: 
Error with provided master pub key

On mainnet, didn't give error, but also didn't create wallet either:

$ ./scripts/wallet-tool.py createwatchonly xpub...
User data location: /home/user/.joinmarket/
2022-05-26 10:57:07,093 [DEBUG]  rpc: getblockchaininfo []
2022-05-26 10:57:07,095 [DEBUG]  rpc: listwallets []
2022-05-26 10:57:07,096 [DEBUG]  rpc: getwalletinfo []
Input wallet file name (default: watchonly.jmdat): mainnet-watchonly-test.jmdat
Enter new passphrase to encrypt wallet: 
Reenter new passphrase to encrypt wallet: 

$ ls ~/.joinmarket/wallets/mainnet-watchonly-test.jmdat
ls: cannot access '/home/user/.joinmarket/wallets/mainnet-watchonly-test.jmdat': No such file or directory

gutwrinch

Think I've tried about everything on areas to try so this doesn't really scare me that much we're going to give it a whirl

kristapsk · 2024-01-05T03:50:11Z

Replaced by #1632.

BitcoinWukong force-pushed the wukong--watch-only-wallet branch from ad5b2a5 to 83af3c9 Compare August 12, 2021 23:10

kristapsk reviewed Aug 25, 2021

View reviewed changes

jmclient/jmclient/wallet_utils.py Outdated Show resolved Hide resolved

BitcoinWukong mentioned this pull request Aug 26, 2021

YG coinjoin out to cold storage wallet #998

Open

BitcoinWukong force-pushed the wukong--watch-only-wallet branch from d98d0a6 to 7d5cd9b Compare August 26, 2021 18:23

BitcoinWukong force-pushed the wukong--watch-only-wallet branch from 7d5cd9b to 2bcaef6 Compare September 6, 2021 03:52

BitcoinWukong force-pushed the wukong--watch-only-wallet branch from 2bcaef6 to 8a7937e Compare September 9, 2021 02:55

kristapsk reviewed Sep 9, 2021

View reviewed changes

jmclient/jmclient/cryptoengine.py Outdated Show resolved Hide resolved

BitcoinWukong force-pushed the wukong--watch-only-wallet branch from 8a7937e to 52d1fac Compare September 9, 2021 20:43

BitcoinWukong force-pushed the wukong--watch-only-wallet branch from f79e829 to f90a760 Compare December 10, 2021 00:27

kristapsk reviewed Dec 12, 2021

View reviewed changes

jmclient/jmclient/wallet_utils.py Outdated Show resolved Hide resolved

BitcoinWukong force-pushed the wukong--watch-only-wallet branch from 67700e1 to 0a43381 Compare May 11, 2022 08:23

BitcoinWukong force-pushed the wukong--watch-only-wallet branch from b9cf7a4 to 58d514b Compare May 13, 2022 01:18

Create SegwitWatchonlyWallet and SegwitLegacyWatchonlyWallet

2dcf2c9

Renamed the original createwatchonly command to createfbwatchonly, and repurposed the createwatchonly command to instead create a general purpose watch only wallet.

BitcoinWukong force-pushed the wukong--watch-only-wallet branch from 58d514b to 2dcf2c9 Compare May 13, 2022 01:22

kristapsk mentioned this pull request Jun 6, 2022

Showing xpub instead of ypub for JoinMarket wallet #558

Closed

gutwrinch approved these changes Apr 13, 2023

View reviewed changes

kristapsk mentioned this pull request Jan 5, 2024

Implement general purpose watch only wallet, take 2 #1632

Open

kristapsk closed this Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement general purpose watch only wallet. #989

Implement general purpose watch only wallet. #989

BitcoinWukong commented Aug 12, 2021 •

edited

Loading

undeath commented Aug 13, 2021

BitcoinWukong commented Aug 13, 2021 •

edited

Loading

BitcoinWukong commented Aug 13, 2021 •

edited

Loading

kristapsk commented Aug 16, 2021

kristapsk commented Aug 16, 2021

BitcoinWukong commented Aug 19, 2021 •

edited

Loading

kristapsk commented Aug 19, 2021

kristapsk commented Aug 25, 2021

BitcoinWukong commented Sep 6, 2021

AdamISZ commented Sep 6, 2021

BitcoinWukong commented Sep 6, 2021

kristapsk commented Sep 8, 2021

BitcoinWukong commented Sep 9, 2021

BitcoinWukong commented Sep 9, 2021

kristapsk commented Sep 10, 2021 •

edited

Loading

kristapsk commented Sep 10, 2021 •

edited

Loading

BitcoinWukong commented Sep 14, 2021 •

edited

Loading

kristapsk commented Sep 29, 2021

kristapsk commented Nov 2, 2021

BitcoinWukong commented Dec 10, 2021

BitcoinWukong commented Dec 10, 2021

BitcoinWukong commented May 11, 2022

kristapsk commented May 11, 2022

BitcoinWukong commented May 13, 2022

BitcoinWukong commented May 13, 2022 •

edited

Loading

kristapsk commented May 26, 2022

gutwrinch left a comment

kristapsk commented Jan 5, 2024

Implement general purpose watch only wallet. #989

Implement general purpose watch only wallet. #989

Conversation

BitcoinWukong commented Aug 12, 2021 • edited Loading

undeath commented Aug 13, 2021

BitcoinWukong commented Aug 13, 2021 • edited Loading

BitcoinWukong commented Aug 13, 2021 • edited Loading

kristapsk commented Aug 16, 2021

kristapsk commented Aug 16, 2021

BitcoinWukong commented Aug 19, 2021 • edited Loading

kristapsk commented Aug 19, 2021

kristapsk commented Aug 25, 2021

BitcoinWukong commented Sep 6, 2021

AdamISZ commented Sep 6, 2021

BitcoinWukong commented Sep 6, 2021

kristapsk commented Sep 8, 2021

BitcoinWukong commented Sep 9, 2021

BitcoinWukong commented Sep 9, 2021

kristapsk commented Sep 10, 2021 • edited Loading

kristapsk commented Sep 10, 2021 • edited Loading

BitcoinWukong commented Sep 14, 2021 • edited Loading

kristapsk commented Sep 29, 2021

kristapsk commented Nov 2, 2021

BitcoinWukong commented Dec 10, 2021

BitcoinWukong commented Dec 10, 2021

BitcoinWukong commented May 11, 2022

kristapsk commented May 11, 2022

BitcoinWukong commented May 13, 2022

BitcoinWukong commented May 13, 2022 • edited Loading

kristapsk commented May 26, 2022

gutwrinch left a comment

Choose a reason for hiding this comment

kristapsk commented Jan 5, 2024

BitcoinWukong commented Aug 12, 2021 •

edited

Loading

BitcoinWukong commented Aug 13, 2021 •

edited

Loading

BitcoinWukong commented Aug 13, 2021 •

edited

Loading

BitcoinWukong commented Aug 19, 2021 •

edited

Loading

kristapsk commented Sep 10, 2021 •

edited

Loading

kristapsk commented Sep 10, 2021 •

edited

Loading

BitcoinWukong commented Sep 14, 2021 •

edited

Loading

BitcoinWukong commented May 13, 2022 •

edited

Loading