Implement general purpose watch only wallet.

[BitcoinWukong]

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.

1. To create a new watch only wallet:
   python wallet-tool.py createwatchonly [xpub/ypub/zpub]
2. A rescan is needed after the wallet creation:
   bitcoin-cli -rpcwallet=[jm_wallet] rescanblockchain [starting_block_height]
3. 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]

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]

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]

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]

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]

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]

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]

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

[kristapsk]

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]

Rebased to latest master, did not make any change.

[AdamISZ]

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]

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]

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]

Rebase

[BitcoinWukong]

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

[kristapsk]

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]

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

[BitcoinWukong]

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]

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

[kristapsk]

Needs rebase.

[BitcoinWukong]

Rebased to latest master.

[BitcoinWukong]

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

[BitcoinWukong]

@kristapsk PR updated, PTAL, thanks!

[kristapsk]

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

[BitcoinWukong]

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]

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]

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]

Replaced by #1632.