Backwards-compatibility issue in descriptor ID calculation for existing Miniscript wallets #35432

Is there an existing issue for this?

• I have searched the existing issues

Current behaviour

After updating bitcoin core, on startup fails to load an existing descriptor wallet containing a Miniscript descriptor. Startup aborts with the wallet DB corruption error: The descriptor ID calculated by the wallet differs from the one in DB

The wallet previously loaded successfully on older bitcoin core versions.

Expected behaviour

Existing descriptor wallets should continue to load after upgrading, as long as the stored descriptor is valid and describes the same scripts. A change in descriptor string serialization/canonicalization should not make an existing wallet appear corrupted.

This patch fixes the issue. But I am unsure about other cases, eg if wallet is a Taproot descriptor with a non-Miniscript internal key plus Miniscript taptree, the old mixed-format ID may be more subtle

diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 5ad29970d2..38247b9f7d 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -8,9 +8,11 @@
 #include <wallet/walletdb.h>
 
 #include <common/system.h>
+#include <crypto/sha256.h>
 #include <key_io.h>
 #include <primitives/transaction_identifier.h>
 #include <protocol.h>
+#include <script/descriptor.h>
 #include <script/script.h>
 #include <serialize.h>
 #include <sync.h>
@@ -69,6 +71,17 @@ void LogDBInfo()
     LogInfo("Using SQLite Version %s", SQLiteDatabaseVersion());
 }
 
+static uint256 LegacyMiniscriptDescriptorID(const Descriptor& desc)
+{
+    // Before [#31734](/bitcoin-bitcoin/31734/), MiniscriptDescriptor::ToStringHelper() did not respect
+    // StringType::COMPAT, so the wallet descriptor id for Miniscript
+    // descriptors was calculated from the non-compat descriptor string.
+    const std::string desc_str{desc.ToString(/*compat_format=*/false)};
+    uint256 id;
+    CSHA256().Write(reinterpret_cast<const unsigned char*>(desc_str.data()), desc_str.size()).Finalize(id.begin());
+    return id;
+}
+
 //
 // WalletBatch
 //
@@ -772,10 +785,11 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
             return DBErrors::UNKNOWN_DESCRIPTOR;
         }
 
-        if (id != desc.id) {
+        if (id != desc.id && id != LegacyMiniscriptDescriptorID(*desc.descriptor)) {
             strErr = "The descriptor ID calculated by the wallet differs from the one in DB";
             return DBErrors::CORRUPT;
         }
+        desc.id = id;
 
         DescriptorCache cache;

I also don't know if there are side-effects elsewhere due to change in descriptor id

Steps to reproduce

1. On an older Bitcoin Core version prior to 53b72372da91c5e90df865bc15961e16feb4a983 #31734, create/import a descriptor wallet containing a Miniscript multisig descriptor, especially one with key origin information containing hardened derivation markers.
2. Shut down Bitcoin Core.
3. Upgrade to current master.
4. Start Bitcoin Core / Bitcoin Qt and load the same wallet.
5. Wallet loading fails with a descriptor ID mismatch.

Relevant log output

init message: Loading wallet… The descriptor ID calculated by the wallet differs from the one in DB Error loading */wallet.dat: Wallet corrupted

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

master@fbe628756cc417dd4b6ccd9d3a709ca8e2f6023c

Operating system and version

N/A

Machine specifications

N/A

This is the second time we've had an issue with descriptor IDs, so I think we should re-evaluate what descriptor ID even means.

The descriptor ID is used for a few things:

1. Identifying related records in the database (e.g. key and cache records)
2. Records identifying which SPKMs are active
3. Identifying whether a descriptor being imported was already imported before.

For uses 1 and 2, we could get away with treating IDs as some random blob and drop the check that computed ID matches read ID. I think in an ideal world, this is what we would have done from the beginning. As long as the IDs are attached and passed around correctly, it doesn't really matter where the value came from, but the ID needs to be associated correctly across versions.

The problem is with use case 3. Here, we're getting a descriptor from the user that has no ID attached to it. We can't just attach a new random one to it because this descriptor might already exist in the wallet. Since IDs are currently assumed to be calculated consistently from a string representation of the descriptor, we currently re-calculate that string to compute the ID, then lookup whether that ID exists in our SPKM set. As long as we are able to calculate the same ID for the same descriptor (with some wiggle room because different descriptor strings representing the same set if scripts can have the same ID), it doesn't really matter that the calculation is consistent across versions.

I think ultimately what we need to do is separate the ID into 2 different IDs, which ends up doubling the number of maps that we have for tracking SPKMs, and probably makes finding things a little bit confusing. We would have an opaque ID blob for the disk things of cases 1 and 2, and a separate in memory ID that is largely used for case 3.

But that only addresses how we handle IDs going forward. We still need to enable going backwards, i.e. importing a Miniscript descriptor after the fixes, and then loading that wallet in older versions. There's basically 2 different routes: A. Compatibility with only 31.0 - compute disk IDs by using StringType::COMPAT B. Compatibility with pre 31.0 - revert disk ID computation to not use StringType::COMPAT

I think we should go with B and just make a 31.1 that doesn't have the compatibility problem. This would leave 31.0 as an outlier that cannot be downgraded. But with the disk and memory ID separation proposed above, future versions would still be able to read 31.0, even if they only create miniscript descriptor records that can be read by every version other than 31.0.

For uses 1 and 2, we could get away with treating IDs as some random blob and drop the check that computed ID matches read ID

This makes sense. A check that computed id matches stored/read id doesn't seem necessary. Although failing to load the wallet due to this check (as I report here) may prevent the worse case where duplicate SPKMs lead to accidental address reuse

Identifying whether a descriptor being imported was already imported before.

Maybe this is simple to solve though? ie check that there exists a SPKM whose descriptor has the same (recomputed) id as the one being imported.

Rough draft testing my understanding of the above: https://github.com/bitcoin/bitcoin/commit/6ae6233e80fd4e3d86e76606c134e37cca9c3a51