-
Notifications
You must be signed in to change notification settings - Fork 58
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
descriptors: fix max satisfaction weight for Taproot multisig primary path spends #1451
descriptors: fix max satisfaction weight for Taproot multisig primary path spends #1451
Conversation
This includes the sizes of the script and control block elements.
This reverts commit 74a53ba. I also removed the extra sats being added in the functional test.
// We need to calculate the size manually before calculating the varint length. | ||
// See https://docs.rs/miniscript/11.0.0/src/miniscript/util.rs.html#35-36. | ||
Placeholder::TapScript(s) => varint_len(s.len()), | ||
Placeholder::TapControlBlock(cb) => varint_len(cb.serialize().len()), | ||
_ => 0, |
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.
So any stack item but the leaf script and control block account for the varint length? This is incredibly confusing, it'd be worth opening an issue there. (And if they fix it it would be nice to also fix rust-bitcoin/rust-miniscript#701.)
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.
It appears so. I did some debugging to inspect all the elements and their sizes against an actual transaction and the sizes of these two placeholder elements didn't include the varint lengths.
I've created rust-bitcoin/rust-miniscript#771 to track this.
light-ACK. The code looks correct to me but i haven't reviewed the test nor reviewed in depth but from a quick skim of their codebase it seems correct. |
From Andrew Poelstra comment: rust-bitcoin/rust-miniscript#771, the issue is acknowledged and it seems you are on the good track. I am okay to proceed to merge these changes. |
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.
utACK 6e74a96
e4ae6dc descriptors: include segwit marker and flag (Michael Mallan) 03a3230 descriptors: add method to get max tx weight (Michael Mallan) Pull request description: This fixes #1406, building on the fix from #1451 for estimating the weight of each input. An extra 2 is added to the unsigned transaction's weight to account for the Segwit marker and flag. ACKs for top commit: edouardparis: utACK e4ae6dc Tree-SHA512: de78763d85ed38d5e2797d6f95b1ef293bbedf3cd4d5d1f69085bd134a117ae270a24c3487d738398191fb4126c4f8319bba53b902cec1625dad1c501f3f2aa9
This is a fix to #1371 and the related #1322.
As per #1371 (comment), this adds the varint length for the script and control block elements of a Taproot primary path spend.
I think this is what caused #1322 and so I have reverted the change from #1323.