wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101 · Pull Request #28333 · bitcoin/bitcoin

achow101 · 2023-08-24T02:55:39Z

Instead of constructing ScriptPubKeyMans with no data, and then loading data as we find it, we should gather everything first and then load it all on construction. If there actually is no data and we want to setup generation, then that should also occur in a constructor rather than afterwards.

This change is only applied to DescriptorScriptPubKeyMan and ExternalSignerScriptPubKeyMan, and should be done for any ScriptPubKeyMans added in the future. I don't think it's really worth it to do this for LegacyScriptPubKeyMan since it would make loading performance worse (or cause layer violations) and it's (supposed to be) going away soon.

DrahtBot · 2023-08-24T02:55:41Z

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/28333.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	polespinasa, w0xlt, davidgumberg
Concept ACK	murchandamus
Stale ACK	rkrux, adyshimony

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35302 (Silent Payments: Sending (take 2) by Eunovo)
#34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
#34193 (wallet: make migration more robust against failures by furszy)
#33112 (wallet: relax external_signer flag constraints, add musig2 test (partial) by Sjors)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

Parse(desc_str, provider, error, false) in src/wallet/scriptpubkeyman.cpp

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/wallet_descriptor.py] assert len(key_rows) >= 1 -> use assert_greater_than_or_equal(len(key_rows), 1)

^{2026-04-29 21:19:49}

S3RK · 2023-08-28T07:00:04Z

Is this change of behaviour or not?

It is not.

EncryptWallet does not end up calling the SetupGeneration functions when the blank flag is set. Previously, we would set those up manually after encrypting. While making this PR, I realized that we could just unset the blank flag before calling EncryptWallet so that it can do the setup for us and we don't have to do it again afterwards.

maflcko · 2023-11-27T13:35:43Z

wallet/test/fuzz/scriptpubkeyman.cpp: In lambda function:
wallet/test/fuzz/scriptpubkeyman.cpp:114:67: error: ‘void wallet::DescriptorScriptPubKeyMan::AddDescriptorKey(const CKey&, const CPubKey&)’ is private within this context
  114 |                 spk_manager->AddDescriptorKey(key, key.GetPubKey());
      |

rkrux

Paused review at f4894a8.

Nice to see more activity on this PR, moving it closer to merge.

However, the PR seems to have changed a bit from last year when I reviewed it and I'm unable to place the second commit in the context of this PR refactor: wallet: Don't reuse WALLET_BLANK flag for born-encrypted wallets. now - it seems fine to do but going through it seems like a deviation while reviewing the PR that's supposed to construct SPKMs with all data rather than loaded progressively.

I see the second commit was added recently (#28333 (comment)) but I really feel it should be its own PR. I ended up adding suggestions in this commit but I don't see a reason why they should be addressed in this PR that's supposed to do something else.

rkrux · 2026-04-27T12:24:09Z

    // Try to top up keypool. No-op if the wallet is locked.
    walletInstance->TopUpKeyPool();


In 03993ad "refactor: wallet: Don't reuse WALLET_BLANK flag for born-encrypted wallets."

This needs to be inside the is_encrypted conditional as well, I suppose. Seems like a no-op for encrypted wallet creation flow.

It does not need to be, it's a no-op when the wallet has no descriptors. It's a no-op for blank wallets too.

polespinasa

reviewed 83c810a

left some comments

polespinasa · 2026-04-27T16:40:45Z

+    }
+
    // Encrypt the wallet
-    if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {


I don't think this is a good idea. Someone could call CreateNew with is_encrypted=true and WALLET_FLAG_DISABLE_PRIVATE_KEYS set.

I think a safer approach is to add an assertion to ensure it cannot happen.
assert(!passphrase.empty() || !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS))

polespinasa · 2026-04-27T17:12:08Z

+
+    // Top up key pool, to generate scriptPubKeys
+    if (!TopUpWithDB(batch)) {
+        throw std::runtime_error("Could not top up scriptPubKeys");


in fcffb36 wallet: include keys when constructing DescriptorSPKM during import

I am not really familiar with error handling, but the error type changed here. I think this way it cannot be translated? Which is a problem as this is propagated till importdescriptor RPC.

not sure of this tbh feel free to ignore if I am mistaken

It doesn't matter for RPCs as the output to RPC is never translated. This could be an issue for the GUI once import descriptors in the GUI is implemented. However, I'm pretty sure that this condition can't be hit in practice as it would mean that the descriptor cannot be expanded even when the private keys are available. This is checked for within importdescriptors before we get to this point.

It doesn't matter for RPCs as the output to RPC is never translated

didn't know it, make sense then :)

Even if it can't be hit, I think it is never catch?

polespinasa

Thanks for addressing comments :)

Just three more things I just saw and I will ack :P

DrahtBot · 2026-04-29T01:49:28Z

🚧 At least one of the CI tasks failed.
_{Task macOS native, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/25084663233/job/73497385306}
_{LLM reason (✨ experimental): CI failed because the spkm_migration fuzz target crashed with UpdateWithSigningProvider: writing descriptor private key failed (exit code 1).}

Hints

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.

If a wallet has multiple HD chains that have the same seed, we should only migrate that seed a single time. This fixes a fuzz crash that occurs once the return value of AddDescriptorKeyWithDB is checked during descriptor construction.

…llets. With the split between LoadWallet and CreateNew, it's no longer necessary to utilize the blank flag to prevent the wallet from having descriptors automatically being generated. Instead, CreateNew can take a separate parameter to indicate whether the wallet is to be born encrypted and therefore should not have any keys generated.

Instead of creating a DescSPKM that is then progressively loaded, we should instead create it all at once in a factory function when loading.

…rypted keys fails.

When importing a descriptor, all of the descriptor data should be provided at the same time in the constructor.

Instead of constructing then setting the descriptor with SetupDescriptor, just pass in that descriptor to the constructor.

Instead of having a caller use SetupDescriptorGeneration, just have a constructor that takes those arguments and sets up the descriptor with the autogenerated key.

achow101 · 2026-04-29T21:19:47Z

Added a commit to fix the fuzz crash.

polespinasa · 2026-04-29T22:15:22Z

ACK 451fdd2

thanks for addressing all comments, the code lgtm :)

w0xlt

ACK 451fdd2

davidgumberg · 2026-05-26T22:24:07Z

+    return spkm;
+}
+
+std::unique_ptr<DescriptorScriptPubKeyMan> DescriptorScriptPubKeyMan::CreateFromMigration(WalletStorage& storage, WalletBatch& batch, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider)


In aa4f782 (wallet: include keys when constructing DescriptorSPKM during import)

disregard-worthy-nit: the names CreateFromImport() and CreateFromMigration() seem to be more about the callers than telling you anything about the difference between these two functions.

Couldn't think of better names 🤷‍♀️

davidgumberg · 2026-05-26T23:56:35Z

re crACK 451fdd2

I reviewed the range-diff, the main changes since my last review:

fuzz: Migrate hd seed added.
Factory pattern suggested by @pseudoramdom
Thread annotation suggested by @polespinasa

fanquake · 2026-06-01T15:35:07Z

Possible that this change has resulted in the fuzz crash in #35434.

kevkevinpal · 2026-06-02T03:38:50Z

Possible that this change has resulted in the fuzz crash in #35434.

It looks like specifically on aa4f782 it crashes

to reproduce the crash

# Add input
echo 'IQIAJ1sAo6MYAcBQGP+VdP////////83v9sAAAAAAAAAAAAAAP8VAAAAAAAAAUUAAAAAWwD+////////CwAAAAAAAAB///8AAAAAAAAAAAAAAP8VAAAAAAAAAUUAAAAAAAD/k5OTk5OTk5OTk5OTk5OTk5OTk5OTk5OTk5OTk5MhAgAnWwCjoxgDwFAY/5V0/////////z+/2wAAAAABAAAAAAAA/xUAAAAAAAABRQAAAAAAAP////////8LAAAAAAAA/////wAA/P8AAAAAAAAA/xUAAAAAAAABRQAAAAAAAP+Tk5OTk5OTk5OTk5OTk5OTk5OTk5OTk5OTk5OTkyECACdbAKOjGAPqUBhRa3Q+Pj4+Pj4+Pj4+Pj4+Pj4+Pj48Pj4+Pj4+wsHBuT4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+/////////zm/2wAAAAAAAAAAhqurq6urAAD/FQAAXAAAANoAAAAAAAAAAAAAAPgA0wAAAAAAAAAA/v/DX+Wel3+Tk5OTk5OTk5OTk5OTk5OTk0aB6YU2k5OTk5OTk5OTk5MhAgAAAAAAAP+Tk5OTk5OTk5OTk5OTk5OTk5OTopOTk5OTk5OTkyECACdbAKOjGAPAUBj/lXT/////////P7/bAAAAAAAAAAAAAAAAAABkAAAAAAFFAAAAAAAA/////////wsAAAAAAAD/////AAD8/wAAAAAAAAD/FQAAAAAAAAFFAAAAAAAA/5OTk5OTk5OTk5OTk5OTk5OTk5OTk5OTk5OTk5OTIQIAJ1sAo6MYA+pQGFFrdEA+Pj4+Pj4+Pj4+Pj4+Pj4+Pjw+Pj4+Pj7CwcG5Pj4+Pj4+Pv83v9sAAAAAAAAAAAAAAP8VAAAAAAAAAUUAAAAAWwD+/////v//CwAAAAAAAAB///8AAAAAAAAAAAAAAP8VAAAAAAAAAUUAAAAAAAD/k5OTk5OTk5OTk5OTk5N6k5OTk5OTk5OTk5OTk5MhAgAnWwCjoxgDwFAY/5V0/////////z+/2wAAAAAAAAAAAAAA/xUAAAAAAAABRQAAAAAAAP////////8LAAAAAAAA/////wAA/P8AAAAAAAAA/xUAAAAAAAABRQAAAAAAAP+Tk5OTk5OTk5OTk5OTk5OTk5OTk5OTk5OTk5OTkyECACdbAKOjGAPqUBhRa3Q+Pj4+Pj4+Pj4+Pj4+Pj4+Pj48Pj4+Pj4+wsHBuT4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+/////////zm/2wAAAAAAAAAAhqurq6urAAD/Fd7/WwAAANoAAAAAAAAAAAAAAPgA0wAAAAAAAAAA/v/YX+Wel3+Tk5MAAP+Y/6Ghnpd/k5OTk5OTkwAAAKE='   | base64 -d > spkmmigrate_input

# Checkout failing commit
git checkout aa4f7823aa18eea7e310a355981bbf70c0496b98
cmake --preset=libfuzzer && cmake --build build_fuzz --parallel

# Run failing input
FUZZ=spkm_migration build_fuzz/bin/fuzz -runs=1 spkmmigrate_input

# Test with working/prior commit
git checkout aa4f7823aa18eea7e310a355981bbf70c0496b98^
cmake --preset=libfuzzer && cmake --build build_fuzz --parallel

# Run failing input (observe it passes)
FUZZ=spkm_migration build_fuzz/bin/fuzz -runs=1 spkmmigrate_input

DrahtBot added the Wallet label Aug 24, 2023

S3RK reviewed Aug 28, 2023

View reviewed changes

ghost approved these changes Aug 30, 2023

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Sep 3, 2023

DrahtBot mentioned this pull request Sep 11, 2023

wallet: Receive silent payment transactions #28453

Closed

DrahtBot mentioned this pull request Oct 3, 2023

wallet: optimize migration process, batch db transactions #28574

Merged

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

This was referenced Nov 16, 2023

wallet: batch all individual spkms setup db writes in a single db txn #28894

Merged

refactor: VectorWriter and SpanReader without nVersion #28912

Merged

DrahtBot added the CI failed label Nov 25, 2023

achow101 force-pushed the raii-spkm branch from d91026a to 12d12fa Compare November 27, 2023 22:15

DrahtBot removed the CI failed label Nov 27, 2023

DrahtBot mentioned this pull request Nov 28, 2023

refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream #28451

Merged

DrahtBot added the Needs rebase label Nov 28, 2023

achow101 force-pushed the raii-spkm branch from 12d12fa to 632961c Compare November 28, 2023 19:31

DrahtBot removed the Needs rebase label Nov 28, 2023

This was referenced Apr 24, 2026

wallet: Follow-ups to create/load split (#32636) #34490

Closed

wallet: make migration more robust against failures #34193

Open

rkrux reviewed Apr 27, 2026

View reviewed changes

polespinasa suggested changes Apr 27, 2026

View reviewed changes

pseudoramdom reviewed Apr 27, 2026

View reviewed changes

Comment thread src/wallet/scriptpubkeyman.h Outdated

polespinasa reviewed Apr 28, 2026

View reviewed changes

Comment thread src/wallet/scriptpubkeyman.h Outdated

Comment thread src/wallet/scriptpubkeyman.h Outdated

Comment thread src/wallet/test/scriptpubkeyman_tests.cpp

rkrux reviewed Apr 28, 2026

View reviewed changes

Comment thread src/wallet/wallet.h Outdated

Comment thread src/wallet/scriptpubkeyman.h Outdated

Comment thread src/wallet/scriptpubkeyman.h Outdated

w0xlt reviewed Apr 28, 2026

View reviewed changes

Comment thread src/wallet/scriptpubkeyman.cpp Outdated

achow101 and others added 10 commits April 29, 2026 14:15

wallet migration, fuzz: Migrate hd seed once

0301c75

If a wallet has multiple HD chains that have the same seed, we should only migrate that seed a single time. This fixes a fuzz crash that occurs once the return value of AddDescriptorKeyWithDB is checked during descriptor construction.

wallet: Consolidate generation setup callers into one function

cd912c4

wallet: Load everything into DescSPKM on construction

80b0c25

Instead of creating a DescSPKM that is then progressively loaded, we should instead create it all at once in a factory function when loading.

test: wallet: Check that loading wallet with both unencrypted and enc…

8be5ee5

…rypted keys fails.

fuzz: Skip adding descriptor to wallet if it cannot be expanded

6538f69

wallet: include keys when constructing DescriptorSPKM during import

aa4f782

When importing a descriptor, all of the descriptor data should be provided at the same time in the constructor.

wallet: Construct ExternalSignerSPKM with the new descriptor

e20aaff

Instead of constructing then setting the descriptor with SetupDescriptor, just pass in that descriptor to the constructor.

wallet: Setup new autogenerated descriptors on construction

32946e0

Instead of having a caller use SetupDescriptorGeneration, just have a constructor that takes those arguments and sets up the descriptor with the autogenerated key.

test: wallet: Constructing a DSPKM that can't TopUp() throws.

451fdd2

rkrux mentioned this pull request May 1, 2026

wallet: unfriend LegacyDataSPKM and DescriptorScriptPubKeyMan #35192

Merged

DrahtBot mentioned this pull request May 16, 2026

Silent Payments: Sending (take 2) #35302

Draft

1 task

w0xlt reviewed May 26, 2026

View reviewed changes

davidgumberg reviewed May 26, 2026

View reviewed changes

kevkevinpal mentioned this pull request Jun 2, 2026

wallet: Fuzz crash in spkm_migration #35434

Closed

rkrux mentioned this pull request Jun 3, 2026

wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated #35445

Open

		// Try to top up keypool. No-op if the wallet is locked.
		walletInstance->TopUpKeyPool();

Uh oh!

Conversation

achow101 commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 27, 2023

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polespinasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polespinasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Apr 29, 2026

Uh oh!

achow101 commented Apr 29, 2026

Uh oh!

polespinasa commented Apr 29, 2026

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidgumberg commented May 26, 2026

Uh oh!

fanquake commented Jun 1, 2026

achow101 commented Aug 24, 2023 •

edited

Loading

DrahtBot commented Aug 24, 2023 •

edited

Loading

achow101 Aug 29, 2023 •

edited

Loading