- 
                Notifications
    You must be signed in to change notification settings 
- Fork 168
expression: implement parsing of policies and miniscripts non-recursively #784
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
expression: implement parsing of policies and miniscripts non-recursively #784
Conversation
All of the terminals in Miniscript can be constructed infallibly. Add constructors for this so we don't need to go through Miniscript::from_ast and deal with an impossible-to-hit error path.
This also drops the FromTree impls for Terminal and Arc<Terminal>. As argued elsewhere, the Terminal types are not first-class types and it doesn't make sense to allow them to be constructed in elaborate ways. The benchmarks are pretty noisy but appear to show a pretty big speedup. On master: test expression::benches::parse_tree ... bench: 885.43 ns/iter (+/- 412.97) test expression::benches::parse_tree_deep ... bench: 1,951.10 ns/iter (+/- 845.66) test miniscript::benches::parse_segwit0 ... bench: 9,571.35 ns/iter (+/- 565.98) test miniscript::benches::parse_segwit0_deep ... bench: 25,571.58 ns/iter (+/- 468.93) On this commit: test expression::benches::parse_tree ... bench: 1,020.52 ns/iter (+/- 139.58) test expression::benches::parse_tree_deep ... bench: 1,632.75 ns/iter (+/- 176.35) test miniscript::benches::parse_segwit0 ... bench: 10,747.28 ns/iter (+/- 2,338.26) test miniscript::benches::parse_segwit0_deep ... bench: 19,090.65 ns/iter (+/- 1,040.62) So if you take the numbers at face value, that's a 33% speedup on the parse_segwit0_deep benchmark. Nice.
| Benchmarks on master (copied from #775 (comment) ) Benchmarks on masterBenchmarks on this PRYou can see a 5-10% improvement across the board, which honestly isn't as great as I was expecting. But this leaves our types and error structures in much better shape so I think it was worth the effort. | 
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.
ACK 26c6dc3
Nice. 🚀 , no more recursion. Stack overflow bugs.
… of policies and miniscripts non-recursively
26c6dc3a51498dfb641ae2be3db22f8a7a9755d6 miniscript: implement from_tree nonrecursively (Andrew Poelstra)
2c422af5478ea9d1016b007b83a2d13c07be3163 policy: make Concrete::from_tree non-recursive (Andrew Poelstra)
61a89a2655ad04a6bf03329ad730410957635fb9 policy: make Semantic::from_tree non-recursive (Andrew Poelstra)
8a78025160fb8ba6e18c7ccaed728dc54796a743 miniscript: add constructors for all the terminals (Andrew Poelstra)
Pull request description:
  This brings us to the end of my cached "rewrite expression module" branch, by making expression parsing non-recursive. The diff here is actually reasonably short but it's pretty invasive. Hopefully the new fuzz test from the last PR is enough to convince us of correctness.
  I have some more work to do in two directions:
  * Breaking up the two `Policy` types into a structs with inner enums (or possibly just a flat tree type), which will allow us to attach metadata to nodes and trees; after this, I will introduce the `ValidationParams` structure and start moving sanity/validation/parsing logic into there as suggested in #723. I have some incomplete commits in this direction but I probably want to redo them with lessons learned from the Taproot work.
  * Replacing the `TapTree` enum with a non-recursive type, which will let us replace the taproot iteration API with a more natural one (as well as getting some performance benefit). I have more-complete work on this, but I'm getting a bit hamstrung right now handling the difference between `MiniscriptKey` and `ToPublicKey` in this specialization-less language.
ACKs for top commit:
  sanket1729:
    ACK 26c6dc3a51498dfb641ae2be3db22f8a7a9755d6
Tree-SHA512: b576d779ec1bf495176867e8e35602e1ed8e2a6a9df99fb0058ab437f9e43cf680a0b3bd459c27ba4e21ac58f534b35f7cac6566273c66615d7dc47015243c9a
    
This brings us to the end of my cached "rewrite expression module" branch, by making expression parsing non-recursive. The diff here is actually reasonably short but it's pretty invasive. Hopefully the new fuzz test from the last PR is enough to convince us of correctness.
I have some more work to do in two directions:
Policytypes into a structs with inner enums (or possibly just a flat tree type), which will allow us to attach metadata to nodes and trees; after this, I will introduce theValidationParamsstructure and start moving sanity/validation/parsing logic into there as suggested in Can we eliminate the `Ctx` parameter? #723. I have some incomplete commits in this direction but I probably want to redo them with lessons learned from the Taproot work.TapTreeenum with a non-recursive type, which will let us replace the taproot iteration API with a more natural one (as well as getting some performance benefit). I have more-complete work on this, but I'm getting a bit hamstrung right now handling the difference betweenMiniscriptKeyandToPublicKeyin this specialization-less language.