wallet, descriptor: Revert `StringType::COMPAT` for Miniscript expressions and drop the concept of a Descriptor ID that can be validated #35445

achow101 commented at 10:21 PM on June 2, 2026: member

Since keys in Miniscript expressions were not correctly handling StringType::COMPAT when generating the Descriptor ID, in order to keep compatibility with previous versions, we need to continue to handle that enum incorrectly when computing the ID.

Given that this it the second time that we have had this issue, this PR also drops the concept of Descriptor ID being something that we can validate. Instead, the ID read in from the database is treated as an opaque blob that is used only to tie together the records related to a particular SPKM. It is instead treated as a ScriptPubKeyMan ID and users of it must be retrieving the ID from somewhere rather than computing it from a descriptor. The check of comparing the read ID to the computed ID is removed so that all previously created wallets can be read.

To clarify that the ID is not actually an ID, the function DescriptorID is renamed to CompatDescriptorHash and it is still used to generate the SPKM ID that is written to the database.

The ID was additionally being used to determine whether a descriptor is equal to another descriptor. This was used only by importdescriptors and createwalletdescriptor. These uses have been changed to do a string comparison rather than computing a hash and comparing the hashes. This removes the need to rely on CompatDescriptorHash.

The only caveat is that previously the hash was being used to do a map lookup in m_spk_managers, but this is now changed to use std::find_if. The lookup complexity changes from logarithmic to linear, which may be really bad for wallets with a lot of descriptors, e.g. migrated formerly non-HD wallets. I think in general though, the tradeoff is okay, and neither of these functions purport to be performant, especially as importdescriptors may also do a rescan which can take a long time. However, if that is a concern, an additional map of CompatDescriptorHash to DescriptorSPKM can be added.

Lastly, the wallet backwards compatibility test is updated to have 30.2 and 31.0 nodes, and a wallet with miniscript expressions. This exercises both creating wallets in previous versions and making sure they load in master, and making new wallets on master and checking whether they load, depending on the version.

Fixes #35432

DrahtBot commented at 10:21 PM on June 2, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35445.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	pseudoramdom
Concept ACK	furszy
Stale ACK	mjdietzx

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36033 ([wip,nomerge,rfc] build: Require C++23 compiler by maflcko)
#35834 (Test checkunparsable errors by Herb-ops)
#35429 (wallet: avoid global access in external signer SPKM by w0xlt)
#33112 (wallet: relax external_signer flag constraints by Sjors)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

// string calculation that always use h -> // string calculation that always uses h [grammar error: subject-verb agreement]
// Get the path to the last hardened stup -> // Get the path to the last hardened step [“stup” appears to be a misspelling]

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0) in src/wallet/external_signer_scriptpubkeyman.cpp

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/wallet_backwards_compatibility.py] assert miniscript_apos != miniscript_desc -> use assert_not_equal(miniscript_apos, miniscript_desc)

2026-08-25 22:28:39

sipa commented at 10:30 PM on June 2, 2026: member

Can you store a vector with the (normalized descriptor string, SPKM pointer) pairs in sorted order? That will allow lookup by descriptor string in O(log n) time, probably with better constant factor than the existing map.

The downside is an O(n log n) sorting step any time the list of descriptors changes, but that should be a rare occurrence.

achow101 commented at 10:38 PM on June 2, 2026: member

Can you store a vector with the (normalized descriptor string, SPKM pointer) pairs in sorted order? That will allow lookup by descriptor string in O(log n) time, probably with better constant factor than the existing map.

Probably yes, but I don't really think we need to optimize for the duplication check.

The duplication check is already kinda not that great; for example it doesn't detect that a descriptor and its normalized form are the same. This could probably be more significantly improved by looking up the computed scripts which already live in a std::unordered_map, and that would also help with the lookup time if it's really a concern.

achow101 force-pushed on Jun 2, 2026

DrahtBot added the label CI failed on Jun 2, 2026

DrahtBot removed the label CI failed on Jun 3, 2026

sedited added this to the milestone 32.0 on Jun 3, 2026

in src/script/descriptor.cpp:1636 in bbcb415386

1631 | @@ -1632,7 +1632,9 @@ class StringMaker {
1632 |              if (!m_pubkeys[key]->ToNormalizedString(*m_arg, ret, m_cache)) return {};
1633 |              break;
1634 |          case DescriptorImpl::StringType::COMPAT:
1635 | -            ret = m_pubkeys[key]->ToString(PubkeyProvider::StringType::COMPAT);
1636 | +            // For backwards compatibility, we do not pass StringType::COMPAT as
1637 | +            // desdescriptors with miniscript did not handle string types until 31.0

rkrux commented at 12:59 PM on June 3, 2026:

In bbcb4153865eca89ddec7b02606941a472e8b046 "miniscript: Don't use StringType::COMPAT"

s/desdescriptors/descriptors s/"did not handle string types"/"did not handle all string types"

achow101 commented at 5:21 PM on June 3, 2026:

Done

in test/functional/wallet_backwards_compatibility.py:269 in 7b1413a7f1

 265 |          node_master.unloadwallet("w2")
 266 |          node_master.unloadwallet("w3")
 267 | +        node_master.unloadwallet("miniscript")
 268 |  
 269 | -        for node in legacy_nodes:
 270 | +        for node in self.nodes[2:]:

rkrux commented at 1:02 PM on June 3, 2026:

In 7b1413a7f1e4c802d8b237dfc7c8734d5d4425ed "test: Add v30.2 and Miniscript to wallet backwards compatibility test"

diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py
index 6bb6015c4c..7f2d338604 100755
--- a/test/functional/wallet_backwards_compatibility.py
+++ b/test/functional/wallet_backwards_compatibility.py
@@ -203,6 +203,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
 
         legacy_nodes = self.nodes[-6:] # Nodes that support legacy wallets
         descriptors_nodes = self.nodes[2:-1] # Nodes that support descriptor wallets
+        previous_versions = self.nodes[2:]
 
         self.generatetoaddress(node_miner, COINBASE_MATURITY + 1, node_miner.getnewaddress())
 
@@ -268,7 +269,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         node_master.unloadwallet("w3")
         node_master.unloadwallet("miniscript")
 
-        for node in self.nodes[2:]:
+        for node in previous_versions:
             # Copy wallets to previous version
             for wallet in os.listdir(node_master_wallets_dir):
                 dest = node.wallets_path / wallet

achow101 commented at 5:21 PM on June 3, 2026:

Done

in src/wallet/external_signer_scriptpubkeyman.cpp:33 in e589e3497e

  34 | -    assert(storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
  35 | -    assert(storage.IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER));
  36 | -
  37 |      int64_t creation_time = GetTime();
  38 |  
  39 | +    // Store the descriptor

rkrux commented at 1:37 PM on June 3, 2026:

In e589e3497e1d30d1f2f6bc7f7c0f4720c76ef4ab "spkm: Remove DescriptorSPKM constructor that doesn't take a descriptor"

Is the moving of this comment intentional? Reads a bit odd.

achow101 commented at 5:21 PM on June 3, 2026:

No, fixed

in src/wallet/external_signer_scriptpubkeyman.cpp:1 in e589e3497e outdated

rkrux commented at 1:38 PM on June 3, 2026:

In e589e34 "spkm: Remove DescriptorSPKM constructor that doesn't take a descriptor"

So, this whole commit seems like a standalone follow-up of #28333, make it a separate PR? The wallet_* tests pass on this standalone commit.

achow101 commented at 5:07 PM on June 3, 2026:

It's necessary so that m_id can be const. Without it, the constructor that doesn't take a descriptor is used and that will initialize m_id to all 0s.

rkrux commented at 5:35 PM on June 3, 2026:

Yes, I see it's necessary because the following commits depend on it.

I thought it could go in a separate PR and this one could be stacked over it while being in draft until that one is merged.

But it might slow down the process for the fix overall, so not a strong opinion.

achow101 commented at 8:48 PM on June 3, 2026:

I don't think that this commit by itself would make a good PR. It's more clearly useful as a refactor required in this PR.

pseudoramdom commented at 9:12 PM on August 20, 2026:

This comment is outdated now that we added the cache :)

pseudoramdom commented at 9:17 PM on August 20, 2026:

In wallet, export: Include descriptor cache when exporting descriptors

It looks like listdescriptors RPC uses the ExportDescriptors() which does not appear to use the cache. Should we make descriptor cache optional?

achow101 commented at 1:12 AM on August 21, 2026:

I think it's fine to give it, I'd prefer to keep this simple.

achow101 commented at 1:17 AM on August 21, 2026:

Fixed

rkrux commented at 1:47 PM on June 3, 2026: contributor

Reviewing.

achow101 force-pushed on Jun 3, 2026

in test/functional/wallet_backwards_compatibility.py:399 in b0967280ea outdated

 394 | +                for desc in wallet.listdescriptors()["descriptors"]:
 395 | +                    if desc["desc"].startswith("wsh(or_b(pk"):
 396 | +                        break
 397 | +                else:
 398 | +                    assert False, "Did not find miniscript descriptor"
 399 | +

mjdietzx commented at 8:16 PM on June 3, 2026:

Additional test coverage would be useful, something like:

# Re-importing the descriptor must update the existing descriptor rather than add a
# duplicate, even though its stored id was computed by an older version.
num_descs = len(wallet.listdescriptors()["descriptors"])
assert_equal(wallet.importdescriptors([{"desc": miniscript_desc, "timestamp": "now"}])[0]["success"], True)
assert_equal(len(wallet.listdescriptors()["descriptors"]), num_descs)

achow101 commented at 8:52 PM on June 3, 2026:

I've added the checks to the importdescriptors test.

in src/wallet/test/walletload_tests.cpp:83 in b0967280ea outdated

  77 | @@ -86,8 +78,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup)
  78 |      {
  79 |          // Now try to load the wallet and verify the error.
  80 |          const std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", std::move(database)));
  81 | -        BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(_error, _warnings), DBErrors::CORRUPT);
  82 | -        BOOST_CHECK(found); // The error must be logged
  83 | +        BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(_error, _warnings), DBErrors::LOAD_OK);
  84 |      }
  85 |  }

mjdietzx commented at 8:18 PM on June 3, 2026:

Additional unit test useful, something like:

BOOST_FIXTURE_TEST_CASE(wallet_reimport_opaque_id, TestingSetup)
{
    // The stored descriptor id is an opaque SPKM id, not a value that is recomputed and validated.
    // Re-importing a descriptor whose stored id differs from its recomputed hash (e.g. one written
    // by an older version) must reuse the existing ScriptPubKeyMan instead of creating a duplicate.
    bilingual_str error;
    std::vector<bilingual_str> warnings;

    FlatSigningProvider keys;
    std::string parse_error;
    const std::string desc = "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)";
    auto parsed = Parse(desc, keys, parse_error, /*require_checksum=*/false);
    BOOST_REQUIRE_MESSAGE(!parsed.empty(), parse_error);
    std::shared_ptr<Descriptor> descriptor{std::move(parsed.at(0))};

    std::unique_ptr<WalletDatabase> database = CreateMockableWalletDatabase();
    {
        // Store the descriptor under an arbitrary (opaque) id, as an older version might have.
        WalletBatch batch(*database);
        WalletDescriptor wallet_descriptor(descriptor, /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0);
        BOOST_CHECK(batch.WriteWalletFlags(WALLET_FLAG_DESCRIPTORS | WALLET_FLAG_LAST_HARDENED_XPUB_CACHED));
        BOOST_CHECK(batch.WriteDescriptor(uint256::ONE, wallet_descriptor));
        BOOST_CHECK(batch.WriteActiveScriptPubKeyMan(static_cast<uint8_t>(*descriptor->GetOutputType()), uint256::ONE, /*internal=*/false));
    }

    const std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", std::move(database)));
    BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(error, warnings), DBErrors::LOAD_OK);
    BOOST_CHECK(wallet->GetScriptPubKeyMan(uint256::ONE) != nullptr);

    // Re-importing the same descriptor must not add a second ScriptPubKeyMan.
    LOCK(wallet->cs_wallet);
    const size_t spkms_before = wallet->GetAllScriptPubKeyMans().size();
    WalletDescriptor reimport(descriptor, /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0);
    BOOST_CHECK(wallet->AddWalletDescriptor(reimport, keys, /*label=*/"", /*internal=*/false).has_value());
    BOOST_CHECK_EQUAL(wallet->GetAllScriptPubKeyMans().size(), spkms_before);
}

achow101 commented at 8:52 PM on June 3, 2026:

There doesn't need to be a unit test for something covered by functional tests.

mjdietzx commented at 8:19 PM on June 3, 2026: contributor

crACK https://github.com/bitcoin/bitcoin/commit/b0967280ea45493bf7e02028f2a882da5f29593b and it fixes the issue for me

in src/wallet/scriptpubkeyman.h:335 in 749097b9f9 outdated

 331 |      //! Create a new DescriptorScriptPubKeyMan from a descriptor (e.g. from an import, newly generated outside of constructor)
 332 |      DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size)
 333 |          : ScriptPubKeyMan(storage),
 334 |          m_keypool_size(keypool_size),
 335 | +        m_id(DescriptorID(*descriptor.descriptor)),
 336 |          m_wallet_descriptor(descriptor)

furszy commented at 9:35 PM on June 3, 2026:

nit: could turn this into a CreateNew static function. Just so we don't use it for anything else. If DescriptorID differs from the one in db, any update would create a new record.

achow101 commented at 5:19 PM on June 10, 2026:

It already is? This constructor is only called by static functions and it's protected so that it cannot be called externally except by subclasses.

furszy commented at 9:35 PM on June 3, 2026: member

Concept ACK, will review.

DrahtBot added the label Needs rebase on Jun 17, 2026

achow101 force-pushed on Jun 19, 2026

DrahtBot removed the label Needs rebase on Jun 19, 2026

DrahtBot added the label Needs rebase on Jul 3, 2026

achow101 force-pushed on Jul 7, 2026

DrahtBot added the label CI failed on Jul 7, 2026

DrahtBot commented at 10:25 PM on July 7, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/28901969616/job/85740603311 LLM reason (✨ experimental): CI failed due to a C++ build error: src/wallet/export.cpp references a non-existent wallet::WalletDescriptor::id member (error: no member named 'id').

<details><summary>Hints</summary>

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.

</details>

DrahtBot removed the label Needs rebase on Jul 8, 2026

achow101 force-pushed on Jul 8, 2026

furszy commented at 6:16 PM on July 9, 2026: member

CI failing, the third commit needs an adjustment.

achow101 force-pushed on Jul 9, 2026

DrahtBot removed the label CI failed on Jul 9, 2026

in src/wallet/scriptpubkeyman.cpp:890 in 243a86b60f outdated

 887 | +    if (spkm->m_storage.HasEncryptionKeys()) {
 888 | +        spkm->m_decryption_thoroughly_checked = true;
 889 | +    }
 890 | +
 891 | +    // TopUp
 892 | +    spkm->TopUpWithDB(batch);

furszy commented at 6:22 PM on July 13, 2026:

just a colorful note: we actually write the descriptor inside TopUp too. So the initial WriteDescriptor is slightly redundant. I would still leave it there.

in src/wallet/export.cpp:40 in deecc59c13 outdated

  35 | @@ -36,7 +36,8 @@ util::Expected<std::vector<WalletDescInfo>, std::string> ExportDescriptors(const
  36 |              wallet.IsActiveScriptPubKeyMan(*desc_spk_man),
  37 |              wallet.IsInternalScriptPubKeyMan(desc_spk_man),
  38 |              is_range ? std::optional(std::make_pair(wallet_descriptor.range_start, wallet_descriptor.range_end)) : std::nullopt,
  39 | -            wallet_descriptor.next_index
  40 | +            wallet_descriptor.next_index,
  41 | +            wallet_descriptor.cache

furszy commented at 6:27 PM on July 13, 2026:

It would be nice to get a test for the cache export. Could check logging to see scripts are not re-derived etc.

achow101 commented at 7:59 PM on July 13, 2026:

There already is a test for it, it's the test for exporting a descriptor with hardened child derivation. The only way to get addresses from the export with such a descriptor is if the cache was correctly copied.

in src/wallet/test/walletload_tests.cpp:68 in 990c3bbd57 outdated

  64 | @@ -65,17 +65,9 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup)
  65 |      }
  66 |  
  67 |      // Test 2
  68 | -    // Now write a valid descriptor with an invalid ID.
  69 | -    // As the software produces another ID for the descriptor, the loading process must be aborted.
  70 | +    // Now write a valid descriptor with a different ID which must be accepted

furszy commented at 6:31 PM on July 13, 2026:

Do we have a test that imports the same descriptor twice, using different IDs and seeing how the wallet behaves? they both should have the same txs, balance, etc.

achow101 commented at 8:02 PM on July 13, 2026:

IIRC There is a test for different descriptors that produce the same scripts and how that doesn't change anything. I don't think a specific test for same descriptor, different ID, is necessary.

sedited requested review from mjdietzx on Jul 24, 2026

sedited requested review from furszy on Jul 24, 2026

sedited requested review from rkrux on Jul 24, 2026

DrahtBot added the label Needs rebase on Aug 11, 2026

achow101 force-pushed on Aug 11, 2026

DrahtBot removed the label Needs rebase on Aug 11, 2026

w0xlt commented at 11:17 PM on August 11, 2026: contributor

The following test passes on master but fails with this PR.

The test imports the same descriptor twice. The first uses 0h/0h, while the second uses 0'/0'.

diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index 5d5570cfb1..8ef3d1a813 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -576,12 +576,24 @@ class ImportDescriptorsTest(BitcoinTestFramework):
             'bcrt1qsg97266hrh6cpmutqen8s4s962aryy77jp0fg0', # m/0'/0'/4
         ]
 
-        self.test_importdesc({'desc': descsum_create('wpkh([80002067/0h/0h]' + xpub + '/*)'),
-                              'active': True,
-                              'range' : [0, 2],
-                              'timestamp': 'now'
-                             },
-                             success=True)
+        num_descs = len(w1.listdescriptors()["descriptors"])
+        wpkh_request = {
+            'desc': descsum_create('wpkh([80002067/0h/0h]' + xpub + '/*)'),
+            'active': True,
+            'range': [0, 2],
+            'timestamp': 'now',
+        }
+        self.test_importdesc(wpkh_request, success=True)
+        num_descs = len(w1.listdescriptors()["descriptors"])
+        expanded_wpkh_request = {**wpkh_request, 'range': [0, 4]}
+        with self.nodes[1].assert_debug_log(["Update existing descriptor"]):
+            self.test_importdesc({
+                **expanded_wpkh_request,
+                'desc': descsum_create("wpkh([80002067/0'/0']" + xpub + '/*)'),
+                }, success=True)
+        assert_equal(len(w1.listdescriptors()["descriptors"]), num_descs)
+        self.test_importdesc(expanded_wpkh_request, success=True)
+
         self.test_importdesc({'desc': descsum_create('sh(wpkh([abcdef12/0h/0h]' + xpub + '/*))'),
                               'active': True,
                               'range' : [0, 2],

</details>

If I am understanding correctly, it is not intended. The current PR code compares descriptors using ToString(), which keeps that spelling difference and treats them as different. However, both forms have the same CompatDescriptorHash, which is used as the ID for a newly created SPKM.

Using ToString(/*compat_format=*/true) could fix it.

<details> <summary>suggestion</summary>

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 6dcddc16b8..6cc02fcc35 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -865,10 +865,8 @@ std::unique_ptr<DescriptorScriptPubKeyMan> DescriptorScriptPubKeyMan::LoadFromSt
     return std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(storage, id, descriptor, keypool_size, keys, ckeys));
 }
 
-std::unique_ptr<DescriptorScriptPubKeyMan> DescriptorScriptPubKeyMan::GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal)
+std::unique_ptr<DescriptorScriptPubKeyMan> DescriptorScriptPubKeyMan::GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, WalletDescriptor& desc)
 {
-    WalletDescriptor desc = GenerateWalletDescriptor(master_key.Neuter(), addr_type, internal);
-
     auto spkm = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(storage, desc, keypool_size));
 
     LOCK(spkm->cs_desc_man);
@@ -1506,7 +1504,7 @@ void DescriptorScriptPubKeyMan::Load()
 bool DescriptorScriptPubKeyMan::HasWalletDescriptor(const WalletDescriptor& desc) const
 {
     LOCK(cs_desc_man);
-    return m_wallet_descriptor.descriptor->ToString() == desc.descriptor->ToString();
+    return m_wallet_descriptor.descriptor->ToString(/*compat_format=*/true) == desc.descriptor->ToString(/*compat_format=*/true);
 }
 
 void DescriptorScriptPubKeyMan::WriteDescriptor()
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 2c694ece63..f1553a58b1 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -343,7 +343,7 @@ public:
     static std::unique_ptr<DescriptorScriptPubKeyMan> LoadFromStorage(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys);
     static std::unique_ptr<DescriptorScriptPubKeyMan> CreateFromImport(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);
     static std::unique_ptr<DescriptorScriptPubKeyMan> CreateFromMigration(WalletStorage& storage, WalletBatch& batch, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);
-    static std::unique_ptr<DescriptorScriptPubKeyMan> GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal);
+    static std::unique_ptr<DescriptorScriptPubKeyMan> GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, WalletDescriptor& desc);
 
     mutable RecursiveMutex cs_desc_man;
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 706ecb2d9b..11910e015a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3526,14 +3526,22 @@ LegacyDataSPKM* CWallet::GetLegacyDataSPKM() const
 
 void CWallet::AddScriptPubKeyMan(const uint256& id, std::unique_ptr<ScriptPubKeyMan> spkm_man)
 {
+    Assert(spkm_man && spkm_man->GetID() == id);
+    Assert(!m_spk_managers.contains(id));
+
     // Add spkm_man to m_spk_managers before calling any method
     // that might access it.
-    const auto& spkm = m_spk_managers[id] = std::move(spkm_man);
+    const auto& spkm = m_spk_managers.emplace(id, std::move(spkm_man)).first->second;
 
     // Update birth time if needed
     MaybeUpdateBirthTime(spkm->GetTimeFirstKey());
 }
 
+bool CWallet::HasScriptPubKeyManID(const Descriptor& desc) const
+{
+    return m_spk_managers.contains(CompatDescriptorHash(desc));
+}
+
 LegacyDataSPKM* CWallet::GetOrCreateLegacyDataSPKM()
 {
     SetupLegacyScriptPubKeyMan();
@@ -3605,7 +3613,11 @@ DescriptorScriptPubKeyMan& CWallet::SetupDescriptorScriptPubKeyMan(WalletBatch&
     if (IsLocked()) {
         throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors");
     }
-    auto spk_manager = DescriptorScriptPubKeyMan::GenerateNewSingleSig(*this, batch, m_keypool_size, master_key, output_type, internal);
+    WalletDescriptor desc = GenerateWalletDescriptor(master_key.Neuter(), output_type, internal);
+    if (HasScriptPubKeyManID(*desc.descriptor)) {
+        throw std::runtime_error(std::string(__func__) + ": a different ScriptPubKeyMan with the same ID already exists");
+    }
+    auto spk_manager = DescriptorScriptPubKeyMan::GenerateNewSingleSig(*this, batch, m_keypool_size, master_key, desc);
     DescriptorScriptPubKeyMan* out = spk_manager.get();
     uint256 id = spk_manager->GetID();
     AddScriptPubKeyMan(id, std::move(spk_manager));
@@ -3677,6 +3689,9 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
                     continue;
                 }
                 OutputType t =  *desc->GetOutputType();
+                if (HasScriptPubKeyManID(*desc)) {
+                    throw std::runtime_error(std::string(__func__) + ": a different ScriptPubKeyMan with the same ID already exists");
+                }
                 auto spk_manager = ExternalSignerScriptPubKeyMan::CreateNew(*this, batch, m_keypool_size, std::move(desc));
                 uint256 id = spk_manager->GetID();
                 AddScriptPubKeyMan(id, std::move(spk_manager));
@@ -3798,6 +3813,9 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
             return util::Error{util::ErrorString(spkm_res)};
         }
     } else {
+        if (HasScriptPubKeyManID(*desc.descriptor)) {
+            return util::Error{_("A different descriptor with the same identifier already exists")};
+        }
         auto new_spk_man = DescriptorScriptPubKeyMan::CreateFromImport(*this, desc, m_keypool_size, signing_provider);
         spk_man = new_spk_man.get();
 
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 9b1bb8b6dd..6f19dd5374 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -424,6 +424,8 @@ private:
     // Must be the only method adding data to it.
     void AddScriptPubKeyMan(const uint256& id, std::unique_ptr<ScriptPubKeyMan> spkm_man);
 
+    bool HasScriptPubKeyManID(const Descriptor& desc) const;
+
     // Same as 'AddActiveScriptPubKeyMan' but designed for use within a batch transaction context
     void AddActiveScriptPubKeyManWithDb(WalletBatch& batch, uint256 id, OutputType type, bool internal);
 
diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index 5d5570cfb1..789c37548e 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -576,12 +576,22 @@ class ImportDescriptorsTest(BitcoinTestFramework):
             'bcrt1qsg97266hrh6cpmutqen8s4s962aryy77jp0fg0', # m/0'/0'/4
         ]
 
-        self.test_importdesc({'desc': descsum_create('wpkh([80002067/0h/0h]' + xpub + '/*)'),
-                              'active': True,
-                              'range' : [0, 2],
-                              'timestamp': 'now'
-                             },
-                             success=True)
+        wpkh_request = {
+            'desc': descsum_create('wpkh([80002067/0h/0h]' + xpub + '/*)'),
+            'active': True,
+            'range': [0, 2],
+            'timestamp': 'now',
+        }
+        self.test_importdesc(wpkh_request, success=True)
+        num_descs = len(w1.listdescriptors()["descriptors"])
+        expanded_wpkh_request = {**wpkh_request, 'range': [0, 4]}
+        with self.nodes[1].assert_debug_log(["Update existing descriptor"]):
+            self.test_importdesc({
+                **expanded_wpkh_request,
+                'desc': descsum_create("wpkh([80002067/0'/0']" + xpub + '/*)'),
+            }, success=True)
+        assert_equal(len(w1.listdescriptors()["descriptors"]), num_descs)
+        self.test_importdesc(expanded_wpkh_request, success=True)
         self.test_importdesc({'desc': descsum_create('sh(wpkh([abcdef12/0h/0h]' + xpub + '/*))'),
                               'active': True,
                               'range' : [0, 2],

achow101 force-pushed on Aug 12, 2026

achow101 commented at 6:34 PM on August 12, 2026: member

The test imports the same descriptor twice. The first uses 0h/0h, while the second uses 0'/0'.

Added a simplified version of the test and fix.

in src/script/descriptor.cpp:1670 in 6c7a5e0825

1665 | @@ -1666,7 +1666,9 @@ class StringMaker {
1666 |              if (!m_pubkeys[key]->ToNormalizedString(*m_arg, ret, m_cache)) return {};
1667 |              break;
1668 |          case DescriptorImpl::StringType::COMPAT:
1669 | -            ret = m_pubkeys[key]->ToString(PubkeyProvider::StringType::COMPAT);
1670 | +            // For backwards compatibility, we do not pass StringType::COMPAT as
1671 | +            // desdescriptors with miniscript did not handle all string types until 31.0

davidgumberg commented at 10:42 PM on August 12, 2026:

nit: typo here

pseudoramdom commented at 8:32 PM on August 20, 2026:

Typo in descriptors

pseudoramdom commented at 8:55 PM on August 20, 2026:

In miniscript: Don't use StringType::COMPAT

nit: The comment can be a bit more clear as to why

// For backwards compatibility, we do not pass StringType::COMPAT.
// Pre-31.0, keys inside Miniscript used `PUBLIC` formatting even with
// `COMPAT` serialization. DescriptorIDs were computed from that
// representation, so preserve the historical behavior for backwards compatibility.

achow101 commented at 1:16 AM on August 21, 2026:

Fixed

achow101 commented at 1:16 AM on August 21, 2026:

Fixed

achow101 commented at 1:16 AM on August 21, 2026:

Expanded the comment

in src/wallet/external_signer_scriptpubkeyman.cpp:36 in 2fe5f17a0b

  38 |  
  39 |      // Make the descriptor
  40 |      WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
  41 | -    spkm->m_wallet_descriptor = w_desc;
  42 |  
  43 |      // Store the descriptor

davidgumberg commented at 10:45 PM on August 12, 2026:

in commit https://github.com/bitcoin/bitcoin/pull/35445/changes/2fe5f17a0ba5b8bf1a9209d86341ce25c18d240c (spkm: Remove DescriptorSPKM constructor that doesn't take a descriptor)

nit: this comment is now in the wrong place, it should be above WriteDescriptor

achow101 commented at 1:16 AM on August 21, 2026:

Fixed

w0xlt commented at 12:36 AM on August 14, 2026: contributor

Thanks for taking the suggestion. However, the PR still treats the same Miniscript descriptor as different descriptors when it is imported once using h hardened markers and again using '.

The previous test does not detect this because it uses a regular wpkh() descriptor, not a descriptor containing a Miniscript expression. This does not happen on master, so it is a regression / behavior change introduced by this PR.

The functional test below demonstrates the issue:

diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index 328b4c99b8..8953402ef4 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -629,6 +629,24 @@ class ImportDescriptorsTest(BitcoinTestFramework):
                                  success=True)
             assert_equal(w1.getnewaddress('', 'bech32'), addresses[i])
 
+        self.log.info("Equivalent Miniscript descriptors should not be duplicated")
+        self.nodes[1].createwallet(wallet_name="wminiscript", disable_private_keys=True, blank=True)
+        wminiscript = self.nodes[1].get_wallet_rpc("wminiscript")
+        miniscript_request = {
+            'active': True,
+            'range': [0, 9],
+            'timestamp': 'now',
+        }
+        self.test_importdesc({
+            **miniscript_request,
+            'desc': descsum_create(f"wsh(and_v(v:pk([80002067/0h/0h]{xpub}/*),older(1)))"),
+        }, success=True, wallet=wminiscript)
+        self.test_importdesc({
+            **miniscript_request,
+            'desc': descsum_create(f"wsh(and_v(v:pk([80002067/0'/0']{xpub}/*),older(1)))"),
+        }, success=True, wallet=wminiscript)
+        assert_equal(len(wminiscript.listdescriptors()["descriptors"]), 1)
+
         # Check active=False default
         self.log.info('Check imported descriptors are not active by default')
         self.test_importdesc({'desc': descsum_create('pkh([12345678/1h]' + xpub + '/*)'),

</details>

Wrapping the second import in assert_debug_log(["Update existing descriptor"]) also detects the issue. However, checking listdescriptors is more direct: on this PR the assertion fails with 2 == 1, showing that two SPKMs were created for equivalent descriptors.

w0xlt commented at 12:47 AM on August 14, 2026: contributor

The descriptor equality (DescriptorScriptPubKeyMan::HasWalletDescriptor() in src/wallet/scriptpubkeyman.cpp) now uses the compatibility string:

old_desc->ToString(/*compat_format=*/true) == new_desc->ToString(/*compat_format=*/true);

For keys within a Miniscript expression such as wsh(and_v(...)), serialization goes through StringMaker::ToString() in src/script/descriptor.cpp:

case StringType::COMPAT:
      ret = m_pubkeys[key]->ToString(); // Keeps `h` or `'`

Thus, equivalent descriptors compare as different and a second SPKM is created.

in src/script/descriptor.cpp:1671 in 6c7a5e0825

1665 | @@ -1666,7 +1666,9 @@ class StringMaker {
1666 |              if (!m_pubkeys[key]->ToNormalizedString(*m_arg, ret, m_cache)) return {};
1667 |              break;
1668 |          case DescriptorImpl::StringType::COMPAT:
1669 | -            ret = m_pubkeys[key]->ToString(PubkeyProvider::StringType::COMPAT);
1670 | +            // For backwards compatibility, we do not pass StringType::COMPAT as
1671 | +            // desdescriptors with miniscript did not handle all string types until 31.0
1672 | +            ret = m_pubkeys[key]->ToString();

davidgumberg commented at 8:34 PM on August 20, 2026:

https://github.com/bitcoin/bitcoin/pull/35445/changes/6c7a5e0825092f9f07b4eb9ce0a8561c60ff677f (miniscript: Don't use StringType::COMPAT)

Just a note for other reviewers, this partially reverts https://github.com/bitcoin/bitcoin/pull/31734/changes/975783cb79e929260873c1055d4b415cd33bb6b9.

Before the above commit, the request for a COMPAT string, which was only made by DescriptorID:

https://github.com/bitcoin/bitcoin/blob/56db08d5291a533f75df89ba88f8b06deac0eebd/src/script/descriptor.cpp#L2979-L2981

was ignored, and a public string was returned. After the above fix, miniscript descriptor id's are computed using the compat form instead of the public form, which caused the incompatibility.

pseudoramdom commented at 8:57 PM on August 20, 2026:

In miniscript: Don't use StringType::COMPAT

nit: We could also be explicit here instead of relying on the default argument.

ret = m_pubkeys[key]->ToString(PubkeyProvider::StringType::PUBLIC);

achow101 commented at 1:16 AM on August 21, 2026:

I've added a commit that dropped the default parameter entirely.

in src/wallet/export.cpp:116 in 3615fdd08d outdated

 111 |              if (!w_desc.descriptor->CanSelfExpand()) {
 112 |                  w_desc.cache = desc_info.cache;
 113 |              }
 114 |  
 115 |              // Add to the watchonly wallet
 116 | -            if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, dummy_keys, /*label=*/"", /*internal=*/false); !spkm_res) {

davidgumberg commented at 8:57 PM on August 20, 2026:

+1

in src/wallet/scriptpubkeyman.h:327 in 2fe5f17a0b

 326 |  protected:
 327 |      //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading)
 328 |      DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys);
 329 |  
 330 | -    DescriptorScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size)
 331 | +    //! Create a new DescriptorScriptPubKeyMan from a descriptor (e.g. from an import, newly generated outside of constructor)

pseudoramdom commented at 9:08 PM on August 20, 2026:

In spkm: Remove DescriptorSPKM constructor that doesn't take a descriptor

nit: Can we drop "outside of constructor" ? //! Create a new DescriptorScriptPubKeyMan from an imported or newly generated descriptor.

achow101 commented at 1:16 AM on August 21, 2026:

Done

in src/wallet/scriptpubkeyman.cpp:1510 in 3615fdd08d outdated

1505 | @@ -1506,7 +1506,8 @@ void DescriptorScriptPubKeyMan::Load()
1506 |  bool DescriptorScriptPubKeyMan::HasWalletDescriptor(const WalletDescriptor& desc) const
1507 |  {
1508 |      LOCK(cs_desc_man);
1509 | -    return !m_wallet_descriptor.id.IsNull() && !desc.id.IsNull() && m_wallet_descriptor.id == desc.id;
1510 | +    // Compare by using the compat format string to make the hardened indicators consistent for comparison
1511 | +    return m_wallet_descriptor.descriptor->ToString(/*compat_format=*/true) == desc.descriptor->ToString(/*compat_format=*/true);

pseudoramdom commented at 9:38 PM on August 20, 2026:

In wallet, spkm: Treat Descriptor ID as an opaque SPKM ID

I'm confused. The change here contradicts the change made in miniscript: Don't use StringType::COMPAT A miniscript descriptor with h would not be equal to the same descriptor with ' as per above line.

davidgumberg commented at 10:21 PM on August 20, 2026:

I think we do want to normalize them to the same form, because the only reason we use this check is to make sure we don't import the same descriptor twice, so we want to treat those two forms as the same.

pseudoramdom commented at 10:41 PM on August 20, 2026:

We probably need a new StringType::CANONICAL or a ToStringForComparison() (I'd prefer the former) that that consistently formats hardened indicators including inside Miniscript

w0xlt commented at 10:46 PM on August 20, 2026:

Yes, something like StringType::CANONICAL should do the trick.

davidgumberg commented at 11:08 PM on August 20, 2026:

achow101 commented at 1:22 AM on August 21, 2026:

I've added a couple commits to add a StringType::CANONICAL

in src/wallet/test/walletload_tests.cpp:72 in 3615fdd08d

  76 | -        found = true;
  77 | -        return false;
  78 | -    });
  79 | -
  80 |      {
  81 |          // Write valid descriptor with invalid ID

pseudoramdom commented at 9:42 PM on August 20, 2026:

Since IDs are opaque strings now, there are no invalid IDs. Maybe "arbitrary ID"?

achow101 commented at 1:27 AM on August 21, 2026:

Done

in src/wallet/test/walletload_tests.cpp:80 in 3615fdd08d

  78 | @@ -87,8 +79,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup)
  79 |      {
  80 |          // Now try to load the wallet and verify the error.

pseudoramdom commented at 9:43 PM on August 20, 2026:

verify the error.

LOAD_OK is an error?

achow101 commented at 1:28 AM on August 21, 2026:

Fixed

in src/test/descriptor_tests.cpp:237 in 8bc7d0a284

 234 | @@ -235,8 +235,8 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
 235 |      }
 236 |  
 237 |      // Check that the COMPAT identifier did not change

pseudoramdom commented at 9:49 PM on August 20, 2026:

identifier -> descriptor hash maybe?

achow101 commented at 1:28 AM on August 21, 2026:

Done

pseudoramdom commented at 9:50 PM on August 20, 2026: contributor

Approach ACK. Left a few comments and nits

in src/script/descriptor.cpp:1666 in 6c7a5e0825 outdated

1665 | @@ -1666,7 +1666,9 @@ class StringMaker {
1666 |              if (!m_pubkeys[key]->ToNormalizedString(*m_arg, ret, m_cache)) return {};

davidgumberg commented at 10:24 PM on August 20, 2026:

note for other reviewers:

The reason why this commit is necessary at all, even though future versions don't care what is written in the DescID field, is so that wallets created by a new version, can be loaded by an older version which does check if the computed descriptor ID matches the one written in the DB.

Will we ever be able to remove this?

It kind of sucks that we want to avoid the troubles of descriptor ID but will always have to preserve compatibility with old wallets that do care about it.

davidgumberg commented at 10:28 PM on August 20, 2026:

This is also the reason why the case suggested by @w0xlt above, where two miniscript descriptors both get imported without error, even though they are identical except one uses ' and the other uses h

<summary> miniscript equiv. case </summary>

diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index 328b4c99b8..8953402ef4 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -629,6 +629,24 @@ class ImportDescriptorsTest(BitcoinTestFramework):
                                  success=True)
             assert_equal(w1.getnewaddress('', 'bech32'), addresses[i])
 
+        self.log.info("Equivalent Miniscript descriptors should not be duplicated")
+        self.nodes[1].createwallet(wallet_name="wminiscript", disable_private_keys=True, blank=True)
+        wminiscript = self.nodes[1].get_wallet_rpc("wminiscript")
+        miniscript_request = {
+            'active': True,
+            'range': [0, 9],
+            'timestamp': 'now',
+        }
+        self.test_importdesc({
+            **miniscript_request,
+            'desc': descsum_create(f"wsh(and_v(v:pk([80002067/0h/0h]{xpub}/*),older(1)))"),
+        }, success=True, wallet=wminiscript)
+        self.test_importdesc({
+            **miniscript_request,
+            'desc': descsum_create(f"wsh(and_v(v:pk([80002067/0'/0']{xpub}/*),older(1)))"),
+        }, success=True, wallet=wminiscript)
+        assert_equal(len(wminiscript.listdescriptors()["descriptors"]), 1)
+
         # Check active=False default
         self.log.info('Check imported descriptors are not active by default')
         self.test_importdesc({'desc': descsum_create('pkh([12345678/1h]' + xpub + '/*)'),

</details>

achow101 force-pushed on Aug 21, 2026

DrahtBot added the label CI failed on Aug 21, 2026

DrahtBot removed the label CI failed on Aug 21, 2026

DrahtBot added the label Needs rebase on Aug 24, 2026

achow101 force-pushed on Aug 24, 2026

DrahtBot added the label CI failed on Aug 24, 2026

DrahtBot commented at 8:40 PM on August 24, 2026: contributor

🚧 At least one of the CI tasks failed. Task MSan, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/32773289628/job/97578390651 LLM reason (✨ experimental): CI failed due to a C++ build error in src/script/descriptor.cpp (“too few arguments” calling ToString(StringType)).

<details><summary>Hints</summary>

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.

</details>

achow101 force-pushed on Aug 24, 2026

DrahtBot removed the label CI failed on Aug 24, 2026

miniscript: Don't use StringType::COMPAT

Previous versions did not pass down StringType::COMPAT when that was
given as the serialization string type. As COMPAT is used for descriptor
id calculation, we need to maintain the previous (incorrect) behavior of
not passing StringType::COMPAT.

1c7f9aaf75

descriptors: Remove default StringType from PubkeyProvider::ToString()

Implementations of ToString() must remember to handle the different
StringTypes. Removing the default argument forces implementors to
consider it.

1d87af26ce

achow101 force-pushed on Aug 25, 2026

DrahtBot removed the label Needs rebase on Aug 25, 2026

in src/test/descriptor_tests.cpp:242 in 55c2b51761

 237 |      }
 238 |  
 239 | +    std::string priv_canonical = parse_priv->ToCanonicalString();
 240 | +    std::string pub_canonical = parse_pub->ToCanonicalString();
 241 | +    BOOST_CHECK_MESSAGE(EqualDescriptor(priv_canonical, canonical), "Private ser: " + priv_canonical + " Expected desc: " + canonical);
 242 | +    BOOST_CHECK_MESSAGE(EqualDescriptor(pub_canonical, canonical), "Private ser: " + pub_canonical + " Expected desc: " + canonical);

pseudoramdom commented at 6:07 PM on August 25, 2026:

In descriptor: Add ToCanonicalString 55c2b51761ed4ca88e863ef3c68301c490a10524

Should be Public ser:

achow101 commented at 8:41 PM on August 25, 2026:

Fixed

in src/wallet/scriptpubkeyman.h:330 in 57269b2202

 326 |  
 327 |      //! Create a new DescriptorScriptPubKeyMan from a descriptor (e.g. from an import, newly generated)
 328 |      DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size)
 329 |          : ScriptPubKeyMan(storage),
 330 |          m_keypool_size(keypool_size),
 331 | +        m_id(DescriptorID(*descriptor.descriptor)),

pseudoramdom commented at 6:35 PM on August 25, 2026:

In wallet, spkm: Treat Descriptor ID as an opaque SPKM ID -

AddScriptPubKeyMan() currently assigns m_spk_managers[id] to the created SPKM. Now that IDs loaded from the database are treated as opaque, should we check if a descriptor exists at that ID before replacing it?

For example, say a (corrupted) wallet contains descriptor A under an ID equal to DescriptorID(B). Importing descriptor B would then calculate the same ID and AddScriptPubKeyMan would silently replace descriptor A. Previously, this scenario would have required a hash collision.

achow101 commented at 8:41 PM on August 25, 2026:

That's not possible because the descriptor records are keyed by the ID and there cannot be duplicate records in the database.

pseudoramdom commented at 6:42 PM on August 25, 2026: contributor

Code review 073b3f94ca0cd7ead954eaa93b0aeb4451915fe8

descriptor: Add ToCanonicalString 35d6a60dbf

test: Add v30.2 and Miniscript to wallet backwards compatibility test 770ff64bd7

spkm: Remove DescriptorSPKM constructor that doesn't take a descriptor

Instead of creating a DescriptorSPKM that doesn't have a descriptor,
only to then generate the descriptor, combine SetupDescriptorGeneration
into the GenerateNewSingleSig factory function, and within that
function, generate the descriptor first before constructing the new
DescriptorSPKM.

9fc7b2618b

wallet, export: Include descriptor cache when exporting descriptors 1113f7590e

achow101 force-pushed on Aug 25, 2026

w0xlt commented at 9:08 PM on August 25, 2026: contributor

When a Miniscript descriptor is first imported using the h spelling and later reimported using the equivalent ' spelling, current PR code correctly finds the existing descriptor through canonical comparison.

However, UpdateWalletDescriptor() replaces the stored descriptor while retaining its existing m_id.

Pre-31 releases derive and validate the descriptor ID using a compatibility serialization that preserves this spelling inside Miniscript. The updated descriptor therefore no longer hashes to its stored ID. When the wallet produced by node_master is subsequently loaded by v30.2, loading fails with Wallet corrupted (-4).

The functional test below reproduces the failure.

diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py
index 078a8681ba..9afc6088d3 100755
--- a/test/functional/wallet_backwards_compatibility.py
+++ b/test/functional/wallet_backwards_compatibility.py
@@ -330,9 +330,13 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):

         node_master.createwallet(wallet_name="miniscript")
         wallet = node_master.get_wallet_rpc("miniscript")
-        miniscript_desc = descsum_create("wsh(or_b(pk([deadbeef/0h/1h/2h]tprv8ZgxMBicQKsPerQj6m35no46amfKQdjY7AhLnmatHYXs8S4MTgeZYkWAn4edSGwwL3vkSiiGqSZQrmy5D3P5gBoqgvYP2fCUpBwbKTMTAkL/3h/*),s:pk([beefdead/4h/5h]tpubD6NzVbkrYhZ4YU9vM1s53UhD75UyJatx8EMzMZ3VUjR2FciNfLLkAw6a4pWACChzobTseNqdWk4G7ZdBqRDLtLSACKykTScmqibb1ZrCvJu/6/7/*)))")
-        res = wallet.importdescriptors([{"desc": miniscript_desc, "timestamp":"now"}])
-        assert_equal(res[0]["success"], True)
+        miniscript = "wsh(or_b(pk([deadbeef/0h/1h/2h]tprv8ZgxMBicQKsPerQj6m35no46amfKQdjY7AhLnmatHYXs8S4MTgeZYkWAn4edSGwwL3vkSiiGqSZQrmy5D3P5gBoqgvYP2fCUpBwbKTMTAkL/3h/*),s:pk([beefdead/4h/5h]tpubD6NzVbkrYhZ4YU9vM1s53UhD75UyJatx8EMzMZ3VUjR2FciNfLLkAw6a4pWACChzobTseNqdWk4G7ZdBqRDLtLSACKykTScmqibb1ZrCvJu/6/7/*)))"
+        miniscript_desc = descsum_create(miniscript)
+        miniscript_alias = descsum_create(miniscript.replace("[beefdead/4h/5h]", "[beefdead/4'/5']"))
+        # Reimporting an equivalent spelling must preserve compatibility with older releases.
+        for desc in [miniscript_desc, miniscript_alias]:
+            res = wallet.importdescriptors([{"desc": desc, "timestamp":"now"}])
+            assert_equal(res[0]["success"], True)

         # Unload wallets and copy to older nodes:
         node_master_wallets_dir = node_master.wallets_path

</details>

<details> <summary>Suggested patch</summary>

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 948f6e90c7..d8cccd74cd 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -1632,10 +1632,18 @@ util::Result<void> DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescr
         return util::Error{Untranslated(std::move(error))};
     }

+    WalletDescriptor updated_descriptor{descriptor};
+    // Canonical comparison can match descriptors whose hardened-marker spellings produce
+    // different compatibility hashes. Keep the stored descriptor in that case because m_id may
+    // have been derived from its compatibility hash, and older releases validate this relationship.
+    if (CompatDescriptorHash(*m_wallet_descriptor.descriptor) != CompatDescriptorHash(*updated_descriptor.descriptor)) {
+        updated_descriptor.descriptor = m_wallet_descriptor.descriptor;
+    }
+
     m_map_pubkeys.clear();
     m_map_script_pub_keys.clear();
     m_max_cached_index = -1;
-    m_wallet_descriptor = descriptor;
+    m_wallet_descriptor = std::move(updated_descriptor);

     WalletBatch batch(m_storage.GetDatabase());
     UpdateWithSigningProvider(batch, provider);

</details>

wallet: Update WalletDescriptor from another one instead of overwriting

If a descriptor is being reimported, we should only update the metadata
and cache from the other one, rather than overwriting the entire thing.
This avoids a potential issue where the on-disk record is overwritten
with a backwards incompatible string.

62e826fa76

wallet, spkm: Treat Descriptor ID as an opaque SPKM ID

Instead of treating the descriptor ID as something which has a meaning
which can be verified, treat the ID read from disk as some opaque blob
used solely to identify and tie together specific records from disk.

This removes the usage of the ID for duplication checks or comparison,
and removes the check that the read ID matches a computed ID.

When writing new descriptors to disk, the ID is still calculated from
the old Descriptor ID method for backwards compatibility. But this fact
is opaque to all further usages of the ID.

2a6c53371b

test: Add 31.0 to wallet backwards compatibility test

Since 31.0 has a compatibility issue with wallets containing miniscript
descriptors, this should be in the test, with a test for the failure
condition.

6ad31c062c

descriptor: Rename DescriptorID to CompatDescriptorHash

There is no such thing as a descriptor ID; rename the function to
reflect that and to indicate that the hash should not be used as an ID.

e2b2f1c5c6

test: Enforce descriptor reimport is an update a2d001b57c

test: Check miniscript descriptor h and apostrophe equivalence ec2adf3c51

achow101 force-pushed on Aug 25, 2026

achow101 commented at 10:28 PM on August 25, 2026: member

Pre-31 releases derive and validate the descriptor ID using a compatibility serialization that preserves this spelling inside Miniscript. The updated descriptor therefore no longer hashes to its stored ID. When the wallet produced by node_master is subsequently loaded by v30.2, loading fails with Wallet corrupted (-4).

Added a commit with a test and fix.

DrahtBot added the label CI failed on Aug 25, 2026

DrahtBot commented at 11:56 PM on August 25, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS native, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/32906283555/job/97991012562 LLM reason (✨ experimental): CI failed due to a fuzz test crash/failure in the rpc fuzz target (exit code 1: “Error processing input”).

<details><summary>Hints</summary>

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.

</details>

pseudoramdom commented at 4:59 AM on August 26, 2026: contributor

ACK ec2adf3c51ca7322307be3d052bc0e9fa4332dd2 CI Checks are failing though

DrahtBot removed the label CI failed on Aug 26, 2026