miniscript: Use a reference to key_exp_index in KeyParser
What the developer wrote
Authored by Ava Chow
miniscript: Use a reference to key_exp_index in KeyParser
For key_exp_index to count correctly for miniscript expressions,
KeyParser should hold a reference to it.
What changed, and why it matters
This change fixes how Bitcoin Core counts key expressions when parsing Miniscript descriptors. Previously, the parser kept a private copy of a counter (m_offset), so updates inside the parser did not propagate back to the caller. By switching to a shared reference (m_expr_index), the caller's counter stays correct across nested or repeated Miniscript parsing. A wrong counter could lead to incorrect descriptor metadata, which in wallet software might cause addresses, derivation paths, or key identifiers to be mis-indexed. There is no direct remote exploit shown in the diff, but the bug could affect wallet correctness and user funds if a descriptor relying on the counter is used.
Review related descriptor parsing tests to ensure key_exp_index is correctly incremented across nested miniscripts, multipath descriptors, and repeated ParseScript calls. Add regression tests that verify key expression indices round-trip correctly. Consider auditing other parser state that is passed by value instead of by reference for similar synchronization issues.
Security signals we found
State synchronization bug between parser and caller
Descriptor parsing correctness issue
Potential key index miscounting in Miniscript descriptors
No explicit bounds check or overflow handling visible in the diff
Evidence from the diff
The patch changes KeyParser in src/script/descriptor.cpp so that m_offset (a copied uint32_t) becomes m_expr_index (a reference to the caller’s uint32_t). Previously, ParseScript passed key_exp_index by value as an offset, and after parsing a miniscript it manually added parser.m_keys.size() to key_exp_index. That manual bump is removed because the parser now increments the caller’s variable directly during Key(). InferScript is updated to provide a local key_exp_index variable. The bug being fixed is that the old code could miscount key expression indices when miniscript parsing was nested or when the parser was used in contexts where the caller expected live updates. Correct indexing matters for descriptor expansion, key derivation path reporting, and wallet descriptor round-tripping.
Changed components
src/script/descriptor.cppKeyParser structParseScript functionInferScript functionMiniscript descriptor parsingInspect captured patch +8 / −8
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 2be3301b..d1780592 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -2183,12 +2183,12 @@ struct KeyParser {
mutable std::string m_key_parsing_error;
//! The script context we're operating within (Tapscript or P2WSH).
const miniscript::MiniscriptContext m_script_ctx;
- //! The number of keys that were parsed before starting to parse this Miniscript descriptor.
- uint32_t m_offset;
+ //! The current key expression index
+ uint32_t& m_expr_index;
KeyParser(FlatSigningProvider* out LIFETIMEBOUND, const SigningProvider* in LIFETIMEBOUND,
- miniscript::MiniscriptContext ctx, uint32_t offset = 0)
- : m_out(out), m_in(in), m_script_ctx(ctx), m_offset(offset) {}
+ miniscript::MiniscriptContext ctx, uint32_t& key_exp_index LIFETIMEBOUND)
+ : m_out(out), m_in(in), m_script_ctx(ctx), m_expr_index(key_exp_index) {}
bool KeyCompare(const Key& a, const Key& b) const {
return *m_keys.at(a).at(0) < *m_keys.at(b).at(0);
@@ -2206,8 +2206,8 @@ struct KeyParser {
{
assert(m_out);
Key key = m_keys.size();
- uint32_t exp_index = m_offset + key;
- auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
+ auto pk = ParsePubkey(m_expr_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
+ ++m_expr_index;
if (pk.empty()) return {};
m_keys.emplace_back(std::move(pk));
return key;
@@ -2634,7 +2634,6 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
// A signature check is required for a miniscript to be sane. Therefore no sane miniscript
// may have an empty list of public keys.
CHECK_NONFATAL(!parser.m_keys.empty());
- key_exp_index += parser.m_keys.size();
// Make sure all vecs are of the same length, or exactly length 1
// For length 1 vectors, clone subdescs until vector is the same length
size_t num_multipath = std::max_element(parser.m_keys.begin(), parser.m_keys.end(),
@@ -2809,7 +2808,8 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
if (ctx == ParseScriptContext::P2WSH || ctx == ParseScriptContext::P2TR) {
const auto script_ctx{ctx == ParseScriptContext::P2WSH ? miniscript::MiniscriptContext::P2WSH : miniscript::MiniscriptContext::TAPSCRIPT};
- KeyParser parser(/* out = */nullptr, /* in = */&provider, /* ctx = */script_ctx);
+ uint32_t key_exp_index = 0;
+ KeyParser parser(/* out = */nullptr, /* in = */&provider, /* ctx = */script_ctx, key_exp_index);
auto node = miniscript::FromScript(script, parser);
if (node && node->IsSane()) {
std::vector<std::unique_ptr<PubkeyProvider>> keys;
Why this scored 47/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.