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, w0xlt, davidgumberg
Concept ACK	furszy, Sjors, jeanpablojp
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:

#36167 ([RFC] Enable -Wunused by fanquake)
#36154 (wallet: fill PSBT_GLOBAL_XPUB for descriptors with more than one key by jeanpablojp)
#36133 (wallet: store multipath descriptor by Sjors)
#36033 ([wip,nomerge,rfc] build: Require C++23 compiler by maflcko)
#35834 (Test checkunparsable errors by Herb-ops)
#35742 (descriptors: check duplicate keys in all multipath Miniscript branches by yashbhutwala)
#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

in src/script/descriptor.cpp:206 in 35d6a60dbf

 202 | @@ -203,6 +203,7 @@ struct PubkeyProvider
 203 |  
 204 |      enum class StringType {
 205 |          PUBLIC,
 206 | +        CANONICAL, // string calculation that always use h

Sjors commented at 12:49 PM on September 3, 2026:

In 35d6a60dbf5a3424804430041f5f4091efb82b6c descriptor: Add ToCanonicalString: it also forces lower case for hex encoded keys. But Claude caught missing handling of the x-only parity byte. Potential fix:

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index a246383a09..4d706b197d 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -204,5 +204,7 @@ public:
     enum class StringType {
         PUBLIC,
-        CANONICAL, // string calculation that always use h
+        CANONICAL, // string calculation for comparing descriptors: always uses h for hardened derivation, and
+                   // keys in tr() context are always x-only (the parity byte of a 33 byte key is dropped).
+                   // Hex data (public keys, fingerprints, hashes) is always lowercase in every string form.
         COMPAT // string calculation that mustn't change over time to stay compatible with previous software versions
     };
@@ -336,4 +338,7 @@ class ConstPubkeyProvider final : public PubkeyProvider
     CPubKey m_pubkey;
     bool m_xonly;
+    //! Whether only the x coordinate of the key is used (tr() context) even though it was given as a 33 byte key.
+    //! The canonical string then drops the parity byte, while the other string forms keep the key as given.
+    bool m_xonly_ctx;

     std::optional<CKey> GetPrivKey(const SigningProvider& arg) const
@@ -346,5 +351,5 @@ class ConstPubkeyProvider final : public PubkeyProvider

 public:
-    ConstPubkeyProvider(uint32_t exp_index, const CPubKey& pubkey, bool xonly) : PubkeyProvider(exp_index), m_pubkey(pubkey), m_xonly(xonly) {}
+    ConstPubkeyProvider(uint32_t exp_index, const CPubKey& pubkey, bool xonly, bool xonly_ctx = false) : PubkeyProvider(exp_index), m_pubkey(pubkey), m_xonly(xonly), m_xonly_ctx(xonly_ctx) {}
     std::optional<CPubKey> GetPubKey(int pos, const SigningProvider&, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
     {
@@ -359,5 +364,9 @@ public:
     size_t GetSize() const override { return m_pubkey.size(); }
     bool IsBIP32() const override { return false; }
-    std::string ToString(StringType type) const override { return m_xonly ? HexStr(m_pubkey).substr(2) : HexStr(m_pubkey); }
+    std::string ToString(StringType type) const override
+    {
+        const bool xonly = m_xonly || (type == StringType::CANONICAL && m_xonly_ctx);
+        return xonly ? HexStr(m_pubkey).substr(2) : HexStr(m_pubkey);
+    }
     bool ToPrivateString(const SigningProvider& arg, std::string& ret) const override
     {
@@ -391,5 +400,5 @@ public:
     std::unique_ptr<PubkeyProvider> Clone() const override
     {
-        return std::make_unique<ConstPubkeyProvider>(m_expr_index, m_pubkey, m_xonly);
+        return std::make_unique<ConstPubkeyProvider>(m_expr_index, m_pubkey, m_xonly, m_xonly_ctx);
     }
     bool CanSelfExpand() const final { return true; }
@@ -876,5 +885,5 @@ public:
         PRIVATE,
         NORMALIZED,
-        CANONICAL,
+        CANONICAL, // string calculation for comparing descriptors: public keys only, always uses h for hardened derivation
         COMPAT, // string calculation that mustn't change over time to stay compatible with previous software versions
     };
@@ -1970,5 +1979,5 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t& key_exp_
             if (pubkey.IsFullyValid()) {
                 if (permit_uncompressed || pubkey.IsCompressed()) {
-                    ret.emplace_back(std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, false));
+                    ret.emplace_back(std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, /*xonly=*/false, /*xonly_ctx=*/ctx == ParseScriptContext::P2TR));
                     ++key_exp_index;
                     return ret;
diff --git a/src/script/descriptor.h b/src/script/descriptor.h
index 8d9f8a4e71..560b2c7bd6 100644
--- a/src/script/descriptor.h
+++ b/src/script/descriptor.h
@@ -119,6 +119,9 @@ struct Descriptor {
     virtual std::string ToString(bool compat_format=false) const = 0;

-    /** Convert the descriptor to the canonical string.
-     * The canonical string is the same as the public string but always uses h as the hardened indicator
+    /** Convert the descriptor to the canonical string, for comparing descriptors.
+     * The canonical string is the same as the public string but always uses h as the hardened indicator,
+     * and keys in tr() context are always x-only (the parity byte of a 33 byte key is dropped).
+     * As with all string forms, hex encoded data (public keys, fingerprints, hashes) is lowercase and
+     * a checksum is appended, so descriptors that only differ in those aspects have the same canonical string.
      */
     virtual std::string ToCanonicalString() const = 0;
diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index 5b5b10aaa0..87de7c37c2 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -612,4 +612,6 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
     Check("pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)", "pk(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)", "pk(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)", SIGNABLE, {{"2103a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bdac"}}, std::nullopt, /*op_desc_id=*/uint256{"5fe175b43c58ac2cdde40521dc7d1dbc607f3dd795d00770206f4fdefb42229e"});
     Check("pkh([deadbeef/1/2'/3/4']L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)", "pkh([deadbeef/1/2'/3/4']03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)", "pkh([deadbeef/1/2h/3/4h]03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)", SIGNABLE, {{"76a9149a1c78a507689f6f54b847ad1cef1e614ee23f1e88ac"}}, OutputType::LEGACY, /*op_desc_id=*/uint256{"628130ae0530f2b24faf1ad2744a83568ac0ffac43e703e30c00d5f137869b84"}, {{1,0x80000002UL,3,0x80000004UL}});
+    // Uppercase hex data (fingerprint and pubkey) is always serialized lowercase
+    Check("pkh([DEADBEEF/1/2h/3/4h]L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)", "pkh([DEADBEEF/1/2h/3/4h]03A34B99F22C790C4E36B2B3C2C35A36DB06226E41C692FC82B8B56AC1C540C5BD)", "pkh([deadbeef/1/2h/3/4h]03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)", SIGNABLE, {{"76a9149a1c78a507689f6f54b847ad1cef1e614ee23f1e88ac"}}, OutputType::LEGACY, /*op_desc_id=*/uint256{"628130ae0530f2b24faf1ad2744a83568ac0ffac43e703e30c00d5f137869b84"}, {{1,0x80000002UL,3,0x80000004UL}}, /*spender_nlocktime=*/0, /*spender_nsequence=*/CTxIn::SEQUENCE_FINAL, /*preimages=*/{}, /*expected_prv=*/"pkh([deadbeef/1/2h/3/4h]L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)", /*expected_pub=*/"pkh([deadbeef/1/2h/3/4h]03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)");
     Check("wpkh(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)", "wpkh(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)", "wpkh(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)", SIGNABLE, {{"00149a1c78a507689f6f54b847ad1cef1e614ee23f1e"}}, OutputType::BECH32, /*op_desc_id=*/uint256{"4a47b7f497721bf3fc48c69a5d22bc1f3617238649a8ba7cb96fbd92fec84a7e"});
     Check("sh(wpkh(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1))", "sh(wpkh(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd))", "sh(wpkh(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd))", SIGNABLE, {{"a91484ab21b1b2fd065d4504ff693d832434b6108d7b87"}}, OutputType::P2SH_SEGWIT, /*op_desc_id=*/uint256{"a13112753066b5c59473a87c5771b1694a10531944a60e0ab2d7ad66ecb65bcd"});
@@ -1145,4 +1147,6 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
     // Same for sha256
     Check("wsh(and_v(v:sha256(7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863),pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)))", "wsh(and_v(v:sha256(7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", "wsh(and_v(v:sha256(7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", SIGNABLE_FAILS, {{"002071f7283dbbb9a55ed43a54cda16ba0efd0f16dc48fe200f299e57bb5d7be8dd4"}}, OutputType::BECH32, /*op_desc_id=*/uint256{"a1809a65ba5ca2f09a06c114d4881eed95d1b62f38743cf126cf71b2dd411374"}, {{}}, /*spender_nlocktime=*/0, /*spender_nsequence=*/CTxIn::SEQUENCE_FINAL, {});
+    // Uppercase hex data (hash) is always serialized lowercase
+    Check("wsh(and_v(v:sha256(7426BA0604C3F8682C7016B44673F85C5BD9DA2FA6C1080810CF53AE320C9863),pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)))", "wsh(and_v(v:sha256(7426BA0604C3F8682C7016B44673F85C5BD9DA2FA6C1080810CF53AE320C9863),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", "wsh(and_v(v:sha256(7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", SIGNABLE_FAILS, {{"002071f7283dbbb9a55ed43a54cda16ba0efd0f16dc48fe200f299e57bb5d7be8dd4"}}, OutputType::BECH32, /*op_desc_id=*/uint256{"a1809a65ba5ca2f09a06c114d4881eed95d1b62f38743cf126cf71b2dd411374"}, {{}}, /*spender_nlocktime=*/0, /*spender_nsequence=*/CTxIn::SEQUENCE_FINAL, {}, /*expected_prv=*/"wsh(and_v(v:sha256(7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863),pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)))", /*expected_pub=*/"wsh(and_v(v:sha256(7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))");
     Check("wsh(and_v(v:sha256(7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863),pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)))", "wsh(and_v(v:sha256(7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", "wsh(and_v(v:sha256(7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863),pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)))", SIGNABLE, {{"002071f7283dbbb9a55ed43a54cda16ba0efd0f16dc48fe200f299e57bb5d7be8dd4"}}, OutputType::BECH32, /*op_desc_id=*/uint256{"a1809a65ba5ca2f09a06c114d4881eed95d1b62f38743cf126cf71b2dd411374"}, {{}}, /*spender_nlocktime=*/0, /*spender_nsequence=*/CTxIn::SEQUENCE_FINAL, {{"7426ba0604c3f8682c7016b44673f85c5bd9da2fa6c1080810cf53ae320c9863"_hex_v_u8, "000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"_hex_v_u8}});
     // Same for hash160
@@ -1159,5 +1163,5 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
     Check("tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{{pkh(KykUPmR5967F4URzMUeCv9kNMU9CNRWycrPmx3ZvfkWoQLabbimL),pk(L3Enys1jFgTq4E24b8Uom1kAz6cNkz3Z82XZpBKCE2ztErq9fqvJ)},thresh(1,pk(L1NKM8dVA1h52mwDrmk1YreTWkAZZTu2vmKLpmLEbFRqGQYjHeEV),s:pk(Kz3iCBy3HNGP5CZWDsAMmnCMFNwqdDohudVN9fvkrN7tAkzKNtM7))})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{{pkh(1c9bc926084382e76da33b5a52d17b1fa153c072aae5fb5228ecc2ccf89d79d5),pk(0dd6b52b192ab195558d22dd8437a9ec4519ee5ded496c0d55bc9b1a8b0e8c2b)},thresh(1,pk(30a6069f344fb784a2b4c99540a91ee727c91e3a25ef6aae867d9c65b5f23529),s:pk(9918d400c1b8c3c478340a40117ced4054b6b58f48cdb3c89b836bdfee1f5766))})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{{pkh(1c9bc926084382e76da33b5a52d17b1fa153c072aae5fb5228ecc2ccf89d79d5),pk(0dd6b52b192ab195558d22dd8437a9ec4519ee5ded496c0d55bc9b1a8b0e8c2b)},thresh(1,pk(30a6069f344fb784a2b4c99540a91ee727c91e3a25ef6aae867d9c65b5f23529),s:pk(9918d400c1b8c3c478340a40117ced4054b6b58f48cdb3c89b836bdfee1f5766))})", MISSING_PRIVKEYS | XONLY_KEYS, {{"5120d8ea39b29de2b550b68bd2ada8b075c888c2b2df3290c7a35856482747848934"}}, OutputType::BECH32M);
     // Can have two Miniscripts in a Taproot with mixed private and public keys, and mixed ranged extended keys and raw keys.
-    Check("tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(v:pk(xpub6AGbgdKcAGeUWaGNKH2o3sRvjtvJCGZ1NwrHqMJDwD4bN1QuwPQSsdeAYkPZGPt2FTAyu6nWGsC3fN2nsBELrLPcRNuwwr5k1X7yW5WV4aX/*),pk(02daf6e3477fc3906a1997820ed2940c8f5fa0942946d0368f981b001fdd85afcb)),and_v(v:pk(xprv9wCN7tTqN5ATsmBGEijuNeUgQjma9tv3GmdWLmbYiuArPsAMj6tD1uASiBfm47kdoi7bDBAVxUZNLM2MkeouPK5menDTyCNZtExQrKhVu7C/*),pk(03272c0c1ae2c07528283b91ca57b45d2cc84e7960e1f17f58815372285f35e99a))})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(v:pk(xpub6AGbgdKcAGeUWaGNKH2o3sRvjtvJCGZ1NwrHqMJDwD4bN1QuwPQSsdeAYkPZGPt2FTAyu6nWGsC3fN2nsBELrLPcRNuwwr5k1X7yW5WV4aX/*),pk(02daf6e3477fc3906a1997820ed2940c8f5fa0942946d0368f981b001fdd85afcb)),and_v(v:pk(xpub6ABiXPzjCSim6FFjLkGujnRQxmc4ZMdtdzZ79A1AHEhqGfVWGeCTZhUvZTSf1mNnGUtyNqgfE9eWaYdYReDKbPYqgqi9LLVZSmWnLQRx477/*),pk(03272c0c1ae2c07528283b91ca57b45d2cc84e7960e1f17f58815372285f35e99a))})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(v:pk(xpub6AGbgdKcAGeUWaGNKH2o3sRvjtvJCGZ1NwrHqMJDwD4bN1QuwPQSsdeAYkPZGPt2FTAyu6nWGsC3fN2nsBELrLPcRNuwwr5k1X7yW5WV4aX/*),pk(02daf6e3477fc3906a1997820ed2940c8f5fa0942946d0368f981b001fdd85afcb)),and_v(v:pk(xpub6ABiXPzjCSim6FFjLkGujnRQxmc4ZMdtdzZ79A1AHEhqGfVWGeCTZhUvZTSf1mNnGUtyNqgfE9eWaYdYReDKbPYqgqi9LLVZSmWnLQRx477/*),pk(03272c0c1ae2c07528283b91ca57b45d2cc84e7960e1f17f58815372285f35e99a))})", MISSING_PRIVKEYS | XONLY_KEYS | RANGE | MIXED_PUBKEYS, {{"5120793185cd1a9a0bb710fa57df3845ac4ddf7df63b74beadce2573cbb0b508b3a4"}}, OutputType::BECH32M, /*op_desc_id=*/{}, {{}, {0}});
+    Check("tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(v:pk(xpub6AGbgdKcAGeUWaGNKH2o3sRvjtvJCGZ1NwrHqMJDwD4bN1QuwPQSsdeAYkPZGPt2FTAyu6nWGsC3fN2nsBELrLPcRNuwwr5k1X7yW5WV4aX/*),pk(daf6e3477fc3906a1997820ed2940c8f5fa0942946d0368f981b001fdd85afcb)),and_v(v:pk(xprv9wCN7tTqN5ATsmBGEijuNeUgQjma9tv3GmdWLmbYiuArPsAMj6tD1uASiBfm47kdoi7bDBAVxUZNLM2MkeouPK5menDTyCNZtExQrKhVu7C/*),pk(272c0c1ae2c07528283b91ca57b45d2cc84e7960e1f17f58815372285f35e99a))})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(v:pk(xpub6AGbgdKcAGeUWaGNKH2o3sRvjtvJCGZ1NwrHqMJDwD4bN1QuwPQSsdeAYkPZGPt2FTAyu6nWGsC3fN2nsBELrLPcRNuwwr5k1X7yW5WV4aX/*),pk(daf6e3477fc3906a1997820ed2940c8f5fa0942946d0368f981b001fdd85afcb)),and_v(v:pk(xpub6ABiXPzjCSim6FFjLkGujnRQxmc4ZMdtdzZ79A1AHEhqGfVWGeCTZhUvZTSf1mNnGUtyNqgfE9eWaYdYReDKbPYqgqi9LLVZSmWnLQRx477/*),pk(272c0c1ae2c07528283b91ca57b45d2cc84e7960e1f17f58815372285f35e99a))})", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,{and_v(v:pk(xpub6AGbgdKcAGeUWaGNKH2o3sRvjtvJCGZ1NwrHqMJDwD4bN1QuwPQSsdeAYkPZGPt2FTAyu6nWGsC3fN2nsBELrLPcRNuwwr5k1X7yW5WV4aX/*),pk(daf6e3477fc3906a1997820ed2940c8f5fa0942946d0368f981b001fdd85afcb)),and_v(v:pk(xpub6ABiXPzjCSim6FFjLkGujnRQxmc4ZMdtdzZ79A1AHEhqGfVWGeCTZhUvZTSf1mNnGUtyNqgfE9eWaYdYReDKbPYqgqi9LLVZSmWnLQRx477/*),pk(272c0c1ae2c07528283b91ca57b45d2cc84e7960e1f17f58815372285f35e99a))})", MISSING_PRIVKEYS | XONLY_KEYS | RANGE | MIXED_PUBKEYS, {{"5120793185cd1a9a0bb710fa57df3845ac4ddf7df63b74beadce2573cbb0b508b3a4"}}, OutputType::BECH32M, /*op_desc_id=*/{}, {{}, {0}});
     // Can sign for a Miniscript expression containing a hash challenge inside a Taproot tree. (Fails without the
     // preimages and the sequence, passes with.)
@@ -1319,4 +1323,28 @@ BOOST_AUTO_TEST_CASE(descriptor_literal_null_byte)
 }

+BOOST_AUTO_TEST_CASE(descriptor_canonical_string)
+{
+    const auto parse = [](const std::string& desc) {
+        FlatSigningProvider keys;
+        std::string err;
+        auto descs = Parse(desc, keys, err, /*require_checksum=*/false);
+        BOOST_REQUIRE_MESSAGE(!descs.empty(), err);
+        return std::move(descs.at(0));
+    };
+    const std::string key{"79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798"};
+    const std::string key2{"c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5"};
+
+    // In tr() context only the x coordinate of a key is used, so the canonical string drops the parity byte of a 33 byte key,
+    // while the public string keeps the key as given.
+    for (const std::string& prefix : {"02", "03"}) {
+        BOOST_CHECK_EQUAL(parse("tr(" + prefix + key + ")")->ToCanonicalString(), parse("tr(" + key + ")")->ToCanonicalString());
+        BOOST_CHECK_EQUAL(parse("tr(" + key + ",pk(" + prefix + key2 + "))")->ToCanonicalString(), parse("tr(" + key + ",pk(" + key2 + "))")->ToCanonicalString());
+        BOOST_CHECK(parse("tr(" + prefix + key + ")")->ToString().starts_with("tr(" + prefix + key + ")"));
+    }
+    // Outside of tr(), and for musig() participants, the parity byte matters
+    BOOST_CHECK_NE(parse("pkh(02" + key + ")")->ToCanonicalString(), parse("pkh(03" + key + ")")->ToCanonicalString());
+    BOOST_CHECK_NE(parse("tr(musig(02" + key + ",02" + key2 + "))")->ToCanonicalString(), parse("tr(musig(03" + key + ",02" + key2 + "))")->ToCanonicalString());
+}
+
 BOOST_AUTO_TEST_CASE(descriptor_older_warnings)
 {

Could be simplified further depending on how you want to handle public and compat for this.

achow101 commented at 8:29 PM on September 7, 2026:

But Claude caught missing handling of the x-only parity byte.

I'm going to consider that to be not a problem. This starts to go down the path of whether 2 expressions that produce the same key are equivalent, which we are intentionally not accounting for.

Sjors commented at 1:13 PM on September 8, 2026:

Indeed e.g. having the xpub at a different derivation path is also something we don't disambiguate.

However, if the goal of this PR is to allow older wallets to open new wallet with miniscript, then the narrow fix and test coverage here may still be worth it.

achow101 commented at 6:30 PM on September 8, 2026:

I would not say that this fix is narrow. Further, changing CANONICAL has no bearing on whether old (or new) wallets can be opened. CANONICAL is only used to check for duplication in imports.

davidgumberg commented at 11:32 PM on September 8, 2026:

+1 to not fixing this, I think it's better to take the approach that the wallet is resilient to equivalent but distinct descriptors rather than trying to guard against equivalent descriptors being imported which seems unbounded.

in src/script/descriptor.cpp:263 in 35d6a60dbf

 259 | @@ -259,7 +260,7 @@ class OriginPubkeyProvider final : public PubkeyProvider
 260 |      std::string OriginString(StringType type, bool normalized=false) const
 261 |      {
 262 |          // If StringType==COMPAT, always use the apostrophe to stay compatible with previous versions
 263 | -        bool use_apostrophe = (!normalized && m_apostrophe) || type == StringType::COMPAT;
 264 | +        bool use_apostrophe = (type != StringType::CANONICAL && !normalized && m_apostrophe) || type == StringType::COMPAT;

Sjors commented at 12:51 PM on September 3, 2026:

In 35d6a60dbf5a3424804430041f5f4091efb82b6c descriptor: Add ToCanonicalString: can you expand the comment to e.g. say:

If StringType::CANONICAL, always use h.

achow101 commented at 8:45 PM on September 7, 2026:

If I need to retouch

davidgumberg commented at 6:29 PM on September 8, 2026:

In https://github.com/bitcoin/bitcoin/pull/35445/changes/35d6a60dbf5a3424804430041f5f4091efb82b6c (descriptor: Add ToCanonicalString)

nit: I think this is more readable and more correct if each StringType is handled explicitly with e.g. a switch

        bool use_apostrophe;
        switch (type) {
            case StringType::COMPAT:
                use_apostrophe = true;
                break;
            case StringType::PUBLIC:
                use_apostrophe = (!normalized && m_apostrophe) ? true : false;
                break;
            case StringType::CANONICAL:
                use_apostrophe = false;
                break;
        } // no default case, so the compiler can warn about missing cases

Using != StringType::CANONICAL leaves a footgun if another StringType is added in the future.

Also below.

davidgumberg commented at 6:34 PM on September 8, 2026:

If leaving as-is, this one should be marked const as below

achow101 commented at 7:34 PM on September 8, 2026:

If I retouch

in test/functional/wallet_backwards_compatibility.py:391 in 770ff64bd7

 387 |                          assert_equal(info['keypoolsize'], 0)
 388 | +                    elif wallet_name == "miniscript":
 389 | +                        for desc in wallet.listdescriptors()["descriptors"]:
 390 | +                            if desc["desc"].startswith("wsh(or_b(pk"):
 391 | +                                break
 392 | +                        else:

Sjors commented at 1:00 PM on September 3, 2026:

In 770ff64bd7fd52e2c3d2634dc55046c5137cbe86 test: Add v30.2 and Miniscript to wallet backwards compatibility test: possibly more readable:

descs = wallet.listdescriptors()["descriptors"]
assert any(desc["desc"].startswith("wsh(or_b(pk") for desc in descs), "Miniscript descriptor missing"

achow101 commented at 8:45 PM on September 7, 2026:

If I need to retouch

in src/wallet/walletutil.h:132 in 62e826fa76

 127 | @@ -128,6 +128,8 @@ class WalletDescriptor
 128 |        descriptor(descriptor),
 129 |        id(DescriptorID(*descriptor)),
 130 |        creation_time(creation_time) {}
 131 | +
 132 | +    void UpdateFrom(const WalletDescriptor& other);

Sjors commented at 2:04 PM on September 3, 2026:

In 62e826fa76172210572356eef9fa3cd3309baf56 wallet: Update WalletDescriptor from another one instead of overwriting: it's not very clear what this is trying to do and why, unless the following is correct:

    /** Take over the metadata (range, next index, creation time) and the cache from
     *  another WalletDescriptor for the same descriptor, keeping this object's
     *  descriptor and id unchanged.
     *
     *  Used when a descriptor is reimported. The parsed descriptor is deliberately not
     *  replaced: the reimported string may be an equivalent but different spelling
     *  (e.g. ' instead of h for hardened derivation, or a private instead of public
     *  key), and rewriting the on-disk record with it could produce a string that an
     *  older version cannot load, or that no longer hashes to the id the record is
     *  stored under.
     */

But why not keep everything else unchanged, or just refuse to replace it?

achow101 commented at 8:33 PM on September 7, 2026:

I don't understand what is unclear. The commit message says what's happening

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.

Sjors commented at 1:09 PM on September 8, 2026:

I worded that a bit strange. My suggestion is to take the comment (if it's correct), so the function is documented without needing git blame. Without documentation UpdateFrom is not intuitive.

achow101 commented at 6:29 PM on September 8, 2026:

If I need to retouch.

in test/functional/wallet_backwards_compatibility.py:44 in 6ad31c062c

  40 | +        self.num_nodes = 10
  41 |          # Add new version after each release:
  42 |          self.extra_args = [
  43 |              ["-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to mine blocks. noban for immediate tx relay
  44 |              ["-nowallet", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to receive coins, swap wallets, etc
  45 | +            ["-nowallet", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v31.0

Sjors commented at 2:17 PM on September 3, 2026:

In 6ad31c062c70101fe6463fe516de7edc651881be test: Add 31.0 to wallet backwards compatibility test: consider using v31.1 since it's the last point release with the bug.

Also, do you want to backport this to 31.x?

achow101 commented at 8:45 PM on September 7, 2026:

If I need to retouch

in test/functional/wallet_importdescriptors.py:348 in a2d001b57c

 344 | @@ -345,11 +345,14 @@ def run_test(self):
 345 |          assert_equal(w1.getwalletinfo()['keypoolsize'], 0)
 346 |  
 347 |          self.log.info("Test can import same descriptor with public key twice")
 348 | +        list_descs = w1.listdescriptors()

Sjors commented at 2:35 PM on September 3, 2026:

In a2d001b57c5f7adc6649e96fc2254d729f4ba750 test: Enforce descriptor reimport is an update: maybe make it the first commit (where it passes), and rename to test: check descriptor reimport leaves listdescriptors unchanged

achow101 commented at 8:45 PM on September 7, 2026:

If I need to retouch

in src/script/descriptor.cpp:1668 in 1c7f9aaf75

1660 | @@ -1661,7 +1661,11 @@ class StringMaker {
1661 |              if (!m_pubkeys[key]->ToNormalizedString(*m_arg, ret, m_cache)) return {};
1662 |              break;
1663 |          case DescriptorImpl::StringType::COMPAT:
1664 | -            ret = m_pubkeys[key]->ToString(PubkeyProvider::StringType::COMPAT);
1665 | +            // For backwards compatibility, we do not pass StringType::COMPAT.
1666 | +            // Prior to 31.0, COMPAT was not provided, so PUBLIC was in use. From this string,
1667 | +            // DescriptorSPKM IDs were computed from this string, so the incorrect behavior
1668 | +            // must be preserved for wallets with Miniscript descriptors to be loaded
1669 | +            ret = m_pubkeys[key]->ToString();

Sjors commented at 2:46 PM on September 3, 2026:

Are our backward compatible tests thorough enough to prevent any future change to PUBLIC serialization from re-introducing this issue? It might be better to just stick to COMPAT and document that (rare) v31+ miniscript wallets can't be opened by old software.

achow101 commented at 8:39 PM on September 7, 2026:

Are our backward compatible tests thorough enough to prevent any future change to PUBLIC serialization from re-introducing this issue?

Yes. It should be impossible for any future serialization changes from preventing future versions from opening wallets created in previous versions.

It might be better to just stick to COMPAT and document that (rare) v31+ miniscript wallets can't be opened by old software.

I disagree.

Sjors commented at 2:55 PM on September 3, 2026: member

Concept ACK, although I would be fine, maybe even prefer, with making miniscript wallets not-downgradable.

Code review ec2adf3c51ca7322307be3d052bc0e9fa4332dd2.

jeanpablojp commented at 8:50 PM on September 4, 2026: contributor

Concept ACK

Ran wallet_backwards_compatibility against the eight previous releases, and 30.2 opens the miniscript wallet created on master, including after the reimport with the other spelling.

AddScriptPubKeyMan assigns into m_spk_managers[id] without checking what is already there. If a wallet stores descriptor A under an ID that is the hash of some canonically different descriptor B, importing B replaces A. A goes from listdescriptors and from the file with no warning, and the raw pointer left behind in m_cached_spks makes the next lookup of one of its scripts a use-after-free, which ASan puts in CWallet::GetSolvingProvider with the free in AddWalletDescriptor. I could not find a released version that writes that pair. Every ID computation the wallet has had differs only by hardened spelling, which is canonically equal, so I had to move the records onto that ID with sqlite. ApplyMigrationData already guards the same call with m_spk_managers.contains, worth doing here too?

in src/script/descriptor.h:251 in ec2adf3c51

 248 | -*   This is not part of BIP 380, not guaranteed to be interoperable and should not be exposed to the user.
 249 | +/** Hash of the COMPAT string representation of the descriptor that is not supposed to change over time.
 250 | + * Due to the hash's usage in previous versions, the COMPAT string is computed with some quirks.
 251 | + *
 252 | + * The hash is the sha256 of the public descriptor using apostrophes as the hardened indicator, except inside of
 253 | + * Miniscript expressions, where "h" is the hardened indicator.

jeanpablojp commented at 8:50 PM on September 4, 2026:

Inside a Miniscript expression the hash follows the public serialization, not h, because StringMaker passes StringType::PUBLIC in the COMPAT case. wsh(and_v(v:pk([deadbeef/0h/1h]xpub.../*),older(1))) and the same descriptor with 0'/1' hash to different values here. The comment over in StringMaker gets it right, it's only this sentence.

 * The hash is the sha256 of the public descriptor using apostrophes as the hardened indicator, except inside of
 * Miniscript expressions, where the public serialization is used as is.

achow101 commented at 8:45 PM on September 7, 2026:

If I need to retouch

in src/wallet/wallet.cpp:3768 in ec2adf3c51

3764 | @@ -3765,14 +3765,13 @@ void CWallet::DeactivateScriptPubKeyMan(uint256 id, OutputType type, bool intern
3765 |  
3766 |  DescriptorScriptPubKeyMan* CWallet::GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const
3767 |  {
3768 | -    auto spk_man_pair = m_spk_managers.find(desc.id);
3769 | +    auto spk_man_pair = std::find_if(m_spk_managers.begin(), m_spk_managers.end(), [&desc](const auto& item) {

jeanpablojp commented at 8:50 PM on September 4, 2026:

HasWalletDescriptor builds two canonical strings per SPKM visited, and the one for the descriptor being looked up is the same across the whole scan. It also hits the watch-only and solvable loops in DoMigration and the one in ExportWatchOnlyWallet, which call AddWalletDescriptor once per descriptor. Timing migratewallet on a legacy wallet with watch-only scripts, the time this adds over the merge base is about 31s at 3000 scripts and about 122s at 6000, so it nearly quadruples when the count doubles. Computing the looked-up string once outside the find_if removes one of the two serializations per SPKM and came out faster in every run here. Worth doing before the map you mentioned?

achow101 commented at 8:45 PM on September 7, 2026:

I don't think this is worth doing unless people start to complain. Migration is already known to be slow in a variety of other places, and very few people will have wallets affected by this, almost no one is going to have more than a couple watchonly scripts.

in src/script/descriptor.cpp:513 in 35d6a60dbf

 509 | @@ -509,7 +510,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider
 510 |      std::string ToString(StringType type, bool normalized) const
 511 |      {
 512 |          // If StringType==COMPAT, always use the apostrophe to stay compatible with previous versions
 513 | -        const bool use_apostrophe = (!normalized && m_apostrophe) || type == StringType::COMPAT;
 514 | +        const bool use_apostrophe = (type != StringType::CANONICAL && !normalized && m_apostrophe) || type == StringType::COMPAT;

davidgumberg commented at 6:33 PM on September 8, 2026:

higher-level remark: it seems quite odd to me that the StringType is distinct from normalized, I'm not sure I follow why there are multiple dimensions for callers to pass information about how they want the string returned, given that normalized is only relevant to StringType::PUBLIC it seems like it would be better to split that into two string types.

achow101 commented at 6:53 PM on September 8, 2026:

ToNormalizedString requires passing either private keys or a cache in order to derive keys at hardened paths. The normalized string may not be possible to create if either of those are not given. In contrast, ToString does not do any key derivation and can always produce a string, and the StringTypes are to control which kind of always-producable string is created.

w0xlt commented at 6:57 PM on September 8, 2026: contributor

ACK ec2adf3c51ca7322307be3d052bc0e9fa4332dd2

DrahtBot requested review from Sjors on Sep 8, 2026

DrahtBot requested review from jeanpablojp on Sep 8, 2026

davidgumberg commented at 11:39 PM on September 8, 2026: contributor

crACK https://github.com/bitcoin/bitcoin/commit/ec2adf3c51ca7322307be3d052bc0e9fa4332dd2

I think as a bundle it would be worth addressing the reviewer comments that the author has offered to do if retouching, but none of them are important enough on their own to block this PR so I think a follow-up is a good idea here.