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.

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

Fix max_satisfaction_weight calculations for taproot. #473

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
8 changes: 4 additions & 4 deletions examples/taproot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,11 @@ fn main() {

// Max Satisfaction Weight for compilation, corresponding to the script-path spend
// `multi_a(2,PUBKEY_1,PUBKEY_2) at taptree depth 1, having
// Max Witness Size = scriptSig len + control_block size + varint(script_size) + script_size +
// varint(max satisfaction elements) + max satisfaction size
// = 4 + 65 + 1 + 70 + 1 + 132
// Max Witness Size = scriptSig len + witnessStack len + varint(control_block_size) +
// control_block size + varint(script_size) + script_size + max_satisfaction_size
// = 4 + 1 + 1 + 65 + 1 + 70 + 132 = 274
let max_sat_wt = real_desc.max_satisfaction_weight().unwrap();
assert_eq!(max_sat_wt, 273);
assert_eq!(max_sat_wt, 274);

// Compute the bitcoin address and check if it matches
let network = Network::Bitcoin;
Expand Down
52 changes: 31 additions & 21 deletions src/descriptor/tr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,27 +264,37 @@ impl<Pk: MiniscriptKey> Tr<Pk> {
/// # Errors
/// When the descriptor is impossible to safisfy (ex: sh(OP_FALSE)).
pub fn max_satisfaction_weight(&self) -> Result<usize, Error> {
let mut max_wieght = Some(65);
for (depth, ms) in self.iter_scripts() {
let script_size = ms.script_size();
let max_sat_elems = match ms.max_satisfaction_witness_elements() {
Ok(elem) => elem,
Err(..) => continue,
};
let max_sat_size = match ms.max_satisfaction_size() {
Ok(sz) => sz,
Err(..) => continue,
};
let control_block_sz = control_block_len(depth);
let wit_size = 4 + // scriptSig len byte
control_block_sz + // first element control block
varint_len(script_size) +
script_size + // second element script len with prefix
varint_len(max_sat_elems) +
max_sat_size; // witness
max_wieght = cmp::max(max_wieght, Some(wit_size));
}
max_wieght.ok_or(Error::ImpossibleSatisfaction)
let tree = match self.taptree() {
// key spend path:
// scriptSigLen(4) + stackLen(1) + stack[Sig]Len(1) + stack[Sig](65)
None => return Ok(4 + 1 + 1 + 65),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the 65 here is implicitly taking into account an optional sighash flag (because we're doing max I guess this strictly correct). Maybe worth doing a 64 + 1 to indicate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LLFourn Thank you for the review.

However, at the same time, the sig+sigHash is a single stack item? Maybe a comment would be more appropriate?

// script path spend..
Some(tree) => tree,
};

tree.iter()
.filter_map(|(depth, ms)| {
let script_size = ms.script_size();
let max_sat_elems = ms.max_satisfaction_witness_elements().ok()?;
let max_sat_size = ms.max_satisfaction_size().ok()?;
let control_block_size = control_block_len(depth);
Some(
// scriptSig len byte
4 +
// witness field stack len (+2 for control block & script)
varint_len(max_sat_elems + 2) +
// size of elements to satisfy script
max_sat_size +
// second to last element: script
varint_len(script_size) +
script_size +
// last element: control block
varint_len(control_block_size) +
control_block_size,
)
})
.max()
.ok_or(Error::ImpossibleSatisfaction)
}
}

Expand Down