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 for information on the review process.

Type	Reviewers
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:

#bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
#35444 (wallet: make descriptor SPKM mutex non-recursive by w0xlt)
#35429 (wallet: avoid global access in external signer SPKM by w0xlt)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

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:

desdescriptors -> descriptors [misspelled word in the comment about Miniscript string handling]

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.

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

Reviewing.

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.

31a4350824

test: Add v30.2 and Miniscript to wallet backwards compatibility test 15cc6128a5

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.

4cb85d3062

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.

749097b9f9

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.

053eae1ec1

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.

b0967280ea

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

test: Enforce descriptor reimport is an update c8b68e1e44

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

 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.