Merge bitcoin/bitcoin#27920: wallet: bugfix, always use apostrophe for spkm descriptor ID

5df988b534df842ddb658ad2a7ff0f12996c8478 test: add coverage for descriptor ID (furszy)
6a9510d2dabde1c9ee6c4226e3d16ca32eb48ac5 wallet: bugfix, always use apostrophe for spkm descriptor ID (furszy)
97a965d98f1582ea1b1377bd258c9088380e1b8b refactor: extract descriptor ID calculation from spkm GetID() (furszy)
1d207e3931cf076f69d4a8335cdd6c8ebb2a963f wallet: do not allow loading descriptor with an invalid ID (furszy)

Pull request description:

  Aiming to fix #27915.

  As we re-write the descriptor's db record every time that
  the wallet is loaded (at `TopUp` time), if the spkm ID differs
  from the one in db, the wallet will enter in an unrecoverable
  corruption state (due to the storage of a descriptor with an ID
  that is not linked to any other descriptor record in DB), and
  no soft version will be able to open it anymore.

  Because we cannot change the past, to stay compatible between
  releases, we need to always use the apostrophe version for the
  spkm IDs.

ACKs for top commit:
  achow101:
    ACK 5df988b534df842ddb658ad2a7ff0f12996c8478
  Sjors:
    tACK 5df988b534df842ddb658ad2a7ff0f12996c8478

Tree-SHA512: f63fc4aac7d21a4e515657471758d28857575e751865bfa359298f8b89b2568970029ca487a873c1786a5716325f453f06cd417ed193f3366417f6e8c2987332

3 files changed, 37 insertions, 6 deletions

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 796b7f11c5..5b110b4d14 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -2584,10 +2584,7 @@ std::unique_ptr<CKeyMetadata> DescriptorScriptPubKeyMan::GetMetadata(const CTxDe
 uint256 DescriptorScriptPubKeyMan::GetID() const
 {
     LOCK(cs_desc_man);
-    std::string desc_str = m_wallet_descriptor.descriptor->ToString();
-    uint256 id;
-    CSHA256().Write((unsigned char*)desc_str.data(), desc_str.size()).Finalize(id.begin());
-    return id;
+    return DescriptorID(*m_wallet_descriptor.descriptor);
 }
 
 void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache)
diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp
index 73a4b77188..1bd2bf012f 100644
--- a/src/wallet/test/walletload_tests.cpp
+++ b/src/wallet/test/walletload_tests.cpp
@@ -4,6 +4,7 @@
 
 #include <wallet/test/util.h>
 #include <wallet/wallet.h>
+#include <test/util/logging.h>
 #include <test/util/setup_common.h>
 
 #include <boost/test/unit_test.hpp>
@@ -19,7 +20,7 @@ public:
     explicit DummyDescriptor(const std::string& descriptor) : desc(descriptor) {};
     ~DummyDescriptor() = default;
 
-    std::string ToString() const override { return desc; }
+    std::string ToString(bool compat_format) const override { return desc; }
     std::optional<OutputType> GetOutputType() const override { return OutputType::UNKNOWN; }
 
     bool IsRange() const override { return false; }
@@ -32,7 +33,7 @@ public:
     void ExpandPrivate(int pos, const SigningProvider& provider, FlatSigningProvider& out) const override {}
 };
 
-BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup)
+BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup)
 {
     std::unique_ptr<WalletDatabase> database = CreateMockableWalletDatabase();
     {
@@ -49,6 +50,33 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup)
         const std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", std::move(database)));
         BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::UNKNOWN_DESCRIPTOR);
     }
+
+    // Test 2
+    // Now write a valid descriptor with an invalid ID.
+    // As the software produces another ID for the descriptor, the loading process must be aborted.
+    database = CreateMockableWalletDatabase();
+
+    // Verify the error
+    bool found = false;
+    DebugLogHelper logHelper("The descriptor ID calculated by the wallet differs from the one in DB", [&](const std::string* s) {
+        found = true;
+        return false;
+    });
+
+    {
+        // Write valid descriptor with invalid ID
+        WalletBatch batch(*database, false);
+        std::string desc = "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)#cjjspncu";
+        WalletDescriptor wallet_descriptor(std::make_shared<DummyDescriptor>(desc), 0, 0, 0, 0);
+        BOOST_CHECK(batch.WriteDescriptor(uint256::ONE, wallet_descriptor));
+    }
+
+    {
+        // Now try to load the wallet and verify the error.
+        const std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", std::move(database)));
+        BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::CORRUPT);
+        BOOST_CHECK(found); // The error must be logged
+    }
 }
 
 bool HasAnyRecordOfType(WalletDatabase& db, const std::string& key)
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index ff0b83dc38..ffe5fd4a18 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -803,6 +803,12 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
         }
         pwallet->LoadDescriptorScriptPubKeyMan(id, desc);
 
+        // Prior to doing anything with this spkm, verify ID compatibility
+        if (id != pwallet->GetDescriptorScriptPubKeyMan(desc)->GetID()) {
+            strErr = "The descriptor ID calculated by the wallet differs from the one in DB";
+            return DBErrors::CORRUPT;
+        }
+
         DescriptorCache cache;
 
         // Get key cache for this descriptor