Implement BCS deserialization from_reader #6

preston-evans98 · 2023-09-10T15:43:03Z

This Pull Request implements BCS deserialization from implementers of std::io::Read.

Strategy

The PR aims to maintain a minimal diff over the previous implementation. To that end, the following changes have been made.

Make Deserializer generic over an inner type R
Factor as much of the implementation as possible into a new trait BcsReader, which implements BCS in terms of a few primitive operations. Implement serde::Deserialize<'de> for Deserializer<R> where Deserializer<R>: BcsReader<'de>
Implement BcsReader<'de> for Deserializer<&'de [u8]> using existing code
Implement BcsReader generically for (a version of) Deserializer<R: Read>.

Implementing `BcsReader` generically

The existing implementation is very close to supporting deserialization from readers. There are only two places in which its behavior relied on access to a byte slice was in the implementation of de::MapAccess.

The first case is map deserialization. bcs needs access to the serialized representation of map keys in order to enforce canonicity. When deserializing from a slice, this is straightforward - the implementation can simply deserialize a map key, and then "look back" at the original input slice to determine its serialized representation. When deserializing from a reader, however, this kind of rewinding is not generally possible. To solve this problem, I introduce a the TeeReader struct, which wraps a Reader and optionally copies its bytes into a capture_buffer for later retrieval. Then, I simply implement BcsReader for Deserializer<TeeReader<...>>.

The second case is the end method, which checks that all input bytes have been consumed. To handle this case, I simply attempt to read one extra byte from the Reader after deserialization and assert that an EOF error is returned from the underlying Reader.

Testing

All existing unit tests except for zero_copy_parse have been modified to test that deserialization from_bytes and from_reader yield the same output. That test is not applicable since readers are not capable of zero-copy deserialization.

** Stragety ** The commit aims to maintain a minimal diff over the previous implementation. To that end, the following changes have been made. - Make `Deserializer` generic over an inner type `R` - Factor as much of the implementation as possible into a new trait `BcsReader`, which implements BCS in terms of a few primitive operations. Implement `serde::Deserialize<'de> for Deserializer<R> where Deserializer<R>: BcsReader<'de>` - Implement `BcsReader<'de>` for `Deserializer<&'de [u8]>` using existing code - Implement `BcsReader` generically for (a version of) `Deserializer<R: Read>`. The existing implementation is very close to supporting deserialization from readers. There are only two places in which its behavior relied on access to a byte slice was in the implementation of `de::MapAccess`. The first case is `map` deserialization. `bcs` needs access to the serialized representation of map keys in order to enforce canonicity. When deserializing from a slice, this is straightforward - the implementation can simply deserialize a map key, and then "look back" at the original input slice to determine its serialized representation. When deserializing from a reader, however, this kind of rewinding is not generally possible. To solve this problem, I introduce a the `TeeReader` struct, which wraps a `Read`er and optionally copies its bytes into a `capture_buffer` for later retrieval. Then, I simply implement `BcsReader` for `Deserializer<TeeReader<...>>`. The second case is the `end` method, which checks that all input bytes have been consumed. To handle this case, I simply attempt to read one extra byte from the `Read`er after deserialization and assert that an EOF error is returned from the underlying `Read`er. ** Testing ** All existing unit tests except for `zero_copy_parse` have been modified to test that deserialization `from_bytes` and `from_reader` yield the same output. That test is not applicable since readers are not capable of zero-copy deserialization.

ma2bd · 2023-09-11T04:08:04Z

Thanks @preston-evans98 for the changes. Hopefully, @bmwill and I will find time for a proper code review soon.

preston-evans98 · 2023-09-11T20:22:42Z

It looks like CI failed initially due to an outdated clippy allow directive, and because cargo selected a version of thiserror which is only compatible with the 2021 rust edition. I removed the lint, and pinned thiserror to exactly the version specified in the Cargo.toml. This should be safe to do because thiserror does not appear in the crate's public API.

preston-evans98 · 2023-09-12T02:22:54Z

Hmm, looks like the MSRV failing due to unrelated changes. Are you opposed to checking in a Cargo.lock?

ma2bd · 2023-10-07T23:50:15Z

@preston-evans98 Thanks for investigating. I fixed the CI on the main branch. Can you rebase your PR?

ma2bd

Thank for the new feature. This is looking pretty good. Mostly style comments. I found an issue with a corner case and proposed a tentative fix (see other comment).

ma2bd · 2023-10-07T23:56:27Z

src/lib.rs

-pub use de::{from_bytes, from_bytes_seed, from_bytes_seed_with_limit, from_bytes_with_limit};
+pub use de::{
+    from_bytes, from_bytes_seed, from_bytes_seed_with_limit, from_bytes_with_limit, from_reader,
+    from_reader_seed,


I don't suppose anything is preventing from_reader_seed_with_limit, right?

Not that I can see, no. Will add.

Also added from_reader_with_limit for completeness.

src/error.rs

src/de.rs

ma2bd · 2023-10-08T00:59:34Z

src/de.rs

+}
+
+impl<'de, R> TeeReader<'de, R> {
+    /// Wrapse the provided reader in a new [`TeeReader`].


typo: Wraps

src/de.rs

ma2bd · 2023-10-08T02:11:50Z

src/de.rs

+        &mut self,
+        seed: K,
+    ) -> Result<(K::Value, Self::MaybeBorrowedBytes), Error> {
+        self.input.capture_buffer = Some(Vec::new());


Interestingly, this code won't work if a map is itself keyed by a map like BTreeMap<BTreeMap<u8, u8>, u32>
(which does compile, see https://gist.github.com/rust-play/4f4819775a567501e2dfd8e41699bbce )

We could easily return a error, say UnsupportedMapInKeyposition in the case where self.input.capture_buffer.is_some() before line 298, but I believe that there is a simple fix:

Make capture_buffer a vector of bytes. Let's rename it in the process, say captured_keys: Vec<Vec<u8>>

Add an empty vector at the end to start recording a new key.

When the current key is finished, pop the vector to take the bytes, but do not forget to also append the bytes at the end of the previous (still ongoing) recorded key, if any.

I made a branch pursuing this idea here: #7

@preston-evans98 I didn't write a test so I could be missing something. Your help with finalizing this PR is still much appreciated! Can you review and pick the suggested changes, add a unit test for BTreeMap<BTreeMap<u8, u8>, u32>, then update this PR?

Thank you!

Very good catch! Here's a quick test case to detect the issue:

#[test] fn test_nested_map() { use std::collections::BTreeMap as Map; let mut m = Map::new(); m.insert(Map::from_iter([(1u8, 0u8); 5]), 3); let bytes = to_bytes(&m).unwrap(); assert_eq!(from_bytes(&bytes).as_ref(), Ok(&m)); assert_eq!(from_bytes_via_reader(&bytes), Ok(m)); }

Working on a fix now.

Your changes look good to me! I also tested that they worked with an additional layer of nesting. Pushing the changes for review 👍

Cargo.toml

ma2bd

LGTM

bmwill

Thanks for adding this! There's been times in the past where i've wanted this but haven't ever had the time to implement it myself so thank you :)

ma2bd · 2023-10-13T03:14:26Z

@preston-evans98 @bmwill This was just released in bcs v0.1.6

preston-evans98 · 2023-10-17T20:35:41Z

Fantastic, thanks for the review @ma2bd @bmwill!

ma2bd requested review from ma2bd and bmwill September 11, 2023 04:07

preston-evans98 added 3 commits September 11, 2023 08:06

fmt; clippy

873f085

remove outdated clippy lint

46b0f9a

pin thiserror to support msrv

bf9f42b

ma2bd requested changes Oct 8, 2023

View reviewed changes

Bug fix: handle nested maps. Fix nits

ec2dc87

ma2bd approved these changes Oct 12, 2023

View reviewed changes

bmwill approved these changes Oct 13, 2023

View reviewed changes

ma2bd merged commit 56cfe89 into zefchain:main Oct 13, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement BCS deserialization from_reader #6

Implement BCS deserialization from_reader #6

preston-evans98 commented Sep 10, 2023

ma2bd commented Sep 11, 2023

preston-evans98 commented Sep 11, 2023

preston-evans98 commented Sep 12, 2023

ma2bd commented Oct 7, 2023

ma2bd left a comment

ma2bd Oct 7, 2023

preston-evans98 Oct 10, 2023

preston-evans98 Oct 10, 2023

ma2bd Oct 8, 2023

preston-evans98 Oct 10, 2023

ma2bd Oct 8, 2023 •

edited

Loading

preston-evans98 Oct 10, 2023 •

edited

Loading

preston-evans98 Oct 10, 2023

ma2bd Oct 10, 2023

ma2bd left a comment

bmwill left a comment

ma2bd commented Oct 13, 2023

preston-evans98 commented Oct 17, 2023

Implement BCS deserialization from_reader #6

Implement BCS deserialization from_reader #6

Conversation

preston-evans98 commented Sep 10, 2023

Strategy

Implementing BcsReader generically

Testing

ma2bd commented Sep 11, 2023

preston-evans98 commented Sep 11, 2023

preston-evans98 commented Sep 12, 2023

ma2bd commented Oct 7, 2023

ma2bd left a comment

Choose a reason for hiding this comment

ma2bd Oct 7, 2023

Choose a reason for hiding this comment

preston-evans98 Oct 10, 2023

Choose a reason for hiding this comment

preston-evans98 Oct 10, 2023

Choose a reason for hiding this comment

ma2bd Oct 8, 2023

Choose a reason for hiding this comment

preston-evans98 Oct 10, 2023

Choose a reason for hiding this comment

ma2bd Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

preston-evans98 Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

preston-evans98 Oct 10, 2023

Choose a reason for hiding this comment

ma2bd Oct 10, 2023

Choose a reason for hiding this comment

ma2bd left a comment

Choose a reason for hiding this comment

bmwill left a comment

Choose a reason for hiding this comment

ma2bd commented Oct 13, 2023

preston-evans98 commented Oct 17, 2023

Implementing `BcsReader` generically

ma2bd Oct 8, 2023 •

edited

Loading

preston-evans98 Oct 10, 2023 •

edited

Loading