Repository navigation
Conversation
When a leaf of the Taproot tree doesn't compile, compile_tr and compile_tr_native return the error, but compile_tr_private_experimental unwraps it and panics. Return the error, like the other two do. The next commit makes a leaf with an uncompressed key fail to compile, which would make this function panic. Assisted-by: Claude Opus 5.5
check_global_consensus_validity checks the keys of pk_k() and multi() against the script context, but not the key of pk_h(). So pkh() accepts keys that pk() rejects: x-only keys outside Taproot, and uncompressed keys in Segwit v0 and Taproot. Descriptor parsing relies on this check, so wsh(pkh(<uncompressed key>)), wsh(pkh(<x-only key>)), sh(pkh(<x-only key>)) and the bare c:pk_h(<x-only key>) parse, while the same keys in pk() don't. Bitcoin Core rejects the wsh() and sh() ones because of the key. The compiler tries both pk_k() and pk_h() for a key and drops the fragments that fail this check. When pk_k() fails, pk_h() remains, so compiling pk(<uncompressed key>) for wsh() gives wsh(pkh(<uncompressed key>)). Such an output can't be spent with a standard transaction, since Segwit v0 signature checks require compressed keys. For a Taproot leaf, the compiler gives tr(K,pkh(<uncompressed key>)), which doesn't parse. Check the key of pk_h() like the key of pk_k() in all four contexts. Terminal::RawPkH holds only a hash, so it stays unchecked. Miniscript::from_str and tr() leaves already rejected these keys in validate(). They now fail earlier, with a context error instead of a validation error. As with pk_k(), Miniscript::from_ast, translate_pk and parsing with permissive ValidationParams now reject such keys in pk_h() too. Assisted-by: Claude Opus 5.5
Parse pkh() with compressed, uncompressed and x-only keys in every context. Check that compiling pk() with a key that the context doesn't allow fails instead of falling back to pk_h(), in every Taproot compile function too. Assisted-by: Claude Opus 5.5
|
Concept NACK 1ce63da. This will just cause conflicts with our efforts to remove the script context. |
|
@apoelstra Makes sense. Does the same apply to #1069? It calls the existing context checks from the descriptor constructors. I'm going to close this PR if there are no objections and rework #954, adding a descriptor-level check: a non-Taproot descriptor must not contain a musig() key. Do you think this is the right approach? The tests from this PR can go into #991 if they're useful. |
|
@starius yeah, same applies to #1069. I think both should be closed. We should file issues to make sure we don't lose track of these bugs, though I hope that between differential fuzzing and LLM analysis we can find them quickly if they exist. I think the right approach for musig keys is to add a new |
3 participants
Merge after #1069. Without it, the descriptor constructors still accept
pk_h()keys that the context doesn't allow, andinto_definite()then panics on them.Before: the script context checks looked at the key of
pk_k()but notpk_h(). Sowsh(pkh(<uncompressed key>))andsh(pkh(<x-only key>))parsed, and the compiler fell back topk_h()when a key failed thepk_k()check.After:
pk_h()keys are checked likepk_k()keys, so these descriptors fail to parse and the compiler returns an error. The first commit makescompile_tr_private_experimentalreturn a leaf's compile error instead of panicking, since the new check makes thatunwrap()reachable.Split out of #954. #991 will replace these checks eventually.
🤖 AI disclosure: the patch was generated with Claude Opus 5.5 and reviewed with Codex.