Custom spk iterator

[LagginTimes]

PR for #904

What is done:

• Added SpkIterator with new and new_with_range constructors, along with its own nth() and next() implementations
• Replaced all instances of range_descriptor_spks with the new SpkIterator::new_with_range constructor
• Created wildcard and non-wildcard unit tests for nth() and next() functions for SpkIterator

What is in-progress:

• @evanlinjin: Discussing the necessity of Cow<> in this implementation
• @LagginTimes: Documentation for the new functions

[benthecarman]

@LagginTimes any updates here, this is blocking #894

[evanlinjin]

I think the next step would be to squash all these commits in this PR into one commit and cherry-pick it onto #894 to see if we can fix #894 (comment).

[LLFourn]

Looking good. I don't think we want new_with_range.

I think once it's ready we should just merge this. No need to cherry pick. You can make a test checking whether this has done its job by making a dummy trait in a test like trait TestSendStatic: Send + 'static {} and impl TestSendStatic for SpkIterator<Descriptor<DescriptorPublicKey>> { }. This will fail to compile if it didn't work.
Put the SpkIterator into its own spk_iter.mod file. Put tests for it at the end of the module.
Make sure methods that return an SpkIterator return it and not an impl Iterator.

[evanlinjin]

@LLFourn Here is my argument for leaving SpkIterator::new_with_range in.

KeychainTxOutIndex::reveal_to_target, in this PR, now returns an explicit concrete type of SpkIterator. This cannot be achieved if we depend on .skip and .take. Instead we would need to return an implicit impl Iterator<...>.

For me, this justifies leaving it as is, since it provides a better API.

[evanlinjin]

ACK 8b75559

This looks good to me. I guess the last thing to do is to do the "dummy trait" test as mentioned by @LLFourn: #927 (review).

There is also a small nit.

Also note that I rebased for you (sorry!), so you need to force pull.

[LLFourn]

ACK 8b75559

This looks good to me. I guess the last thing to do is to do the "dummy trait" test as mentioned by @LLFourn: #927 (review).

I agree.

@LLFourn Here is my argument for leaving SpkIterator::new_with_range in.

KeychainTxOutIndex::reveal_to_target, in this PR, now returns an explicit concrete type of SpkIterator. This cannot be achieved if we depend on .skip and .take. Instead we would need to return an implicit impl Iterator<...>.

For me, this justifies leaving it as is, since it provides a better API.

Do you mean it provides a API internally? Then I suggest we make it a private function and remove the documentation for it.

[LLFourn]

Nits aside LGTM

[evanlinjin]

We need to make this build with:

$ cargo build --no-default-features -p bdk_chain

To fix this, only include spk_iter module if cargo feature "miniscript" is enabled.

Also a nit: move keychain::BIP32_MAX_INDEX into spk_iter module.

[evanlinjin]

ACK 10fe32e