From d26c3cc44438ecb9e4f618a2427c3c92a292aa16 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 6 Sep 2022 09:52:59 -0300 Subject: [PATCH 1/2] wallet: bugfix, load wallet with an unknown descriptor cause fatal error If the descriptor entry is unrecognized/corrupt, the unserialization fails and `LoadWallet` instead of stop there and return the error, continues reading all the db records. As other records tied to the unrecognized/corrupted descriptor are scanned, a fatal error is thrown. --- src/wallet/wallet.cpp | 8 ++++++-- src/wallet/walletdb.cpp | 27 +++++++++++++++++++++------ src/wallet/walletdb.h | 3 ++- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0949045ff01..784ea24b987 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2819,8 +2819,12 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect." " Rescanning wallet."), walletFile)); rescan_required = true; - } - else { + } else if (nLoadWalletRet == DBErrors::UNKNOWN_DESCRIPTOR) { + error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n" + "The wallet might had been created on a newer version.\n" + "Please try running the latest software version.\n"), walletFile); + return nullptr; + } else { error = strprintf(_("Error loading %s"), walletFile); return nullptr; } diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 30406a22f92..6a8f0d24817 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -314,6 +314,7 @@ public: std::map, std::pair>> m_descriptor_crypt_keys; std::map m_hd_chains; bool tx_corrupt{false}; + bool descriptor_unknown{false}; CWalletScanState() = default; }; @@ -627,7 +628,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, uint256 id; ssKey >> id; WalletDescriptor desc; - ssValue >> desc; + try { + ssValue >> desc; + } catch (const std::ios_base::failure& e) { + strErr = e.what(); + wss.descriptor_unknown = true; + return false; + } if (wss.m_descriptor_caches.count(id) == 0) { wss.m_descriptor_caches[id] = DescriptorCache(); } @@ -767,6 +774,12 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) DBErrors result = DBErrors::LOAD_OK; LOCK(pwallet->cs_wallet); + + // Last client version to open this wallet + int last_client = CLIENT_VERSION; + bool has_last_client = m_batch->Read(DBKeys::VERSION, last_client); + pwallet->WalletLogPrintf("Wallet file version = %d, last client version = %d\n", pwallet->GetVersion(), last_client); + try { int nMinVersion = 0; if (m_batch->Read(DBKeys::MINVERSION, nMinVersion)) { @@ -832,6 +845,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx) wss.tx_corrupt = false; result = DBErrors::CORRUPT; + } else if (wss.descriptor_unknown) { + strErr = strprintf("Error: Unrecognized descriptor found in wallet %s. ", pwallet->GetName()); + strErr += (last_client > CLIENT_VERSION) ? "The wallet might had been created on a newer version. " : + "The database might be corrupted or the software version is not compatible with one of your wallet descriptors. "; + strErr += "Please try running the latest software version"; + pwallet->WalletLogPrintf("%s\n", strErr); + return DBErrors::UNKNOWN_DESCRIPTOR; } else { // Leave other errors alone, if we try to fix them we might make things worse. fNoncriticalErrors = true; // ... but do warn the user there is something wrong. @@ -884,11 +904,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) if (result != DBErrors::LOAD_OK) return result; - // Last client version to open this wallet - int last_client = CLIENT_VERSION; - bool has_last_client = m_batch->Read(DBKeys::VERSION, last_client); - pwallet->WalletLogPrintf("Wallet file version = %d, last client version = %d\n", pwallet->GetVersion(), last_client); - pwallet->WalletLogPrintf("Keys: %u plaintext, %u encrypted, %u w/ metadata, %u total. Unknown wallet records: %u\n", wss.nKeys, wss.nCKeys, wss.nKeyMeta, wss.nKeys + wss.nCKeys, wss.m_unknown_records); diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 6aa25fae038..da6efe534b5 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -51,7 +51,8 @@ enum class DBErrors EXTERNAL_SIGNER_SUPPORT_REQUIRED, LOAD_FAIL, NEED_REWRITE, - NEED_RESCAN + NEED_RESCAN, + UNKNOWN_DESCRIPTOR }; namespace DBKeys { From e06676377d935c69f0ee51fc18eb0772d524aba5 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 7 Sep 2022 16:02:16 -0300 Subject: [PATCH 2/2] wallet: coverage for loading an unknown descriptor Previously, this was crashing the wallet. --- src/Makefile.test.include | 3 +- src/wallet/test/walletload_tests.cpp | 54 ++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 src/wallet/test/walletload_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 8a2386a2b4b..987f30251c2 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -174,7 +174,8 @@ BITCOIN_TESTS += \ wallet/test/availablecoins_tests.cpp \ wallet/test/init_tests.cpp \ wallet/test/ismine_tests.cpp \ - wallet/test/scriptpubkeyman_tests.cpp + wallet/test/scriptpubkeyman_tests.cpp \ + wallet/test/walletload_tests.cpp FUZZ_SUITE_LD_COMMON +=\ $(SQLITE_LIBS) \ diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp new file mode 100644 index 00000000000..f45b69a4180 --- /dev/null +++ b/src/wallet/test/walletload_tests.cpp @@ -0,0 +1,54 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include +#include + +#include + +namespace wallet { + +BOOST_AUTO_TEST_SUITE(walletload_tests) + +class DummyDescriptor final : public Descriptor { +private: + std::string desc; +public: + explicit DummyDescriptor(const std::string& descriptor) : desc(descriptor) {}; + ~DummyDescriptor() = default; + + std::string ToString() const override { return desc; } + std::optional GetOutputType() const override { return OutputType::UNKNOWN; } + + bool IsRange() const override { return false; } + bool IsSolvable() const override { return false; } + bool IsSingleType() const override { return true; } + bool ToPrivateString(const SigningProvider& provider, std::string& out) const override { return false; } + bool ToNormalizedString(const SigningProvider& provider, std::string& out, const DescriptorCache* cache = nullptr) const override { return false; } + bool Expand(int pos, const SigningProvider& provider, std::vector& output_scripts, FlatSigningProvider& out, DescriptorCache* write_cache = nullptr) const override { return false; }; + bool ExpandFromCache(int pos, const DescriptorCache& read_cache, std::vector& output_scripts, FlatSigningProvider& out) const override { return false; } + void ExpandPrivate(int pos, const SigningProvider& provider, FlatSigningProvider& out) const override {} +}; + +BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup) +{ + std::unique_ptr database = CreateMockWalletDatabase(); + { + // Write unknown active descriptor + WalletBatch batch(*database, false); + std::string unknown_desc = "trx(tpubD6NzVbkrYhZ4Y4S7m6Y5s9GD8FqEMBy56AGphZXuagajudVZEnYyBahZMgHNCTJc2at82YX6s8JiL1Lohu5A3v1Ur76qguNH4QVQ7qYrBQx/86'/1'/0'/0/*)#8pn8tzdt"; + WalletDescriptor wallet_descriptor(std::make_shared(unknown_desc), 0, 0, 0, 0); + BOOST_CHECK(batch.WriteDescriptor(uint256(), wallet_descriptor)); + BOOST_CHECK(batch.WriteActiveScriptPubKeyMan(static_cast(OutputType::UNKNOWN), uint256(), false)); + } + + { + // Now try to load the wallet and verify the error. + const std::shared_ptr wallet(new CWallet(m_node.chain.get(), "", m_args, std::move(database))); + BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::UNKNOWN_DESCRIPTOR); + } +} + +BOOST_AUTO_TEST_SUITE_END() +} // namespace wallet