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

Policy refactor #1228

Merged
merged 3 commits into from
Jun 12, 2024
Merged

Policy refactor #1228

merged 3 commits into from
Jun 12, 2024

Conversation

benma
Copy link
Collaborator

@benma benma commented Jun 12, 2024

No description provided.

benma added 2 commits June 12, 2024 14:50
Turn it into a struct containing the original policy, so we don't have
to repeat it in every enum arm when we add more arms.
So it can be reused when we want to check dups in Tapscripts, when
adding support for Taproot `tr(...)` descriptors/policies.
@benma benma requested a review from asi345 June 12, 2024 14:10
By converting the script configs into a parsed/validated form, we gain
the following advantages:

- for policies, we only parse/validate it once per tx, instead of also
twice per input (compute pk_script, compute sighash_script), and twice for each change output (to
check if it is really change and to compute the payload), making
policy transactions much more efficient
- the match patterns are much simpler as it removes SimpleType parsing
and all the protobuf nesting and unreachable `_ => ...` patterns
- we will be able to check if the script config is a Taproot policy in `is_taproot()`
without parsing it once more once we add Taproot policies
Copy link
Contributor

@asi345 asi345 left a comment

Choose a reason for hiding this comment

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

tACK, i can't see a problem.
However I could not understand why this change was needed. Can you summarize shortly?

@benma
Copy link
Collaborator Author

benma commented Jun 12, 2024

tACK, i can't see a problem. However I could not understand why this change was needed. Can you summarize shortly?

Thanks. Have you checked the commit messages? The script configs preprocess commit has a long explanation. The others are to prepare adding a Tr(...) Taproot enum entry next to Wsh() in ParsedPolicy, as Taproot policies will need re-use this.

@benma benma merged commit 50b05b6 into BitBoxSwiss:master Jun 12, 2024
1 check passed
@benma benma deleted the policy-refactor branch June 12, 2024 20:33
@asi345
Copy link
Contributor

asi345 commented Jun 13, 2024

Oh you're right. Sorry I just missed the commit message paragraphs. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants