Eliminate a bunch more recursion; expand the TreeLike iterator trait a bit
#722
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's lazy and takes some extra allocations to use Nary for the andor combinator; introduce a dedicated Ternary variant for this. This also means that Nary will be exclusively used for thresholds, which should give us some more freedom to mess with it.
This somewhat complicates implementation of the trait, but not by much, and eliminates a ton of refcount accesses and increments, and a ton of Arc::clones. As we will see, having an Arc<[T]> is somewhat restrictive in addition to requiring allocations. Later we will want to replace this with an enum that covers multiple kinds of arrays. I was unsure whether to name the new associated type NAry or Nary. It seems more natural to use NAry but the existing Tree::Nary variant uses the lowercase spelling so I stuck with that. (And similarly used nary_ for the method names rather than n_ary.)
This uses the new iterator extensions to implement a `DisplayNode` wrapper around a `Terminal` which understands that pubkeys, threshold k values, etc., count as "children" for display purposes. We can then embed all the alias/wrapper logic in the `as_node` method and the new `fragment_name` method. The latter is generally useful and ought to be public; given a fragment, it outputs its name, without recursing but with consideration of the aliasing rules. Previously this logic was embedded in the Terminal::condition_fmt method. The resulting display algorithm is much easier to follow, although it uses more lines of code (primarily in the form of large repetitive match statements). cargo bloat shows there is a slight reduction in code size attributed to the miniscript crate, though a slight increase in the total code size for an example binary which basically implements the `string_rtt` test. I think with some effort we should be able to reduce the code size of this algorithm. The important thing though is that it's not recursive. The exact results are as follows (though they have limited use without my committing the actual example program, which is a little ugly and not generally useful). Before: File .text Size Crate Name 0.0% 0.9% 7.3KiB miniscript miniscript::miniscript::astelem::<impl miniscript::expression::FromTree for miniscript::miniscript::decode::Terminal<Pk,Ctx>>::from_tree 0.0% 0.7% 5.8KiB miniscript miniscript::miniscript::types::Type::type_check 0.0% 0.6% 4.4KiB miniscript miniscript::miniscript::types::extra_props::ExtData::threshold 0.0% 0.5% 4.0KiB miniscript <miniscript::Error as core::fmt::Display>::fmt 0.0% 0.5% 4.0KiB miniscript miniscript::miniscript::astelem::<impl miniscript::miniscript::decode::Terminal<Pk,Ctx>>::conditional_fmt 0.0% 0.5% 3.7KiB miniscript miniscript::miniscript::wrap_into_miniscript 0.0% 0.4% 3.0KiB miniscript miniscript::miniscript::types::extra_props::ExtData::or_i 0.0% 0.4% 3.0KiB miniscript <miniscript::miniscript::decode::Terminal<Pk,Ctx> as core::cmp::PartialEq>::eq 0.0% 0.3% 2.8KiB miniscript <miniscript::miniscript::types::Error as core::fmt::Display>::fmt 0.0% 0.3% 2.5KiB miniscript miniscript::expression::Tree::from_slice_delim 0.0% 0.3% 2.4KiB miniscript miniscript::miniscript::types::extra_props::ExtData::type_check 0.0% 0.3% 2.4KiB miniscript alloc::collections::btree::node::BalancingContext<K,V>::bulk_steal_left 0.0% 0.3% 2.2KiB miniscript miniscript::miniscript::split_expression_name 0.0% 0.3% 2.2KiB miniscript <miniscript::Error as core::fmt::Debug>::fmt 0.0% 0.3% 2.1KiB miniscript miniscript::miniscript::types::extra_props::ExtData::and_or 0.0% 0.2% 1.9KiB miniscript miniscript::miniscript::types::extra_props::ExtData::or_b 0.0% 0.2% 1.9KiB miniscript core::slice::sort::merge 0.0% 0.2% 1.8KiB miniscript core::slice::sort::merge 0.0% 0.2% 1.8KiB miniscript core::slice::sort::merge 0.0% 0.2% 1.8KiB miniscript core::slice::sort::merge 0.7% 33.0% 262.1KiB And 1454 smaller methods. Use -n N to show more. 0.9% 40.7% 323.2KiB filtered data size, the file size is 34.8MiB After: File .text Size Crate Name 0.0% 0.9% 7.3KiB miniscript miniscript::miniscript::astelem::<impl miniscript::expression::FromTree for miniscript::miniscript::decode::Terminal<Pk,Ctx>>::from_tree 0.0% 0.7% 5.8KiB miniscript miniscript::miniscript::types::Type::type_check 0.0% 0.7% 5.7KiB miniscript miniscript::miniscript::display::<impl miniscript::miniscript::decode::Terminal<Pk,Ctx>>::conditional_fmt 0.0% 0.6% 4.4KiB miniscript miniscript::miniscript::types::extra_props::ExtData::threshold 0.0% 0.5% 4.3KiB miniscript <miniscript::miniscript::display::DisplayNode<Pk,Ctx> as miniscript::iter::tree::TreeLike>::as_node 0.0% 0.5% 4.0KiB miniscript <miniscript::Error as core::fmt::Display>::fmt 0.0% 0.5% 3.7KiB miniscript miniscript::miniscript::wrap_into_miniscript 0.0% 0.4% 3.0KiB miniscript miniscript::miniscript::types::extra_props::ExtData::or_i 0.0% 0.3% 2.8KiB miniscript <miniscript::miniscript::types::Error as core::fmt::Display>::fmt 0.0% 0.3% 2.5KiB miniscript miniscript::expression::Tree::from_slice_delim 0.0% 0.3% 2.4KiB miniscript miniscript::miniscript::types::extra_props::ExtData::type_check 0.0% 0.3% 2.4KiB miniscript alloc::collections::btree::node::BalancingContext<K,V>::bulk_steal_left 0.0% 0.3% 2.2KiB miniscript miniscript::miniscript::split_expression_name 0.0% 0.3% 2.2KiB miniscript <miniscript::Error as core::fmt::Debug>::fmt 0.0% 0.3% 2.1KiB miniscript miniscript::miniscript::types::extra_props::ExtData::and_or 0.0% 0.2% 1.9KiB miniscript miniscript::miniscript::types::extra_props::ExtData::or_b 0.0% 0.2% 1.9KiB miniscript core::slice::sort::merge 0.0% 0.2% 1.8KiB miniscript core::slice::sort::merge 0.0% 0.2% 1.8KiB miniscript core::slice::sort::merge 0.0% 0.2% 1.8KiB miniscript core::slice::sort::merge 0.7% 32.2% 256.3KiB And 1423 smaller methods. Use -n N to show more. 0.9% 40.2% 320.4KiB filtered data size, the file size is 35.2MiB
This implements an ordering based on the display ordering, though a more efficiently than just serializing to strings and comparing those. Empirically this adds a couple kb to the total code size, but not as much as the nonrecursive fmt shrunk it. We expect somewhat larger code to implement such a nontrivial algorithm nonrecursively (and in fact the old algorithm was "wrong" in that it did not implement any coherent ordering). This is a breaking change because it changes the order of Miniscripts.
apoelstra
force-pushed
the
2024-08--recursion
branch
from
1b53dec to
d0c0c14
Compare
I don't see any harm in having a "deep copy". Or we can wait for someone to complain or explicitly request that feature. I can forsee users using keys with interior mutability when scanning funds from descriptors keys containing wildcards. |
sanket1729
approved these changes
Aug 19, 2024
| /// Display no types. | ||
| None, | ||
| /// Display all types, including the initial type. | ||
| All(Type), |
There was a problem hiding this comment.
nit:
In cccaaec, why do we need Type argument here? We are always calling this with self.ty?
There was a problem hiding this comment.
@sanket1729 yes, but the self of self.ty is a Miniscript while the self of conditional_fmt is a Terminal. So we do need to pass in self.ty explicitly like this. It's not available from conditional_fmt otherwise.
Nice. I'll PR it since I've already got the code written. |
apoelstra
added a commit
to apoelstra/rust-miniscript
that referenced
this pull request
Aug 31, 2024
We are incorrectly serializing and_n as "and_b". In master this is fixed by rust-bitcoin#722 which rewrites the Display impl completely.
apoelstra
added a commit
to apoelstra/rust-miniscript
that referenced
this pull request
Sep 1, 2024
We are incorrectly serializing and_n as "and_b". In master this is fixed by rust-bitcoin#722 which rewrites the Display impl completely.
apoelstra
added a commit
to apoelstra/rust-miniscript
that referenced
this pull request
Sep 1, 2024
When fuzzing my new non-recursive tree parsing logic, I noticed that we were deviating from the released 12.0 in the way that we display l:0. This is an ambiguous fragment which can be displayed either as l:0 or u:0. In our released code we use u:0 so stick with that. This was unintentially changed as part of rust-bitcoin#722. Change it back. While we are at it add a regression test for rust-bitcoin#735
apoelstra
added a commit
that referenced
this pull request
Sep 3, 2024
1259375 miniscript: make display prefer 'u' over 'l' in the fragment l:0 (Andrew Poelstra) 67fdc50 descriptor: reject strings of the form "tr(<key>,)" (Andrew Poelstra) 00cac40 descriptor: add unit test demonstrating sanity-checking behavior in <= 12.x (Andrew Poelstra) Pull request description: This PR has three changes which are mostly unrelated except that they were all found when fuzzing my "rewrite expression parser to be nonrecursive" branch against 12.x. * First is a unit test demonstrating #734. It doesn't fix anything, just tests the current behavior. * Second is a fix for #736 (backported in #735). * Third tweaks the new `Display` code from #722 to change how the ambiguous `l:0`/`u:0` is serialized, to match 12.x. ACKs for top commit: sanket1729: ACK 1259375 Tree-SHA512: 921d65a1efd49bda0f9db488a2854b004e14518f584d832497a9dbc13a845ceec99544375385570c6ac42d4985277e8dcbb3aa8654de93235cf9067ba601f91d
apoelstra
added a commit
that referenced
this pull request
Nov 27, 2024
9cedc49 bump version to 12.3.0 (Andrew Poelstra) 9420012 fix: zero sequence check on relative time (Chris Hyunhum Cho) 5147c6f descriptor: don't accept strings of the form tr(<key>,) (Andrew Poelstra) 6401c22 miniscript: fix string serialization of and_n (Andrew Poelstra) Pull request description: We are incorrectly serializing and_n as "and_b", and incorrectly parsing `tr(<key>,)`. In master the `and_b` thing is fixed by #722 which rewrites the Display impl completely and the `tr(<key>,)` thing will be fixed as part of a coming series of PRs to clean up expression parsing. Also includes #740 since it showed up in time. ACKs for top commit: sanket1729: ACK 9cedc49 Tree-SHA512: edd45781db5b712e6013a396f6840ce74a6b32861b58a4df25e457c46c845c225dcde95f6c33db7fa314a1c4d2857515f40608d9c66e9432994cadf3d2f10e78
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces the recursive derived implementations of
PartialEq,PartialOrd,Hash,fmt::Debugandfmt::Display. Along the way it expands theiter::TreeLiketrait to make it a bit more useful (adding aTernaryvariant and making the existingNaryvariant generic as long as you can get a length out of it and index into it).The new
fmt::Debugmethod works using a newDisplayNodewrapper aroundTerminalwhich allows iterating over a script in the same way as it's displayed, treating things likeor_i(0, X)asl:Xand implementing thec:pk_kaliases and so on. This seems generally useful but this PR does not expose it in the public API because I'd like to make some breaking changes toTerminaldown the line and don't want to expand the API.The new
Ordimpl orders things alphabetically (assuming it is bug-free at least) by using the same iterator as the display logic. This is both nonrecursive and a more useful ordering for anybody who cares about the exact ordering. On the other hand, if anybody is depending on the existing ordering this will break their code. I would assume not, since the existing ordering is a weird ad-hoc thing based on the order thatderive(PartialOrd)happens to use.Similarly, it adds a
Terminal::fragment_nameaccessor which returns a&'static strrepresenting the fragment name as displayed. This is also not put into the public API.The block of type parameters used in the debug output of Miniscripts is moved into a
fmt::Displayimpl onTypeitself. This is part of the public API.This PR does not replace the
Cloneimpl, which is currently derived. This impl actually isn't recursive, since it just clones theArcs in the first layer of the script. Maybe we want to implement this manually and do a "deep copy"? This would be useful for users who have keys with interior mutability or something. I have a followup which does this, but didn't include it because I felt it might be controversial and need its own discussion.This PR also does not replace the implicit
Dropimpl, because that's hard to do and I haven't done it. But we're making progress.