feat: extend range of possible implementations of bytereader/bytewriter

[bitwalker]

This PR contains two main changes:

1. Implement ByteReader/ByteWriter for any implementation of std::io::Read or std::io::Write, and remove the Sized bound on the traits, so as to support a wider array of implementations. NOTE: This is technically a breaking change for downstream crates that have their own implementations of Serializable and Deserializable, as they will need to add the ?Sized bound to a couple of the trait methods which were overly restrictive. I don't know how problematic that is, but actually has the effect of making those traits easier to work with, so it should be a net positive. The caveat is that it probably requires a minor version bump as a result to signal the change.
2. Tweak how the winterfell crates are built, so that they are no-std by default, and are extended with additional features when the std feature is enabled, as is the recommended practice for such crates. This also comes with a deprecation of the boxed, collections, and strings modules (but not removal yet), as they are entirely redundant with liballoc.

As mentioned above, the boxed, collections, and strings modules of winter-utils are redundant. It seems the only reason they exist was due to a misunderstanding on how to set up a crate with no-std support. As part of the changes related to point 2 above, I have marked those modules as deprecated, with a helpful message informing downstream users how to address the deprecation, and updated all of the crates in the winterfell workspace to make use of alloc directly.

While many files are touched here, it's typically one or two lines per module to update the use statements, so should be trivial to skim through. However, I've broken up all the changes into relevant commits to make each easier to review independently. While in theory this could have been two separate PRs, I'm in a bit of a time crunch, so only had time to put this together as a single PR - hopefully that is acceptable, and that the extra work I put in to document each change in its individual commit makes up for the inconvenience.

[facebook-github-bot]

Hi @bitwalker!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[irakliyk]

Looks great! Thank you! I left one comment inline re aligning ByteReader with the standard library I/O traits.

One other question: can we get away without the change removing the Sized bound on the traits? I'm asking for two reasons:

1. As you mentioned, this would mean a breaking change and, thus, we'd need to make v0.9.0 release (as opposed to v0.8.2 release - which would be much easer). On the other hand, if we do make a minor release, we could update the ByteReader as mentioned above.
2. Adding + ?Sized to trait bounds any time we need to implement Serializable and Deserializable is a bit annoying and it will impact a lot of code downstream. Is there a way to avoid this?

[irakliyk]

Yeah - there is no good reason why peek_u8, has_more_bytes, and check_eor don't take a mutable reference. So, if we do change that, it would simplify ReaderAdapter implementation a bit, right?

[bitwalker]

It would certainly allow us to do away with the RefCell hack, and ensure that all uses of the ByteReader API are valid (i.e. there aren't hidden edge cases where a dynamic borrow check could fail).

I think it would also allow us to implement ByteReader for std::io::BufReader directly, and get rid of ReadAdapter entirely, which would be nice for sure. There is one API that would be nice to have, but is still unstable (has_data_left), but we can implement an alternative as long as we have a mutable reference on hand. There is also the question of fallible reads with std::io::BufReader, i.e. if you attempt to read N bytes, but only N-1 are available, but the internal buffer of the BufReader is say, N / 2, then we can't put the extra bytes back into the reader for a subsequent call - in other words, the data is lost.

That said, it's an edge case, and I'm not sure what the guarantees are of ByteReader with regard to read errors and subsequent reads following the error, but it seems to be implied that you could attempt to read the same data multiple times until you succeed, and ReadAdapter implements the necessary bookkeeping to support that.

[bitwalker]

One other question: can we get away without the change removing the Sized bound on the traits? I'm asking for two reasons:

1. As you mentioned, this would mean a breaking change and, thus, we'd need to make v0.9.0 release (as opposed to v0.8.2 release - which would be much easer). On the other hand, if we do make a minor release, we could update the ByteReader as mentioned above.

This is the most compelling reason to punt on that particular change IMO, since there isn't any way to avoid it causing compilation errors after the upgrade, even if it is trivial to handle, it is still a burden. I suppose the most pertinent question is: how many custom implementations of those traits are out in the wild? There isn't really any way to know I guess. I know we have some in Miden, but it's not a problem to fix those as part of upgrading winterfell in the project.

But yeah, if those suggested changes to ByteReader are desirable, then knocking out both changes in one release is probably ideal, since they are so closely related to one another.

2. Adding + ?Sized to trait bounds any time we need to implement Serializable and Deserializable is a bit annoying and it will impact a lot of code downstream. Is there a way to avoid this?

This is one of the tradeoffs of defining traits with generic methods vs using dynamic dispatch with trait objects, i.e. &dyn ByteReader. There are also tricks for supporting both simultaneously (see erased-serde for an example of what I mean. Unfortunately that doesn't help us here, since the issue we're trying to avoid is breaking changes downstream.

I do think adding ?Sized pays its way though: the slight annoyance that comes from having to add it to some of the Serializable/Deserializable methods gains you considerable flexibility in interacting with those traits. At a minimum being able to pass anything that can coerce to &dyn ByteReader/ByteWriter makes for very ergonomic APIs.

Anyway, if the general feeling is that the project would rather punt on this for now, that's fine. I don't believe any of my other changes depended on it, so it should be straightforward to drop those commits and proceed without it.

[irakliyk]

I suppose the most pertinent question is: how many custom implementations of those traits are out in the wild?

If this just affected custom implementations of ByteReader and ByteWriter - I think we could even make it a patch release because I doubt there are many custom implementations of these (though, of course we don't know for sure). But my understanding is that it would also break any struct which implements Serializable and Deserializable traits - and there are a lot of those.

If my understanding is correct, I'm inclined to do the following:

• Drop the ?sized changes from this PR and make a patch release.
• Make these changes for the v0.9.0 release and also add the change to ByteReader to make the relevant methods take mutable references to self.

[bitwalker]

I've created a new branch for the ?Sized changes and can open another PR for that after this is merged

[irakliyk]

All looks good! Thank you!

[irakliyk]

oh - one last thing: could you rebase this PR from main. The next branch is for the v0.9 release.

[bitwalker]

@irakliyk Haha oh man, I originally had based it on main, but then realized there was a next branch, and so went about resolving all the conflicts to rebase it on next 🤦. Luckily I think I still have that branch sitting around.

[bitwalker]

@irakliyk Done!