Allow ext keys #3

Eunovo · 2024-07-16T16:41:13Z

Add support for extended keys to sp()
Add sp() to wallet_miniscript test
this requires that GetNewDestination returns the same address that rpc: deriveaddress returns
move basic label implementation to separate methods to achieve this (label implementation to be expanded in later PR)
SP wallet requires scan private key so ensure that private keys are not disabled in sp wallet

Sjors · 2024-07-16T17:31:20Z

SP wallet requires scan private key so ensure that private keys are not disabled in sp wallet

Bit of a wallet philosophy thing, but I think a watch key should not count as a private key. It's much more similar in functionality and security than an xpub.

In other words, I think it should be possible to import these into a watch-only wallet.

Sjors · 2024-07-16T17:33:22Z

I did manage to succesfully import into a wallet with disable_private_keys=false, though it gave a warning:

[
  {
    "success": true,
    "warnings": [
      "Not all private keys provided. Some wallet functionality may return unexpected errors"
    ]
  }
]

This can probably be avoided with the above.

Sjors · 2024-07-16T17:34:31Z

getnewaddress produces the same address each time now...

Eunovo · 2024-07-17T10:07:50Z

getnewaddress produces the same address each time now...

@Sjors I'm thinking about using the existing label option to decide between returning the default silent payment address or a labelled address. WDYT?

Bit of a wallet philosophy thing, but I think a watch key should not count as a private key. It's much more similar in functionality and security than an xpub.

In other words, I think it should be possible to import these into a watch-only wallet.

Makes sense. I'll add a special case to AddWalletDescriptor for silent payments

Sjors · 2024-07-17T10:38:15Z

I'm thinking about using the existing label option to decide between returning the default silent payment address or a labelled address. WDYT?

I think that's a good idea; that had me confused as well.

Perhaps when reusing a label, you could return the same address (maybe emit a warning).

There's currently no convenient way to get all labels and their addresses - that might require a new RPC.

Sjors · 2024-07-18T19:54:02Z

Perhaps I did something wrong cherry-picking your commits, but trying to import into a watch-only wallet throws an error for me:

bitcoin-cli importdescriptors '[{"desc": "sp(sppub1q...)#00000000", "active":true, "timestamp": 17...}]'
error code: -1
error message:
map::at:  key not found

It seems to have added the descriptor twice, one marked as active and the other not (and with the wrong spub value and timestamp).

It started a rescan but it doesn't seem to make progress.

Calling rescanblockchain triggers the above error again, so it probably happens during the rescan phase of importing. It did manage to find a transaction, as evidenced by the log, though it doesn't show up in the wallet.

Eunovo · 2024-07-20T07:29:56Z

@Sjors Most of this stuff is still a work in progress so there's probably a lot of errors. The idea is to open PRs to josibake/implement-bip352-full with new functionality gradually. I'll investigate these errors that you have pointed out.

Thank you for the early testing!

Eunovo · 2024-07-20T07:54:35Z

Perhaps I did something wrong cherry-picking your commits, but trying to import into a watch-only wallet throws an error for me

Did you cherry-pick on top of josibake/implement-bip352-full?

The previous commit added a test which would fail the unsigned-integer-overflow sanitizer. The warning is harmless and can be triggered on any commit, since the code was introduced. For reference, the warning would happen when the separator `-` was not present. For example: GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json would result in: rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long') #0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77 #1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9 #3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13 #4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12 #5 0x7f078ebcbbb3 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2) #6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o #7 0x7f078e840a93 (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) bitcoin#8 0x7f078e8cdc3b (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77

Sjors · 2024-07-22T10:44:16Z

@Eunovo I rebased the top commits from josibake/implement-bip352-full onto josibake/implement-bip352-sending and then applied the ones in the PR on top of that. I'll try again later when the branches are updated.

josibake · 2024-07-22T11:21:29Z

Bit of a wallet philosophy thing, but I think a watch key should not count as a private key. It's much more similar in functionality and security than an xpub.

Strongly agree. We even talked about renaming scan key to "scan entropy," since it is not a private key (i.e. it is not used to secure funds). A scan key + spend pubkey (sppub) is functionally equivalent to an xpub, so we should be able to use this in a watch-only wallet. Further more, when calling listdescriptors, it should show sp(sppub...) which is the scan key || spend pubkey, whereas listdescriptors with private_keys = true should show sp(spprv...) which is the scan key + spend private key.

Perhaps when reusing a label, you could return the same address (maybe emit a warning).

I would expect getnewaddress address_type=silent-payment to always return the un-labeled silent payment address. Calling getnewaddress address_type=silent-payment label=xyz should always return the same labeled address (i.e., the labeled address corresponding to xyz).

Also, I saw this comment from @Sjors earlier but can't find it now, but if a sp descriptor is imported into a wallet, this should set the silent_payments flag to true.

Eunovo · 2024-07-23T07:27:49Z

@josibake and @Sjors I stopped storing the scan key to DB in 8ed833b since the desc always contains the scan key, we don't need store it separately. The scan key is also no longer added to the private key map after descriptor parsing, so the watch-only wallets should not have a problem with sp descs

Sjors · 2024-07-23T11:51:52Z

I'm still getting "Cannot import private keys to a wallet with private keys disabled" when importing an sppub with 87c1b71.

When importing an sspriv in a wallet (with private keys) the initial rescan doesn't find any transactions. As before, it does find them when doing getnewaddr (with enough different labels) and then rescanning.

Loading a silent payment wallet still crashes with sp_spk_man != nullptr in walletdb.cpp, line 962.

I like how it now produces the same address for the same label (with BIP329 we could also ensure that when restoring a wallet you get the same index->label mapping).

I wonder how easy / hard it would be to get fast rescan working if the user has both -blockfilterindex and the new -bip352index (bitcoin#28241) enabled.

Eunovo · 2024-07-23T13:00:42Z

I'm still getting "Cannot import private keys to a wallet with private keys disabled" when importing an sppub with 87c1b71.

Sorry about that slight error in 87c1b71. I've already fixed this. Will push after I get signing test working for sp descriptor in test/functional/wallet_miniscript.py

…ethods

- Stop adding scan key to singingprovider private key map, this will prevent scan key from being counted among private keys - Stop storing scan keys to db, scan keys are already saved in descriptor

DescriptorScriptPubKeyMan::MarkUnusedAddresses tries to expand the descriptor and read the resulting scripts This does not work for sp descriptors as no scripts are returned This leads to a SegFault

Eunovo · 2024-07-25T13:47:35Z

Loading a silent payment wallet still crashes with sp_spk_man != nullptr in walletdb.cpp, line 962.

I wasn't able to reproduce this on faabf4e. See results below:

> dbtcd
Bitcoin Core starting
> dbtcli getblockchaininfo
{
  "chain": "regtest",
  "blocks": 0,
  "headers": 0,
  "bestblockhash": "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",
  "difficulty": 4.656542373906925e-10,
  "time": 1296688602,
  "mediantime": 1296688602,
  "verificationprogress": 1,
  "initialblockdownload": true,
  "chainwork": "0000000000000000000000000000000000000000000000000000000000000002",
  "size_on_disk": 293,
  "pruned": false,
  "warnings": [
    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
  ]
}
> dbtcli loadwallet alice
{
  "name": "alice"
}
> dbtcli listdescriptors
{
  "wallet_name": "alice",
  "descriptors": [
    {
      "desc": "sp(sprtpub1qqqqqqqqqqqqqqz2cezqye6220rn3xzpxu5lxdh8l89x6mvvz25vx699mlz5tss635p7y6txdrl97smz23cmxylj0wmtygzulgc9ynpctsj8g3zxp9eycgcpvngw9)#nnrxeufl",
      "timestamp": 1296688602,
      "active": true,
      "internal": false
    }
  ]
}

Note that dbtcd and dbtcli are aliases for src/bitcoind and src/bitcoin-cli respectively

josibake

LGTM, left some comments/questions but going to go ahead and merge and start testing. Can address any comments in follow-ups

josibake · 2024-08-04T07:53:21Z

src/rpc/output_script.cpp

+                for (const auto& sppubkey_pair: provider.sppubkeys) {
+                    addresses.push_back(EncodeDestination(V0SilentPaymentDestination(sppubkey_pair.second)));
+                }


in 06281a3:

silent payments aren't ranged, so not clear to me how this line would be hit if calling deriveaddresses with an sp descriptor?

This code always runs at least once. See line 268

josibake · 2024-08-04T07:59:16Z

src/wallet/scriptpubkeyman.cpp

+    V0SilentPaymentDestination main_dest;
+    main_dest.m_scan_pubkey = sppubkey->scanKey.GetPubKey();
+    main_dest.m_spend_pubkey = sppubkey->spendKey;


in e81e28a:

Can simplify this to use the constructor, e.g.:

Suggested change

V0SilentPaymentDestination main_dest;

main_dest.m_scan_pubkey = sppubkey->scanKey.GetPubKey();

main_dest.m_spend_pubkey = sppubkey->spendKey;

auto main_dest = V0SilentPaymentDestination{sppubkey->scanKey.GetPubKey(), sppubkey->spendKey};

josibake · 2024-08-04T08:06:35Z

src/wallet/walletutil.cpp

+        // We only need to save the spend key
+        // The scan key is preserved in the descriptor


in a065b4b:

What would be the harm in persisting the key to the database? I think this is fine, but I am suspicious there might be unintended consequences from not persisting the key. Will need to study this section a bit more, thought, to understand how descriptors are persisted.

No harm that I can think of, It just requires more code. Currently, the wallet does not allow the persistence of private keys when the private keys are disabled. If we don't persist the scan key, this error isn't triggered on watch only wallets.

josibake · 2024-08-04T08:09:40Z

src/wallet/scriptpubkeyman.cpp

-CTxDestination SilentPaymentDescriptorScriptPubKeyMan::GetLabelledDestination(uint64_t index)
+V0SilentPaymentDestination SilentPaymentDescriptorScriptPubKeyMan::GetLabelledDestination(uint64_t index)


in 817e931:

why the change to V0SilentPaymentDestination? Seems like this was fine as a CTxDestination?

I reversed the change locally and it compiles successfully. I'm not sure why I changed it initially.

Sjors · 2024-08-07T07:05:03Z

I wasn't able to reproduce this

I'll let you know if I run into the crash in later updates. Haven't tested again yet.

* rpc/deriveaddress: Add support for sp desc * spscriptpubkeyman: Move labelled destination generation to seperate methods * rpc/getnewaddress: Add silent-payment to addess_type list * descriptor: Allow extended keys in sp() * Fix scan key storage issues in watch only wallet - Stop adding scan key to singingprovider private key map, this will prevent scan key from being counted among private keys - Stop storing scan keys to db, scan keys are already saved in descriptor * spscriptpubkeyman: Fix MarkUnusedAddresses SegFault DescriptorScriptPubKeyMan::MarkUnusedAddresses tries to expand the descriptor and read the resulting scripts This does not work for sp descriptors as no scripts are returned This leads to a SegFault * test: Add E2E test for sp descriptor

Eunovo mentioned this pull request Jul 16, 2024

Implement bip352 full #1

Closed

Eunovo marked this pull request as draft July 16, 2024 16:57

Eunovo force-pushed the allow-ext-keys branch from 8837cc4 to 68ec079 Compare July 18, 2024 16:23

Eunovo force-pushed the allow-ext-keys branch from 68ec079 to 115b281 Compare July 20, 2024 09:47

Eunovo force-pushed the allow-ext-keys branch 2 times, most recently from adcfa5d to 87c1b71 Compare July 22, 2024 21:34

Eunovo force-pushed the allow-ext-keys branch from 87c1b71 to 236f2e0 Compare July 25, 2024 10:01

Eunovo added 7 commits July 25, 2024 11:46

rpc/deriveaddress: Add support for sp desc

06281a3

spscriptpubkeyman: Move labelled destination generation to seperate m…

e81e28a

…ethods

rpc/getnewaddress: Add silent-payment to addess_type list

9712198

descriptor: Allow extended keys in sp()

99e63b2

Fix scan key storage issues in watch only wallet

a065b4b

- Stop adding scan key to singingprovider private key map, this will prevent scan key from being counted among private keys - Stop storing scan keys to db, scan keys are already saved in descriptor

spscriptpubkeyman: Fix MarkUnusedAddresses SegFault

817e931

DescriptorScriptPubKeyMan::MarkUnusedAddresses tries to expand the descriptor and read the resulting scripts This does not work for sp descriptors as no scripts are returned This leads to a SegFault

test: Add E2E test for sp descriptor

faabf4e

Eunovo force-pushed the allow-ext-keys branch from 236f2e0 to faabf4e Compare July 25, 2024 13:43

Eunovo marked this pull request as ready for review July 25, 2024 13:48

Eunovo mentioned this pull request Aug 3, 2024

More sp tests #5

Open

josibake reviewed Aug 4, 2024

View reviewed changes

josibake merged commit 33d9be1 into josibake:implement-bip352-full Aug 4, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ext keys #3

Allow ext keys #3

Eunovo commented Jul 16, 2024

Sjors commented Jul 16, 2024

Sjors commented Jul 16, 2024

Sjors commented Jul 16, 2024

Eunovo commented Jul 17, 2024

Sjors commented Jul 17, 2024 •

edited

Loading

Sjors commented Jul 18, 2024

Eunovo commented Jul 20, 2024 •

edited

Loading

Eunovo commented Jul 20, 2024

Sjors commented Jul 22, 2024

josibake commented Jul 22, 2024

Eunovo commented Jul 23, 2024 •

edited

Loading

Sjors commented Jul 23, 2024 •

edited

Loading

Eunovo commented Jul 23, 2024

Eunovo commented Jul 25, 2024 •

edited

Loading

josibake left a comment

josibake Aug 4, 2024

Eunovo Aug 4, 2024

josibake Aug 4, 2024

josibake Aug 4, 2024

Eunovo Aug 4, 2024 •

edited

Loading

josibake Aug 4, 2024

Eunovo Aug 4, 2024 •

edited

Loading

Sjors commented Aug 7, 2024

		// We only need to save the spend key
		// The scan key is preserved in the descriptor

		CTxDestination SilentPaymentDescriptorScriptPubKeyMan::GetLabelledDestination(uint64_t index)
		V0SilentPaymentDestination SilentPaymentDescriptorScriptPubKeyMan::GetLabelledDestination(uint64_t index)

Allow ext keys #3

Allow ext keys #3

Conversation

Eunovo commented Jul 16, 2024

Sjors commented Jul 16, 2024

Sjors commented Jul 16, 2024

Sjors commented Jul 16, 2024

Eunovo commented Jul 17, 2024

Sjors commented Jul 17, 2024 • edited Loading

Sjors commented Jul 18, 2024

Eunovo commented Jul 20, 2024 • edited Loading

Eunovo commented Jul 20, 2024

Sjors commented Jul 22, 2024

josibake commented Jul 22, 2024

Eunovo commented Jul 23, 2024 • edited Loading

Sjors commented Jul 23, 2024 • edited Loading

Eunovo commented Jul 23, 2024

Eunovo commented Jul 25, 2024 • edited Loading

josibake left a comment

Choose a reason for hiding this comment

josibake Aug 4, 2024

Choose a reason for hiding this comment

Eunovo Aug 4, 2024

Choose a reason for hiding this comment

josibake Aug 4, 2024

Choose a reason for hiding this comment

josibake Aug 4, 2024

Choose a reason for hiding this comment

Eunovo Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

josibake Aug 4, 2024

Choose a reason for hiding this comment

Eunovo Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

Sjors commented Aug 7, 2024

Sjors commented Jul 17, 2024 •

edited

Loading

Eunovo commented Jul 20, 2024 •

edited

Loading

Eunovo commented Jul 23, 2024 •

edited

Loading

Sjors commented Jul 23, 2024 •

edited

Loading

Eunovo commented Jul 25, 2024 •

edited

Loading

Eunovo Aug 4, 2024 •

edited

Loading

Eunovo Aug 4, 2024 •

edited

Loading