Silent Payments: Receiving

Eunovo commented at 8:30 am on July 14, 2025: contributor

This PR is part of integrating silent payments into Bitcoin Core. Status and tracking for the project is managed in #28536 This PR is based on #28201 and will remain in draft until #28201 is merged.

This PR:

Adds a Silent Payments descriptor implemenation
Adds a SilentPaymentsDescriptorScriptPubKeyMan Impl that is a subclass of DescriptorScriptPubKeyMan
Implements Silent Payments scanning for the wallet
Updates sending logic to use silent-payments destination for change when sending to silent-payments destination
Adds unit and functional tests for silent payments-related functionality

Follow-ups

Silent Payments Label functionality is incomplete in this PR and will be added as a follow-up PR.

DrahtBot commented at 8:30 am on July 14, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32966.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33169 (interfaces, chain, refactor: Remove unused getTipLocator and incaccurate getActiveChainLocator by pablomartin4btc)
#33163 (BIP360: Includes the following: by jbride)
#33116 (refactor: Convert uint256 to Txid by marcofleon)
#33112 (wallet: relax external_signer flag constraints by Sjors)
#33043 ([POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32 by w0xlt)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#32977 (wallet: Remove wallet version and several legacy related functions by w0xlt)
#32896 (wallet, rpc: add v3 transaction creation and wallet support by ishaanam)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)
#32763 (wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members by achow101)
#32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
#32579 (headerssync: Preempt unrealistic unit test behavior by hodlinator)
#32523 (wallet: Remove isminetypes by achow101)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#32471 (wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key by Eunovo)
#31974 (Drop testnet3 by Sjors)
#31514 (wallet: allow label for non-ranged external descriptor (if internal=false) & disallow label for ranged descriptors by scgbckbone)
#30243 (descriptors: taproot partial descriptors by Eunovo)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor by achow101)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27260 (Enhanced error messages for invalid network prefix during address parsing. by portlandhodl)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label CI failed on Jul 14, 2025

DrahtBot commented at 10:32 am on July 14, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/45909076324 LLM reason (✨ experimental): The CI failure was caused by errors in clang-tidy checks, specifically in descriptor.cpp and walletutil.cpp, which are treated as failures due to warnings-as-errors settings.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

in src/wallet/rpc/wallet.cpp:361 in 89831dace9 outdated

357@@ -358,6 +358,7 @@ static RPCHelpMan createwallet()
358             {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "If set, must be \"true\""},
359             {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."},
360             {"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."},
361+            {"silent_payments", RPCArg::Type::BOOL, RPCArg::Default{false}, "Enable creating and receiving with a silent payments address. Will disable fast rescans with block filters"},

Sjors commented at 11:23 am on July 14, 2025:

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

Eunovo commented at 7:30 am on July 15, 2025:

Thanks @Sjors. I’ll fix it

josibake commented at 8:53 am on July 15, 2025:

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?

Eunovo commented at 9:43 am on July 21, 2025:

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?

Sjors commented at 12:36 pm on July 21, 2025:

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.

Eunovo commented at 12:55 pm on July 21, 2025:

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

josibake commented at 7:23 am on July 22, 2025:

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

Sjors commented at 8:13 pm on July 14, 2025: member

Another thing I noticed while testing using the send RPC, in a wallet with only an sp() descriptor, is that it insists on having a bech32 descriptor for change:

Transaction needs a change address, but we can’t generate it. Error: No bech32 addresses available.

It seems safer, for backups, to send change to the same silent payment address.

Wallet created by:

0bitcoin rpc createwallet Silent blank=true silent_payments=true
1bitcoin rpc importdescriptors '[{"desc": "sp(xprv...,xprv...)",  "timestamp": "now", "active": true]

Also looking forward to have labels back :-)

in src/script/descriptor.cpp:1669 in cb44e3812c outdated

1433+public:
1434+    SpDescriptor(std::unique_ptr<PubkeyProvider> scan_pubkey, std::unique_ptr<PubkeyProvider> spend_key, CKey scan_key) : DescriptorImpl(Vector(std::move(scan_pubkey), std::move(spend_key)), "sp"), m_scan_key(scan_key) {};
1435+
1436+    std::optional<OutputType> GetOutputType() const override { return OutputType::SILENT_PAYMENTS; }
1437+
1438+    bool IsSingleType() const final { return true; }

Sjors commented at 9:55 am on July 15, 2025:

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

Eunovo commented at 4:48 am on July 20, 2025:

Done

in src/script/descriptor.cpp:209 in 0b3b87c648 outdated

205@@ -206,6 +206,8 @@ struct PubkeyProvider
206     /** Get the descriptor string form including private data (if available in arg). */
207     virtual bool ToPrivateString(const SigningProvider& arg, std::string& out) const = 0;
208 
209+    virtual std::optional<std::string> ToPrivateString(const CKey& key) const = 0;

josibake commented at 2:14 pm on July 15, 2025:

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.

in src/outputtype.cpp:21 in cb44e3812c outdated

17@@ -18,6 +18,7 @@ static const std::string OUTPUT_TYPE_STRING_LEGACY = "legacy";
18 static const std::string OUTPUT_TYPE_STRING_P2SH_SEGWIT = "p2sh-segwit";
19 static const std::string OUTPUT_TYPE_STRING_BECH32 = "bech32";
20 static const std::string OUTPUT_TYPE_STRING_BECH32M = "bech32m";
21+static const std::string OUTPUT_TYPE_STRING_SILENT_PAYMENTS = "silent-payments";

josibake commented at 2:24 pm on July 15, 2025:

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.

Eunovo commented at 4:50 am on July 20, 2025:

Done

in src/script/descriptor.cpp:1003 in cb44e3812c outdated

775@@ -776,7 +776,7 @@ class DescriptorImpl : public Descriptor
776     }
777 
778     // NOLINTNEXTLINE(misc-no-recursion)
779-    void ExpandPrivate(int pos, const SigningProvider& provider, FlatSigningProvider& out) const final
780+    void ExpandPrivate(int pos, const SigningProvider& provider, FlatSigningProvider& out) const override

josibake commented at 2:25 pm on July 15, 2025:

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

josibake commented at 12:27 pm on August 15, 2025:

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

in src/script/descriptor.cpp:1698 in cb44e3812c outdated

1472+
1473+        assert(spend_pubkey.has_value());
1474+        out.sp_keys = std::make_pair(m_scan_key, *spend_pubkey);
1475+
1476+        auto it{tmp.keys.find(spend_pubkey->GetID())};
1477+        if (it != tmp.keys.end()) out.keys.emplace(spend_pubkey->GetID(), it->second);

josibake commented at 2:29 pm on July 15, 2025:

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.

Eunovo commented at 4:31 am on July 20, 2025:

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?

in src/script/descriptor.cpp:1614 in cb44e3812c outdated

1490 enum class ParseScriptContext {
1491-    TOP,     //!< Top-level context (script goes directly in scriptPubKey)
1492-    P2SH,    //!< Inside sh() (script becomes P2SH redeemScript)
1493-    P2WPKH,  //!< Inside wpkh() (no script, pubkey only)
1494-    P2WSH,   //!< Inside wsh() (script becomes v0 witness script)
1495-    P2TR,    //!< Inside tr() (either internal key, or BIP342 script leaf)

josibake commented at 2:34 pm on July 15, 2025:

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

Eunovo commented at 4:48 am on July 20, 2025:

Fixed.

in src/script/descriptor.cpp:2330 in cb44e3812c outdated

1943+
1944+        // Remove all scan private keys to allow
1945+        // importation of sp descriptors into watch-only wallets
1946+        for (auto keyId : scan_keys_rm) {
1947+            out.keys.erase(keyId);
1948+        }

josibake commented at 2:35 pm on July 15, 2025:

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

in src/script/signingprovider.h:219 in cb44e3812c outdated

212@@ -213,6 +213,7 @@ struct FlatSigningProvider final : public SigningProvider
213     std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>> origins;
214     std::map<CKeyID, CKey> keys;
215     std::map<XOnlyPubKey, TaprootBuilder> tr_trees; /** Map from output key to Taproot tree (which can then make the TaprootSpendData */
216+    std::pair<CKey, CPubKey> sp_keys;

josibake commented at 2:38 pm on July 15, 2025:

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.

in src/wallet/test/wallet_tests.cpp:484 in cb44e3812c outdated

480@@ -481,7 +481,7 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, ListCoinsTest)
481     //   2. One UTXO from the change, due to payment address matching logic
482 
483     for (const auto& out_type : OUTPUT_TYPES) {
484-        if (out_type == OutputType::UNKNOWN) continue;
485+        if (out_type == OutputType::UNKNOWN || out_type == OutputType::SILENT_PAYMENTS) continue;

josibake commented at 2:40 pm on July 15, 2025:

This could be moved into the output type commit.

Eunovo commented at 4:49 am on July 20, 2025:

Done

in src/wallet/wallet.cpp:3526 in cb44e3812c outdated

3522@@ -3523,6 +3523,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans(WalletBatch& batch, const CExtKey&
3523     AssertLockHeld(cs_wallet);
3524     for (bool internal : {false, true}) {
3525         for (OutputType t : OUTPUT_TYPES) {
3526+            if (t == OutputType::SILENT_PAYMENTS) continue;

josibake commented at 2:40 pm on July 15, 2025:

This could be moved to the output type commit.

Eunovo commented at 4:49 am on July 20, 2025:

Done

in src/script/descriptor.cpp:2496 in 4cecb01707 outdated

2119@@ -2120,6 +2120,10 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
2120             error = "Address is not valid";
2121             return {};
2122         }
2123+        if (std::holds_alternative<V0SilentPaymentDestination>(dest)) {

josibake commented at 2:44 pm on July 15, 2025:

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

josibake commented at 9:55 am on July 16, 2025:

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.

Eunovo commented at 4:50 am on July 20, 2025:

I’ve dropped this commit from this branch.

in src/wallet/walletutil.cpp:50 in eb1196a5b8 outdated

45@@ -46,13 +46,15 @@ WalletFeature GetClosestWalletFeature(int version)
46     return static_cast<WalletFeature>(0);
47 }
48 
49-WalletDescriptor GenerateWalletDescriptor(const CExtPubKey& master_key, const OutputType& addr_type, bool internal)
50+WalletDescriptor GenerateWalletDescriptor(const CExtKey& master_key, const OutputType& addr_type, bool internal, std::vector<CKey>& out_keys)

josibake commented at 2:48 pm on July 15, 2025:

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.

in src/wallet/walletutil.cpp:83 in eb1196a5b8 outdated

74@@ -73,28 +75,41 @@ WalletDescriptor GenerateWalletDescriptor(const CExtPubKey& master_key, const Ou
75         desc_prefix = "tr(" + xpub + "/86h";
76         break;
77     }
78+    case OutputType::SILENT_PAYMENTS: {
79+        // The actual scan and spend keys will be derived from these XPRIVs
80+        // The actual XPRIV and the spend key will not be retained in the descriptor,
81+        // but the scan key will be.
82+        desc_str = "sp(" + xpriv + "/352h/0h/0h/1h/0," + xpriv + "/352h/0h/0h/0h/0)";

josibake commented at 2:51 pm on July 15, 2025:

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.

Eunovo commented at 5:03 am on July 20, 2025:

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

in src/wallet/walletutil.cpp:96 in eb1196a5b8 outdated

87         // so if we get to this point something is wrong
88         assert(false);
89     }
90     } // no default case, so the compiler can warn about missing cases
91-    assert(!desc_prefix.empty());
92+    assert(!desc_str.empty() || !desc_prefix.empty());

josibake commented at 2:52 pm on July 15, 2025:

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

Eunovo commented at 5:06 am on July 20, 2025:

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.

in src/wallet/walletutil.cpp:115 in eb1196a5b8 outdated

114     // Make the descriptor
115     FlatSigningProvider keys;
116     std::string error;
117     std::vector<std::unique_ptr<Descriptor>> desc = Parse(desc_str, keys, error, false);
118+    for (const auto& key : keys.keys) {
119+        out_keys.push_back(std::move(key.second));

josibake commented at 2:57 pm on July 15, 2025:

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.

Eunovo commented at 5:16 am on July 20, 2025:

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.

josibake commented at 1:13 pm on August 15, 2025:

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

in src/key.cpp:169 in 5d5214b6c1 outdated

165@@ -166,6 +166,13 @@ void CKey::MakeNewKey(bool fCompressedIn) {
166     fCompressed = fCompressedIn;
167 }
168 
169+bool CKey::TweakAdd(const unsigned char* tweak32)

josibake commented at 2:59 pm on July 15, 2025:

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

Eunovo commented at 5:02 am on July 20, 2025:

Maybe they deserve their own PR?

in src/wallet/scriptpubkeyman.cpp:1659 in 0be7ce20f4 outdated

1654+    auto change_label_data = bip352::CreateLabelTweak(provider.sp_keys.first, 0);
1655+    m_map_label_tweaks.insert(change_label_data);
1656+    for (int i = 1; i < m_wallet_descriptor.next_index; i++) {
1657+        // Add the other generated labelled destinations
1658+        m_map_label_tweaks.insert(bip352::CreateLabelTweak(provider.sp_keys.first, i));
1659+    }

josibake commented at 3:02 pm on July 15, 2025:

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.

in src/wallet/scriptpubkeyman.cpp:1651 in 0be7ce20f4 outdated

1646+{
1647+    LOCK(cs_desc_man);
1648+    if (m_wallet_descriptor.descriptor->GetOutputType() != OutputType::SILENT_PAYMENTS) {
1649+        throw std::runtime_error(std::string(__func__) + ": descriptor is not a Silent Payment Descriptor");
1650+    }
1651+    const auto provider{GetSPProvider()};

josibake commented at 3:03 pm on July 15, 2025:

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.

in src/wallet/scriptpubkeyman.cpp:1706 in 0be7ce20f4 outdated

1701+    auto dest = GetLabeledDestination(m_wallet_descriptor.next_index);
1702+    index = m_wallet_descriptor.next_index; // Return the index for this destination
1703+    m_wallet_descriptor.next_index++;
1704+    WalletBatch(m_storage.GetDatabase()).WriteDescriptor(GetID(), m_wallet_descriptor);
1705+    return CTxDestination{dest};
1706+}

josibake commented at 3:04 pm on July 15, 2025:

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

in src/wallet/scriptpubkeyman.cpp:1780 in 0be7ce20f4 outdated

1749+    // Destination at index 0 is the change destination
1750+    auto op_dest = GetLabeledDestination(0);
1751+    return CTxDestination{op_dest};
1752+}
1753+
1754+bool SilentPaymentDescriptorScriptPubKeyMan::TopUp(const uint256& tweak)

josibake commented at 3:09 pm on July 15, 2025:

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)

Eunovo commented at 8:17 am on July 20, 2025:

Updated

in src/wallet/scriptpubkeyman.cpp:1789 in 0be7ce20f4 outdated

1758+    bool res = TopUpWithDB(batch, tweak);
1759+    if (!batch.TxnCommit()) throw std::runtime_error(strprintf("Error during descriptors tweak top up. Cannot commit changes for wallet %s", m_storage.GetDisplayName()));
1760+    return res;
1761+}
1762+
1763+bool SilentPaymentDescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, const uint256& tweak)

josibake commented at 3:11 pm on July 15, 2025:

Could be AddTweakWithDB (based on my previous comment).

josibake commented at 8:00 am on July 16, 2025:

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.

Eunovo commented at 8:17 am on July 20, 2025:

Updated

in src/wallet/scriptpubkeyman.cpp:1782 in 0be7ce20f4 outdated

1780+    FlatSigningProvider provider;
1781+    provider.keys = GetKeys();
1782+    FlatSigningProvider tmp;
1783+    m_wallet_descriptor.descriptor->ExpandPrivate(0, provider, tmp);
1784+    return tmp;
1785+}

josibake commented at 3:11 pm on July 15, 2025:

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

in src/interfaces/chain.h:65 in fc65522812 outdated

61@@ -62,6 +62,12 @@ class FoundBlock
62     //! Read block data from disk. If the block exists but doesn't have data
63     //! (for example due to pruning), the CBlock variable will be set to null.
64     FoundBlock& data(CBlock& data) { m_data = &data; return *this; }
65+    //! Read block's undo data from disk.

josibake commented at 3:14 pm on July 15, 2025:

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.

Eunovo commented at 8:17 am on July 20, 2025:

I broke this into 3 commits.

in src/wallet/wallet.cpp:1482 in fc65522812 outdated

1477+    }
1478+
1479     // Scan block
1480     bool wallet_updated = false;
1481     for (size_t index = 0; index < block.data->vtx.size(); index++) {
1482+        CTransactionRef tx{block.data->vtx.at(index)};

josibake commented at 3:15 pm on July 15, 2025:

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

Eunovo commented at 5:19 am on July 20, 2025:

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

Eunovo commented at 8:19 am on July 20, 2025:

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

josibake commented at 3:19 pm on July 15, 2025: member

Did a big overview with @Sjors , leaving the notes from our discussion as a review. In general, I think we should investigate using a custom type for the scan key since a lot of the changes seem to be hacking around the fact we represent the scan key as a private key, but then often need to use it as not a private key.

Still in the process of reviewing, but leaving the initial notes for now

Eunovo commented at 6:56 pm on July 15, 2025: contributor

Thanks for the reviews @josibake and @Sjors . I am currently travelling, and I will get to them later in the week.

Eunovo force-pushed on Jul 20, 2025

Eunovo commented at 8:33 am on July 20, 2025: contributor

Added @achow101 as co-author on commits with code/ideas taken from https://github.com/bitcoin/bitcoin/pull/28453

DrahtBot removed the label CI failed on Jul 20, 2025

Eunovo force-pushed on Jul 21, 2025

Eunovo commented at 8:23 pm on July 21, 2025: contributor

Another thing I noticed while testing using the send RPC, in a wallet with only an sp() descriptor, is that it insists on having a bech32 descriptor for change:

Transaction needs a change address, but we can’t generate it. Error: No bech32 addresses available.

In https://github.com/bitcoin/bitcoin/pull/32966/commits/ed7f894fc0c1ed3a84f4af73ece619698cee11c0 I modified the change OutputType determination logic to try to use silent payments if bech32m addresses are not available.

It seems safer, for backups, to send change to the same silent payment address.

That may be true, but the wallet tries to match the change OutputType to the other outputs to preserve privacy.

cc @Sjors

Sjors commented at 8:36 am on July 22, 2025: member

@Eunovo that’s an interesting trade-off, and we should probably be consistent with how the rest of the wallet behaves. Otherwise we’d leave a fingerprint of which coins were sent using silent payments.

So in that case I think the bevavior should be:

if coins are sent to a bech32m address, use a silent payment change address (don’t use the regular bech32m descriptor)
otherwise, use a matching output type
- when there’s no descriptor for it, fall back to silent payment change address

Eunovo force-pushed on Jul 22, 2025

Eunovo commented at 12:03 pm on July 22, 2025: contributor

So in that case I think the bevavior should be:

if coins are sent to a bech32m address, use a silent payment change address (don’t use the regular bech32m descriptor)

otherwise, use a matching output type

when there’s no descriptor for it, fall back to silent payment change address

Done

DrahtBot added the label CI failed on Jul 22, 2025

DrahtBot commented at 12:16 pm on July 22, 2025: contributor

🚧 At least one of the CI tasks failed. Task CentOS, depends, gui: https://github.com/bitcoin/bitcoin/runs/46470201152 LLM reason (✨ experimental): The CI failure is caused by an assertion failure in wallet_tests due to a test check failing, leading to subprocess abortion.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

josibake commented at 4:47 pm on July 22, 2025: member

Looks like the most recent change is causing a failure in the unit tests:

2025-07-22T13:09:52.932304Z (mocktime: 2020-08-31T15:34:12Z) [test] [txmempool.cpp:699] [void CTxMemPool::check(const CCoinsViewCache &, int64_t) const] [mempwallet/spend.cpp:1343 CreateTransactionInternal: Assertion `!change_script.empty()’ failed.

Eunovo commented at 6:50 pm on July 22, 2025: contributor

Yes @josibake, it’s a slightly tricky problem. Rather than rush to a solution, I’d like to discuss it next meeting.

DrahtBot added the label Needs rebase on Jul 23, 2025

Eunovo force-pushed on Jul 23, 2025

Eunovo force-pushed on Jul 25, 2025

DrahtBot removed the label Needs rebase on Jul 25, 2025

Eunovo commented at 9:27 am on July 25, 2025: contributor

I’ve fixed all known errors. The current failing CI is unrelated.

josibake commented at 9:36 am on July 25, 2025: member

The current failing CI

Should be fixed in my (just now) pushed changes for the base and sending PRs.

Eunovo force-pushed on Jul 25, 2025

Sjors commented at 1:45 pm on July 25, 2025: member

I did some more testing with 587c63e06955ce5c5f2d64f1c1700ca8628572cf, noticed improvements:

rescan uses the slow version (when -blockfilterindex is enabled), as expected
if I add a label to a transaction, it stores it

Stuff I tested that just works:

listdescriptors gives me a working scan key (tried by importing it into a watch-only wallet)

New bug:

when I send to a bech32m (bcp1...) address with change, using my wallet that only has an sp descriptor, the change output is dropped and added to fees
- strange, because your test in 5be304dfa50bcfd3b0a92eff52bac3d8023ce523 seems to cover exactly this scenario
- perhaps it’s a GUI bug (yikes), because when I use the send RPC instead it behaves correctly
- update: now I can’t reproduce it in the GUI either. It was a pretty small output, so maybe it was an edge case where it wanted to avoid a dust change output - although it was bigger than that.

Some remaining issues:

add silent payments to “output types” in the receive screen (see patch below)
when setting a label in the Send dialog it doesn’t seem to be added to the resulting transaction
if I don’t set silent_payments=true at wallet creation time I can’t change it later and importdescriptors fails with Cannot import silent payment descriptor into a wallet with silent-payments disabled

Some style suggestions:

I prefer giving SilentPaymentDescriptorScriptPubKeyMan in its own file, similar to external_signer_scriptpubkeyman.{h,cpp}

 0diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
 1index 94869aff5a..bbd87c075b 100644
 2--- a/src/interfaces/wallet.h
 3+++ b/src/interfaces/wallet.h
 4@@ -268,6 +268,9 @@ public:
 5     // Return whether the wallet contains a Taproot scriptPubKeyMan
 6     virtual bool taprootEnabled() = 0;
 7
 8+    // Return whether the wallet contains a Silent Payment scriptPubKeyMan
 9+    virtual bool silentPaymentsEnabled() = 0;
10+
11     // Return whether wallet uses an external signer.
12     virtual bool hasExternalSigner() = 0;
13
14diff --git a/src/qt/receivecoinsdialog.cpp b/src/qt/receivecoinsdialog.cpp
15index be7741e8a8..d3164eccf0 100644
16--- a/src/qt/receivecoinsdialog.cpp
17+++ b/src/qt/receivecoinsdialog.cpp
18@@ -99,6 +99,9 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)
19         if (model->wallet().taprootEnabled()) {
20             add_address_type(OutputType::BECH32M, tr("Bech32m (Taproot)"), tr("Bech32m (BIP-350) is an upgrade to Bech32, wallet support is still limited."));
21         }
22+        if (model->wallet().silentPaymentsEnabled()) {
23+            add_address_type(OutputType::SILENT_PAYMENTS, tr("Silent Payment"), tr("A Silent Payment address (BIP-352) can safely be reused. Wallet support is still limited."));
24+        }
25
26         // Set the button to be enabled or disabled based on whether the wallet can give out new addresses.
27         ui->receiveButton->setEnabled(model->wallet().canGetAddresses());
28diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
29index 66b24d1ec2..ed62e21012 100644
30--- a/src/wallet/interfaces.cpp
31+++ b/src/wallet/interfaces.cpp
32@@ -505,6 +505,10 @@ public:
33         auto spk_man = m_wallet->GetScriptPubKeyMan(OutputType::BECH32M, /*internal=*/false);
34         return spk_man != nullptr;
35     }
36+    bool silentPaymentsEnabled() override {
37+        auto spk_man = m_wallet->GetScriptPubKeyMan(OutputType::SILENT_PAYMENTS, /*internal=*/false);
38+        return spk_man != nullptr;
39+    }
40     OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; }
41     CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
42     void remove() override

Note there’s an unrelated GUI glitch that requires you to close and reopen the wallet after importing a new descriptor to get this dropdown to update.

in test/functional/wallet_silentpayments_receiving.py:160 in 5be304dfa5 outdated

156+
157+        descriptor = "sp([eea23daf/352h/0h/0h/1h/0]cRCGWnoELVHTr9oZWz1TUp7jmdBJPS3Kx8UCaHvfpxL6KiVSHH1A,[eea23daf/352h/0h/0h/0h/0]cUxxbQ67tepsEn3AUKSCWAKrvLwP6MPk55DAptcqxqDiXN6yKkNW)#q42ue5dg"
158+        res = wallet.importdescriptors([{
159+            "desc": descriptor,
160+            "active": True,
161+            "next_index": 0,

Sjors commented at 2:03 pm on July 25, 2025:

5be304dfa50bcfd3b0a92eff52bac3d8023ce523 nit: no need to set next_index during import

Eunovo commented at 11:12 am on July 30, 2025:

Done

DrahtBot removed the label CI failed on Jul 25, 2025

in src/script/descriptor.cpp:2330 in 587c63e069 outdated

1945+
1946+        // Remove all scan private keys to allow
1947+        // importation of sp descriptors into watch-only wallets
1948+        for (auto keyId : scan_keys_rm) {
1949+            out.keys.erase(keyId);
1950+        }

macgyver13 commented at 7:39 pm on July 25, 2025:

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”

Eunovo commented at 5:38 am on July 27, 2025:

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

macgyver13 commented at 3:32 pm on July 27, 2025:

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

Eunovo commented at 11:24 am on August 13, 2025:

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

macgyver13 commented at 4:23 pm on August 13, 2025:

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

Eunovo commented at 7:58 am on August 14, 2025:

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 ?

macgyver13 commented at 5:02 pm on August 14, 2025:

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

in test/functional/wallet_silentpayments_receiving.py:156 in 587c63e069 outdated

151+        self.log.info("Check import silent payments descriptor into blank wallet")
152+
153+        self.nodes[0].createwallet(wallet_name="import_blank", blank=True, silent_payments=True)
154+        wallet = self.nodes[0].get_wallet_rpc("import_blank")
155+
156+        descriptor = "sp([eea23daf/352h/0h/0h/1h/0]cRCGWnoELVHTr9oZWz1TUp7jmdBJPS3Kx8UCaHvfpxL6KiVSHH1A,[eea23daf/352h/0h/0h/0h/0]cUxxbQ67tepsEn3AUKSCWAKrvLwP6MPk55DAptcqxqDiXN6yKkNW)#q42ue5dg"

macgyver13 commented at 7:44 pm on July 25, 2025:

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

Eunovo commented at 2:28 pm on July 28, 2025:

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

josibake commented at 6:26 pm on July 28, 2025:

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.

Sjors commented at 6:30 pm on July 28, 2025:

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.

Eunovo commented at 11:12 am on July 30, 2025:

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

macgyver13 commented at 7:45 pm on July 25, 2025: none

working on backup / restore testing I noticed a few issues.

DrahtBot added the label Needs rebase on Jul 28, 2025

bitcoin deleted a comment on Jul 28, 2025

Eunovo force-pushed on Jul 30, 2025

Eunovo force-pushed on Aug 4, 2025

DrahtBot removed the label Needs rebase on Aug 4, 2025

Eunovo force-pushed on Aug 12, 2025

DrahtBot added the label CI failed on Aug 12, 2025

DrahtBot commented at 7:42 pm on August 12, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/47922196595 LLM reason (✨ experimental): The CI failure is caused by a static assertion failure in transaction_tests.cpp due to a mismatch in the expected size of a variant type.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Eunovo force-pushed on Aug 13, 2025

Squashed 'src/secp256k1/' changes from b9313c6e1a..2b57d2ad89

2b57d2ad89 docs: update README
5d2d4adabd ci: enable silentpayments module
d6788a888c tests: add constant time tests
4c32ba9613 tests: add BIP-352 test vectors
183e5414f7 silentpayments: add benchmarks for scanning
288390446a silentpayments: add examples/silentpayments.c
250beff5d2 silentpayments: receiving
f90d7a76b0 silentpayments: recipient label support
1a5f53f2cf silentpayments: sending
ca2538a878 build: add skeleton for new silentpayments (BIP352) module

git-subtree-dir: src/secp256k1
git-subtree-split: 2b57d2ad8964e536508fae0b6ab1331396fe0308

520acba4ef

josibake commented at 9:04 am on August 13, 2025: member

Rebased the base and sending PRs on master and fixed the static assert failure. Rebasing this on the sending PR should fix the CI failure.

Eunovo force-pushed on Aug 13, 2025

DrahtBot removed the label CI failed on Aug 13, 2025

DrahtBot added the label Needs rebase on Aug 13, 2025

Merge commit '520acba4efec147fa97d040a62fbbf8163b6dc8b' into refresh-secp256k1 9ad09bfe87

crypto: add read-only method to KeyPair

Add a method for passing a KeyPair object to secp256k1 functions expecting a secp256k1_keypair.
This allows for passing a KeyPair directly to a secp256k1 function without needing to create a
temporary secp256k1_keypair object.

87b2440d84

Add "sp" HRP c08547838f

Add V0SilentPaymentDestination address type a4c47957d4

common: add bip352.{h,cpp} secp256k1 module

Wrap the silentpayments module from libsecp256k1. This is placed in
common as it is intended to be used by:

  * RPCs: for parsing addresses
  * Wallet: for sending, receiving, spending silent payment outputs
  * Node: for creating silent payment indexes for light clients

87a6ea2133

wallet: disable sending to silent payment address

Have `IsValidDestination` return false for silent payment destinations
and set an error string when decoding a silent payment address.

This prevents anyone from sending to a silent payment address before
sending is implemented in the wallet, but also allows the functions to
be used in the unit testing famework.

450348c982

tests: add BIP352 test vectors as unit tests

Use the test vectors to test sending and receiving. A few cases are not
covered here, namely anything that requires testing specific to the
wallet. For example:

* Taproot script path spending is not tested, as that is better tested in
  a wallets coin selection / signing logic
* Re-computing outputs during RBF is not tested, as that is better
  tested in a wallets RBF logic

The unit tests are written in such a way that adding new test cases is
as easy as updating the JSON file

fc30f2090d

wallet: get serialized size for `V0SilentPayments`

BIP352 v0 specifies that a silent payment output is a taproot output.
Taproot scriptPubKeys are a fixed size, so when calculating the
serialized size for a CRecipient with a V0SilentPayments destination,
use WitnessV1Taproot for the serialized txout size.

6cad71499e

wallet: add method for retreiving a private key

Add a method for retreiving a private key for a given scriptPubKey.
If the scriptPubKey is a taproot output, tweak the private key with the
merkle root or hash of the public key, if applicable.

f2bc953663

wallet: make coin selection silent payment aware

Add a flag to the `CoinControl` object if silent payment destinations
are provided. Before adding the flag, call a function which checks if:

* The wallet has private keys
* The wallet is unlocked

Without both of the above being true, we cannot send to a silent payment
address.

During coin selection, if this flag is set, skip taproot inputs when
script spend data is available. This is based on the assumption that if
a user provides script spend data, they don't have access to the key
path spend. As future improvement, we could instead check to see if we
have access to the key path spend, and only exclude the output when we
don't regardless of whether or not the user provides script spend data.

Also skip UTXOs of type `WITNESS_UNKNOWN`, although it is very unlikely
our wallet would ever try to spend a witness unknown output.

9c0ec7f9ed

wallet: add `IsInputForSharedSecretDerivation` function a6cb8955ce

wallet: add `CreateSilentPaymentOutputs` function

`CreateSilentPaymentsOutputs` gets the correct private keys, adds them
together, groups the silent payment destinations and then generates the
taproot script pubkeys. These are then passed back to
CreateTransactionInternal, which uses these scriptPubKeys to update
vecSend before adding them to the transaction outputs.

b50f02a168

wallet: update TransactionChangeType

If sending to a silent payment destination, the change type should be taproot

28ebef9a04

wallet: enable sending to silent payment address

Treat silent payment addresses as valid destination. Also disable using
silent payment addresses with the `addr()` descriptor, as this
descriptor expects an encoding of a scriptPubKey, whereas a silent
payment address consists of instructions on how to generate a
scriptPubKey for the recipient.

Co-authored-by: Oghenovo Usiwoma <37949128+eunovo@users.noreply.github.com>

31397b3fae

tests: add sending functional tests 9be4755a7b

Add OutputType::SILENT_PAYMENTS 262c78e120

descriptors: impl ScankeyPubkeyProvider

While the scankey looks like a private key, and is represented as a `CKey` in the code,
it is actually much closer to a public key and it needs to be treated as a public key,
so it can be added to watch-only wallets.

2ca8f2957d

descriptor: implement sp(scan, spend) desc

sp() expects a scan private key and a spend key which could be a private key
or a public key.
The scan key is really a "scan entropy" and will not be treated as an actual
private key

bd7e9026af

descriptor: add scankeys to GetPubKeys output 9252ffc4e2

pubkey: add TweakAdd() method

This method makes it easier to tweak silent payment spend keys in later commits

52126c9425

notifications: Add spent_coins to tx added notification

The spent_coins data will be used to scan for silent payments in later commits

Co-authored-by: Ava Chow <github@achow101.com>

00d10dbb7c

interfaces: Load block undo data

This undo data will be used to scan for silent payments in later commits

Co-authored-by: Ava Chow <github@achow101.com>

7fa9eace9c

wallet: generate sp() descriptor 494b8a085f

wallet: create SilentPaymentDescriptorScriptPubKeyMan

Co-authored-by: Ava Chow <github@achow101.com>

b55efdf996

wallet: scan for silent payments

Co-authored-by: Ava Chow <github@achow101.com>

29dc494528

wallet/rpc: add create silent-payments wallet option

Co-authored-by: Ava Chow <github@achow101.com>

991845d34c

wallet: disable fast rescan for silent_payments wallet

silent_payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in it's fast rescan filter.

afc3cbadb4

wallet: import silent payments descriptor 20e73ea491

tests: add silent payments receiving functional tests

Co-authored-by: Ava Chow <github@achow101.com>

d742e90720

wallet/rpc: add sp-address support to getaddressinfo 180b231aba

update addressbook with found silent-payment outputs

traditionally, the receiving scripts are known to the core wallet
because they are added to the addressbook at the time they are requested.

in silent payments, the outputs are not known until found. its important, however,
to have the found scripts in the addressbook so all of the change accounting can be down
properly.

this commits adds found outputs to the address book if they are not change. the way
change is determined is a bit hacky in that we just check if the found output was
found with a label (since this is the only label currently implemented). in the future,
we should specifically check for a change specific label.

9d627d9d84

receiving: check for self-transfers

Check if a send is a self transfer when the transaction is created. This ensures
our cache and address book is properly updated since the wallet will not recheck
this transaction fully if it detects change.

775e83cc99

use silent payments change when sending

The wallet tries to match change OutputTpe to other outputs to preserve privacy.
Use silent payment change address when sending coins to:
- any silent payment address
- any taproot address

705fdbbc90

wallet/spend: Filter out coins not eligible to pay sp change dest caa65f44ed

wallet: retry CreateTransaction without silent_payments change

`CreateTransaction` fails if change_type is set to OutputType::SILENT_PAYMENTS
and there are no eligible inputs for the transaction. This commit modifies
`CreateTransaction` logic to make a second attempt to
create a tx without OutputType::SILENT_PAYMENTS change_type

66caabcc64

wallet: prefer silent_payments over Bech32m as fallback change type

This allows wallets with only sp descs to use sp change destination for transactions.

3ebc86dadd

wallet/tests: Enable Silent Payments in wallet unit tests 7a297ff853

wallet/gui: add silent_payments option to createwalletdialog b7ceb99a99

wallet/gui: add silent-payments to address type dropdown 09277cbc63

Eunovo force-pushed on Aug 14, 2025

DrahtBot removed the label Needs rebase on Aug 14, 2025

DrahtBot added the label Needs rebase on Aug 14, 2025

DrahtBot commented at 7:53 pm on August 14, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

in src/script/descriptor.cpp:889 in bd7e9026af outdated

885@@ -886,7 +886,7 @@ class DescriptorImpl : public Descriptor
886     }
887 
888     // NOLINTNEXTLINE(misc-no-recursion)
889-    bool IsRange() const final
890+    bool IsRange() const override

josibake commented at 12:26 pm on August 15, 2025:

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

in src/script/descriptor.cpp:1697 in bd7e9026af outdated

1692+
1693+    std::unique_ptr<DescriptorImpl> Clone() const override
1694+    {
1695+        return std::make_unique<SpDescriptor>(m_pubkey_args.at(0)->Clone(), m_pubkey_args.at(1)->Clone());
1696+    }
1697+};

josibake commented at 12:47 pm on August 15, 2025:

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

 0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1index 0bd2251e13..fb3252f398 100644
 2--- a/src/script/descriptor.cpp
 3+++ b/src/script/descriptor.cpp
 4@@ -1674,26 +1674,28 @@ public:
 5     {
 6         FlatSigningProvider dummy;
 7         std::string scan_key;
 8-        bool has_scan_key{m_pubkey_args.at(0)->ToPrivateString(dummy, scan_key)};
 9-        assert(has_scan_key); // The ScankeyPubkeyProvider always has the scan key
10-        std::string ret{m_name + "(" + scan_key + ","};
11-        auto& spend_pubkey{m_pubkey_args.at(1)};
12-        std::string tmp;
13+        // This should never fail, since the ScankeyPubkeyProvider will always contain
14+        // the scan key.
15+        //
16+        // The scan key is stored at m_pubkey_args[0], the spend key at m_pubkey_args[1]
17+        if (!m_pubkey_args.at(0)->ToPrivateString(dummy, scan_key)) return false;
18+        std::string scan_key_str{m_name + "(" + scan_key + ","};
19+        std::string spend_key_str;
20         switch (type) {
21         case StringType::NORMALIZED:
22-            if (!spend_pubkey->ToNormalizedString(*arg, tmp, cache)) return false;
23+            if (!m_pubkey_args.at(1)->ToNormalizedString(*arg, spend_key_str, cache)) return false;
24             break;
25         case StringType::PRIVATE:
26-            if (!spend_pubkey->ToPrivateString(*arg, tmp)) return false;
27+            if (!m_pubkey_args.at(1)->ToPrivateString(*arg, spend_key_str)) return false;
28             break;
29         case StringType::PUBLIC:
30-            tmp = spend_pubkey->ToString();
31+            spend_key_str = m_pubkey_args.at(1)->ToString();
32             break;
33         case StringType::COMPAT:
34-            tmp = spend_pubkey->ToString(PubkeyProvider::StringType::COMPAT);
35+            spend_key_str = m_pubkey_args.at(1)->ToString(PubkeyProvider::StringType::COMPAT);
36             break;
37         }
38-        out = std::move(ret) + std::move(tmp) + ")";
39+        out = std::move(scan_key_str) + std::move(spend_key_str) + ")";
40         return true;
41     }

in src/script/descriptor.cpp:1889 in bd7e9026af outdated

1885+                    return ret;
1886+                }
1887+
1888                 out.keys.emplace(pubkey.GetID(), key);
1889-                ret.emplace_back(std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, ctx == ParseScriptContext::P2TR));
1890+                ret.emplace_back(std::move(provider));

josibake commented at 12:56 pm on August 15, 2025:

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

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

in src/script/descriptor.cpp:1939 in bd7e9026af outdated

1934+                ret.emplace_back(std::make_unique<ScankeyPubkeyProvider>(key_exp_index, tmp_extkey.key, std::move(origin_pubkey)));
1935+                continue;
1936+            }
1937+
1938+            if (tmp_extkey.key.IsValid()) out.keys.emplace(tmp_extpubkey.pubkey.GetID(), tmp_extkey.key);
1939+            ret.emplace_back(std::move(origin_pubkey));

josibake commented at 12:57 pm on August 15, 2025:

Same comment regarding push_back vs emplace_back

in src/script/descriptor.cpp:2293 in bd7e9026af outdated

2288@@ -2192,6 +2289,36 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
2289         }
2290         return ret;
2291     }
2292+    if (ctx == ParseScriptContext::TOP && Func("sp", expr)) {
2293+        auto scan_pubkeys{ParsePubkey(key_exp_index, Expr(expr), ParseScriptContext::SP, out, error)};

josibake commented at 12:59 pm on August 15, 2025:

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.

in src/script/descriptor.cpp:1029 in 9252ffc4e2 outdated

1025@@ -1026,16 +1026,23 @@ class DescriptorImpl : public Descriptor
1026     std::optional<int64_t> MaxSatisfactionElems() const override { return {}; }
1027 
1028     // NOLINTNEXTLINE(misc-no-recursion)
1029-    void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const override
1030+    void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs, std::vector<CKey>& scankeys) const override

josibake commented at 1:05 pm on August 15, 2025:

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

josibake commented at 1:07 pm on August 15, 2025:

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.

in src/interfaces/chain.h:66 in 7fa9eace9c outdated

61@@ -62,6 +62,12 @@ class FoundBlock
62     //! Read block data from disk. If the block exists but doesn't have data
63     //! (for example due to pruning), the CBlock variable will be set to null.
64     FoundBlock& data(CBlock& data) { m_data = &data; return *this; }
65+    //! Read block's undo data from disk.
66+    FoundBlock& undoData(CBlockUndo& undo)

josibake commented at 1:12 pm on August 15, 2025:

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.

josibake commented at 1:17 pm on August 15, 2025: member

Looks much better now that you added ScankeyPubkeyProvider. We could even just call this a SilentPaymentsPubKeyProvider, which is perhaps a bit less confusing for reviewers who are less familiar with the details of how silent payments work. Reviewed up until the SilentPaymentsSPKMan commit and onward.

Silent Payments: Receiving #32966

Code Coverage & Benchmarks

Reviews

Conflicts