The Serialize/Deserialize macro does not preserve all data.

[lucasbalieiro]

while working on the improvements proposed in #2028, we noticed that the round-trip serialization is not lossless: deserializing a message and serializing it again can change the original byte stream.

To demonstrate the issue, you can apply this unit test for the NewExtendedMiningJob message that explicitly checks for byte-for-byte round-trip correctness.

Reproducer

The following test parses a raw byte vector into a NewExtendedMiningJob, serializes it back, and asserts that the output matches the original input exactly:

diff --git a/sv2/subprotocols/mining/src/new_mining_job.rs b/sv2/subprotocols/mining/src/new_mining_job.rs
index 9167be97..bf42800d 100644
--- a/sv2/subprotocols/mining/src/new_mining_job.rs
+++ b/sv2/subprotocols/mining/src/new_mining_job.rs
@@ -152,6 +152,8 @@ impl NewExtendedMiningJob<'_> {
 
 #[cfg(test)]
 mod tests {
+    use binary_sv2::GetSize;
+
     use super::*;
     use crate::tests::from_arbitrary_vec_to_array;
     use core::convert::TryFrom;
@@ -217,6 +219,24 @@ mod tests {
             && static_nmj.merkle_root == nmj.merkle_root
     }
 
+    #[test]
+    fn test_lossy_bytes() {
+        let mut bytes = vec![
+            255, 231, 255, 0, 50, 197, 5, 5, 1, 255, 255, 5, 5, 255, 5, 5, 1, 255, 0, 7, 0, 255,
+            255, 255, 255, 173, 0, 197, 0, 0,
+        ];
+
+        let msg = NewExtendedMiningJob::from_bytes(&mut bytes).unwrap();
+
+        let mut encoded = vec![0u8; msg.get_size()];
+        msg.to_bytes(&mut encoded).unwrap();
+
+        assert_eq!(
+            bytes, encoded,
+            "Expected a lossless byte round-trip, but encoding changed the input bytes"
+        );
+    }
+
     pub mod helpers {
         use super::*;
         use alloc::borrow::ToOwned;

Failure

Running the test:

cargo test test_lossy_bytes

results in the following failure:

running 1 test
test new_mining_job::tests::test_lossy_bytes ... FAILED

failures:

---- new_mining_job::tests::test_lossy_bytes stdout ----
thread 'new_mining_job::tests::test_lossy_bytes' panicked at sv2/subprotocols/mining/src/new_mining_job.rs:234:9:
assertion `left == right` failed: Expected a lossless byte round-trip, but encoding changed the input bytes
  left: [255, 231, 255, 0, 50, 197, 5, 5, 1, 255, 255, 5, 5, 255, 5, 5, 1, 255, 0, 7, 0, 255, 255, 255, 255, 173, 0, 197, 0, 0]
 right: [255, 231, 255, 0, 50, 197, 5, 5, 1, 255, 255, 5, 5, 255, 5, 5, 1, 1, 0, 7, 0, 255, 255, 255, 255, 173, 0, 197, 0, 0]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    new_mining_job::tests::test_lossy_bytes

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 26 filtered out; finished in 0.00s

error: test failed, to rerun pass `--lib`

Specifically, the byte at index 17 is modified during the round-trip: its value changes from 255 to 1.

[Fi3]

how did you get the test vector? Would be nice to have in the issue the test vector visualized as a message with each message field with the associated bytes. Any idea of what is causing the issue?

[Shourya742]

how did you get the test vector? Would be nice to have in the issue the test vector visualized as a message with each message field with the associated bytes. Any idea of what is causing the issue?

We’re currently fuzzing the protocol crates and this input was uncovered by the fuzzer. We’re still investigating the issue and will update here once we identify the root cause.

[Fi3]

trying to map the test vector into the message:

255, 231, 255, 0, 50, // channel id u32
197, 5, 5, 1, // job id u32
255, // Option header invalid
255, 5, 5, 255, 5, 5, 1, 255, 0, 7, 0, 255,
255, 255, 255, 173, 0, 197, 0, 0,

looks like the test should fail trying to parse it first byte of an Option should be 0 or 1

[lucasbalieiro]

trying to map the test vector into the message:

255, 231, 255, 0, 50, // channel id u32 197, 5, 5, 1, // job id u32 255, // Option header invalid 255, 5, 5, 255, 5, 5, 1, 255, 0, 7, 0, 255, 255, 255, 255, 173, 0, 197, 0, 0,

looks like the test should fail trying to parse it first byte of an Option should be 0 or 1

@Fi3, I think there’s a small miscount in the byte layout you showed. It looks like channel_id (u32) was counted as 5 bytes instead of 4, which makes Sv2Option look like the culprit when it really isn’t. That one extra byte shifts all subsequent field boundaries and throws off the rest of the parsing.

If we realign everything assuming the correct size (u32 == 4 bytes), the stream parses like this:

255, 231, 255, 0,        // channel_id: u32
50, 197, 5, 5,           // job_id: u32

1, 255, 255, 5, 5        // Sv2Option<'decoder, u32> (header + value)

255, 5, 5, 1,            // version: u32
255,                      // version_rolling_allowed: bool (changed from 255 to 1)

0, 7, 0, 255, 255, 255, 255, 173, 0, 197, 0, 0 // remaining bytes

About the failure behavior

If Sv2Option is given more than one element, it does fail:

---- new_mining_job::tests::test_lossy_bytes stdout ----
thread 'new_mining_job::tests::test_lossy_bytes' panicked at sv2/subprotocols/mining/src/new_mining_job.rs:229:64:
called `Result::unwrap()` on an `Err` value: Sv2OptionHaveMoreThenOneElement(100)

So the option parsing itself is doing the right thing.

The real source of confusion seems to be bool

This line is key:

255, // version_rolling_allowed: bool

At first glance, 255 should be invalid for a bool. But it isn’t, because of how bool is defined in Sv2DataType.

From the implementation:

stratum/sv2/binary-sv2/src/datatypes/copy_data_types.rs

Lines 44 to 58 in f600e21

	impl<'a> Sv2DataType<'a> for bool {
	fn from_bytes_unchecked(data: &'a mut [u8]) -> Self {
	match data
	.first()
	.map(|x: &u8| x << 7)
	.map(|x: u8| x >> 7)
	// This is an unchecked function is fine to panic
	.expect("Try to decode a bool from a buffer of len 0")
	{
	0 => false,
	1 => true,
	// Below panic is impossible value is either 0 or 1
	_ => panic!(),
	}
	}

The logic appears to be:

• Only LSB matters
• byte & 1 == 0 → false
• byte & 1 == 1 → true

Which means:

• 0 → false
• 1 → true
• 255 (0b11111111) → also true

This also implies some slightly unintuitive edge cases:

• 2 (0b00000010) → false
• 3 (0b00000011) → true
• 4 (0b00000100) → false

But reading the specs, seems that this is by design: https://github.com/stratum-mining/sv2-spec/blob/main/03-Protocol-Overview.md#31-data-types-mapping

So I think this explains why we’re seeing: 255 doesn’t remain “255”; it collapses semantically into true (1).

What do you think?

[plebhash]

so IIUC the relevant part of the spec is the description of BOOL type:

Boolean value. Encoded as an unsigned 1-bit integer, True = 1, False = 0 with 7 additional padding bits in the high positions. Recipients MUST NOT interpret bits outside of the least significant bit. Senders MAY set bits outside of the least significant bit to any value without any impact on meaning. This allows future use of other bits as flag bits.

so I guess we simply cannot blindly rely on this round-trip testing methodology, because there's always the chance of having garbage bits on messages with BOOL types?

for round-trip testing, we would need to either:

• mask out the high 7 bits of BOOL fields before comparison
  OR
• test semantic equality (deserialize both and compare fields) instead of byte equality