These outputs are called anchors, and allow
key-less anchor spends which are vsize-minimized
versus keyed anchors which require larger outputs
when creating and inputs when spending.
8ce3739edb test: verify wallet is still active post-migration failure (furszy)
771bc60f13 wallet: Use LegacyDataSPKM when loading (Ava Chow)
61d872f1b3 wallet: Move MigrateToDescriptor and DeleteRecords to LegacyDataSPKM (Ava Chow)
b231f4d556 wallet: Move LegacyScriptPubKeyMan::IsMine to LegacyDataSPKM (Ava Chow)
7461d0c006 wallet: Move LegacySPKM data storage and handling to LegacyDataSPKM (Ava Chow)
517e204bac Change MigrateLegacyToDescriptor to reopen wallet as BERKELEY_RO (Ava Chow)
Pull request description:
#26606 introduced `BerkeleyRODatabase` which is an independent parser for BDB files. This PR uses this in legacy wallet migration so that migration will continue to work once the legacy wallet and BDB are removed. `LegacyDataSPKM` is introduced to have the minimum data and functions necessary for a legacy wallet to be loaded for migration.
ACKs for top commit:
cbergqvist:
ACK 8ce3739edb
theStack:
Code-review ACK 8ce3739edb
furszy:
Code review ACK 8ce3739edb
Tree-SHA512: dccea12d6c597de15e3e42f97ab483cfd069e103611200279a177e021e8e9c4e74387c4f45d2e58b3a1e7e2bdb32a1d2d2060b1f8086c03eeaa0c68579d9d54e
In order to load the necessary data for migrating a legacy wallet
without the full LegacyScriptPubKeyMan, move the data storage and
loading components to LegacyDataSPKM. LegacyScriptPubKeyMan now
subclasses that.
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.
Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
Add TransactionError to node namespace and include it directly instead of
relying on indirect include through common/messages.h
This is a followup to a previous commit which moved the TransactionError enum.
These changes were done in a separate followup just to keep the previous commit
more minimal and easy to review.
Add separate PSBTError enum instead of reusing TransactionError enum for PSBT
operations, and drop unused error codes. The error codes returned by PSBT
operations and transaction broadcast functions mostly do not overlap, so using
an unified enum makes it harder to call any of these functions and know which
errors actually need to be handled.
Define PSBTError in the common library because PSBT functionality is
implemented in the common library and used by both the node (for rawtransaction
RPCs) and the wallet.
e041ed9b75 wallet: Retrieve ID from loaded DescSPKM directly (Ava Chow)
39640dd34e wallet: Use scriptPubKeyCache in GetSolvingProvider (Ava Chow)
b410f68791 wallet: Use scriptPubKey cache in GetScriptPubKeyMans (Ava Chow)
edf4e73a16 wallet: Use scriptPubKey cache in IsMine (Ava Chow)
37232332bd wallet: Cache scriptPubKeys for all DescriptorSPKMs (Ava Chow)
99a0cddbc0 wallet: Introduce a callback called after TopUp completes (Ava Chow)
b276825932 bench: Add a benchmark for ismine (Ava Chow)
Pull request description:
Wallets that have a ton of non-ranged descriptors (such as a migrated non-HD wallet) perform fairly poorly due to looping through all of the wallet's `ScriptPubKeyMan`s. This is done in various places, such as `IsMine`, and helper functions for fetching a `ScriptPubKeyMan` and a `SolvingProvider`. This also has a bit of a performance impact on standard descriptor wallets, although less noticeable due to the small number of SPKMs.
As these functions are based on doing `IsMine` for each `ScriptPubKeyMan`, we can improve this performance by caching `IsMine` scriptPubKeys for all descriptors and use that to determine which `ScriptPubKeyMan` to actually use for those things. This cache is used exclusively and we no longer iterate the SPKMs.
Also added a benchmark for `IsMine`.
ACKs for top commit:
ryanofsky:
Code review ACK e041ed9b75. Just suggested changes since last review
josibake:
ACK e041ed9b75
furszy:
Code review ACK e041ed9b
Tree-SHA512: 8e7081991a025e682e9dea838b4543b0d179832d1c47397fb9fe7a97fa01eb699c15a5d5a785634926844fc83a46e6ac07ef753119f39d84423220ef8a548894
After TopUp completes, the wallet containing each SPKM will want to know
what new scriptPubKeys were generated. In order for all TopUp calls
(including ones internal the the SPKM), we use a callback function in
the WalletStorage interface.
e2ad343f69 wallet: remove unused `SignatureData` instances in spkm's `FillPSBT` methods (Sebastian Falbesoner)
Pull request description:
These are filled with signature data from a PSBT input, but not used anywhere after, hence they can be removed. Note that the same code is in the `SignPSBTInput` function where the `sigdata` result is indeed used.
ACKs for top commit:
achow101:
ACK e2ad343f69
brunoerg:
crACK e2ad343f69
Tree-SHA512: f0cabcc000bcea6bc7d7ec9d3be2e2a8accbdbffbe35252250ea2305b65a5813fde2b8096fbdd2c7cccdf417ea285183dc325fc2d210d88bce62978ce642930e
The wallet decryption process (CheckDecryptionKey() and Unlock())
contains an arg 'accept_no_keys,' introduced in #13926, that has
never been used.
Additionally, this also removes the unimplemented SplitWalletPath
function.
`CWallet::GetEncryptionKey()` would return a reference to the internal
`CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe.
Returning a copy would be a shorter solution, but could have security
implications of the master key remaining somewhere in the memory even
after `CWallet::Lock()` (the current calls to
`CWallet::GetEncryptionKey()` are safe, but that is not future proof).
So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)`
change the `GetEncryptionKey()` method to provide the encryption
key to a given callback:
`m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })`
This silences the following (clang 18):
```
wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
3520 | return vMasterKey;
| ^
```
Making the `GenerateRandomKey` helper available to other modules via
key.{h.cpp} allows us to create random private keys directly at
instantiation of CKey, in contrast to the two-step process of creating
the instance and then having to call `MakeNewKey(...)`.
1ce45baed7 rpc: getwalletinfo, return wallet 'birthtime' (furszy)
83c66444d0 test: coverage for wallet birth time interaction with -reindex (furszy)
6f497377aa wallet: fix legacy spkm default birth time (furszy)
75fbf444c1 wallet: birth time update during tx scanning (furszy)
b4306e3c8d refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime (furszy)
Pull request description:
Fixing #28897.
As the user may have imported a descriptor with a timestamp newer
than the actual birth time of the first key (by setting 'timestamp=now'),
the wallet needs to update the birth time when it detects a transaction
older than the oldest descriptor timestamp.
Testing Notes:
Can cherry-pick the test commit on top of master. It will fail there.
ACKs for top commit:
Sjors:
re-utACK 1ce45baed7
achow101:
ACK 1ce45baed7
Tree-SHA512: 10c2382f87356ae9ea3fcb637d7edc5ed0e51e13cc2729c314c9ffb57c684b9ac3c4b757b85810c0a674020b7287c43d3be8273bcf75e2aff0cc1c037f1159f9
To avoid scanning blocks, as assumed by a wallet with no
generated keys or imported scripts, the default value for
the birth time needs to be set to the maximum int64_t value.
Once the first key is generated or the first script is imported,
the legacy SPKM will update the birth time automatically.
Instead of doing one db transaction per descriptor setup,
batch all descriptors' setup writes in a single db txn.
Speeding up the process and preventing the wallet from entering
an inconsistent state if any of the intermediate transactions
fail.
Right now a wallet descriptor is converted to it's string representation
(via `Descriptor::ToString`) repeatedly at different instances:
- on finding a `DescriptorScriptPubKeyMan` for a given descriptor
(`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the
`importdescriptors` RPC); the string representation is created once
for each spkm in the wallet and at each iteration again for
the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`)
- whenever `DescriptorScriptPubKeyMan::GetID()` is called, e.g. in
`TopUp` or any instances where a descriptor is written to the DB
to determine the database key etc.
As there is no good reason to calculate a fixed descriptor's string/ID
more than once, add the ID as a field to `WalletDescriptor` and
calculate it immediately at initialization (or deserialization).
`HasWalletDescriptor` is changed to compare the spkm's and searched
descriptor's ID instead of the string to take use of that.
This speeds up the functional test `wallet_miniscript.py` by a factor of
5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently
introduced "max-size TapMiniscript" test-case introduced a descriptor
that takes 2-3 seconds to create a string representation, so the
repeated calls to that were significantly hurting the performance.
8e7e3e6149 test: wallet, verify migration doesn't crash for an invalid script (furszy)
1de8a2372a wallet: disallow migration of invalid or not-watched scripts (furszy)
Pull request description:
Fixing #28057.
The legacy wallet allows to import any raw script (#28126), without
checking if it was valid or not. Appending it to the watch-only set.
This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.
These stored scripts internally map to `ISMINE_NO` (same as if they
weren't stored at all..).
So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.
Which, in code words, means `IsMineInner()` returning
`IsMineResult::INVALID` for them.
Note:
To verify this, can run the test commit on top of master.
`wallet_migration.py` will crash without the bugfix commit.
ACKs for top commit:
achow101:
ACK 8e7e3e6149
Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7
The legacy wallet allowed to import any raw script, without checking if
it was valid or not. Appending it to the watch-only set.
This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.
These stored scripts internally map to `ISMINE_NO` (same as if they
weren't stored at all..).
So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.
Which, in code words, means IsMineInner() returning IsMineResult::INVALID
for them.
This allows us to verify the descriptor ID on the descriptors
unit tests in different software versions without requiring to
use the entire DescriptorScriptPubKeyMan machinery.
Note:
The unit test changes are introduced after the bugfix commit
but this commit + the unit test commit can be cherry-picked
on top of the v25 branch to verify IDs correctness. IDs must
be the same for v25 and after the bugfix commit.
cdba23db35 wallet: Document blank flag use in descriptor wallets (Ryan Ofsky)
43310200dc wallet: Ensure that the blank wallet flag is unset after imports (Andrew Chow)
e9379f1ffa rpc, wallet: Include information about blank flag (Andrew Chow)
Pull request description:
The `blank` wallet flag is used to indicate that the wallet intentionally does not have any keys, scripts, or descriptors, and it prevents the automatic generation of those things for such a wallet. Once the wallet contains any of those data, it is unnecessary, and possibly incorrect, to have `blank` set. This PR fixes a few places where this was not properly happening. It also adds a test for this unset behavior.
ACKs for top commit:
S3RK:
reACK cdba23db35
ryanofsky:
Code review ACK cdba23db35. Only change since last review is dropping the commit which makes createwallet RPC set BLANK flag automatically when DISABLE_PRIVATE_KEYS flag is set
Tree-SHA512: 85bc2a9754df0531575d5c8f4ad7e8f38dcd50083dc29b3283dacf56feae842e81f34654c5e1781f2dadb0560ff80e454bbc8ca3b2d1fab1b236499ae9abd7da
To avoid wasting processing power, we can skip blocks that occurred
before the wallet's creation time, since these blocks are guaranteed
not to contain any relevant wallet data.
This has direct implications (an speed improvement) on the underlying
blockchain synchronization process as well.
The reason is that the validation interface queue is limited to
10 tasks per time. This means that no more than 10 blocks can be
waiting for the wallet(s) to be processed while we are synchronizing
the chain (activating the best chain to be more precise).
Which can be a bottleneck if blocks arrive and are processed faster
from the network than what they are processed by the wallet(s).
fe49f06c0e doc: clarify PR 26076 release note (Sjors Provoost)
bd13dc2f46 Switch hardened derivation marker to h in descriptors (Sjors Provoost)
Pull request description:
This makes it easier to handle descriptor strings manually, especially when importing from another Bitcoin Core wallet.
For example the `importdescriptors` RPC call is easiest to use `h` as the marker: `'["desc": ".../0h/..."]'`, avoiding the need for escape characters. With this change `listdescriptors` will use `h`, so you can copy-paste the result, without having to add escape characters or switch `'` to 'h' manually.
Both markers can still be parsed.
The `hdkeypath` field in `getaddressinfo` is also impacted by this change, except for legacy wallets. The latter is to prevent accidentally breaking ancient software that uses our legacy wallet.
See discussion in #15740
ACKs for top commit:
achow101:
ACK fe49f06c0e
darosior:
re-ACK fe49f06c0e
Tree-SHA512: f78bc873b24a6f7a2bf38f5dd58f2b723e35e6b10e4d65c36ec300e2d362d475eeca6e5afa04b3037ab4bee0bf8ebc93ea5fc18102a2111d3d88fc873c08dc89
This makes it easier to handle descriptor strings manually. E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.
Both markers can still be parsed. The default for new descriptors is changed to h. In normalized form h is also used. For private keys the chosen marker is preserved in a round trip.
The hdkeypath field in getaddressinfo is also impacted by this change.
1869310f3c refactor: remove unused param from legacy pubkey (Bushstar)
Pull request description:
Unused param present in legacy pubkey manager interface. This param will not be used and should be removed to prevent unintended usage.
ACKs for top commit:
Sjors:
ACK 1869310f3c
furszy:
ACK 1869310f3c
Tree-SHA512: 0fb41fc8f481f859262f2e8e9a93c990c1b4637e74fd9191ccc0b3c523d0e7d94217a3074bb357276e1941a10d29326f850f9b27eccc1eca57cf6b549353400c
To directly return a CRIPEMD160 hash from data.
Incidentally, decoding this acronym:
* RIPEMD -> RIPE Message Digest
* RIPE -> RACE Integrity Primitives Evaluation
* RACE -> Research and Development in Advanced Communications Technologies in Europe
In addition to the pubkeys in hd_keypaths and tap_bip32_keypaths, also
see if the descriptor can produce a SigningProvider for the output
pubkey.
Also slightly refactors this area to reduce code duplication.
This extra method will be needed for updating the filter set for
faster wallet rescans; after an internal top-up has happened, we only
want to add the newly created scriptPubKeys.
