Silent payments

[josibake]

For general discussion of implementation details for silent payments on the branch josibake/implement-bip352-full.

[josibake]

Regarding @Sjors feedback on #1 :

the sp() descriptor returned by listdescriptors does not have the origin prefix. This makes it look like they're randomly generated and not derived from the seed, though afaik they are derived as m/352h/0h/0h/0h/0 and m/352h/0h/0h/1h/0

Based on the discussion at CoreDev, it was agreed sppub / spprv would not have origin information, since they might have been created from a random set of keys not in a BIP32 tree. I think there is some merit to including the origin information, but will require more thought on how to serialize this. For now, I'd recommend we proceed without the origin information and tackle it as its own task / PR later one once we are confident everything is working as is.

Regarding the GUI, send / receive is only supported on the CLI for now, can tackle the GUI in it's own PR (although I am largely unfamiliar with the GUI codebase 😬 ).

Regarding labels:

It's possible that I used labels in the "wrong" order - and so the one unused label caused the import to not find my transaction?

But this strategy seems problematic, because you can't guarantee that people will pay to your label. Oh hi gap problem! :-) cc @josibake.

Order shouldn't matter at all with labels? In general, I think reusing the range argument for labels could make sense? Generally speaking, the wallet should keep track of how many labels it has used. In order to deal with the gap limit problem, when rescanning the wallet should pre-generate a large number of labels (the BIP recommends 100,000). Another idea is to include the number of labels used in the sppub / spprv format, but this kinda goes against the descriptor philosophy as I understand it in that descriptors are not supposed to include range information.

[Sjors]

(splitting this out so we can use the inline comment feature)

Based on the discussion at CoreDev, it was agreed sppub / spprv would not have origin information, since they might have been created from a random set of keys not in a BIP32 tree.

I don't think there's any difference with other descriptors; when a descriptor is generated from the wallet seed we add origin information, when we import its key from somewhere else, we either don't add origin info or we add the remote origin info if we know it.

For now, I'd recommend we proceed without the origin information and tackle it as its own task / PR later one once we are confident everything is working as is.

It wouldn't break anything to change the behaviour later.

[josibake]

It wouldn't break anything to change the behaviour later.

Right, which is why I'm saying we should punt the origin information discussion for now :)

[Sjors]

Regarding the GUI, send / receive is only supported on the CLI for now, can tackle the GUI in it's own PR (although I am largely unfamiliar with the GUI codebase 😬 ).

Well, we can't merge the cli changes if we don't at least disable sending to silent payment address in the GUI.

From my experience with adding external signer support, the code paths between GUI and cli are almost identical. You probably just missed one call site, with whatever you did to make this work in the cli.

It's probably easier to fix the issue than to figure out how to disable this.

[josibake]

For context, I'm only referring to this branch, not "PRs for Bitcoin Core." I'm viewing this implement-bip352-full branch as strictly focused on the code changes needed to get basic, well tested send and receive functionality working before breaking things out into individual PRs against Bitcoin Core. GUI support isn't a priority here since that's something we can likely tackle once ready to open PRs against Bitcoin Core.

[Sjors]

the BIP recommends 100,000

Generally our keypool is 1000, which is what the range argument is based on. But I'm not against making 100,000 the default range argument for silent descriptors.

At least when importing them. But if we want to encourage this 100,000 recommendation, we should also use when exporting a descriptor. In that case it's easiest to also set this number at wallet creation time.

[josibake]

I think its fine to use a lower value with keypool because the expectation is that addresses were only used once and handed out sequentially. But that's not the case with silent payment labels. I might have generated 20,000 different labelled addresses over the life of the wallet but only ever received payments on the 19,000th address.

Also, IIRC, keypool is not exported with the descriptor? Descriptors themselves don't contain any range information?

[Sjors]

Descriptors don't have range information, but listdescriptors does (along with a birthday timestamp) - and people might use that then they import somewhere else. So if we internally use 100K, we should communicate that in the RPC output.

Descriptor wallets don't have a "keypool", but internally descriptors have a cache - which is a similar mechanism. The range argument determines the initial size of that cache.

There might be some performance bottleneck if we make it too big, @achow101 would know.

But if we don't use 100K internally, it seems strange to recommend it in the BIP. We could even consider something like "wallets MUST scan up to, and SHOULD NOT generate, more than N labels". When N is some number such that a (naive implementation of) processing the labels is >100x faster than the (secp optimized) elliptic curve operation.

[Sjors]

It's possible that I used labels in the "wrong" order - and so the one unused label caused the import to not find my transaction?

Order shouldn't matter at all with labels?

I have no idea what messed up that early wallet.

The approach here should avoid any future confusion #3 (comment):

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).

[Eunovo]

AddPubTweakData to Coin and save to CoinsDB?

scantxoutset accepts descriptors and searches through the UTXO set (stored in CoinsDB) for UTXOs that belong to provided descriptors

• Do we need this functionality for sp? How is scantxoutset used in production(will it be needed for sp descriptors)?

For sp descriptors, we need input_hash . A see bip352 gotten from the transaction inputs to determine ownership. One way to do this is to pull data required from DB when we encounter a coin while scanning. This is inefficient and undesirable. One way to speed up this process is to store the required data input_hash . A with coin data that is stored in the CoinsDB

@josibake @Sjors thoughts?

• Did I miss something?
• Is there another option I haven't considered?

[josibake]

I think it's a bit too invasive at this stage to change CoinsDB for what is intended to be a wallet only feature. The nice thing about the index approach (see Sjors PR) is that it's only used by people who are actively using silent payments and doesn't affect the rest of the node. For scantxoutset, I remember @Sjors and I talked about having the index only track unspents (or at least have a field for spent/unspent), but I'm not sure if this has been implemented. If this exists, then it's as simple as scanning the sp_index but only for tweak data where this at least one unspent output.

If we don't track spent/unspent in the index, another approach would be add an new index for key: spk, value: tweak that is synced with the UTXO set every block. scantxoutset would just every record here, and then whenever a new block comes in, we would delete entries from this index by spk. this is similar to proposal to add tweak data to CoinsDB, except now it goes in its own index.

[Eunovo]

How do we InferDescriptor from sp addresses

We need InferDescriptor to infer an sp desc from an sp address so that getaddressinfo can return the correct desc given an sp address. The problem is that sp address contain the public scan key and public spend key but sp descriptors require (for an sppub at least) a private scan key and public spend key.

[josibake]

Just to make sure I understand: this is because when you pass the address, there is now way to convert from the scan public key to the scan_key, and the descriptor is always stored as scan_key, spend_public_key?

First idea that comes to mind is a bit hacky but a simple solution would be just store a mapping of scan_public_key:scan_key when the descriptor is first saved, so that when we want to look something up with the address, we can map the scan_public_key back to a scan_key?

[Eunovo]

First idea that comes to mind is a bit hacky but a simple solution would be just store a mapping of scan_public_key:scan_key when the descriptor is first saved, so that when we want to look something up with the address, we can map the scan_public_key back to a scan_key?

This just made me realize that, since getaddressinfo is a wallet rpc, we can search the wallet for a Silent Payment DescriptorSPKMan , get it's wallet descriptor and check if the scan key matches with the scan public key in the sp address. So desc will be sp(scan_key,address_spend_key) and parent_desc will be the spkman's desc

[Sjors]

Just tried importing my test descriptions into a fresh wallet using implement-bip352-full @ cd48ed2.

During (or at the end of) the rescan it crashes with:

Assertion failed: (ret), function ScanForSilentPaymentOutputs, file bip352.cpp, line 319.

[Sjors]

BIP341 does not require the 32 bytes to represent a valid public key.

I asked for some more clarification here: https://bitcoin.stackexchange.com/questions/123983/is-a-taproot-output-with-unparseable-x-only-pubkey-unspendable

I think BIP352 needs to be modified:

Let outputs_to_check be the taproot output keys from all taproot outputs in the transaction (spent and unspent).

This should specify that only outputs that can be parsed as a valid xonly pubkey should be added to outputs_to_check.

Code wise, it's a matter of changing assert(ret) in line 319 to if (ret) continue;

Update: the implementation fix is slightly more complicated

[Sjors]

Though it would be surprising if this is the first such transaction.

It might not be, I should try rescanning a wallet from taproot activation.

[Sjors]

This seems to do the trick:

diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp
index c4cd72d9b0..77139ba00f 100644
--- a/src/wallet/walletutil.cpp
+++ b/src/wallet/walletutil.cpp
@@ -142,11 +142,15 @@ std::optional<std::pair<std::vector<XOnlyPubKey>, BIP352::PubTweakData>> GetSile
     std::vector<XOnlyPubKey> output_keys;
     for (const CTxOut& txout : tx.vout) {
         std::vector<std::vector<unsigned char>> solutions;
         TxoutType type = Solver(txout.scriptPubKey, solutions);
         if (type == TxoutType::WITNESS_V1_TAPROOT) {
-            output_keys.emplace_back(solutions[0]);
+            XOnlyPubKey maybe_key{std::move(solutions[0])};
+            // Not all 32 byte v1 witness data represents a valid x-only pubkey
+            if (maybe_key.IsFullyValid()) {
+                output_keys.emplace_back(solutions[0]);
+            }
         } else if (type == TxoutType::WITNESS_UNKNOWN) {
             // Cannot have outputs with unknown witness versions
             return std::nullopt;
         }
     }

[Sjors]

Note that IsFullyValid just calls secp256k1_xonly_pubkey_parse

[josibake]

Ah, interesting! This will be a good one to add to the BIP352 test vectors.

[Eunovo]

generatetoaddress does not warn or return an error when you use a silent payment address

I expected generatetoaddress 101 sprt1qqgxg7xs6d7nnkfltujmd987dap42m95pz0235z4590lrxl26nk3rkq6x8l34c5gnp5d7fxxxa8yeqrfn3afstwrv6h2c470atkf5g5x0evqkjq9c to fail with an error or at least warn that silent payments addresses cannot be used in coinbase transactions

[josibake]

This should error. We can't use a silent payment address when paying to the coinbase because the coinbase is an "input-less" transaction, i.e., there is no private key we can use for ECDH.

[Eunovo]

This has been resolved in #5

[Sjors]

For watch-only wallets the rescan is using "fast variant using block filters", which it shouldn't. I can work around it by turning off -blockfilterindex

[josibake]

Hm, this is more of a meta point, but why can't it use the fast variant if it has a tweak index? i.e., get the tweaks, calculate the potential taproot spks, check if they exist in a block filter?

[Sjors]

I thought you intentionally turned this off for a reason. But I'm also not sure what that reason would be. I might misremember, because I just realised I don't have that index on my Mac.

[josibake]

I think in Ava's original receiving PR she had it turned off, and the current receiving PR is still based on that, but I don't think there is a good reason to not use the fast rescan variant (if you have a tweak index). Without a tweak index, I'm think it has to be turned off, since there's no way to get the tweaks without processing the blocks.

[Sjors]

I would also except that the silent payment index would allow us to enable fast rescan.

[Sjors]

Loading an existing wallet, created with importdescriptors, still crashes for me as of 05bd7ed.

Assertion failed: (sp_spk_man != nullptr), function operator(), file walletdb.cpp, line 962.

Loading a wallet created in the regular manner doesn't crash.