Validate policy keys and hashes before compiling descriptors

CHANGELOG.md

@@ -17,6 +17,8 @@ page. See [DEVELOPMENT_CYCLE.md](DEVELOPMENT_CYCLE.md) for more details.
 - Added `hwi` feature with a hardware wallet `hwi` command to list connected devices, register a wallet policy, display a receive address, and sign PSBTs. Supports Ledger, Coldcard, BitBox02, Jade and Specter.
 - Replaced plain `wallets` with `wallets list`, and added `wallets delete <wallet_name>` for unused saved wallet configurations.
 - Removed the redundant `wallet` field from `wallets list` output; the wallet name is still available as the top-level key
+- Fixed `compile` silently accepting placeholder keys/hashes instead of validating them; added `--placeholders` to opt back into the old permissive behavior
+- Fixed `compile` panicking instead of returning an error when a key is valid on its own but the wrong shape for the requested script type (e.g. an x-only key under `sh`/`wsh`/`sh-wsh`, or an uncompressed key under `tr`)
 
 ## [4.0.0]
 

src/handlers/descriptor.rs

@@ -13,27 +13,14 @@ use crate::{
 };
 use clap::Parser;
 #[cfg(feature = "compiler")]
-use clap::ValueEnum;
-#[cfg(feature = "compiler")]
 use {
-    bdk_wallet::{
-        bitcoin::{
-            XOnlyPublicKey,
-            key::{Parity, rand},
-            secp256k1::{PublicKey, Scalar, Secp256k1, SecretKey},
-        },
-        miniscript::{Descriptor, descriptor::TapTree, policy::Concrete},
-    },
-    std::{str::FromStr, sync::Arc},
+    crate::utils::descriptors::{CompileScriptType, compile_policy},
+    bdk_wallet::miniscript::descriptor::DescriptorPublicKey,
 };
 
 #[cfg(feature = "silent-payments")]
 use crate::utils::types::StatusResult;
 
-#[cfg(feature = "compiler")]
-const NUMS_UNSPENDABLE_KEY_HEX: &str =
-    "50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0";
-
 #[derive(Parser, Debug, Clone, PartialEq)]
 pub struct DescriptorCommand {
     /// Descriptor type (script type)
@@ -77,63 +64,29 @@ pub struct CompileCommand {
         short = 't',
         long = "type",
         value_enum,
-        default_value_t = ScriptType::Wsh
+        default_value_t = CompileScriptType::Wsh
     )]
-    script_type: ScriptType,
-}
-
-/// Script types the compiled policy can be embedded in.
-#[cfg(feature = "compiler")]
-#[derive(Clone, Copy, ValueEnum, Debug, Eq, PartialEq)]
-pub enum ScriptType {
-    Sh,
-    Wsh,
-    ShWsh,
-    Tr,
+    script_type: CompileScriptType,
+    /// 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,
 }
 
 #[cfg(feature = "compiler")]
 impl AppCommand<AppContext<Init>> for CompileCommand {
     type Output = DescriptorResult;
 
     fn execute(&self, _ctx: &mut AppContext<Init>) -> Result<Self::Output, Error> {
-        let policy: Concrete<String> = 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::<String>(&self.policy, self.script_type)?
+        } else {
+            compile_policy::<DescriptorPublicKey>(&self.policy, self.script_type)?
+        };
 
         Ok(DescriptorResult {
-            descriptor: Some(descriptor.to_string()),
+            descriptor: Some(descriptor),
             mnemonic: None,
             multipath_descriptor: None,
             public_descriptors: None,

src/utils/descriptors.rs

@@ -3,7 +3,19 @@ use bdk_wallet::bitcoin::Network;
 use bdk_wallet::bitcoin::bip32::ChildNumber;
 use bdk_wallet::descriptor::IntoWalletDescriptor;
 use bdk_wallet::keys::GeneratableKey;
-#[cfg(feature = "hwi")]
+#[cfg(feature = "compiler")]
+use bdk_wallet::{
+    bitcoin::{
+        XOnlyPublicKey,
+        key::{Parity, rand},
+        secp256k1::{PublicKey, Scalar, SecretKey},
+    },
+    miniscript::{
+        ForEachKey, FromStrKey, Legacy, MiniscriptKey, ScriptContext, Tap, descriptor::TapTree,
+        policy::Concrete,
+    },
+};
+#[cfg(any(feature = "hwi", feature = "compiler"))]
 use clap::ValueEnum;
 use std::{str::FromStr, sync::Arc};
 
@@ -244,6 +256,99 @@ pub fn validate_descriptor_pair(
     Ok(ext_is_multipath)
 }
 
+#[cfg(feature = "compiler")]
+const NUMS_UNSPENDABLE_KEY_HEX: &str =
+    "50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0";
+
+/// Script types the compiled policy can be embedded in.
+#[cfg(feature = "compiler")]
+#[derive(Clone, Copy, ValueEnum, Debug, Eq, PartialEq)]
+pub enum CompileScriptType {
+    Sh,
+    Wsh,
+    ShWsh,
+    Tr,
+}
+
+/// 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.
+#[cfg(feature = "compiler")]
+fn check_policy_keys<Pk: MiniscriptKey, Ctx: ScriptContext>(
+    policy: &Concrete<Pk>,
+) -> 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(()),
+    }
+}
+
+/// 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.
+#[cfg(feature = "compiler")]
+pub fn compile_policy<Pk: FromStrKey>(
+    policy_str: &str,
+    script_type: CompileScriptType,
+) -> Result<(String, Option<String>), Error> {
+    let policy: Concrete<Pk> = 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 {
+        CompileScriptType::Sh => {
+            check_policy_keys::<Pk, Legacy>(&policy)?;
+            Descriptor::new_sh(policy.compile()?)
+        }
+        CompileScriptType::Wsh => {
+            check_policy_keys::<Pk, Segwitv0>(&policy)?;
+            Descriptor::new_wsh(policy.compile()?)
+        }
+        CompileScriptType::ShWsh => {
+            check_policy_keys::<Pk, Segwitv0>(&policy)?;
+            Descriptor::new_sh_wsh(policy.compile()?)
+        }
+        CompileScriptType::Tr => {
+            check_policy_keys::<Pk, Tap>(&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))
+}
 /// Strip a trailing descriptor checksum.
 #[cfg(feature = "hwi")]
 pub(crate) fn strip_checksum(descriptor: &str) -> &str {

tests/integration/init.rs

@@ -508,6 +508,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();
@@ -520,13 +524,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("));
@@ -562,6 +566,102 @@ 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("));
+    }
+
+    /// Mirrors `test_compile_rejects_x_only_key_for_non_tr_types`, for the other
+    /// direction: an uncompressed key is only valid for `sh` (Legacy); `wsh`/`sh-wsh`
+    /// (Segwitv0) and `tr` (Tap) both reject it. This test covers `tr`.
+    #[test]
+    fn test_compile_rejects_uncompressed_key_for_tr() {
+        let temp_dir = TempDir::new().unwrap();
+        let cli = BdkCli::new("testnet", Some(temp_dir.path().to_path_buf()));
+
+        let policy = "pk(04003c3d7e58ed3a9d988b0310928df65943973996a4b2882aee1726bd6046828c4befeaef638ccec55c79ef12b069c8032fed5ccf86c4912d53759ccc2d148c8e)";
+
+        cli.cmd("compile", &[policy, "--type", "tr"])
+            .assert()
+            .failure()
+            .stderr(predicate::str::contains("Invalid policy"));
+    }
 }
 
 // --- CONFIG COMMAND TESTS ---