Skip to content

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jul 20, 2025

This PR does three changes:

  • Fixes Expanders outputting more than len_in_bytes.
  • Changes Expander::fill_bytes() return a Result<usize>, to more closely mimic a rusty Reader.
  • Optimizes ExpandMsgXmd to copy whole slices instead of copying byte by byte.

This is an alternative take to addressing the issues #1317 found.
However, this PR doesn't get rid of any traits or types. I originally intended to get rid of ExpanderXmd, but I remembered again why we kept it in the first place: to remove the lifetime from ExpandMsg.

@carloskiki
Copy link
Contributor

I find the Read API overkill for this. We know there can't be errors while reading from the buffer, and we already know in advance how many bytes will be read. Even before calling the function it is known that the call will return Ok(min(len_in_bytes, buffer.len())). Also by convention, read should return Ok(0) when EOF is reached, not an error.

If the return value of this method is Ok(n), then implementations must guarantee that 0 <= n <= buf.len(). A nonzero n value indicates that the buffer buf has been filled in with n bytes of data from this source. If n is 0, then it can indicate one of two scenarios:

  • This reader has reached its “end of file” and will likely no longer be able to produce bytes. Note that this does not mean that the reader will always no longer be able to produce bytes. As an example, on Linux, this method will call the recv syscall for a TcpStream, where returning zero indicates the connection was shut down correctly. While for File, it is possible to reach the end of file and get zero as result, but if more data is appended to the file, future calls to read will return more data.
  • The buffer specified was 0 bytes in length.

As also written, read may be able to produce more bytes at a later time, which does not really fit our use case.

Copy link
Contributor

@carloskiki carloskiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like having this sort of API over complicates things here.

@daxpedda
Copy link
Contributor Author

I find the Read API overkill for this. We know there can't be errors while reading from the buffer, and we already know in advance how many bytes will be read. Even before calling the function it is known that the call will return Ok(min(len_in_bytes, buffer.len())).

But it is a reader API. This PR is choosing to expose it as-is instead of wrapping it into a less complicated one.

Also by convention, read should return Ok(0) when EOF is reached, not an error.

As also written, read may be able to produce more bytes at a later time, which does not really fit our use case.

This isn't actually a copy of Read, it is trying to expose something useful, appropriate and familiar API to users. But, you can look at it like read_exact(), which does return an error on EOF.

@daxpedda
Copy link
Contributor Author

I believe that invoking the ghost of the Read trait here was a mistake. I was just trying to take some inspiration and familiarity from it not actually copying it, despite how it looks like in the OP. My bad!

So why am I proposing this API?
The calls we are doing internally here are writing into slices. This is a fact, see copy_from_slice() and XofReader::read(). So it seems straightforward to me to expose it as-is. Is this the most convenient, easy-to-use and least-complicated API? No. But it is the most correct and direct API. This isn't supposed to be a high-level API. The high-level API is GroupDigest.

Couldn't we just have left the API as-is?
Right now there was no way for users to check how many bytes were written or when they actually did something wrong, like requested more bytes when there are none. Ofc we can just document exactly how many bytes this API will spit out and call it a day. But Rust gives us a way to encode this information via the type-system, I really don't see a reason not to do so.

In conclusion, I feel like the improvements here have a very clear purpose and direction to me. I'm not seeing the issue nor the downside here exactly.

However, while this change doesn't make the API more complicated to use compared to the current one (we are just exposing more information the user can happily ignore), it is significantly more complicated than #1317.

@daxpedda daxpedda force-pushed the hash2curve-update-5 branch from 0461fe7 to ace4a37 Compare August 13, 2025 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants