diff --git a/CHANGELOG.md b/CHANGELOG.md index 916b581a..bce99feb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ page. See [DEVELOPMENT_CYCLE.md](DEVELOPMENT_CYCLE.md) for more details. - Rejected `--proxy` on the `rpc` backend, and unsupported proxy options (`--proxy_auth`, `--timeout`) on the `rpc` and `cbf` backends, instead of silently ignoring them - Fixed `compile` rejecting policies that are valid for the requested script type - Fixed `warn!` messages, including the mainnet notice, being hidden unless `RUST_LOG` was set; warnings are now shown by default +- Fixed `compile` silently accepting placeholder keys/hashes instead of validating them; added `--placeholders` to opt back into the old permissive behavior ## [4.0.0] diff --git a/src/handlers/descriptor.rs b/src/handlers/descriptor.rs index ba5005c5..c1460a66 100644 --- a/src/handlers/descriptor.rs +++ b/src/handlers/descriptor.rs @@ -22,7 +22,12 @@ use { key::{Parity, rand}, secp256k1::{PublicKey, Scalar, Secp256k1, SecretKey}, }, - miniscript::{Descriptor, descriptor::TapTree, policy::Concrete}, + miniscript::{ + Descriptor, ForEachKey, FromStrKey, Legacy, MiniscriptKey, ScriptContext, Segwitv0, + Tap, + descriptor::{DescriptorPublicKey, TapTree}, + policy::Concrete, + }, }, std::{str::FromStr, sync::Arc}, }; @@ -80,6 +85,11 @@ pub struct CompileCommand { default_value_t = ScriptType::Wsh )] script_type: ScriptType, + /// Skip key and hash validation, accepting placeholders (e.g. pk(A)). Useful for + /// inspecting a policy's shape without real keys. Descriptors compiled with this + /// flag are not usable for spending. + #[arg(long = "placeholders")] + placeholders: bool, } /// Script types the compiled policy can be embedded in. @@ -92,48 +102,99 @@ pub enum ScriptType { Tr, } +#[cfg(feature = "compiler")] +/// Rejects keys that parse fine on their own but aren't valid for `Ctx` (e.g. an +/// x-only key under `Segwitv0`), which miniscript's compiler otherwise turns into +/// a panic instead of an error. A no-op for `Pk = String`, since `String` never +/// reports itself as uncompressed or x-only. +fn check_policy_keys( + policy: &Concrete, +) -> Result<(), Error> { + let mut key_error = None; + policy.for_each_key(|pk| match Ctx::check_pk(pk) { + Ok(()) => true, + Err(e) => { + key_error = Some(e); + false + } + }); + + match key_error { + Some(e) => Err(Error::Generic(format!("Invalid policy: {e}"))), + None => Ok(()), + } +} + +#[cfg(feature = "compiler")] +/// Parses `policy_str` as `Pk` and compiles it for `script_type`. Returns the +/// descriptor string and, for `tr`, the random tweak `r` used to derive the +/// unspendable internal key so the caller can verify it. +fn compile_policy( + policy_str: &str, + script_type: ScriptType, +) -> Result<(String, Option), Error> { + let policy: Concrete = Concrete::from_str(policy_str) + .map_err(|e| Error::Generic(format!("Invalid policy: {e}")))?; + + let mut r = None; + + // Compile per branch, not once up front: the contexts have different script + // limits, and the narrowest one would reject policies valid for the requested type. + let descriptor = match script_type { + ScriptType::Sh => { + check_policy_keys::(&policy)?; + Descriptor::new_sh(policy.compile()?) + } + ScriptType::Wsh => { + check_policy_keys::(&policy)?; + Descriptor::new_wsh(policy.compile()?) + } + ScriptType::ShWsh => { + check_policy_keys::(&policy)?; + Descriptor::new_sh_wsh(policy.compile()?) + } + ScriptType::Tr => { + check_policy_keys::(&policy)?; + // Use a randomized unspendable internal key (H + rG) instead of a fixed NUMS + // point. This improves privacy by preventing observers from determining whether + // key-path spending is disabled. `r` is returned so the user can verify the key + // is derived from the NUMS point. See BIP-341. + let secp = Secp256k1::new(); + let r_secret = SecretKey::new(&mut rand::thread_rng()); + r = Some(r_secret.display_secret().to_string()); + + let nums_key = XOnlyPublicKey::from_str(NUMS_UNSPENDABLE_KEY_HEX) + .map_err(|e| Error::Generic(format!("Invalid NUMS key: {e}")))?; + let nums_point = PublicKey::from_x_only_public_key(nums_key, Parity::Even); + + let internal_key_point = nums_point + .add_exp_tweak(&secp, &Scalar::from(r_secret)) + .map_err(|e| Error::Generic(format!("Failed to tweak NUMS key: {e}")))?; + let (xonly_internal_key, _) = internal_key_point.x_only_public_key(); + + let tree = TapTree::Leaf(Arc::new(policy.compile()?)); + let internal_key = Pk::from_str(&xonly_internal_key.to_string()) + .map_err(|e| Error::Generic(format!("Failed to parse internal key: {e}")))?; + Descriptor::new_tr(internal_key, Some(tree)) + } + }?; + + Ok((descriptor.to_string(), r)) +} + #[cfg(feature = "compiler")] impl AppCommand> for CompileCommand { type Output = DescriptorResult; fn execute(&self, _ctx: &mut AppContext) -> Result { - let policy: Concrete = Concrete::from_str(&self.policy) - .map_err(|e| Error::Generic(format!("Invalid policy: {e}")))?; - - let mut r = None; - - // Compile per branch, not once up front: the contexts have different script - // limits, and the narrowest one would reject policies valid for the requested type. - let descriptor = match self.script_type { - ScriptType::Sh => Descriptor::new_sh(policy.compile()?), - ScriptType::Wsh => Descriptor::new_wsh(policy.compile()?), - ScriptType::ShWsh => Descriptor::new_sh_wsh(policy.compile()?), - ScriptType::Tr => { - // Use a randomized unspendable internal key (H + rG) instead of a fixed NUMS - // point. This improves privacy by preventing observers from determining whether - // key-path spending is disabled. `r` is returned so the user can verify the key - // is derived from the NUMS point. See BIP-341. - let secp = Secp256k1::new(); - let r_secret = SecretKey::new(&mut rand::thread_rng()); - r = Some(r_secret.display_secret().to_string()); - - let nums_key = XOnlyPublicKey::from_str(NUMS_UNSPENDABLE_KEY_HEX) - .map_err(|e| Error::Generic(format!("Invalid NUMS key: {e}")))?; - let nums_point = PublicKey::from_x_only_public_key(nums_key, Parity::Even); - - let internal_key_point = - nums_point - .add_exp_tweak(&secp, &Scalar::from(r_secret)) - .map_err(|e| Error::Generic(format!("Failed to tweak NUMS key: {e}")))?; - let (xonly_internal_key, _) = internal_key_point.x_only_public_key(); - - let tree = TapTree::Leaf(Arc::new(policy.compile()?)); - Descriptor::new_tr(xonly_internal_key.to_string(), Some(tree)) - } - }?; + let (descriptor, r) = if self.placeholders { + compile_policy::(&self.policy, self.script_type)? + } else { + compile_policy::(&self.policy, self.script_type)? + }; Ok(DescriptorResult { - descriptor: Some(descriptor.to_string()), + descriptor: Some(descriptor), mnemonic: None, multipath_descriptor: None, public_descriptors: None, diff --git a/tests/integration/init.rs b/tests/integration/init.rs index 12086ff6..010844f5 100644 --- a/tests/integration/init.rs +++ b/tests/integration/init.rs @@ -223,6 +223,10 @@ mod test_compile { /// A policy can be valid for tr or wsh type and still exceed the limits of the /// legacy context, whose 520-byte redeemScript cap does not apply to it. + /// + /// Uses placeholder keys (K01, K02, ...) since only the policy's shape, not its + /// keys, matters for this test - `--placeholders` keeps that shape-only + /// intent even though real key validation is on by default. #[test] fn test_compile_policy_beyond_legacy_limits() { let temp_dir = TempDir::new().unwrap(); @@ -235,13 +239,13 @@ mod test_compile { let policy = format!("thresh(2,{keys})"); // compile tr - cli.cmd("compile", &[&policy, "--type", "tr"]) + cli.cmd("compile", &[&policy, "--type", "tr", "--placeholders"]) .assert() .success() .stdout(predicate::str::contains("tr(")); // compile wsh - cli.cmd("compile", &[&policy, "--type", "wsh"]) + cli.cmd("compile", &[&policy, "--type", "wsh", "--placeholders"]) .assert() .success() .stdout(predicate::str::contains("wsh(")); @@ -277,6 +281,86 @@ mod test_compile { .failure() .stderr(predicate::str::contains("Invalid policy")); } + + #[test] + fn test_compile_rejects_placeholder_key_by_default() { + let temp_dir = TempDir::new().unwrap(); + let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf())); + + cli.cmd("compile", &["pk(ABC)", "--type", "wsh"]) + .assert() + .failure() + .stderr(predicate::str::contains("Invalid policy")); + } + + #[test] + fn test_compile_rejects_placeholder_hash_by_default() { + let temp_dir = TempDir::new().unwrap(); + let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf())); + + let policy = + "and(pk(02e5b88fdb71c696e1a473f309a47535b7190e21a22bd25e7fc8bd055db3bba12f),sha256(H))"; + + cli.cmd("compile", &[policy, "--type", "wsh"]) + .assert() + .failure() + .stderr(predicate::str::contains("Invalid policy")); + } + + #[test] + fn test_compile_allows_placeholder_key_with_flag() { + let temp_dir = TempDir::new().unwrap(); + let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf())); + + cli.cmd("compile", &["pk(ABC)", "--type", "wsh", "--placeholders"]) + .assert() + .success() + .stdout(predicate::str::contains("wsh(pk(ABC))")); + } + + #[test] + fn test_compile_accepts_xpub_key() { + let temp_dir = TempDir::new().unwrap(); + let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf())); + + let policy = "pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8/0/*)"; + + cli.cmd("compile", &[policy, "--type", "wsh"]) + .assert() + .success() + .stdout(predicate::str::contains("wsh(")); + } + + /// An x-only key is only valid under `tr`. Compiling it as a real key for + /// `sh`/`wsh`/`sh-wsh` must return an error. Without the context check, + /// miniscript's compiler panics when `DescriptorPublicKey` reports an x-only key. + #[test] + fn test_compile_rejects_x_only_key_for_non_tr_types() { + let temp_dir = TempDir::new().unwrap(); + let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf())); + + let policy = "pk(50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0)"; + + for script_type in ["sh", "wsh", "sh-wsh"] { + cli.cmd("compile", &[policy, "--type", script_type]) + .assert() + .failure() + .stderr(predicate::str::contains("Invalid policy")); + } + } + + #[test] + fn test_compile_accepts_x_only_key_for_tr() { + let temp_dir = TempDir::new().unwrap(); + let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf())); + + let policy = "pk(50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0)"; + + cli.cmd("compile", &[policy, "--type", "tr"]) + .assert() + .success() + .stdout(predicate::str::contains("tr(")); + } } // --- CONFIG COMMAND TESTS ---