Silent Payments: Receiving #32966

In 89831dace9d21456eb4a019d86164304e22f458e wallet/rpc: add create silent-payments wallet option: while testing I found that it doesn't disable block filters.

Thanks @Sjors. I'll fix it

We can disable the fast rescan for now just to keep this PR focused, but in theory we should be able to use the fast rescan regardless of whether or not the wallet is a silent payments wallet. Perhaps we can leave a comment to enable fast rescans in a follow up?

IIUC The silent payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in a GCS filter. Since we can't skip blocks, fast rescans shouldn't provide any advantages here.

EDIT It might also be possible to use fast rescan when importing descriptors if none of the descriptors are silent payment descriptors. Is this what you were referring to @josibake?

The silent payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in a GCS filter.

This makes sense. Is it documented anywhere so we don't forget by the next review round? :-)

It might also be possible to use fast rescan when importing descriptors if none of the descriptors are silent payment descriptors.

It wasn't referring to that and I wouldn't worry about this. Somewhere in the RPC documentation (createwallet and/or importdescriptors) we should warn that silent payments disables fast rescan. But -blockfilterindex isn't on by default and so it's probably only advanced users that will run into this.

This makes sense. Is it documented anywhere so we don't forget by the next review round? :-)

I added a separate commit that disables fast rescan and I added an explanation to the commit message. Will push soon.

EDIT see https://github.com/bitcoin/bitcoin/pull/32966/commits/82db7ccaa60a4e1dc0dac6b8f0368ff933303e39

The silent payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in a GCS filter

This is correct. I was imagining a world where we can pre-compute the scriptPubKeys, but realised we have to iterate the blocks anyways to get the tweaks so its likely faster to just check directly. In the future, if the wallet has some sort of silent payments index, then I think fast rescans would be possible (and beneficial).

cb44e3812c741e3a939a64799130bbf0787ef89a : I think we should set bool IsRange() const override { return false; } so we don't have to think about ranged descriptors.

Done

While reviewing the PR, it seems a lot of things could be simplified by if we just treated the scan key as something that isn't a CKey, i.e., we introduce a completely new object for holding the scan key. I think that would allow us to drop this commit and simplify other places where we are trying to work around the fact that a scan key isnt really a private key.

I haven't thought this all the way through yet, but in my initial pass on the PR it seems like it should be possible.

Would recommend splitting the output type changes into their own commit(s). Its a self-contained change, and splitting them out from the descriptor changes makes the descriptor commit easier to review.

Done

Potentially another area we could simplify if we end up not treating a scan key as a CKey.

Confirmed, considering we don't need ExpandPrivate anymore, this can remain const final.

AFAICT, we only use the sp_keys member of the FlatSigningProvider as a way of passing around the scan key and the spend pubkey, and then also for the spend private key (for signing). It makes sense to me that we would later add the spend private key + tweaks to the FlatSigningProvider, but ideally thats the only thing we use it for and can get rid of sp_keys.

Without sp_keys, the wallet can't identify which keys are scan and spend keys from the signing provider. Do you have an alternative approach in mind?

nit: unnecessary formatting change, should just be a one-line change.

Fixed.

Another place we could simplify things if we instead came up with a custom type for the scan key.

Feels like the SigningProvider should only care about the spend_key + tweak for signing, so another place to potentially simplify things if scan key is a custom type instead of a CKey.

This could be moved into the output type commit.

Done

This could be moved to the output type commit.

Done

I think its more appropriate to move this commit into the PR where we first introduce V0SilentPaymentDestination. I'll cherry pick this out into https://github.com/bitcoin/bitcoin/pull/28122

Cherry-picked this commit into https://github.com/bitcoin/bitcoin/pull/28201/commits/b94934e089e70b478ea6aa535098586fedb5a12d, in #28201. I added it to the commit where we enable silent payment addresses as valid addresses, at the end of the sending PR.

I've dropped this commit from this branch.

If we can create a custom type for the scan key, it seems better to have a special function just for silent payments that returns this custom key type from an xpriv, rather than change GenerateWalletDescriptor to work on an xpriv.

Ideally we can do this without needing an xpriv, but I vaguely remember a reason why we need the xpriv here. Will dig in a bit more to see if we can avoid this.

Don't we need to derive the scan and spend keys?

Just curious, why the extra assert? Seems unrelated to the other changes.

In the OutputType::SILENT_PAYMENTS case, I set the desc_str, not the desc_prefix; I have to modify the assert to prevent a failure.

This is returning just the spend private key, right? IIRC, the scan private key is removed from out.keys as part of the Parse function. Not clear to me why we would want the spend private key in the out_keys out param.

The derived spend private key must be saved to the DB by the wallet.

We can avoid this by doing what other descriptors do; the sp descriptor will then be in this form sp(xpriv/352h/0h/0h/1h/0,xpub/352h/0h/0h/0h/0) . The Parse function will then derive the scan key and save it in the descriptor. The spend key can be derived later when needed from the master key.

With this alternative method, the wallet only saves the master key to DB as it has always done.

The alternative method seems preferable to me. I dont see any reason to treat the spend key differently from other keys in the wallet.

I'm wondering if these two methods (CKey::TweakAdd, CPubKey::TweakAdd) can also be moved into #28122, as they seem a little out of place in this PR (though, admittedly its only in this PR that we actually use them).

Maybe they deserve their own PR?

I'd remove this (and any other code) related to non-change labels from this PR. Ideally, implement just enough for labels to get change working, and then we can fully implement labels in a follow-up PR.

It feels strange to call the provider just to get the scan key, feels like another area we can simplify with a custom scan key type.

Same comment regarding moving labels related code to a follow-up PR.

The name TopUp here makes sense in that we are trying to match how the rest of the wallet does it, but @Sjors pointed out this can be confusing for people who think of TopUp in terms of the gap limit / key pool refresh context, which we aren't really doing here. We could just have one function called AddTweak and then use a boolean for writing to the database, e.g., AddTweak(uint256& tweak, store_in_db=false)

Updated

Could be AddTweakWithDB (based on my previous comment).

Worth mentioning, the only reason we wouldn't be able to do this is if this function (and TopUp) are called generically on SPKMans, e.g., in a loop. I don't think this is the case, but wanted to mention it.

Updated

Can likely be removed if we have custom methods for storing scan/spend pubkey data, or custom methods for getting them.

This commit is quite long, making it difficult to focus on the main thing which is the scanning logic. I'd suggest breaking out smaller commits for just the mempool related changes, and just the undoblock related changes. Possibly more, but will need to dig into the commit a bit more.

I broke this into 3 commits.

Do we need the CTransactionRef here, as opposed to just operating on block.data->vtx directly?

We only use the reference once, so it's not needed. I'll take this out as I retouch.

I ended up leaving the reference and used it in 2 other locations. The code looks slightly cleaner this way.

5be304dfa50bcfd3b0a92eff52bac3d8023ce523 nit: no need to set next_index during import

Done

I think this change is breaking the ability to import sp descriptor with private keys defined for scan and spend This is the warning I receive when using importdescriptors: "Not all private keys provided. Some wallet functionality may return unexpected errors"

@macgyver13 Did you use the same private key for scan and spend?

This is the format I used to import scan and spend private keys: sp([fingerprint/352'/1'/0'/1'/0]WIF(private scan key),[fingerprint/352'/1'/0'/0'/0]WIF(private spend key)#checksum

@macgyver13 I changed the implementation and succeeded in removing the need for this code entirely

I can confirm the import warning is resolved. 👏

However when I try to generate a silent-payments address after importdescriptor I do generate an exception Error: No silent-payments addresses available. (-12) imported_wallet.getnewaddress(address_type="silent-payments")

I haven't been able to reproduce the error. Can you share the following information:

• Your wallet creation parameters
• Your importdescriptors parameters (without the private keys) and response ?

Great news, I was able to use dynamic descriptor creation -> export -> import using private keys and confirmed imported_wallet.getnewaddress(address_type="silent-payments") now works as expected.

Still have 1 issue when trying to use sender.send(outputs={sp_addr: TOTAL_SPEND}, options=options) in a different test - you can review the test here

Shouldn't the coin_type be different for regtest/signet? /352h/1h instead of /352h/0h

User descriptor keys do not follow any derivation specification. They can import descriptors with keys derived using any path.

Per the BIP, it does specify coin_type (which is defined in BIP44). BIP44 does reserve 1 as a registered coin type for Bitcoin test networks (i.e., testnet, signet). Revisiting the language in the BIP, I'm realising this is not worded strongly enough and should be updated.

I do think we should follow the convention of deriving mainnet keys and test keys from distinct chains in our tests. This also matches what we do in other functional tests, e.g., wallet_descriptor.py.

That's definitely a good idea. Some hardware wallets may even refuse to sign if the coin type derivation is wrong, e.g. to prevent a testnet app from moving mainnet coins.

I've changed this to use to /352h/1h. I also edited GenerateWalletDescriptor to use /352h/1h for test chain.

We shouldn't need to mark this as override, considering IsRange on the DescriptorImpl is wrapping a lower IsRange call:

https://github.com/bitcoin/bitcoin/blob/bd7e9026af484a0b0df4ef40800969860ffb5949/src/script/descriptor.cpp#L889-L898

Done

nit: I was finding this function hard to follow while reviewing. I think something like this would be slightly more readable:

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 0bd2251e13..fb3252f398 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1674,26 +1674,28 @@ public:
     {
         FlatSigningProvider dummy;
         std::string scan_key;
-        bool has_scan_key{m_pubkey_args.at(0)->ToPrivateString(dummy, scan_key)};
-        assert(has_scan_key); // The ScankeyPubkeyProvider always has the scan key
-        std::string ret{m_name + "(" + scan_key + ","};
-        auto& spend_pubkey{m_pubkey_args.at(1)};
-        std::string tmp;
+        // This should never fail, since the ScankeyPubkeyProvider will always contain
+        // the scan key.
+        //
+        // The scan key is stored at m_pubkey_args[0], the spend key at m_pubkey_args[1]
+        if (!m_pubkey_args.at(0)->ToPrivateString(dummy, scan_key)) return false;
+        std::string scan_key_str{m_name + "(" + scan_key + ","};
+        std::string spend_key_str;
         switch (type) {
         case StringType::NORMALIZED:
-            if (!spend_pubkey->ToNormalizedString(*arg, tmp, cache)) return false;
+            if (!m_pubkey_args.at(1)->ToNormalizedString(*arg, spend_key_str, cache)) return false;
             break;
         case StringType::PRIVATE:
-            if (!spend_pubkey->ToPrivateString(*arg, tmp)) return false;
+            if (!m_pubkey_args.at(1)->ToPrivateString(*arg, spend_key_str)) return false;
             break;
         case StringType::PUBLIC:
-            tmp = spend_pubkey->ToString();
+            spend_key_str = m_pubkey_args.at(1)->ToString();
             break;
         case StringType::COMPAT:
-            tmp = spend_pubkey->ToString(PubkeyProvider::StringType::COMPAT);
+            spend_key_str = m_pubkey_args.at(1)->ToString(PubkeyProvider::StringType::COMPAT);
             break;
         }
-        out = std::move(ret) + std::move(tmp) + ")";
+        out = std::move(scan_key_str) + std::move(spend_key_str) + ")";
         return true;
     }

Done

Since we are creating the provider a few lines up, I think push_back is more appropriate here:

                ret.push_back(std::move(provider));

Done

Same comment regarding push_back vs emplace_back

Done

Probably fine to leave all of these as plurals for now, e.g., scan_pubkeys, but I think its more appropriate to have these be scan_pubkey and spend_pubkey and have a comment explaining the silent payments descriptor does not support multipath descriptors. We could even add an assert for this, since we expect a silent payments descriptor to only ever have one scan key and one spend key.

Revisiting this, it would be nice to if we enforce that an SP descriptor cannot be multipath. Beyond that, I think its fine to keep the plurals and for loops, since that matches the style of the rest of the wallet code.

I don't have a better alternative to recommend, but this feels a bit off. As discussed offline, an alternative we can look into is subclassing the Descriptor class, e.g., SilentPaymentsDescriptor and adding a new method just for getting the scan key.

Perhaps another direction worth considering is modelling the scan key as an ExtPubKey, where the scan key is saved as a member (similar to how the chaincode is treated).

Another idea: add a new method to the descriptor class , GetPubKeyProviders ? This avoids needing to subclass just for silent payments, and may prove to be a generally useful method. This way, we can return the ScankeyPubkeyProvider and access the scan "private" key directly from that.

I'm curious if there is another use case for undoData outside of Silent payments. If so, we could break this out into its own commit. Might be worth doing anyways, even if silent payments is (currently) the only use case.

Reiterating this point. I think I've seen a few PRs related to undodata recently, seems like a good time to bundle all of them as a well motivated refactor? cc @TheCharlatan

You mention it in the commit message, but I think its worth adding a detailed comment above that:

• Explains the nuance around m_scan_key
• Explains why we are using a PubKeyProvider, e.g., getting origin information without needing to write custom code

I think a comment is slightly better than a commit message as I imagine this will trip people up in the future who are looking at this code and surprised to see a private key as a member field inside a pubkey provider.

In ToPrivateString above , you omit the argument name to indicate its not used; should do the same here:

    void GetPrivKey(int, const SigningProvider&, FlatSigningProvider& out) const override

I'm still curious if we could avoid the public SpDescriptor, since its only needed for this GetScanKey function. That being said, this also seems fine and does help avoid us needing to special case and write custom code in other places.

Maybe we can use a namespace?

This is only used to recreate the silent payment taproot scriptpubkeys, i.e., $B_{spend} + tweak \cdot G$. But since we already have the full scriptpubkey when scanning, we could just save the tweak and scriptpubkey to the database.

This means more storage, but then we can drop this CPubKey::TweakAdd method. This seems better to me because storage is cheap, this should make loading the wallet faster, and it removes a footgun from CPubKey (we dont really want to have a generic method for adding public keys in this codebase).

I think we could also remove this function by introducing a SilentPaymentsSign function (similar to how we have a taproot sign function). I'll dig into this a bit more, but it seems better to have a special signing function that can only be used for silent payments, instead of a generic CKey::TweakAdd function that might be abused/footgunny.

FYI, I have a WIP commit that implements a SilentPaymentsSign function, I'll push the commit later today. Its a bit more code than what we have with CKey::TweakAdd, but I do like that its silent payments specific and can't be used as a footgun.

Not quite sure why this is necessary?

We need it to extract the change_dest from the results of CreateSilentPaymentOutputs.

Not a comment for this commit specifically, but I do think we should document either in a commit or in a commit message the entire logic and flow of using silent payments for change. Its a bit complicated and without a clear understanding of all of the cases we need to cover, these changes are a bit hard to follow.

Even better, if we dont have one already, we can create a test case for all of the possible change scenarios and document there.