-
Notifications
You must be signed in to change notification settings - Fork 155
Add TapCtx #255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add TapCtx #255
Changes from all commits
4da439f
9a5384d
888cd62
3eb63d5
95ab99d
954df35
498bad2
96cb2ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -921,7 +921,7 @@ mod tests { | |
struct SimpleSat { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated to this PR: was really confused by the name, spent some time understanding how this is related to bitcoin amount in sats (satoshis). Was hard to guess that this structure is about satisfaction, not sats. For the future, IMO, |
||
sig: secp256k1::Signature, | ||
pk: bitcoin::PublicKey, | ||
}; | ||
} | ||
|
||
impl Satisfier<bitcoin::PublicKey> for SimpleSat { | ||
fn lookup_sig(&self, pk: &bitcoin::PublicKey) -> Option<BitcoinSig> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,7 +87,6 @@ | |
//! ``` | ||
//! | ||
//! | ||
#![allow(bare_trait_objects)] | ||
#![cfg_attr(all(test, feature = "unstable"), feature(test))] | ||
// Coding conventions | ||
#![deny(unsafe_code)] | ||
|
@@ -125,7 +124,7 @@ use bitcoin::hashes::{hash160, sha256, Hash}; | |
|
||
pub use descriptor::{Descriptor, DescriptorPublicKey, DescriptorTrait}; | ||
pub use interpreter::Interpreter; | ||
pub use miniscript::context::{BareCtx, Legacy, ScriptContext, Segwitv0}; | ||
pub use miniscript::context::{BareCtx, Legacy, ScriptContext, Segwitv0, Tap}; | ||
pub use miniscript::decode::Terminal; | ||
pub use miniscript::satisfy::{BitcoinSig, Preimage32, Satisfier}; | ||
pub use miniscript::Miniscript; | ||
|
@@ -142,16 +141,6 @@ pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Ha | |
|
||
/// Converts an object to PublicHash | ||
fn to_pubkeyhash(&self) -> Self::Hash; | ||
|
||
/// Computes the size of a public key when serialized in a script, | ||
/// including the length bytes | ||
fn serialized_len(&self) -> usize { | ||
if self.is_uncompressed() { | ||
66 | ||
} else { | ||
34 | ||
} | ||
} | ||
} | ||
|
||
impl MiniscriptKey for bitcoin::PublicKey { | ||
|
@@ -170,6 +159,14 @@ impl MiniscriptKey for bitcoin::PublicKey { | |
} | ||
} | ||
|
||
impl MiniscriptKey for bitcoin::schnorr::PublicKey { | ||
type Hash = hash160::Hash; | ||
|
||
fn to_pubkeyhash(&self) -> Self::Hash { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The question is why we need to support pubkey hashes in Taproot - and why it is of hash160::Hash type, when we have specific hash types for other cases (like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call. Basically, when this was originally implemented I was not aware of specific hash types in rust-bitcoin. I will change bitcoin::schnorr::PublicKey in this PR and change the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! But in which cases do we need pubkey hashes in Taproot/script? I understand that nothing prohibits from adding key hash to arbitrary tapscript leaf, but I think we should discourage this nonstandard practice in API and just handle that as any other hash, unrelated to pubkey There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more a spec question. I don't see why we should disallow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But would this smaller size matter for the branches of the tapscript which are never used? It will never hit blockchain. But while spent, it will have a larger size for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider the following: It is difficult to always decompose scripts into ORs(DNF). Therefore, it might be necessary to a leaf miniscript that contains a or |
||
hash160::Hash::hash(&self.serialize()) | ||
} | ||
} | ||
|
||
impl MiniscriptKey for String { | ||
type Hash = String; | ||
|
||
|
@@ -183,6 +180,12 @@ pub trait ToPublicKey: MiniscriptKey { | |
/// Converts an object to a public key | ||
fn to_public_key(&self) -> bitcoin::PublicKey; | ||
|
||
/// Convert an object to x-only pubkey | ||
fn to_x_only_pubkey(&self) -> bitcoin::schnorr::PublicKey { | ||
let pk = self.to_public_key(); | ||
bitcoin::schnorr::PublicKey::from(pk.key) | ||
} | ||
|
||
/// Converts a hashed version of the public key to a `hash160` hash. | ||
/// | ||
/// This method must be consistent with `to_public_key`, in the sense | ||
|
@@ -202,6 +205,25 @@ impl ToPublicKey for bitcoin::PublicKey { | |
} | ||
} | ||
|
||
impl ToPublicKey for bitcoin::schnorr::PublicKey { | ||
fn to_public_key(&self) -> bitcoin::PublicKey { | ||
// This code should never be used. | ||
// But is implemented for completeness | ||
let mut data: Vec<u8> = vec![0x02]; | ||
data.extend(self.serialize().iter()); | ||
bitcoin::PublicKey::from_slice(&data) | ||
.expect("Failed to construct 33 Publickey from 0x02 appended x-only key") | ||
} | ||
|
||
fn to_x_only_pubkey(&self) -> bitcoin::schnorr::PublicKey { | ||
*self | ||
} | ||
|
||
fn hash_to_hash160(hash: &hash160::Hash) -> hash160::Hash { | ||
*hash | ||
} | ||
} | ||
|
||
/// Dummy key which de/serializes to the empty string; useful sometimes for testing | ||
#[derive(Copy, Clone, PartialOrd, Ord, PartialEq, Eq, Debug)] | ||
pub struct DummyKey; | ||
|
@@ -448,7 +470,7 @@ pub enum Error { | |
InvalidOpcode(opcodes::All), | ||
/// Some opcode occurred followed by `OP_VERIFY` when it had | ||
/// a `VERIFY` version that should have been used instead | ||
NonMinimalVerify(miniscript::lex::Token), | ||
NonMinimalVerify(String), | ||
/// Push was illegal in some context | ||
InvalidPush(Vec<u8>), | ||
/// rust-bitcoin script error | ||
|
@@ -517,6 +539,8 @@ pub enum Error { | |
ImpossibleSatisfaction, | ||
/// Bare descriptors don't have any addresses | ||
BareDescriptorAddr, | ||
/// PubKey invalid under current context | ||
PubKeyCtxError(miniscript::decode::KeyParseError, &'static str), | ||
} | ||
|
||
#[doc(hidden)] | ||
|
@@ -563,7 +587,7 @@ fn errstr(s: &str) -> Error { | |
} | ||
|
||
impl error::Error for Error { | ||
fn cause(&self) -> Option<&error::Error> { | ||
fn cause(&self) -> Option<&dyn error::Error> { | ||
match *self { | ||
Error::BadPubkey(ref e) => Some(e), | ||
_ => None, | ||
|
@@ -580,7 +604,7 @@ impl fmt::Display for Error { | |
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
match *self { | ||
Error::InvalidOpcode(op) => write!(f, "invalid opcode {}", op), | ||
Error::NonMinimalVerify(tok) => write!(f, "{} VERIFY", tok), | ||
Error::NonMinimalVerify(ref tok) => write!(f, "{} VERIFY", tok), | ||
Error::InvalidPush(ref push) => write!(f, "invalid push {:?}", push), // TODO hexify this | ||
Error::Script(ref e) => fmt::Display::fmt(e, f), | ||
Error::CmsTooManyKeys(n) => write!(f, "checkmultisig with {} keys", n), | ||
|
@@ -634,6 +658,9 @@ impl fmt::Display for Error { | |
Error::AnalysisError(ref e) => e.fmt(f), | ||
Error::ImpossibleSatisfaction => write!(f, "Impossible to satisfy Miniscript"), | ||
Error::BareDescriptorAddr => write!(f, "Bare descriptors don't have address"), | ||
Error::PubKeyCtxError(ref pk, ref ctx) => { | ||
write!(f, "Pubkey error: {} under {} scriptcontext", pk, ctx) | ||
} | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just
bitcoin::PublicKey
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using miniscript fuzzing, I generally preferred reusing the bitcoin from miniscript instead of importing a separate crate into fuzzer