Skip to content
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.

Sign up for GitHub

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

expression: replace recursive Tree datatype with a non-recursive one #780

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

expression: replace recursive Tree datatype with a non-recursive one #780

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4137a59
expression: encapsulate threshold parsing
apoelstra Sep 2, 2024
616b6b6
expression: hide all Tree fields and encapsulate API
apoelstra Sep 2, 2024
d05a034
expression: rewrite Tree module to no longer use a recursive data type
apoelstra Sep 2, 2024
ebae0ef
miniscript: remove unused Result from extdata type_check
apoelstra Sep 3, 2024
8235d5f
descriptor: add fuzz test comparing to published rust-miniscript 12
apoelstra Aug 30, 2024
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
12 changes: 6 additions & 6 deletions src/descriptor/bare.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,8 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Bare<Pk> {
}

impl<Pk: FromStrKey> FromTree for Bare<Pk> {
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
let sub = Miniscript::<Pk, BareCtx>::from_tree(top)?;
fn from_tree(root: expression::TreeIterItem) -> Result<Self, Error> {
let sub = Miniscript::<Pk, BareCtx>::from_tree(root)?;
BareCtx::top_level_checks(&sub)?;
Bare::new(sub)
}
Expand All @@ -186,7 +186,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Bare<Pk> {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let top = expression::Tree::from_str(s)?;
Self::from_tree(&top)
Self::from_tree(top.root())
}
}

Expand Down Expand Up @@ -369,8 +369,8 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Pkh<Pk> {
}

impl<Pk: FromStrKey> FromTree for Pkh<Pk> {
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
let pk = top
fn from_tree(root: expression::TreeIterItem) -> Result<Self, Error> {
let pk = root
.verify_terminal_parent("pkh", "public key")
.map_err(Error::Parse)?;
Pkh::new(pk).map_err(Error::ContextError)
Expand All @@ -381,7 +381,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Pkh<Pk> {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let top = expression::Tree::from_str(s)?;
Self::from_tree(&top)
Self::from_tree(top.root())
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/descriptor/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,7 @@ impl Descriptor<DefiniteDescriptorKey> {

impl<Pk: FromStrKey> crate::expression::FromTree for Descriptor<Pk> {
/// Parse an expression tree into a descriptor.
fn from_tree(top: &expression::Tree) -> Result<Descriptor<Pk>, Error> {
fn from_tree(top: expression::TreeIterItem) -> Result<Descriptor<Pk>, Error> {
Ok(match (top.name(), top.n_children()) {
("pkh", 1) => Descriptor::Pkh(Pkh::from_tree(top)?),
("wpkh", 1) => Descriptor::Wpkh(Wpkh::from_tree(top)?),
Expand All @@ -981,7 +981,7 @@ impl<Pk: FromStrKey> FromStr for Descriptor<Pk> {
type Err = Error;
fn from_str(s: &str) -> Result<Descriptor<Pk>, Error> {
let top = expression::Tree::from_str(s)?;
let ret = Self::from_tree(&top)?;
let ret = Self::from_tree(top.root())?;
if let Descriptor::Tr(ref inner) = ret {
// FIXME preserve weird/broken behavior from 12.x.
// See https://github.com/rust-bitcoin/rust-miniscript/issues/734
Expand Down
8 changes: 4 additions & 4 deletions src/descriptor/segwitv0.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wsh<Pk> {
}

impl<Pk: FromStrKey> crate::expression::FromTree for Wsh<Pk> {
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
fn from_tree(top: expression::TreeIterItem) -> Result<Self, Error> {
let top = top
.verify_toplevel("wsh", 1..=1)
.map_err(From::from)
Expand Down Expand Up @@ -284,7 +284,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Wsh<Pk> {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let top = expression::Tree::from_str(s)?;
Wsh::<Pk>::from_tree(&top)
Wsh::<Pk>::from_tree(top.root())
}
}

Expand Down Expand Up @@ -483,7 +483,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wpkh<Pk> {
}

impl<Pk: FromStrKey> crate::expression::FromTree for Wpkh<Pk> {
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
fn from_tree(top: expression::TreeIterItem) -> Result<Self, Error> {
let pk = top
.verify_terminal_parent("wpkh", "public key")
.map_err(Error::Parse)?;
Expand All @@ -495,7 +495,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Wpkh<Pk> {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let top = expression::Tree::from_str(s)?;
Self::from_tree(&top)
Self::from_tree(top.root())
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/descriptor/sh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ impl<Pk: MiniscriptKey> fmt::Display for Sh<Pk> {
}

impl<Pk: FromStrKey> crate::expression::FromTree for Sh<Pk> {
fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
fn from_tree(top: expression::TreeIterItem) -> Result<Self, Error> {
let top = top
.verify_toplevel("sh", 1..=1)
.map_err(From::from)
Expand All @@ -105,7 +105,7 @@ impl<Pk: FromStrKey> core::str::FromStr for Sh<Pk> {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let top = expression::Tree::from_str(s)?;
Self::from_tree(&top)
Self::from_tree(top.root())
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/descriptor/sortedmulti.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
}

/// Parse an expression tree into a SortedMultiVec
pub fn from_tree(tree: &expression::Tree) -> Result<Self, Error>
pub fn from_tree(tree: expression::TreeIterItem) -> Result<Self, Error>
where
Pk: FromStrKey,
{
Expand Down
142 changes: 63 additions & 79 deletions src/descriptor/tr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use sync::Arc;
use super::checksum;
use crate::descriptor::DefiniteDescriptorKey;
use crate::expression::{self, FromTree};
use crate::iter::TreeLike as _;
use crate::miniscript::satisfy::{Placeholder, Satisfaction, SchnorrSigType, Witness};
use crate::miniscript::Miniscript;
use crate::plan::AssetProvider;
Expand Down Expand Up @@ -495,99 +494,84 @@ impl<Pk: FromStrKey> core::str::FromStr for Tr<Pk> {

fn from_str(s: &str) -> Result<Self, Self::Err> {
let expr_tree = expression::Tree::from_str(s)?;
Self::from_tree(&expr_tree)
Self::from_tree(expr_tree.root())
}
}

impl<Pk: FromStrKey> crate::expression::FromTree for Tr<Pk> {
fn from_tree(expr_tree: &expression::Tree) -> Result<Self, Error> {
fn from_tree(root: expression::TreeIterItem) -> Result<Self, Error> {
use crate::expression::{Parens, ParseTreeError};

expr_tree
.verify_toplevel("tr", 1..=2)
struct TreeStack<'s, Pk: MiniscriptKey> {
inner: Vec<(expression::TreeIterItem<'s>, TapTree<Pk>)>,
}

impl<'s, Pk: MiniscriptKey> TreeStack<'s, Pk> {
fn new() -> Self { Self { inner: Vec::with_capacity(128) } }
Copy link
Member

Choose a reason for hiding this comment

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

nit: Should capacity allocation be optimized to better match the expected average use case? For example, a tree with depth 4 might suffice for most scenarios. Allocating a fixed capacity of 128 for everything seems potentially excessive, especially if it goes unused in most cases.

Copy link
Member

Choose a reason for hiding this comment

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

need not be addressed in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but we're talking about a savings of a kilobyte or so of heap space. We can maybe revisit this later as part of a "can we make this library work on constrained memory devices" project.


fn push(&mut self, parent: expression::TreeIterItem<'s>, tree: TapTree<Pk>) {
let mut next_push = (parent, tree);
while let Some(top) = self.inner.pop() {
if next_push.0.index() == top.0.index() {
next_push.0 = top.0.parent().unwrap();
next_push.1 = TapTree::combine(top.1, next_push.1);
} else {
self.inner.push(top);
break;
}
}
self.inner.push(next_push);
}

fn pop_final(&mut self) -> Option<TapTree<Pk>> {
assert_eq!(self.inner.len(), 1);
self.inner.pop().map(|x| x.1)
}
}

root.verify_toplevel("tr", 1..=2)
.map_err(From::from)
.map_err(Error::Parse)?;

let mut round_paren_depth = 0;
let mut root_children = root.children();
let internal_key: Pk = root_children
.next()
.unwrap() // `verify_toplevel` above checked that first child existed
.verify_terminal("internal key")
.map_err(Error::Parse)?;

let mut internal_key = None;
let mut tree_stack = vec![];
let tap_tree = match root_children.next() {
None => return Tr::new(internal_key, None),
Some(tree) => tree,
};

for item in expr_tree.verbose_pre_order_iter() {
// Top-level "tr" node.
if item.index == 0 {
if item.is_complete {
debug_assert!(
internal_key.is_some(),
"checked above that top-level 'tr' has children"
);
let mut tree_stack = TreeStack::new();
let mut tap_tree_iter = tap_tree.pre_order_iter();
// while let construction needed because we modify the iterator inside the loop
// (by calling skip_descendants to skip over the contents of the tapscripts).
while let Some(node) = tap_tree_iter.next() {
if node.parens() == Parens::Curly {
if !node.name().is_empty() {
return Err(Error::Parse(ParseError::Tree(ParseTreeError::IncorrectName {
actual: node.name().to_owned(),
expected: "",
})));
}
} else if item.index == 1 {
// First child of tr, which must be the internal key
internal_key = item
.node
.verify_terminal("internal key")
.map_err(Error::Parse)
.map(Some)?;
node.verify_n_children("taptree branch", 2..=2)
.map_err(From::from)
.map_err(Error::Parse)?;
} else {
// From here on we are into the taptree.
if item.n_children_yielded == 0 {
match item.node.parens() {
Parens::Curly => {
if !item.node.name().is_empty() {
return Err(Error::Parse(ParseError::Tree(
ParseTreeError::IncorrectName {
actual: item.node.name().to_owned(),
expected: "",
},
)));
}
if round_paren_depth > 0 {
return Err(Error::Parse(ParseError::Tree(
ParseTreeError::IllegalCurlyBrace {
pos: item.node.children_pos(),
},
)));
}
}
Parens::Round => round_paren_depth += 1,
_ => {}
}
}
if item.is_complete {
if item.node.parens() == Parens::Curly {
if item.n_children_yielded == 2 {
let rchild = tree_stack.pop().unwrap();
let lchild = tree_stack.pop().unwrap();
tree_stack.push(TapTree::combine(lchild, rchild));
} else {
return Err(Error::Parse(ParseError::Tree(
ParseTreeError::IncorrectNumberOfChildren {
description: "Taptree node",
n_children: item.n_children_yielded,
minimum: Some(2),
maximum: Some(2),
},
)));
}
} else {
if item.node.parens() == Parens::Round {
round_paren_depth -= 1;
}
if round_paren_depth == 0 {
let script = Miniscript::from_tree(item.node)?;
// FIXME hack for https://github.com/rust-bitcoin/rust-miniscript/issues/734
if script.ty.corr.base != crate::miniscript::types::Base::B {
return Err(Error::NonTopLevel(format!("{:?}", script)));
};
tree_stack.push(TapTree::Leaf(Arc::new(script)));
}
}
}
let script = Miniscript::from_tree(node)?;
// FIXME hack for https://github.com/rust-bitcoin/rust-miniscript/issues/734
if script.ty.corr.base != crate::miniscript::types::Base::B {
return Err(Error::NonTopLevel(format!("{:?}", script)));
};

tree_stack.push(node.parent().unwrap(), TapTree::Leaf(Arc::new(script)));
tap_tree_iter.skip_descendants();
}
}
assert!(tree_stack.len() <= 1);
Tr::new(internal_key.unwrap(), tree_stack.pop())
Tr::new(internal_key, tree_stack.pop_final())
}
}

Expand Down
Loading