compiler: reject policy keys invalid in the target script context - #1044
portlandhodl wants to merge 2 commits into
Conversation
|
Thanks for finding this! Yeah, this was definitely not an intentional regression. Can you
|
dc94cfa to
8b93927
Compare
| } | ||
|
|
||
| #[doc(hidden)] | ||
| impl From<ScriptContextError> for CompilerError { |
There was a problem hiding this comment.
In d2b2290:
Please drop this from impl and add a .map_err(CompilerError::ContextError wherever it's used. These From impls hide error conversions behind ?s, making the code harder to read and follow, plus they're an API commitment that doc(hidden) does little to absolve us of. (Users won't avoid this because it's missing from docs ... they'll just type ? and it'll "just work" and they'll expect it to continue working.)
There was a problem hiding this comment.
Thanks for the solid reasoning behind this! Easy enough as a change to make. Will be more cognizant of this going forward.
Compiling a policy containing a key that is not valid in the target script context (e.g. an x-only key under Segwitv0/Legacy/Bare, or an uncompressed key under Segwitv0/Tap) used to panic, and since the compiler rewrite it silently produced a Miniscript that is invalid for that context, such as wsh(pkh(xonly)) whose outputs are unspendable. Check Ctx::check_pk() when compiling Key fragments and return a new CompilerError::ContextError instead. The check sits at the single point where keys become terminals, so multi/multi_a fragments and all public compilation entry points are covered as well. Assisted-by: Kimi kimi-latest
X-only keys must not compile under Segwitv0/Legacy/Bare (but are fine under Tap), and uncompressed keys must not compile under Segwitv0/Tap (but are fine under Legacy). Adapted from the test case in rust-bitcoin#761. Closes rust-bitcoin#761 Co-authored-by: Nadav Ivgi <nadav@shesek.info> Assisted-by: Kimi kimi-latest
8b93927 to
545c89a
Compare
|
|
Oh, #991 fixes this bug. I'd kinda prefer to close this in favor of that, since this one introduces new usage of I will cherry-pick your tests (or at least check that they're covered elsewhere). |
|
More than fine. |
2 participants
Supersedes and closes #761.
Concrete::pk(xonly).compile::<Segwitv0>()used to panic (Terminal creation must always succeed, reported in #761). Since the compiler rewrite (ea221af, part of the recent compiler refactor series), it no longer panics — it silently returnsOkwith a Miniscript that is invalid for the target context, e.g.wsh(pkh(<32-byte x-only key>)): a fundable address whose coins can never be spent (a 32-byte key is not a valid segwit-v0 ECDSA pubkey, soCHECKSIGcan never succeed). Verified end-to-end against Bitcoin Core 26.0 consensus (libbitcoinconsensus): funding succeeds, and the owner's spend with the correct private key and a valid signature is rejected.The same hole lets uncompressed keys compile under Segwitv0/Tap, and x-only keys under Legacy/Bare.
Fix: call
Ctx::check_pkwhen compilingConcrete::Keyfragments — the single point where policy keys become terminals, somulti/multi_aand all public entry points (compile,compile_to_descriptor,compile_tr*) are covered. Invalid keys now produceCompilerError::ContextError, reusing the informativeScriptContextErrormessages from #760. (CompilerErrorloses itsCopy/Hashderives sinceScriptContextErrorhas neither; nothing in-tree relied on them.)Test:
context_invalid_keyscovers x-only keys under Segwitv0/Legacy/Bare (Err) and Tap (Ok), themultipath, and uncompressed keys under Segwitv0/Tap (Err) and Legacy (Ok). Adapted from #761; @shesek is credited as co-author.