Manual fixes to claudes changes

tcharding · tcharding · commit f8d9b5293086 · 2025-11-12T13:30:48.000+11:00
diff --git a/examples/psbt_sign_finalize.rs b/examples/psbt_sign_finalize.rs
@@ -9,6 +9,7 @@ use miniscript::bitcoin::script::ScriptPubKey;
 use miniscript::bitcoin::sighash::SighashCache;
 use miniscript::bitcoin::Witness;
 //use miniscript::bitcoin::secp256k1; // https://github.com/rust-lang/rust/issues/121684
+use miniscript::bitcoin::ext::*;
 use miniscript::bitcoin::{
     transaction, Address, Amount, Network, OutPoint, PrivateKey, Sequence, Transaction,
     TxIn, TxOut,
@@ -92,12 +93,12 @@ fn main() {
 
     psbt.unsigned_tx.outputs.push(TxOut {
         script_pubkey: receiver.script_pubkey(),
-        amount: Amount::from_sat(amount / 5 - 500).unwrap(),
+        amount: Amount::from_sat_u32(amount / 5 - 500),
     });
 
     psbt.unsigned_tx.outputs.push(TxOut {
         script_pubkey: bridge_descriptor.script_pubkey(),
-        amount: Amount::from_sat(amount * 4 / 5).unwrap(),
+        amount: Amount::from_sat_u32(amount * 4 / 5),
     });
 
     // Generating signatures & witness data
@@ -152,7 +153,7 @@ fn main() {
 fn get_vout(tx: &Transaction, spk: &ScriptPubKey) -> (OutPoint, TxOut) {
     for (i, txout) in tx.clone().outputs.into_iter().enumerate() {
         if spk == &txout.script_pubkey {
-            return (OutPoint { txid: tx.compute_txid(), vout: i as u32 }, txout);
+            return (OutPoint::new(tx.compute_txid(), u32::try_from(i).expect("index is always positive")), txout);
         }
     }
     panic!("Only call get vout on functions which have the expected outpoint");
diff --git a/examples/sign_multisig.rs b/examples/sign_multisig.rs
@@ -89,7 +89,7 @@ fn spending_transaction() -> bitcoin::Transaction {
         }],
         outputs: vec![bitcoin::TxOut {
             script_pubkey: ScriptPubKeyBuf::new(),
-            amount: Amount::from_sat(100_000_000).unwrap(),
+            amount: Amount::from_sat_u32(100_000_000),
         }],
     }
 }
diff --git a/examples/taptree_of_horror/taptree_of_horror.rs b/examples/taptree_of_horror/taptree_of_horror.rs
@@ -217,15 +217,15 @@ fn main() {
         witness: Witness::default(),
     };
 
-    let prev_amount = Amount::from_sat(100_000_000).unwrap();
+    let prev_amount = Amount::from_sat_u32(100_000_000);
     let witness_utxo =
         TxOut { amount: prev_amount, script_pubkey: derived_descriptor.clone().script_pubkey() };
 
     let destination_address =
         Address::from_str("bcrt1p2tl8zasepqe3j6m7hx4tdmqzndddr5wa9ugglpdzgenjwv42rkws66dk5a")
             .unwrap();
     let destination_output: TxOut = TxOut {
-        amount: bitcoin::Amount::from_sat(99_999_000).unwrap(),
+        amount: bitcoin::Amount::from_sat_u32(99_999_000),
         script_pubkey: destination_address.assume_checked().script_pubkey(),
     };
 
diff --git a/src/descriptor/key.rs b/src/descriptor/key.rs
@@ -720,11 +720,19 @@ impl DescriptorPublicKey {
                 if let Some((fingerprint, _)) = single.origin {
                     fingerprint
                 } else {
-                    let bytes = match single.key {
-                        SinglePubKey::FullKey(pk) => pk.to_bytes(),
-                        SinglePubKey::XOnly(x_only_pk) => x_only_pk.serialize().to_vec(),
-                    };
-                    let hash = hash160::Hash::hash(&bytes);
+                    use bitcoin::hashes::HashEngine as _;
+
+                    // FIXME: Should we support this usecase for `XKeyIdentifier`? I.e., should one
+                    // be able to hash arbitrary data into it without having to know the inner hash?
+                    let mut engine = hash160::Hash::engine();
+                    match single.key {
+                        SinglePubKey::FullKey(pk) => {
+                            pk.write_into(&mut engine).expect("engines don't error")
+                        }
+                        SinglePubKey::XOnly(x_only_pk) => engine.input(&x_only_pk.serialize()),
+                    }
+                    let hash = engine.finalize();
+                    // FIXME: This is also a bit klunky.
                     bip32::Fingerprint::from(
                         &hash.as_byte_array()[..4]
                             .try_into()
@@ -1252,12 +1260,7 @@ impl DefiniteDescriptorKey {
         match self.0 {
             DescriptorPublicKey::Single(ref pk) => match pk.key {
                 SinglePubKey::FullKey(pk) => pk,
-                SinglePubKey::XOnly(xpk) => {
-                    let bytes = xpk.serialize();
-                    let secp_xonly = bitcoin::secp256k1::XOnlyPublicKey::from_byte_array(bytes)
-                        .expect("32-byte array is valid XOnlyPublicKey");
-                    secp_xonly.to_public_key()
-                },
+                SinglePubKey::XOnly(xpk) => xpk.to_public_key(),
             },
             DescriptorPublicKey::XPub(ref xpk) => match xpk.wildcard {
                 Wildcard::Unhardened | Wildcard::Hardened => {
diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs
@@ -516,6 +516,9 @@ impl<Pk: MiniscriptKey + ToPublicKey> Descriptor<Pk> {
     ///
     /// # Errors
     /// If the descriptor is a taproot descriptor.
+    // FIXME: I don't know what script type this function should return.
+    // Is this something to do with `primitives::script::ScriptHashableTag` (I'm still confused
+    // because that trait mentions redeemScript but I expected it to say witnessScript?)
     pub fn script_code(&self) -> Result<ScriptPubKeyBuf, Error> {
         match *self {
             Descriptor::Bare(ref bare) => Ok(bare.ecdsa_sighash_script_code()),
diff --git a/src/interpreter/inner.rs b/src/interpreter/inner.rs
@@ -128,8 +128,9 @@ pub(super) fn from_txdata<'txin>(
             Err(Error::NonEmptyWitness)
         } else {
             match ssig_stack.pop() {
+                // FIXME: Andrew to check that we are happy with the new more-restrictive hashes API.
                 Some(elem) => {
-                        let pk = pk_from_stack_elem(&elem, false)?;
+                    let pk = pk_from_stack_elem(&elem, false)?;
                     let hash160 = pk.to_pubkeyhash(SigType::Ecdsa);
                     let pkh = bitcoin::key::PubkeyHash::from_byte_array(hash160.to_byte_array());
                     if *spk == ScriptPubKeyBuf::new_p2pkh(pkh) {
@@ -398,6 +399,8 @@ impl<Ctx: ScriptContext> ToNoChecks for Miniscript<bitcoin::XOnlyPublicKey, Ctx>
     }
 }
 
+// FIXME: Added by Claude, is this correct or should we refactor no that there is a new
+// `XOnlyPublicKey` wrapper type in `bitcoin`?
 impl<Ctx: ScriptContext> ToNoChecks for Miniscript<bitcoin::secp256k1::XOnlyPublicKey, Ctx> {
     fn to_no_checks_ms(&self) -> Miniscript<BitcoinKey, NoChecks> {
         struct TranslateSecp256k1XOnlyPk;
diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs
@@ -96,12 +96,7 @@ impl BitcoinKey {
     fn to_pubkeyhash(self, sig_type: SigType) -> hash160::Hash {
         match self {
             BitcoinKey::Fullkey(pk) => pk.to_pubkeyhash(sig_type),
-            BitcoinKey::XOnlyPublicKey(pk) => {
-                // XOnly keys are used in Taproot (Schnorr signatures)
-                // Convert to full public key assuming even parity
-                let full_pk = pk.public_key(secp256k1::Parity::Even);
-                full_pk.to_pubkeyhash(sig_type)
-            }
+            BitcoinKey::XOnlyPublicKey(pk) => pk.to_pubkeyhash(sig_type),
         }
     }
 }
@@ -111,11 +106,7 @@ impl fmt::Display for BitcoinKey {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             BitcoinKey::Fullkey(pk) => pk.to_public_key().fmt(f),
-            BitcoinKey::XOnlyPublicKey(pk) => {
-                // Convert XOnly key to full public key assuming even parity
-                let full_pk = pk.public_key(secp256k1::Parity::Even);
-                full_pk.fmt(f)
-            }
+            BitcoinKey::XOnlyPublicKey(pk) => pk.to_public_key().fmt(f),
         }
     }
 }
@@ -252,7 +243,7 @@ impl<'txin> Interpreter<'txin> {
                 };
 
                 let success = msg.map(|msg| {
-                    secp.verify_ecdsa(msg, &ecdsa_sig.signature, &key.inner)
+                    secp256k1::ecdsa::verify(&ecdsa_sig.signature, msg, &key.inner)
                         .is_ok()
                 });
                 success.unwrap_or(false) // unwrap_or checks for errors, while success would have checksig results
@@ -1102,7 +1093,7 @@ mod tests {
                 inner: secp256k1::PublicKey::from_secret_key(&sk),
                 compressed: true,
             };
-            let signature = secp.sign_ecdsa(msg, &sk);
+            let signature = secp256k1::ecdsa::sign(msg, &sk);
             ecdsa_sigs.push(bitcoin::ecdsa::Signature {
                 signature,
                 sighash_type: bitcoin::sighash::EcdsaSighashType::All,
diff --git a/src/lib.rs b/src/lib.rs
@@ -204,6 +204,8 @@ impl MiniscriptKey for bitcoin::secp256k1::XOnlyPublicKey {
     fn is_x_only_key(&self) -> bool { true }
 }
 
+// FIXME: Do we want to do this for both the `bitcoin::XOnlyPublicKey` and the sepc
+// one or should we refactor to only ever use one of them?
 impl MiniscriptKey for bitcoin::XOnlyPublicKey {
     type Sha256 = sha256::Hash;
     type Hash256 = hash256::Hash;
diff --git a/src/miniscript/astelem.rs b/src/miniscript/astelem.rs
@@ -38,6 +38,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
     where
         Pk: ToPublicKey,
     {
+        // FIXME: Changes done here are semantically correct but all a bit ugly.
+        // See https://github.com/rust-bitcoin/rust-bitcoin/issues/5262
         match *self {
             Terminal::PkK(ref pk) => builder.push_ms_key::<_, Ctx>(pk),
             Terminal::PkH(ref pk) => builder
diff --git a/src/miniscript/lex.rs b/src/miniscript/lex.rs
@@ -217,9 +217,7 @@ pub fn lex<T>(script: &'_ script::Script<T>) -> Result<Vec<Token>, Error> {
                     ret.push(Token::Bytes65(bytes));
                 } else {
                     // check minimality of the number
-                    let push_bytes = <&PushBytes>::try_from(bytes.as_bytes())
-                        .expect("push bytes from instruction iterator should be valid");
-                    match push_bytes.read_scriptint() {
+                    match bytes.read_scriptint() {
                         Ok(v) if v >= 0 => {
                             ret.push(Token::Num(v as u32));
                         }
diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs
@@ -1645,7 +1645,16 @@ mod tests {
             .translate_pk(&mut StrXOnlyKeyTranslator::new())
             .unwrap();
         // script rtt test
-        // Note: decode_consensus not available for bitcoin::key::XOnlyPublicKey
+        //
+        // Claud wrote: Note: decode_consensus not available for bitcoin::key::XOnlyPublicKey
+        // 
+        // FIXME: We should be able to un-comment this, I _think_ we can't because of
+        //
+        // **Taken from** ./context.rs
+        // impl ScriptContext for Tap {
+        // FIXME: Does this have to change to `bitcoin::XOnlyPublicKey`.
+        // type Key = bitcoin::secp256k1::XOnlyPublicKey;
+        //
         // assert_eq!(
         //     Miniscript::<bitcoin::key::XOnlyPublicKey, Tap>::decode_consensus(&tap_ms.encode()).unwrap(),
         //     tap_ms
@@ -1699,8 +1708,8 @@ mod tests {
             "02c2fd50ceae468857bb7eb32ae9cd4083e6c7e42fbbec179d81134b3e3830586c",
         )
         .unwrap();
-        let hash160 = pk.pubkey_hash().to_byte_array();
-        let ms_str = &format!("c:expr_raw_pkh({})", hash160::Hash::from_byte_array(hash160));
+        let hash160 = hash160::Hash::from_byte_array(pk.pubkey_hash().to_byte_array());
+        let ms_str = &format!("c:expr_raw_pkh({})", hash160);
         type SegwitMs = Miniscript<bitcoin::PublicKey, Segwitv0>;
 
         // Test that parsing raw hash160 from string does not work without extra features
@@ -1716,7 +1725,7 @@ mod tests {
 
         // Try replacing the raw_pkh with a pkh
         let mut map = BTreeMap::new();
-        map.insert(hash160::Hash::from_byte_array(hash160), pk);
+        map.insert(hash160, pk);
         let ms_no_raw = ms.substitute_raw_pkh(&map);
         assert_eq!(ms_no_raw.to_string(), format!("pkh({})", pk),);
     }
diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs
@@ -1583,7 +1583,7 @@ mod tests {
             lock_time: absolute::LockTime::ZERO,
             inputs: vec![],
             outputs: vec![TxOut {
-                amount: Amount::from_sat(1_000).unwrap(),
+                amount: Amount::from_sat_u32(1_000),
                 script_pubkey: ScriptPubKeyBuf::from_hex(
                     "5120a60869f0dbcf1dc659c9cecbaf8050135ea9e8cdc487053f1dc6880949dc684c",
                 )
@@ -1647,7 +1647,7 @@ mod tests {
             lock_time: absolute::LockTime::ZERO,
             inputs: vec![],
             outputs: vec![TxOut {
-                amount: Amount::from_sat(1_000).unwrap(),
+                amount: Amount::from_sat_u32(1_000),
                 script_pubkey: ScriptPubKeyBuf::from_hex(
                     "5120a60869f0dbcf1dc659c9cecbaf8050135ea9e8cdc487053f1dc6880949dc684c",
                 )

Original file line number	Diff line number	Diff line change
`@@ -89,7 +89,7 @@ fn spending_transaction() -> bitcoin::Transaction {`
`89`	`89`	`}],`
`90`	`90`	`outputs: vec![bitcoin::TxOut {`
`91`	`91`	`script_pubkey: ScriptPubKeyBuf::new(),`
`92`		`- amount: Amount::from_sat(100_000_000).unwrap(),`
	`92`	`+ amount: Amount::from_sat_u32(100_000_000),`
`93`	`93`	`}],`
`94`	`94`	`}`
`95`	`95`	`}`
Original file line number	Diff line number	Diff line change
`@@ -204,6 +204,8 @@ impl MiniscriptKey for bitcoin::secp256k1::XOnlyPublicKey {`
`204`	`204`	`fn is_x_only_key(&self) -> bool { true }`
`205`	`205`	`}`
`206`	`206`
	`207`	+// FIXME: Do we want to do this for both the `bitcoin::XOnlyPublicKey` and the sepc
	`208`	`+// one or should we refactor to only ever use one of them?`
`207`	`209`	`impl MiniscriptKey for bitcoin::XOnlyPublicKey {`
`208`	`210`	`type Sha256 = sha256::Hash;`
`209`	`211`	`type Hash256 = hash256::Hash;`
Original file line number	Diff line number	Diff line change
`@@ -38,6 +38,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {`
`38`	`38`	`where`
`39`	`39`	`Pk: ToPublicKey,`
`40`	`40`	`{`
	`41`	`+ // FIXME: Changes done here are semantically correct but all a bit ugly.`
	`42`	`+ // See https://github.com/rust-bitcoin/rust-bitcoin/issues/5262`
`41`	`43`	`match *self {`
`42`	`44`	`Terminal::PkK(ref pk) => builder.push_ms_key::<_, Ctx>(pk),`
`43`	`45`	`Terminal::PkH(ref pk) => builder`