Blank legacy wallets do not have active SPKM. They can only be detected by checking the descriptors’ flag or the db format.
This enables the migration of blank legacy wallets in the GUI.
To test this:
CWallet::IsLegacy() returns false for blank legacy wallet.
-> In this PR: the button will be enabled, allowing the migration of legacy wallets.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28591532781
Make sure to run all tests locally, according to the documentation.
The failure may 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.
ACK 30f701317b4a674fea422766ac3ee179c9c6f554
Light code review. On the PR branch, created a blank legacy wallet, opened in bitcoin-qt. Saw that the migrate option was available and performed the migration.
1624@@ -1625,7 +1625,11 @@ isminetype CWallet::IsMine(const CScript& script) const
1625 }
1626
1627 // Legacy wallet
1628- if (IsLegacy()) return GetLegacyScriptPubKeyMan()->IsMine(script);
1629+ if (IsLegacy()) { // Blank legacy wallets have a null spkm
In 30f701317b4a674fea422766ac3ee179c9c6f554 “wallet: fix, detect blank legacy wallets in IsLegacy”
IsLegacy() is unnecessary here now since GetLegacyScriptPubKeyMan() also checks for WALLET_FLAG_DESCRIPTORS.
3592@@ -3588,7 +3593,10 @@ std::unique_ptr<SigningProvider> CWallet::GetSolvingProvider(const CScript& scri
3593 }
3594
3595 // Legacy wallet
3596- if (IsLegacy() && GetLegacyScriptPubKeyMan()->CanProvide(script, sigdata)) return GetLegacyScriptPubKeyMan()->GetSolvingProvider(script);
3597+ if (IsLegacy()) {
In 30f701317b4a674fea422766ac3ee179c9c6f554 “wallet: fix, detect blank legacy wallets in IsLegacy”
IsLegacy() is unnecessary, same as above.
Blank legacy wallets do not have active SPKM. They can
only be detected by checking the descriptors' flag or
the db format.
This enables the migration of blank legacy wallets in
the GUI.
Approach ACK
Nice simplifications.
Retested (https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2231909317) with same result (migration allowed).
Saw some may be used uninitialized warnings when building, e.g.
0 inlined from ‘bool wallet::CWallet::IsSpentKey(const CScript&) const’ at wallet/wallet.cpp:1046:37:
1./prevector.h:175:37: warning: ‘*(const prevector<28, unsigned char, unsigned int, int>*)((char*)&<unnamed> + offsetof(const std::CTxDestination, std::variant<CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, PayToAnchor, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Variant_base<CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, PayToAnchor, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Move_assign_base<false, CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, PayToAnchor, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Copy_assign_base<false, CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, PayToAnchor, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Move_ctor_base<false, CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, PayToAnchor, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Copy_ctor_base<false, CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, PayToAnchor, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Variant_storage<false, CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, PayToAnchor, WitnessUnknown>::_M_u)).prevector<28, unsigned char>::_size’ may be used uninitialized [-Wmaybe-uninitialized]
2 175 | bool is_direct() const { return _size <= N; }
3 | ^~~~~
4wallet/wallet.cpp: In member function ‘bool wallet::CWallet::IsSpentKey(const CScript&) const’:
5wallet/wallet.cpp:1046:37: note: ‘<anonymous>’ declared here
6 1046 | if (IsAddressPreviouslySpent(wpkh_dest)) {
Saw some
may be used uninitializedwarnings when building, e.g.
What compiler? I don’t see those warnings.
What compiler? I don’t see those warnings.
Probably the known false positives, see https://cirrus-ci.com/task/5429325778911232?logs=ci#L2365 and https://github.com/bitcoin/bitcoin/issues/29359
Saw some
may be used uninitializedwarnings when building, e.g.What compiler? I don’t see those warnings.
gcc/g++ (Debian 12.2.0-14) 12.2.0
Looks like it’s on my end. Did a fresh clone and build and didn’t see the same warnings.
