Eliminate a bunch of recursion #567
Merged
Conversation
This file contains hidden or 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
apoelstra
force-pushed
the
2023-07--dag-overhaul
branch
from
311135f to
6250791
Compare
Introduces the new `iter` module which provides iteration abilities over various tree-like structures. This is largely copied from rust-simplicity, but is both simpler (because there is no sharing) and more complicated (because we need to handle n-ary nodes, not just binary ones). This currently uses Arc<[T]> in a number of places; we should eventually replace these with some sort of ArrayVec-type structure.
This is a breaking change because we no longer implement `ForEachKey` on `Terminal`. But IMHO we should never have implemented that trait (or any trait, really..) directly on `Terminal`. The fact that we did was just an implementation detail of how we used to do a lot of iteration.
apoelstra
force-pushed
the
2023-07--dag-overhaul
branch
3 times, most recently
from
654e234 to
0576007
Compare
sanket1729
approved these changes
Jul 15, 2023
Member
sanket1729
left a comment
sanket1729
left a comment
There was a problem hiding this comment.
Some commits are empty? Is that intentional.
ACK 0576007.
| _ => {} | ||
| } | ||
| } | ||
| true |
Member
There was a problem hiding this comment.
After reading about possible compiler optimization speedup from using functional style code. I feel we can write this as:
self.pre_order_iter().all(|ms| match ms.node {
Terminal::PkK(ref p) => pred(p),
Terminal::PkH(ref p) => pred(p),
Terminal::Multi(_, ref keys) | Terminal::MultiA(_, ref keys) => {
keys.iter().all(|k| pred(k))
}
_ => true,
})
Member
There was a problem hiding this comment.
Member
|
Agreed, this is a step in the right direction. |
Member
|
I can merge these and address the nits in a follow up PR. Just waiting for your response about the empty commits. |
Member
Author
|
@sanket1729 no, I'll remove the empty commits. I believe those occur during rebasing when my gpg agent fails to sign a commit. I'll do that and then you can merge this. For optimizations I think we should first introduce a wider set of benchmarks (miniscript parsing from string, decoding from script, serializing to string, encoding to script, then some borderline-trivial key conversions etc). |
This is again a breaking change because we remove the trait impl from `Terminal`, but again I don't think the trait impl should've existed. This also exposed an "off-label" use of `real_translate_pk` to convert context types as well as pk types, which I apparently explicitly reviewed and ACKed when reviewing rust-bitcoin#426, but which in retrospect looks a little funny. Rename this function to translate_pk_ctx so it's clear that it's doing two jobs.
As usual, we remove a method of the same name from `Terminal`, which is a breaking change.
As always, dropping the same method from Terminal
apoelstra
force-pushed
the
2023-07--dag-overhaul
branch
from
0576007 to
67e91b9
Compare
tcharding
reviewed
Aug 30, 2023
| @@ -0,0 +1,93 @@ | |||
| // Written in 2023 by Andrew Poelstra <apoelstra@wpsoftware.net> | |||
Member
There was a problem hiding this comment.
Hey @apoelstra, I saw this line and wondered if you added it as a habit or if I have been annoying you by removing them? Or was this PR just before we discussed all that? Cheers.
Member
Author
There was a problem hiding this comment.
I think it predates your PR to remove these. I would've just mildlessly copied the line from other files.
I'm certainly not annoyed :).
heap-coder
added a commit
to heap-coder/rust-miniscript
that referenced
this pull request
Sep 27, 2025
67e91b9115031c7f773b67314370685e0c5d2f03 miniscript: eliminate recursion in substitute_raw_pkh (Andrew Poelstra)
d858ffc30643261189d805391e5a17a26cc53f80 miniscript: remove recursion from script_size (Andrew Poelstra)
8c32a19b4813b7030bc6851a30a1d38ce4241bc3 miniscript: eliminate recursion from TranslatePk (Andrew Poelstra)
2930938783f9953e1c1c9b3f14ab7131b92e1787 miniscript: eliminate recursion in for_each_key (Andrew Poelstra)
3c0ff73d14e0a570225cec3a543bb0191e38acda iter: add module, copied in large from rust-simplicity (Andrew Poelstra)
Pull request description:
This introduces a new `iter` module with facilities for iterating over tree structures, and uses it to eliminate a bunch of recursion.
There's a lot more "obvious" work after this, so I figured I'd PR this now for review and then do followup ones if it's accepted.
Then there's some less obvious work around getting rid of the recursion in `Expression::from_str` and tightening up the memory allocation/locality there, and some even less obvious work in improving memory allocation/locality in `Miniscript`. But even if those later stages don't work out, this is a step in the right direction because it eliminates a bunch of stack overflow vectors, and reduces the amount of code.
ACKs for top commit:
sanket1729:
reACK 67e91b9115031c7f773b67314370685e0c5d2f03
Tree-SHA512: 30038e2e185f9ff5a87d32508a03db61f7de9997900ce381df3974f5dc1818402a2b444808907f5d849f7ca2f9812dd0c2f578a4fbad56ef2cd01b9382a60d70
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 introduces a new
itermodule with facilities for iterating over tree structures, and uses it to eliminate a bunch of recursion.There's a lot more "obvious" work after this, so I figured I'd PR this now for review and then do followup ones if it's accepted.
Then there's some less obvious work around getting rid of the recursion in
Expression::from_strand tightening up the memory allocation/locality there, and some even less obvious work in improving memory allocation/locality inMiniscript. But even if those later stages don't work out, this is a step in the right direction because it eliminates a bunch of stack overflow vectors, and reduces the amount of code.