Add IntoIterator<Item = &Pk> for various types

[stevenroose]

Related to #223. Might make ForEachKey obsolete. However, these impls use the heap with Boxes.

[sgeisler]

I feel quite neutral towards this particular implementation, it's probably good enough. But I'm wondering how hard it would be to, instead of essentially re-building the miniscript tree as an interator tree, to build a custom iterator that can traverse the miniscript tree. I'll take a more thorough look once the fuzz test runs through.

[stevenroose]

I never really understand the fuzzer error outputs.. Anyone that does? https://github.com/rust-bitcoin/rust-miniscript/runs/3076686285

About your comment, @sgeisler I thought about a custom iterator struct that takes keeps a descriptor reference and some "incrementor metadata" to see where it is. You'd kinda need inspection into the descriptor, but I think it'd be possible. Not really sure how the different Terminal vs Policy vs Miniscript vs Descriptor implementations would interact, though.

[sanket1729]

@stevenroose, the fuzzer bug is not a bug in miniscript, but a bug in rust-secp-zkp handwritten crypto for fuzzing.

[sgeisler]

Afaik there exists the quite common case that the key "hash" is actually also a key in our representation of miniscript (e.g. when using a descriptor to derive child descriptors). For the key mapping functions there exist very ugly-named different functions for that (TranslatePk1-TranslatePk3). It might be worth thinking about this here too, because a user may not care about if it is a p2pkh or p2pk node and just wants to treat them all the same.

[stevenroose]

Hmm, I think only for TranslatePk2 that could be relevant here. Though if the "pubkey hash" type is secretly a key type, that's something pretty messy. It seems that indeed for DescriptorPublicKey that is the case and the Self::Hash type is set to Self. That's pretty neat.

I wonder if we can do some conditional on the K::Hash type also being K or something.

[stevenroose]

Hmm, it would create conflicting implementations and also it's not possible to do equality conditions in where clauses. So I can't see how it can be done without creating messy alternative traits and the thing I liked about this impl is that it doesn't need a trait.

As an alternative, I thought about having an IntoIterator where the output iter type is an iterator of Pk::Hash items and we'd call hash on all keys, so that you'd get all the key hashes. But I think if Pk and Pk::Hash are identical, it might again cause problems of duplicate implementations. Because then the type system won't figure out which iterator you want.

[stevenroose]

We could yield an iterator of miniscript::ForEach elements. That name is a bit unfortunately chosen though because it's named with the use case it's used in instead of actually naming what it is.

[stevenroose]

See my comment here: #252 (comment)

[sgeisler]

@mmm0171 what are you trying to tell us?

[RCasatta]

First I thought it was an hash of a CVE

Then I recognized the Ethereum address mixed case style

[stevenroose]

So I added fixup commits that would change the implementation to yield an iterator of ForEach elements so that you can also get easy access to key hashes.

However the ForEach type is named quite specifically for the for_each method use-case, so perhaps it makes sense to find a more general-purpose name for it like KeyRef or KeyId or something?

[sanket1729]

CR ack 3a73c7d. This looks good to me.

I will wait on Andrew to comment before proceeding on this because he primarily implemented the ForEachKey related stuff.

[apoelstra]

I don't think it makes sense to have the iterator return only public keys. Miniscripts are not public key containers, they contain different kinds of data. It might make sense to iterate over all the sub-miniscripts, or over all the terminals, or over all the pk/hash/timelocks.

[stevenroose]

Hmm, I mean keys are the most meaningful things miniscripts contain. Other than keys there's timelocks and hashes.

I'd be ok trying to make this into a more general iterator over interesting things, MiniscriptIterator or so that iterators over MiniscriptItem::{PublicKey, Hash256, Sha256, Hash160, Timelock} or something. If we have a newtype on it, we can even define filters on them fn keys(self) -> impl Interator<Item = &PublicKey>.

[apoelstra]

I like the idea of having newtypes that can wrap (references to) things! Would be cool if we could genericize this so it worked on all the objects in the crate.

[dr-orlovsky]

Just as a reminder: we already have iterators for miniscript items: https://github.com/rust-bitcoin/rust-miniscript/blob/master/src/miniscript/iter.rs