Skip to content

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

Merged
merged 8 commits into from
Oct 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# Taproot updates
- Changed the ToPublicKey trait to support x-only keys.
# 6.0.1 - Aug 5, 2021

- The `lift` method on a Miniscript node was fixed. It would previously mix up
Expand Down
2 changes: 1 addition & 1 deletion fuzz/fuzz_targets/roundtrip_miniscript_script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ fn do_test(data: &[u8]) {
// Try round-tripping as a script
let script = script::Script::from(data.to_owned());

if let Ok(pt) = Miniscript::<_, Segwitv0>::parse(&script) {
if let Ok(pt) = Miniscript::<miniscript::bitcoin::PublicKey, Segwitv0>::parse(&script) {
Copy link
Contributor

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?

Copy link
Member Author

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

let output = pt.encode();
assert_eq!(pt.script_size(), output.len());
assert_eq!(output, script);
Expand Down
2 changes: 1 addition & 1 deletion src/descriptor/bare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ impl<Pk: MiniscriptKey> DescriptorTrait<Pk> for Pkh<Pk> {
}

fn max_satisfaction_weight(&self) -> Result<usize, Error> {
Ok(4 * (1 + 73 + self.pk.serialized_len()))
Ok(4 * (1 + 73 + BareCtx::pk_len(&self.pk)))
}

fn script_code(&self) -> Script
Expand Down
2 changes: 1 addition & 1 deletion src/descriptor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -921,7 +921,7 @@ mod tests {
struct SimpleSat {
Copy link
Contributor

Choose a reason for hiding this comment

The 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, satisfy::Simple or SimpleSatisfier will be better

sig: secp256k1::Signature,
pk: bitcoin::PublicKey,
};
}

impl Satisfier<bitcoin::PublicKey> for SimpleSat {
fn lookup_sig(&self, pk: &bitcoin::PublicKey) -> Option<BitcoinSig> {
Expand Down
10 changes: 7 additions & 3 deletions src/descriptor/segwitv0.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,9 @@ impl<Pk: MiniscriptKey> Wpkh<Pk> {
pub fn new(pk: Pk) -> Result<Self, Error> {
// do the top-level checks
if pk.is_uncompressed() {
Err(Error::ContextError(ScriptContextError::CompressedOnly))
Err(Error::ContextError(ScriptContextError::CompressedOnly(
pk.to_string(),
)))
} else {
Ok(Self { pk: pk })
}
Expand Down Expand Up @@ -376,7 +378,9 @@ where
impl<Pk: MiniscriptKey> DescriptorTrait<Pk> for Wpkh<Pk> {
fn sanity_check(&self) -> Result<(), Error> {
if self.pk.is_uncompressed() {
Err(Error::ContextError(ScriptContextError::CompressedOnly))
Err(Error::ContextError(ScriptContextError::CompressedOnly(
self.pk.to_string(),
)))
} else {
Ok(())
}
Expand Down Expand Up @@ -430,7 +434,7 @@ impl<Pk: MiniscriptKey> DescriptorTrait<Pk> for Wpkh<Pk> {
}

fn max_satisfaction_weight(&self) -> Result<usize, Error> {
Ok(4 + 1 + 73 + self.pk.serialized_len())
Ok(4 + 1 + 73 + Segwitv0::pk_len(&self.pk))
}

fn script_code(&self) -> Script
Expand Down
2 changes: 1 addition & 1 deletion src/descriptor/sortedmulti.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
script_num_size(self.k)
+ 1
+ script_num_size(self.pks.len())
+ self.pks.iter().map(|pk| pk.serialized_len()).sum::<usize>()
+ self.pks.iter().map(|pk| Ctx::pk_len(pk)).sum::<usize>()
}

/// Maximum number of witness elements used to satisfy the Miniscript
Expand Down
2 changes: 1 addition & 1 deletion src/interpreter/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl From<::Error> for Error {
}

impl error::Error for Error {
fn cause(&self) -> Option<&error::Error> {
fn cause(&self) -> Option<&dyn error::Error> {
match *self {
Error::Secp(ref err) => Some(err),
ref x => Some(x),
Expand Down
5 changes: 3 additions & 2 deletions src/interpreter/inner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ fn script_from_stackelem<'a>(
) -> Result<Miniscript<bitcoin::PublicKey, NoChecks>, Error> {
match *elem {
stack::Element::Push(sl) => {
Miniscript::parse_insane(&bitcoin::Script::from(sl.to_owned())).map_err(Error::from)
Miniscript::<bitcoin::PublicKey, _>::parse_insane(&bitcoin::Script::from(sl.to_owned()))
.map_err(Error::from)
}
stack::Element::Satisfied => Miniscript::from_ast(::Terminal::True).map_err(Error::from),
stack::Element::Dissatisfied => {
Expand Down Expand Up @@ -270,7 +271,7 @@ pub fn from_txdata<'txin>(
// ** bare script **
} else {
if wit_stack.is_empty() {
let miniscript = Miniscript::parse_insane(spk)?;
let miniscript = Miniscript::<bitcoin::PublicKey, _>::parse_insane(spk)?;
Ok((
Inner::Script(miniscript, ScriptType::Bare),
ssig_stack,
Expand Down
2 changes: 1 addition & 1 deletion src/interpreter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@ mod tests {
height: 1002,
has_errored: false,
}
};
}

let pk = ms_str!("c:pk_k({})", pks[0]);
let pkh = ms_str!("c:pk_h({})", pks[1].to_pubkeyhash());
Expand Down
57 changes: 42 additions & 15 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@
//! ```
//!
//!
#![allow(bare_trait_objects)]
#![cfg_attr(all(test, feature = "unstable"), feature(test))]
// Coding conventions
#![deny(unsafe_code)]
Expand Down Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 PubkeyHash, WPubkeyHash)?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 impl MiniscriptKey for bitcoin::PublicKey in a separate PR.

Copy link
Contributor

@dr-orlovsky dr-orlovsky Sep 19, 2021

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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 pkh. It's more efficient in case the fragment is not satisfied.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in DUP HASH160 <hash> EQUALVERIFY CHECKSIG is still a valid script and has less size than <x_only_pk> CHECKSIG. So, there is one use-case

Copy link
Contributor

Choose a reason for hiding this comment

The 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 witness in the spending tx input.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the following:
thresh(11,S1,S2,S2......,S15) where S15 is or(pk(likely),pkh(unlikely))

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;

Expand All @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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,
Expand All @@ -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),
Expand Down Expand Up @@ -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)
}
}
}
}
Expand Down
9 changes: 6 additions & 3 deletions src/miniscript/astelem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ use expression;
use miniscript::types::{self, Property};
use miniscript::ScriptContext;
use script_num_size;

use util::MsKeyBuilder;
use {Error, ForEach, ForEachKey, Miniscript, MiniscriptKey, Terminal, ToPublicKey, TranslatePk};

impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
Expand Down Expand Up @@ -628,7 +630,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
Pk: ToPublicKey,
{
match *self {
Terminal::PkK(ref pk) => builder.push_key(&pk.to_public_key()),
Terminal::PkK(ref pk) => builder.push_ms_key::<_, Ctx>(pk),
Terminal::PkH(ref hash) => builder
.push_opcode(opcodes::all::OP_DUP)
.push_opcode(opcodes::all::OP_HASH160)
Expand Down Expand Up @@ -734,6 +736,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
.push_opcode(opcodes::all::OP_EQUAL)
}
Terminal::Multi(k, ref keys) => {
debug_assert!(!Ctx::is_tap());
builder = builder.push_int(k as i64);
for pk in keys {
builder = builder.push_key(&pk.to_public_key());
Expand All @@ -754,7 +757,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
/// will handle the segwit/non-segwit technicalities for you.
pub fn script_size(&self) -> usize {
match *self {
Terminal::PkK(ref pk) => pk.serialized_len(),
Terminal::PkK(ref pk) => Ctx::pk_len(pk),
Terminal::PkH(..) => 24,
Terminal::After(n) => script_num_size(n as usize) + 1,
Terminal::Older(n) => script_num_size(n as usize) + 1,
Expand Down Expand Up @@ -794,7 +797,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
script_num_size(k)
+ 1
+ script_num_size(pks.len())
+ pks.iter().map(|pk| pk.serialized_len()).sum::<usize>()
+ pks.iter().map(|pk| Ctx::pk_len(pk)).sum::<usize>()
}
}
}
Expand Down
Loading