Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

skip policy compilation when parsing descriptor #1553

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
32 changes: 28 additions & 4 deletions liana/src/descriptors/analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -464,10 +464,14 @@ pub struct LianaPolicy {

impl LianaPolicy {
/// Create a new Liana policy from a given configuration.
///
/// `compile` controls whether to check the policy compiles
/// to miniscript before returning.
fn _new(
primary_path: PathInfo,
recovery_paths: BTreeMap<u16, PathInfo>,
is_taproot: bool,
compile: bool,
) -> Result<LianaPolicy, LianaPolicyError> {
if recovery_paths.is_empty() {
return Err(LianaPolicyError::MissingRecoveryPath);
Expand Down Expand Up @@ -523,7 +527,9 @@ impl LianaPolicy {
recovery_paths,
is_taproot,
};
policy.clone().into_multipath_descriptor_fallible()?;
if compile {
policy.clone().into_multipath_descriptor_fallible()?;
}
Ok(policy)
}

Expand All @@ -532,15 +538,25 @@ impl LianaPolicy {
primary_path: PathInfo,
recovery_paths: BTreeMap<u16, PathInfo>,
) -> Result<LianaPolicy, LianaPolicyError> {
Self::_new(primary_path, recovery_paths, /* is_taproot = */ true)
Self::_new(
primary_path,
recovery_paths,
/* is_taproot = */ true,
/* compile = */ true,
)
}

/// Create a new Liana policy for use under a P2WSH context.
pub fn new_legacy(
primary_path: PathInfo,
recovery_paths: BTreeMap<u16, PathInfo>,
) -> Result<LianaPolicy, LianaPolicyError> {
Self::_new(primary_path, recovery_paths, /* is_taproot = */ false)
Self::_new(
primary_path,
recovery_paths,
/* is_taproot = */ false,
/* compile = */ true,
)
}

/// Create a Liana policy from a descriptor. This will check the descriptor is correctly formed
Expand Down Expand Up @@ -631,7 +647,15 @@ impl LianaPolicy {
// Use the constructor for sanity checking the keys and the Miniscript policy. Note this
// makes sure the recovery paths mapping isn't empty, too.
let prim_path = primary_path.ok_or(LianaPolicyError::IncompatibleDesc)?;
LianaPolicy::_new(prim_path, recovery_paths, is_taproot)
// We don't compile the policy as we assume it compiles given we started with a descriptor.
// This will still perform all other checks to make sure the descriptor conforms to
// a Liana policy.
LianaPolicy::_new(
prim_path,
recovery_paths,
is_taproot,
/* compile = */ false,
)
Comment on lines +650 to +658
Copy link
Member

Choose a reason for hiding this comment

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

The policy compilation would perform Miniscript sanity checks. Is it necessary to manually call sanity_check on the descriptor instead? Or maybe it's already called at parsing time (although technically this function is not aware of that).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing this out 🙏

Yes I think then it makes sense to call this sanity_check method on the descriptor within the LianaDescriptor::from_str method.

I've checked that doing so does not noticeably harm performance when running the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like they're only called for Taproot: rust-bitcoin/rust-miniscript#734.

To be on the safe side, I've added a call to this method in all cases.

}

pub fn primary_path(&self) -> &PathInfo {
Expand Down
3 changes: 3 additions & 0 deletions liana/src/descriptors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,10 @@ impl str::FromStr for LianaDescriptor {
fn from_str(s: &str) -> Result<LianaDescriptor, Self::Err> {
// Parse a descriptor and check it is a multipath descriptor corresponding to a valid Liana
// spending policy.
// Sanity checks are not always performed when calling `Descriptor::from_str`, so we perform
// them explicitly. See https://github.com/rust-bitcoin/rust-miniscript/issues/734.
let desc = descriptor::Descriptor::<descriptor::DescriptorPublicKey>::from_str(s)
.and_then(|desc| desc.sanity_check().map(|_| desc))
.map_err(LianaDescError::Miniscript)?;
LianaPolicy::from_multipath_descriptor(&desc)?;

Expand Down
Loading