The final step of #20160.
A bare minimum of legacy wallet code is kept in order to perform wallet migration. Migration of legacy wallets uses the independent BDB parser and a minimal LegacyDataSPKM that allows the legacy data to be loaded so that the migration can be completed.
BDB has been removed as a dependency and documentation have been updated to reflect that.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28710.
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.
Reviewers, this pull request conflicts with the following ones:
dependencies.md by NicolaLS)addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)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.
Possible typos and grammar issues:
41+enum class RecordType : uint8_t
42+{
43+ KEYDATA = 1,
44+ DUPLICATE = 2,
45+ OVERFLOW_DATA = 3,
46+ DELETE = 0x80, // Indicate this record is deleted. This is AND'd with the real type.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
BerkeleyDatabaseSanityCheck related suppression in contrib/devtools/check-devs.sh
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29441840067
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.
3487@@ -3684,9 +3488,8 @@ void CWallet::SetupLegacyScriptPubKeyMan()
3488 return;
3489 }
3490
3491- std::unique_ptr<ScriptPubKeyMan> spk_manager = m_database->Format() == "bdb_ro" ?
3492- std::make_unique<LegacyDataSPKM>(*this) :
3493- std::make_unique<LegacyScriptPubKeyMan>(*this, m_keypool_size);
3494+ Assert(m_database->Format() == "bdb_ro");
3495+ std::unique_ptr<ScriptPubKeyMan> spk_manager = std::make_unique<LegacyDataSPKM>(*this);
In order to continue testing this without manually writing the bdb file, we need to allow the ‘mock’ db format here:
0const std::string& db_format{m_database->Format()};
1Assert(db_format == "bdb_ro" || db_format == "mock");
80@@ -79,7 +81,7 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const LegacyDataSPKM& keystor
81 //! scripts or simply treat any script that has been
82 //! stored in the keystore as spendable
83 // NOLINTNEXTLINE(misc-no-recursion)
84-IsMineResult IsMineInner(const LegacyDataSPKM& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true)
85+IsMineResult LegacyWalletIsMineInnerDONOTUSE(const LegacyDataSPKM& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true)
e6ccd17275e06e6278473587257f80c2d9bd8834: maybe move the legacy wallet migration code to a separate file? And mark it read-only :-)
e.g. https://github.com/Sjors/bitcoin/commit/364088a306f8de9ece2c5201dbe48a252c39ea03
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36513189138
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38265463467
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40297294016
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/41034814129
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.
79@@ -80,7 +80,6 @@
80 "ENABLE_HARDENING": "ON",
81 "ENABLE_WALLET": "ON",
82 "WARN_INCOMPATIBLE_BDB": "OFF",
nit: shouldn’t this one be removed too?
113@@ -123,6 +114,15 @@ Path | Description | Repository notes
114 `addr.dat` | Peer IP address BDB database; replaced by `peers.dat` in [0.7.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.7.0.md) | [PR #1198](https://github.com/bitcoin/bitcoin/pull/1198), [`928d3a01`](https://github.com/bitcoin/bitcoin/commit/928d3a011cc66c7f907c4d053f674ea77dc611cc)
115 `onion_private_key` | Cached Tor onion service private key for `-listenonion` option. Was used for Tor v2 services; replaced by `onion_v3_private_key` in [0.21.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.21.0.md) | [PR #19954](https://github.com/bitcoin/bitcoin/pull/19954)
116
117+### Berkeley DB database based wallets
118+
nit: something like this just for reference, if you agree ofc…
0
1These wallets, Legacy Wallets, are not longer supported (since v3x), can't be created nor loaded but migrated into SQLite (Descriptor Wallets).
13-$(package)_cppflags_netbsd=-D_XOPEN_SOURCE=600
14-$(package)_cppflags_mingw32=-DUNICODE -D_UNICODE
15-endef
16-
17-define $(package)_preprocess_cmds
18- patch -p1 < $($(package)_patch_dir)/clang_cxx_11.patch && \
clang_cxx_11.patch also needs deleting.
libdb++-dev instance in workflows/ci.yml & deadlock:libdb / BerkeleyBatch etc in tsan suppressions. Berkeley* in walletdb.h. BerkeleyEnvironment::Salvage in utils_tests.cpp.
There’s also a
libdb++-devinstance inworkflows/ci.yml&deadlock:libdb/BerkeleyBatchetc in tsan suppressions.Berkeley*in walletdb.h.BerkeleyEnvironment::Salvageinutils_tests.cpp.
Removed
All tests which tested legacy wallet behavior have been removed. The
--descriptorsand--legacy-walletoptions are removed from the functional tests.
nit in OP, I don’t think this is true. This was already done in https://github.com/bitcoin/bitcoin/pull/31250
181@@ -182,7 +182,7 @@ BOOST_AUTO_TEST_CASE(parse_hex)
182 result = TryParseHex<uint8_t>("12 34 56 78").value();
183 BOOST_CHECK_EQUAL_COLLECTIONS(result.begin(), result.end(), expected.begin(), expected.end());
184
185- // Leading space must be supported (used in BerkeleyEnvironment::Salvage)
186+ // Leading space must be supported
labelWatch* still needed?
NotifyWatchonlyChanged/fHaveWatchOnly: It doesn’t look to be called anywhere, so can be removed.
176@@ -177,11 +177,7 @@ class WalletImpl : public Wallet
177 }
178 bool haveWatchOnly() override
179 {
180- auto spk_man = m_wallet->GetLegacyScriptPubKeyMan();
181- if (spk_man) {
182- return spk_man->HaveWatchOnly();
183- }
184- return false;
185+ return m_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS);
Also, the comment Legacy wallets are being deprecated, warn if the loaded wallet is legacy in the code should be removed?
0$ git grep 'Legacy wallets are being deprecated, warn if the loaded wallet is legacy' bc3f07e384c2e145d6d14cfa3ad65b976233b538
1bc3f07e384c2e145d6d14cfa3ad65b976233b538:src/wallet/wallet.cpp: // Legacy wallets are being deprecated, warn if the loaded wallet is legacy
false like before? And if so, can’t the entire helper be dropped?
478@@ -479,15 +479,6 @@ RPCHelpMan getbalances()
479 }
106@@ -107,14 +107,6 @@ static RPCHelpMan getwalletinfo()
107 }
108 obj.pushKV("keypoolsize", (int64_t)kpExternalSize);
109
110- LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan();
111- if (spk_man) {
112- CKeyID seed_id = spk_man->GetHDChain().seed_id;
113- if (!seed_id.IsNull()) {
114- obj.pushKV("hdseedid", seed_id.GetHex());
Also, some more stuff:
0$ git grep -i 'legacy wallets' src/script/ src/wallet doc/*.md
1doc/build-netbsd.md:`db4` is required to enable support for legacy wallets.
2doc/managing-wallets.md:## Migrating Legacy Wallets to Descriptor Wallets
3doc/managing-wallets.md:Legacy wallets (traditional non-descriptor wallets) can be migrated to become Descriptor wallets
4doc/managing-wallets.md:Legacy wallets can know about, be watching for, and be able to sign for, `migratewallet` only
5doc/psbt.md:If you are using legacy wallets feel free to continue with the example provided here.
6src/script/descriptor.h: * TODO: Remove this method once legacy wallets are removed as it is only necessary for importmulti.
7src/wallet/rpc/addresses.cpp: // In legacy wallets hdkeypath has always used an apostrophe for
8src/wallet/rpc/wallet.cpp: {RPCResult::Type::NUM_TIME, "keypoololdest", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " of the oldest pre-generated key in the key pool. Legacy wallets only."},
9src/wallet/scriptpubkeyman.cpp: // Legacy wallets can also contain scripts whose P2SH, P2WSH, or P2SH-P2WSH it is not watching for
10src/wallet/scriptpubkeyman.h:/** struct containing information needed for migrating legacy wallets to descriptor wallets */
11src/wallet/wallet.cpp: // Legacy wallets are being deprecated, warn if the loaded wallet is legacy
12src/wallet/wallet.cpp: warnings.emplace_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet."));
13src/wallet/wallet.cpp: error = Untranslated("Legacy wallets can no longer be created");
14src/wallet/wallet.cpp: // Legacy wallets are being deprecated, warn if a newly created wallet is legacy
15src/wallet/wallet.cpp: warnings.emplace_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."));
16src/wallet/wallet.cpp: // Legacy wallets need SetupGeneration here.
17src/wallet/wallet.cpp: // Activating ScriptPubKeyManager for a given output and change type is incompatible with legacy wallets.
18src/wallet/wallet.cpp: // Legacy wallets have only one ScriptPubKeyManager and it's active for all output and change types.
Removed from the help text.
I’ve cleaned up some of the legacy wallet mentions that are no longer relevant. Some are still relevant, and others are less obviously legacy wallet only things so I will leave those for a followup.
12@@ -13,9 +13,7 @@ target_sources(test_bitcoin
13 feebumper_tests.cpp
14 group_outputs_tests.cpp
15 init_tests.cpp
16- ismine_tests.cpp
CMakeLists.txt.
Concept ACK.
bc3f07e384c2e145d6d14cfa3ad65b976233b538 looks good, except:
The GUI now shows a watch-only balance for descriptor wallets. See inline for the culprit.
In that same commit, the churn in wallet/scriptpubkeyman.cpp is hard to follow, both here on Github as well as a git show --color-moved=dimmed-zebra. I’m guessing you’re moving stuff around, deleting and modifying? Update: use --histogram, it’s just deletes.
And a few details:
-DWITH_BDB=ON in test-each-commit-exec.shtest_framework/bdb.py seems unused nowfuzz/wallet_dbd_parser checks for the presence of USE_BDB which is dropped.DEFAULT_FLUSHWALLET is unusedwallet_createwallet.py has a comment referring to sethdseedimportprivkey and friends are still in bitcoin-cli.bashAlso it might be good to rename the importprivkey test helper to e.g. importkey, so it’s not confused with the disappeared RPC method.
As I mentioned in an earlier review https://github.com/bitcoin/bitcoin/pull/28710/commits/bc3f07e384c2e145d6d14cfa3ad65b976233b538#r1913267051, rather than naming functions DONOTUSE you could also move the migration code into a new file. But that could be a nice followup, along with changes to remove any legacy wallet weirdness from scriptpubkeyman.h
2666@@ -2776,68 +2667,6 @@ void CWallet::ListLockedCoins(std::vector<COutPoint>& vOutpts) const
2667 }
2668 }
2669
2670-/** @} */ // end of Actions
735- int64_t nIndex;
736- key >> nIndex;
737- CKeyPool keypool;
738- value >> keypool;
739- pwallet->GetOrCreateLegacyDataSPKM()->LoadKeyPool(nIndex, keypool);
740- return DBErrors::LOAD_OK;
Descriptor wallets can’t use non-HD keys in the “keypool”, so it doesn’t make sense to keep this for such wallets.
All migrated wallets get new descriptors anyways for new addresses. The keypool is entirely dumped.
586@@ -587,7 +587,7 @@ class WalletLoaderImpl : public WalletLoader
587 m_context.scheduler = &scheduler;
588 return StartWallets(m_context);
589 }
590- void flush() override { return FlushWallets(m_context); }
591+ void flush() override {}
flush() is inherited from ChainClient, which defines it as pure virtual, so it needs to have an implementation here even if it does nothing.
234@@ -235,8 +235,7 @@ static std::set<std::string> g_unloading_wallet_set GUARDED_BY(g_wallet_release_
235 static void FlushAndDeleteWallet(CWallet* wallet)
236 {
237 const std::string name = wallet->GetName();
238- wallet->WalletLogPrintf("Releasing wallet %s..\n", name);
239- wallet->Flush();
240+ wallet->WalletLogPrintf("Releasing wallet\n");
lgtm ACK bc3f07e384c2e145d6d14cfa3ad65b976233b538 💇
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK bc3f07e384c2e145d6d14cfa3ad65b976233b538 💇
3eF2eywtBy1XVEkBKNQpSjNzCFy6rQijB2u0oSJ5ekqHuSEbmLrtrlwP1Y1QQ9QHaog9QUeesH7zr4jfYiCYqAg==
2. In that same commit, the churn in
wallet/scriptpubkeyman.cppis hard to follow, both here on Github as well as agit show --color-moved=dimmed-zebra. I’m guessing you’re moving stuff around, deleting and modifying?
You can use --patience. Maybe add this to the commit message?
And a few details:
Removed these
Also it might be good to rename the
importprivkeytest helper to e.g.importkey, so it’s not confused with the disappeared RPC method.
I’m going to leave that for a followup.
You can use
--patience. Maybe add this to the commit message?
Done, also mentioned --histogram.
updateWatchOnlyLabels stuff is still wrong. I think the entire second column for watch-only needs to be stripped from the GUI. But at least not rendered.
The
updateWatchOnlyLabelsstuff is still wrong. I think the entire second column for watch-only needs to be stripped from the GUI. But at least not rendered.
Fixed it so it doesn’t render. The full removal can be done in a followup.
Some small berkeleydb (AFAIK, non-migration) related cruft after this:
0src/wallet/load.cpp: // The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
1test/sanitizer_suppressions/tsan:race:BerkeleyBatch
2vcpkg.json: "berkeleydb",
3vcpkg.json: "berkeleydb": {
4vcpkg.json: "berkeleydb"
In 425c39c6f520a1b0d18c6e782141f711509b2947: Could probably also remove the proxy functions we have in the test framework for those RPC commands. Look at this line for example (same for importpubkey, importprivkey and addmultisigaddress).
Also, on an orthogonal topic — I’m wondering if, instead of completely removing importaddress(), we might want to quickly make it compatible with descriptor wallets (e.g., by reviving #27034). This is also connected to #30175.
I’m bringing this topic up because removing something in one release and reintroducing it in another one could be annoying for users.
Could probably also remove the proxy functions we have in the test framework for those RPC commands. Look at this line for example (same for importpubkey, importprivkey and addmultisigaddress).
See #31250 (review)
25@@ -27,7 +26,7 @@ def skip_test_if_missing_module(self):
26
27 def run_test(self):
28 node = self.nodes[0]
29- self.generate(node, 1) # Leave IBD for sethdseed
30+ self.generate(node, 1)
deprecatedrpc=create_bdb?
7@@ -8,14 +8,14 @@
8
nit in a29101b56541d4bf72fcf69460e8eb2a8cc33979: I guess you wanted to leave some of those for follow-ups, but the doc/descriptors.md one seems in-scope?
0$ git grep -i 'legacy wallet' 1c0d89358e12fc871e99c8304d5cb50965cf7b9d doc/descriptors.md doc/managing-wallets.md src/bitcoin-wallet.cpp src/wallet src/script/ test/
11c0d89358e12fc871e99c8304d5cb50965cf7b9d:doc/descriptors.md:- `importmulti` takes as input descriptors to import into a legacy wallet
21c0d89358e12fc871e99c8304d5cb50965cf7b9d:doc/managing-wallets.md:In the RPC, the destination parameter must include the name of the file. Otherwise, the command will return an error message like "Error: Wallet backup failed!" for descriptor wallets. If it is a legacy wallet, it will be copied and a file will be created with the default file name `wallet.dat`.
31c0d89358e12fc871e99c8304d5cb50965cf7b9d:doc/managing-wallets.md:## Migrating Legacy Wallets to Descriptor Wallets
41c0d89358e12fc871e99c8304d5cb50965cf7b9d:doc/managing-wallets.md:Legacy wallets (traditional non-descriptor wallets) can be migrated to become Descriptor wallets
51c0d89358e12fc871e99c8304d5cb50965cf7b9d:doc/managing-wallets.md:Legacy wallets can know about, be watching for, and be able to sign for, `migratewallet` only
61c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/bitcoin-wallet.cpp: argsman.AddArg("-legacy", "Create legacy wallet. Only for 'create'", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
71c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/script/descriptor.h: * TODO: Remove this method once legacy wallets are removed as it is only necessary for importmulti.
81c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/rpc/addresses.cpp: // In legacy wallets hdkeypath has always used an apostrophe for
91c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/rpc/wallet.cpp: {RPCResult::Type::NUM_TIME, "keypoololdest", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " of the oldest pre-generated key in the key pool. Legacy wallets only."},
101c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/rpc/wallet.cpp: throw JSONRPCError(RPC_WALLET_ERROR, "descriptors argument must be set to \"true\"; it is no longer possible to create a legacy wallet.");
111c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/scriptpubkeyman.cpp:// Legacy wallet IsMine(). Used only in migration
121c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/scriptpubkeyman.cpp: // Legacy wallets can also contain scripts whose P2SH, P2WSH, or P2SH-P2WSH it is not watching for
131c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/scriptpubkeyman.h:// This is the minimum necessary to load a legacy wallet so that it can be migrated.
141c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/scriptpubkeyman.h:/** struct containing information needed for migrating legacy wallets to descriptor wallets */
151c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: // Legacy wallets are being deprecated, warn if the loaded wallet is legacy
161c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: warnings.emplace_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet."));
171c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: error = Untranslated("Legacy wallets can no longer be created");
181c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: // Legacy wallets are being deprecated, warn if a newly created wallet is legacy
191c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: warnings.emplace_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."));
201c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), walletFile);
211c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: // Legacy wallets need SetupGeneration here.
221c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: // Activating ScriptPubKeyManager for a given output and change type is incompatible with legacy wallets.
231c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: // Legacy wallets have only one ScriptPubKeyManager and it's active for all output and change types.
241c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"));
251c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: error = _("Error: Unable to produce descriptors for this legacy wallet. Make sure to provide the wallet's passphrase if it is encrypted.");
261c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))};
271c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: // When the legacy wallet has no spendable scripts, the main wallet will be empty, leaving its script cache empty as well.
281c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: return util::Error{_("Error: cannot remove legacy wallet records")};
291c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.cpp: // Get all of the descriptors from the legacy wallet
301c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.h: //! Get all of the descriptors from a legacy wallet
311c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/wallet.h://! Do all steps to migrate a legacy wallet to a descriptor wallet
321c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/walletdb.cpp: pwallet->WalletLogPrintf("Legacy Wallet Keys: %u plaintext, %u encrypted, %u w/ metadata, %u total.\n",
331c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/walletdb.cpp: // Load legacy wallet keys
341c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/walletdb.cpp: error = Untranslated(strprintf("Failed to open database path '%s'. The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC).", fs::PathToString(path)));
351c0d89358e12fc871e99c8304d5cb50965cf7b9d:src/wallet/walletdb.h:// Keys in this set pertain only to the legacy wallet (LegacyScriptPubKeyMan) and are removed during migration from legacy to descriptors.
361c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/tool_wallet.py: self.log.info("Test that legacy wallets cannot be created")
371c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_backwards_compatibility.py: legacy_nodes = self.nodes[2:] # Nodes that support legacy wallets
381c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_backwards_compatibility.py: # since we can no longer create legacy wallets.
391c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_backwards_compatibility.py: # Legacy wallets are no longer supported. Trying to load these should result in an error
401c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_backwards_compatibility.py: assert_raises_rpc_error(-18, "The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC)", node_master.restorewallet, wallet_name, backup_path)
411c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_createwallet.py: self.log.info("Test that legacy wallets cannot be created")
421c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_createwallet.py: assert_raises_rpc_error(-4, 'descriptors argument must be set to "true"; it is no longer possible to create a legacy wallet.', self.nodes[0].createwallet, wallet_name="legacy", descriptors=False)
431c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_migration.py: # For a P2WSH output script stored in the legacy wallet's mapScripts, both the native P2WSH
441c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_migration.py: # The invalid addresses are invalid, so the legacy wallet should not detect them as ismine,
451c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_migration.py: # nor consider them watchonly. However, because the legacy wallet has the witnessScripts/redeemScripts,
461c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_migration.py: # It turns out that due to how signing logic works, legacy wallets that have valid miniscript witnessScripts
471c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_migration.py: self.log.info("Test migration of a legacy wallet containing miniscript")
481c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_migration.py: # Check that the miniscript can be spent by the legacy wallet
491c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_migration.py: # It turns out that due to how signing logic works, legacy wallets that have the private key for a Taproot
501c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_migration.py: # Check that the rawtr can be spent by the legacy wallet
511c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_migration.py: # Check that the tr() cannot be spent by the legacy wallet
521c0d89358e12fc871e99c8304d5cb50965cf7b9d:test/functional/wallet_reindex.py: # For legacy wallet: There is no way of importing a script/address with a custom time. The wallet always imports it with birthtime=1.
Removed:
WriteHDCHain and WriteCScript`I would prefer to leave further cleanups for follow up RPs as I am sure there are plenty of things that were missed and trying to catch them all in this one PR is going to be very annoying and just delay this even more.
re-review ACK 1c0d89358e 🎽
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-review ACK 1c0d89358e 🎽
3PZKgfTbviYeE2sXUccefHEGzNEiLnxq231kR9x+Kbs/03JOhEOQNRuGw8xDKO5Oz1bBAAIPB1ljq05mIAK6UBQ==
Only stuff is some follow-up ideas:
279@@ -640,66 +280,6 @@ BOOST_AUTO_TEST_CASE(ismine_standard)
280 result = spk_manager->IsMine(scriptPubKey);
281 BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE);
282 }
283-
284- // OP_RETURN
24@@ -26,7 +25,6 @@ deadlock:src/qt/test/*
25
26 # External libraries
27 # https://github.com/bitcoin/bitcoin/pull/27658#issuecomment-1547639621
This test was mistakenly disabled.
33@@ -34,8 +34,7 @@ static void WalletCreate(CWallet* wallet_instance, uint64_t wallet_creation_flag
34 wallet_instance->InitWalletFlags(wallet_creation_flags);
35
36 if (!wallet_instance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
37- auto spk_man = wallet_instance->GetOrCreateLegacyScriptPubKeyMan();
38- spk_man->SetupGeneration(false);
39+ assert(false);
0assert(wallet_instance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
ACK https://github.com/bitcoin/bitcoin/pull/28710/commits/be3c13a22fa8d49fe15cde941e8030e1cd2b522f with 2 non-blocking nits commented earlier.
Regarding the removed RPC functions, I think less is better in this case, and unless I’m missing something, the cases mentioned in #30175 can be performed by importdescriptors.
Deletes LegacyScriptPubKeyMan and related tests
Best reviewed with `git diff --patience` or `git diff --histogram`
SOme db functions were for BDB, these are no longer needed.
These RPCs no longer exist.
952 {},
953 RPCResult{RPCResult::Type::BOOL, "", "Whether the abort was successful"},
954 RPCExamples{
955 "\nImport a private key\n"
956- + HelpExampleCli("importprivkey", "\"mykey\"") +
957+ + HelpExampleCli("rescanblockchain", "") +
s/an/a above, the comment “Import a private key” doesn’t go well with rescanblockchain.
806- }
807- std::tie(range_start, range_end) = ParseDescriptorRange(data["range"]);
808- }
809-
810- // Only single key descriptors are allowed to be imported to a legacy wallet's keypool
811- bool can_keypool = parsed_descs.at(0)->IsSingleKey();
Concept ACK de054df6dc32cbd8b127c6761d9c65d95025e08f
Couple of points in 8ede6dea0c55bb4afefa790b83dd4da48a2f84da, can be done in follow up if no more changes being done in this PR.
re-ACK de054df6dc32cbd8b127c6761d9c65d95025e08 🔗
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK de054df6dc32cbd8b127c6761d9c65d95025e08 🔗
3BCpwbQ/aUjnaK6cKbU2NA6V6EpINW1XAGCEG7C6Kp8TVtlZz/xsxtLcBwdx6KWznzxX0v4qc0hC17WfOYE41CA==
