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:

#33036 (Update secp256k1 subtree to latest master by fanquake)
#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)
#32745 (scripted-diff: Update DeriveType enum values to mention ranged derivations by rkrux)
#32724 (Musig2 tests by w0xlt)
#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)
#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)
#31244 (descriptors: MuSig2 by achow101)
#30243 (descriptors: taproot partial descriptors by Eunovo)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
#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:1440 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:779 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.

in src/script/descriptor.cpp:1479 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:1399 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:1950 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:216 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:2125 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:92 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:111 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.

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.

Squashed 'src/secp256k1/' changes from 4187a46649..9e85256bbe

9e85256bbe docs: update README
4b1fb2c186 ci: enable silentpayments module
de508a78ac tests: add constant time tests
45427dd4d7 tests: add BIP-352 test vectors
6975614517 silentpayments: add benchmarks for scanning
a9af9ebf35 silentpayments: add examples/silentpayments.c
b06254b6c7 silentpayments: receiving
3c9362dd6a silentpayments: recipient label support
70e20b7145 silentpayments: sending
cf44324b5e build: add skeleton for new silentpayments (BIP352) module
ad60ef7ea7 Merge bitcoin-core/secp256k1#1689: ci: Convert `arm64` Cirrus tasks to GHA jobs
c498779096 Merge bitcoin-core/secp256k1#1687: cmake: support the use of launchers in ctest -S scripts
0dfe387dbe cmake: support the use of launchers in ctest -S scripts
89096c234d Merge bitcoin-core/secp256k1#1692: cmake: configure libsecp256k1.pc during install
7106dce6fd cmake: configure libsecp256k1.pc during install
29e73f4ba5 Merge bitcoin-core/secp256k1#1685: cmake: Emulate Libtool's behavior on FreeBSD
746e36b141 Merge bitcoin-core/secp256k1#1678: cmake: add a helper for linking into static libs
a28c2ffa5c Merge bitcoin-core/secp256k1#1683: README: add link to musig example
2a9d374735 Merge bitcoin-core/secp256k1#1690: ci: Bump GCC snapshot major version to 16
add146e101 ci: Bump GCC snapshot major version to 16
004f57fcd8 ci: Move Valgrind build for `arm64` from Cirrus to GHA
5fafdfc30f ci: Move `gcc-snapshot` build for `arm64` from Cirrus to GHA
e814b79a8b ci: Switch `arm64_debian` from QEMU to native `arm64` Docker image
bcf77346b9 ci: Add `arm64` architecture to `docker_cache` job
b77aae9226 ci: Rename Docker image tag to reflect architecture
145ae3e28d cmake: add a helper for linking into static libs
819210974b README: add link to musig example, generalize module enabling hint
95db29b144 Merge bitcoin-core/secp256k1#1679: cmake: Use `PUBLIC_HEADER` target property in installation logic
37dd422b5c cmake: Emulate Libtool's behavior on FreeBSD
f24b838bed Merge bitcoin-core/secp256k1#1680: doc: Promote "Building with CMake" to standard procedure
3f31ac43e0 doc: Promote "Building with CMake" to standard procedure
6f67151ee2 cmake: Use `PUBLIC_HEADER` target property
c32715b2a0 cmake, move-only: Move module option processing to `src/CMakeLists.txt`
201b2b8f06 Merge bitcoin-core/secp256k1#1675: cmake: Bump minimum required CMake version to 3.22
3af71987a8 cmake: Bump minimum required CMake version to 3.22
92394476e9 Merge bitcoin-core/secp256k1#1673: Assert field magnitude at control-flow join
3a4f448cb4 Assert field magnitude at control-flow join
9fab425256 Merge bitcoin-core/secp256k1#1668: bench_ecmult: add benchmark for ecmult_const_xonly
05445377f4 bench_ecmult: add benchmark for ecmult_const_xonly
bb597b3d39 Merge bitcoin-core/secp256k1#1670: tests: update wycheproof files
d73ed99479 tests: update wycheproof files

git-subtree-dir: src/secp256k1
git-subtree-split: 9e85256bbe527bf084222ee08dade9ea497d5c99

46f5c2f1f3

Merge commit '46f5c2f1f382922c19a070b78803fbd29cedd62b' into refresh-secp256k1 14224fd2f3

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.

702eb96c46

Add "sp" HRP 91d588d839

Add V0SilentPaymentDestination address type d6070d3437

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

a0384589e4

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.

1765c1ff3d

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

d820cf6c2d

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.

23182ba053

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.

7a253a80e7

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.

e0404435ff

wallet: add `IsInputForSharedSecretDerivation` function 51ad5fccb4

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.

910b219900

wallet: update TransactionChangeType

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

3e7812364f

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>

d6fb7426c7

tests: add sending functional tests 4225ebc655

descriptor: add PubkeyProvider::ToPrivateString(CKey)

This method will be used by sp descriptor to convert the scan_key to private string

e9b180eb51

Add OutputType::SILENT_PAYMENTS 41f7bf58ff

descriptor: implement sp(scan, spend) desc d006aa8956

Eunovo force-pushed on Jul 20, 2025

pubkey: add TweakAdd() method

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

681ee95154

Eunovo force-pushed on Jul 20, 2025

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>

b07f6d9ae6

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>

29e7384516

wallet: generate sp() descriptor 7d2b004732

wallet: create SilentPaymentDescriptorScriptPubKeyMan

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

cf157fe756

wallet: scan for silent payments

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

97c1713039

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

wallet/rpc: add create silent-payments wallet option

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

dfd09ee75b

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.

82db7ccaa6

wallet: import silent payments descriptor 432f886214

wallet/rpc: add sp outputtype to doc 50113324fb

tests: add silent payments receiving functional tests

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

4d745d7989

wallet/rpc: add sp-address support to getaddressinfo 1c1fc220b7

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.

66d1cc5ed0

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.

8ec1fd2ec8

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

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

1156a52e0d

wallet/tests: Enable Silent Payments in wallet unit tests 298731ffb6

wallet: use silent_payments as change fallback OutputType

This will allow wallets with only silent payment descriptors to use silent payment outputs as a change

913d824566

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

DrahtBot commented at 10:27 am on July 23, 2025: contributor

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

Silent Payments: Receiving #32966

Code Coverage & Benchmarks

Reviews

Conflicts