From: Olorunshogo Moses BAMTEFA Date: Fri, 23 Oct 2026 09:54:23 +0000 (+0100) Subject: fix(compile): move compile helpers to utils, reorder doc comments, add uncompressed... X-Git-Url: http://internal-gitweb-vhost/-script/src/example_cli/database/FromScriptException.UnrecognizedScript.html?a=commitdiff_plain;h=822fcb211dc601c6643b3ad42ff5939f00d07281;p=bdk-cli fix(compile): move compile helpers to utils, reorder doc comments, add uncompressed-key tr test, and update changelog --- diff --git a/CHANGELOG.md b/CHANGELOG.md index b11435b..e984932 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ page. See [DEVELOPMENT_CYCLE.md](DEVELOPMENT_CYCLE.md) for more details. - Replaced plain `wallets` with `wallets list`, and added `wallets delete ` 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] diff --git a/src/handlers/descriptor.rs b/src/handlers/descriptor.rs index c1460a6..d6e69ae 100644 --- a/src/handlers/descriptor.rs +++ b/src/handlers/descriptor.rs @@ -13,32 +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, ForEachKey, FromStrKey, Legacy, MiniscriptKey, ScriptContext, Segwitv0, - Tap, - descriptor::{DescriptorPublicKey, TapTree}, - policy::Concrete, - }, - }, - std::{str::FromStr, sync::Arc}, + crate::utils::descriptors::{ScriptType, 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) @@ -92,96 +74,6 @@ pub struct CompileCommand { placeholders: bool, } -/// 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, -} - -#[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; diff --git a/src/utils/descriptors.rs b/src/utils/descriptors.rs index dcbac46..c14b120 100644 --- a/src/utils/descriptors.rs +++ b/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 ScriptType { + 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( + 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(()), + } +} + +/// 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( + 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)) +} /// Strip a trailing descriptor checksum. #[cfg(feature = "hwi")] pub(crate) fn strip_checksum(descriptor: &str) -> &str { diff --git a/tests/integration/init.rs b/tests/integration/init.rs index 3074421..e0fcea2 100644 --- a/tests/integration/init.rs +++ b/tests/integration/init.rs @@ -646,6 +646,22 @@ mod test_compile { .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 ---