Use Iterator<Item = u8> instead of Expander

[carloskiki]

Discussed in #1295.

Changes

• Expander trait removed in favor of Iterator.
• ExpandMsgXmd and ExpandMsgXof stop generating bytes after having reached len_in_bytes.
• ExpandMsgXmd and ExpandMsgXof are themselves the "expanders", instead of having an Expander associated type.

Regression

On small workloads (len_in_bytes < 256), both xmd and xof expanders are ~ 20% slower with Iterator than with Expander. On the largest workloads (len_in_bytes = 4k), xmd is 50% slower and xof is 100% slower with iterator. I think ExpandMsgXof could be made equally as fast if XofReader allowed a method that generates an Iterator over its inner buffer, just like how ExpandMsgXmd is implemented. I also think that adding bound checks (like there should have been) on Expander would cause similar slow downs on small workloads.

Metrics

small (frequent use case):

expand_xmd              time:   [168.61 ns 168.68 ns 168.76 ns]

expand_xmd_iter         time:   [226.10 ns 226.51 ns 227.08 ns]

expand_xof              time:   [489.15 ns 490.44 ns 492.19 ns]

expand_xof_iter         time:   [532.37 ns 532.61 ns 532.92 ns]

large:

expand_xmd              time:   [12.074 µs 12.078 µs 12.083 µs]

expand_xmd_iter         time:   [18.433 µs 18.440 µs 18.449 µs]

expand_xof              time:   [7.0494 µs 7.0514 µs 7.0538 µs]

expand_xof_iter         time:   [15.713 µs 15.718 µs 15.725 µs]

I did not include the benchmarks in the PR because I did not want to add criterion as a dev-dependency. Let me know if you want me to add them.

I let the maintainers decide whether this regression is worth it.

[daxpedda]

I'm not terribly concerned about the slowdown. Overall executing the whole hash2curve operation is quite expensive compared to the Expander part, I would assume.

However I'm uncertain now what we are trying to achieve here. We are getting rid of a trait in favor of worse performance and improving the feedback (returning None when we are done).

In hindsight, what we want to model is the Read API. This would be an accurate representation to whats going on there. Unfortunately the Read trait isn't available on no_std.

I will make a PR to propose an alternative, then we can do a comparison and see what we get.

[daxpedda]

Lets turn this into a debug_assert!.

[carloskiki]

I think the logic here is succinct enough to not need a dbg assert. Since len_in_bytes <= 255 * b_in_bytes, len_in_bytes / b_in_bytes <= 255.

[daxpedda]

Seems fine to me!

[daxpedda]

See #1318.

[carloskiki]

#1318 raises the concern that the ExpandMsg trait now requires a lifetime parameter. I could change that by returning an opaque impl Iterator<Item = u8> + use<'dst>, or use an associated type as before if desired.

[daxpedda]

#1318 raises the concern that the ExpandMsg trait now requires a lifetime parameter. I could change that by returning an opaque impl Iterator<Item = u8> + use<'dst>, or use an associated type as before if desired.

Yes please, removing that lifetime from ExpandMsg made it a whole lot easier to use that trait in more complicated generic setups downstream.

[carloskiki]

The trait no longer has an input lifetime, it has an associated type Expanded, analogous to the current Expander.