embeds versioning into shred binary #25631

behzadnouri · 2022-05-29T22:39:14Z

- This is the versioning part extracted out of:
- https://github.com/solana-labs/solana/pull/25237

Problem

Need backward compatible way to change binary layout of shreds;
which requires some form of versioning embedded into shred binary

Summary of Changes

First commit:
In preparation of #25237
which adds a new shred variant with merkle tree branches, the commit
embeds versioning into shred binary by encoding a new ShredVariant
type at byte 65 of payload replacing previously ShredType at this
offset.

enum ShredVariant {
    LegacyCode, // 0b0101_1010
    LegacyData, // 0b1010_0101
}

0b0101_1010 indicates a legacy coding shred, which is also equal to
ShredType::Code for backward compatibility.
0b1010_0101 indicates a legacy data shred, which is also equal to
ShredType::Data for backward compatibility.

Following commits will add merkle variants to this type:

 enum ShredVariant {
     LegacyCode, // 0b0101_1010
     LegacyData, // 0b1010_0101
+    MerkleCode(/*proof_size:*/ u8), // 0b0100_????
+    MerkleData(/*proof_size:*/ u8), // 0b1000_????
}

Second commit:
Adds support for different variants for ShredCode and ShredData by adding two
new types:

pub enum ShredCode {
    Legacy(legacy::ShredCode),
}
pub enum ShredData {
    Legacy(legacy::ShredData),
}

#25237 will extend these types by adding merkle variants:

 pub enum ShredCode {
     Legacy(legacy::ShredCode),
+    Merkle(merkle::ShredCode),
 }
 pub enum ShredData {
     Legacy(legacy::ShredData),
+    Merkle(merkle::ShredData),
 }

behzadnouri · 2022-05-29T22:42:32Z

ledger/src/shred.rs

-const SIZE_OF_DATA_SHRED_HEADER: usize = 5;
-const SIZE_OF_CODING_SHRED_HEADER: usize = 6;


Removing these two because they are not used individually, and ..._HEADER vs ..._HEADERS is error-prone.

behzadnouri · 2022-05-29T22:51:04Z

ledger/src/shred/shred_code.rs

+pub enum ShredCode {
+    Legacy(legacy::ShredCode),
+}


#25237 will add merkle variant:

pub enum ShredCode { Legacy(legacy::ShredCode), + Merkle(merkle::ShredCode), }

behzadnouri · 2022-05-29T22:51:40Z

ledger/src/shred/shred_data.rs

+pub enum ShredData {
+    Legacy(legacy::ShredData),
+}


#25237 will add merkle variant:

pub enum ShredData { Legacy(legacy::ShredData), + Merkle(merkle::ShredData), }

behzadnouri · 2022-05-30T13:52:23Z

ledger/src/shred.rs

+enum ShredVariant {
+    LegacyCode, // 0b0101_1010
+    LegacyData, // 0b1010_0101
+}


#25237 adds merkle variants:

enum ShredVariant { LegacyCode, // 0b0101_1010 LegacyData, // 0b1010_0101 + MerkleCode(/*proof_size:*/ u8), // 0b0100_???? + MerkleData(/*proof_size:*/ u8), // 0b1000_???? }

jbiseda

Thanks for addressing comments from the original review.

jbiseda · 2022-05-31T21:14:27Z

ledger/src/shred.rs


 /// The following constants are computed by hand, and hardcoded.
 /// `test_shred_constants` ensures that the values are correct.
 /// Constants are used over lazy_static for performance reasons.
 const SIZE_OF_COMMON_SHRED_HEADER: usize = 83;
-const SIZE_OF_DATA_SHRED_HEADER: usize = 5;
-const SIZE_OF_CODING_SHRED_HEADER: usize = 6;
+const SIZE_OF_DATA_SHRED_HEADERS: usize = 88;


I think it is useful to have these constants defined as a sum of their constituent parts.

Well they are not used anywhere, and as mentioned on the side comment, I think SIZE_OF_..._HEADER vs ..._HEADERS is confusing and easy to miss.

yeah, I guess I'm looking for breaking it down into the sum of basic types (type/slot/index/nonce/...) but I'm ok with this. I'm happy that we won't have both the HEADER and HEADERS consts.

jbiseda · 2022-05-31T21:34:21Z

ledger/src/shred.rs

+impl From<ShredVariant> for u8 {
+    fn from(shred_variant: ShredVariant) -> u8 {
+        match shred_variant {
+            ShredVariant::LegacyCode => u8::from(ShredType::Code),


Will ShredType be used to distinguish data/code for both legacy and merkle shreds? If yes, then it seems that this should be an internal type which isn't serialized. I think the value for the wire encoding should be tied to ShredVariant and not as part of the ShredType enum.

Will ShredType be used to distinguish data/code for both legacy and merkle shreds?

yes

then it seems that this should be an internal type

ShredVariant is the internal type. The code outside this module should not know/care that there are different variants. But ShredType is public; There is logic outside this module based on if the shred is code or data.

which isn't serialized

ShredType is not serialized in this crate, but, elsewhere:
https://github.com/solana-labs/solana/blob/3ed2e0ce2/gossip/src/duplicate_shred.rs#L36

I think the value for the wire encoding should be tied to ShredVariant and not as part of the ShredType enum.

How would you tie the value on ShredVariant?
The merkle variants will be tuple variants, and it errors out:

error[E0658]: custom discriminant values are not allowed in enums with tuple or struct variants

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f596727bd87dc7d1d590e8d041b824de

There is a tracking issue which might allow this one-day:
rust-lang/rust#60553
but even then I am using lower 4 bits to encode proof size, so not sure if that RFC would be useful.

I'm suggesting that we define the following for the shred module.

const SHRED_VARIANT_LEGACY_DATA: u8 = 0b1010_0101; const SHRED_VARIANT_LEGACY_CODE: u8 = 0b0101_1010;

and we remove the custom values from the ShredType enum.

Then we handle the u8 to/from conversion without relying on u8::from(ShredType::Code) within this function.

ShredType doesn't seem like the right place to have the wire format now that we have ShredVariant. Ideally we'd be able to use custom enum values for ShredVariant but as you note it's not possible.

Well the commit cannot change ShredType for backward compatibility:
https://github.com/solana-labs/solana/blob/3ed2e0ce2/gossip/src/duplicate_shred.rs#L36

Anyways, this seems like a non blocker issue which can be revisited later

I agree that it's not going to introduce incorrect behavior.

Backwards compatibility just requires that it serializes/deserializes from u8. It seems strange to make ShredVariant private, expose ShredType as a generic value to indicate only data/code distinction, but keep the specific legacy wire value as a property of the generic type.

Backwards compatibility just requires that it serializes/deserializes from u8.

It requires to serialize to and deserialize from those specific constant u8 values; not any u8. Effectively those specific u8 values are already part of the abi.

Will duplicate shred need to understand merkle variant? If so, we'll need to extend ShredType for that. At which point I'm not sure what purpose it serves to have both ShredType and ShredVariant.

Will duplicate shred need to understand merkle variant?

It should not.

The code outside this module should not know or care if a code or data shred is merkle variant or legacy variant.

It seems some distinction must be made to account for batch signing and retransmit behavior of merkle.

We can move any code logic that needs to dispatch implementation based on shred-variant into ledger/src/shred.rs.
e.g. something like below if that turns out to be needed

impl Shred { fn sign_erasure_batch(keypair: &Keypair, shreds: &mut [Shred]) { ... } }

jbiseda · 2022-05-31T22:19:33Z

ledger/src/shred.rs

@@ -894,6 +934,7 @@ mod tests {
        assert_matches!(bincode::deserialize::<ShredType>(&[1u8]), Err(_));
        // data shred
        assert_eq!(ShredType::Data as u8, 0b1010_0101);


Tie u8 wire representation to ShredVariant rather than ShredType.

Commented above

jbiseda · 2022-05-31T22:23:44Z

ledger/src/shred.rs

@@ -1012,7 +1090,7 @@ mod tests {
            let seed = <[u8; 32]>::try_from(bs58_decode(SEED)).unwrap();
            ChaChaRng::from_seed(seed)
        };
-        let mut parity_shard = vec![0u8; /*ENCODED_PAYLOAD_SIZE:*/ 1139];
+        let mut parity_shard = vec![0u8; /*SIZE_OF_ERASURE_ENCODED_SLICE:*/ 1139];


Can 1139 be defined as a const at the beginning of mod tests?

changed to legacy::SIZE_OF_ERASURE_ENCODED_SLICE

jbiseda · 2022-05-31T22:36:40Z

ledger/src/shred/legacy.rs

-        &self.payload[..self.data_header.size as usize]
+        self.payload
+            .get(SIZE_OF_DATA_SHRED_HEADERS..size)
+            .ok_or(Error::InvalidPayloadSize(self.payload.len()))


Isn't this duplicating the prior check but returning a different error?

jbiseda · 2022-05-31T22:44:08Z

ledger/src/shred/shred_code.rs

+
+#[inline]
+pub(super) fn erasure_shard_index<T: ShredCodeTrait>(shred: &T) -> Option<usize> {
+    // Assert that the last shred index in the erasure set does not


change "Assert" to "return None if ..."

This is an old code just moved here:
https://github.com/solana-labs/solana/blob/v1.9.10/ledger/src/shred.rs#L546-L547

The comment is referring to sanity checks being done, not the implementation.

jbiseda · 2022-05-31T22:51:10Z

ledger/src/sigverify_shreds.rs

-            let msg = shred::layout::get_signed_message_range(packet);
-            let msg = add_offset(msg, pubkeys_end);
+            let shred = shred::layout::get_shred(packet);
+            // TODO: signature verification should fail for empty messages!


Is there something to do for the "TODO"? Or just let the verification fail if the buffer is empty?

changed the comment

jbiseda · 2022-05-31T22:57:04Z

ledger/src/sigverify_shreds.rs

-    let msg = shred::layout::get_signed_message_range(packet);
+    let msg = shred::layout::get_shred(packet)
+        .and_then(shred::layout::get_signed_message_range)
+        .unwrap();
    assert!(


Is this assert superfluous after the unwrap() above?

The assert is on sig.end, the unwrap is on msg.
Here it does not make any assumptions about the underlying layout of shreds.

In preparation of solana-labs#25237 which adds a new shred variant with merkle tree branches, the commit embeds versioning into shred binary by encoding a new ShredVariant type at byte 65 of payload replacing previously ShredType at this offset. enum ShredVariant { LegacyCode, // 0b0101_1010 LegacyData, // 0b0101_1010 } * 0b0101_1010 indicates a legacy coding shred, which is also equal to ShredType::Code for backward compatibility. * 0b1010_0101 indicates a legacy data shred, which is also equal to ShredType::Data for backward compatibility. Following commits will add merkle variants to this type: enum ShredVariant { LegacyCode, // 0b0101_1010 LegacyData, // 0b1010_0101 MerkleCode(/*proof_size:*/ u8), // 0b0100_???? MerkleData(/*proof_size:*/ u8), // 0b1000_???? }

The commit implements two new types: pub enum ShredCode { Legacy(legacy::ShredCode), } pub enum ShredData { Legacy(legacy::ShredData), } Following commits will extend these types by adding merkle variants: pub enum ShredCode { Legacy(legacy::ShredCode), Merkle(merkle::ShredCode), } pub enum ShredData { Legacy(legacy::ShredData), Merkle(merkle::ShredData), }

ryoqun · 2022-10-05T13:08:40Z

ledger/src/shred/shred_code.rs

+impl ShredCode {
+    pub(super) const SIZE_OF_PAYLOAD: usize = PACKET_DATA_SIZE - SIZE_OF_NONCE;
+
+    dispatch!(fn coding_header(&self) -> &CodingShredHeader);


@behzadnouri just curious, why enum_dispatch isn't used here?: https://docs.rs/enum_dispatch/latest/enum_dispatch/
maybe, there was some shortcomings in the crate? (in fact, i haven't used it but I'm wondering we could use it for cleaning lots of blockstore's ColumnFaimily types and hand-written dispatching there).

I just found this dispatch! macro simpler and more explicit.
enum_dispatch requires a trait; this does not.

behzadnouri mentioned this pull request May 29, 2022

adds a new shred variant embedding merkle tree hashes of the erasure batch #25237

Merged

behzadnouri commented May 29, 2022

View reviewed changes

behzadnouri requested review from carllin, jbiseda, pgarg66 and sakridge May 29, 2022 23:27

behzadnouri force-pushed the shred-trait-legacy branch 3 times, most recently from e3e06e0 to e277597 Compare May 30, 2022 13:47

behzadnouri commented May 30, 2022

View reviewed changes

behzadnouri force-pushed the shred-trait-legacy branch from e277597 to e1c4f09 Compare May 30, 2022 14:10

jbiseda reviewed May 31, 2022

View reviewed changes

behzadnouri added 2 commits June 1, 2022 09:46

behzadnouri force-pushed the shred-trait-legacy branch from e1c4f09 to 411f1b2 Compare June 1, 2022 13:46

jbiseda approved these changes Jun 2, 2022

View reviewed changes

behzadnouri merged commit 81231a8 into solana-labs:master Jun 2, 2022

behzadnouri deleted the shred-trait-legacy branch June 2, 2022 18:55

behzadnouri mentioned this pull request Jun 2, 2022

removes raw indexing into packet data #25554

Merged

ryoqun reviewed Oct 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

embeds versioning into shred binary #25631

embeds versioning into shred binary #25631

behzadnouri commented May 29, 2022 •

edited

Loading

behzadnouri May 29, 2022

behzadnouri May 29, 2022 •

edited

Loading

behzadnouri May 29, 2022 •

edited

Loading

behzadnouri May 30, 2022 •

edited

Loading

jbiseda left a comment

jbiseda May 31, 2022

behzadnouri Jun 1, 2022

jbiseda Jun 1, 2022

jbiseda May 31, 2022

behzadnouri Jun 1, 2022

jbiseda Jun 1, 2022

behzadnouri Jun 1, 2022

jbiseda Jun 2, 2022

behzadnouri Jun 2, 2022

jbiseda Jun 2, 2022

behzadnouri Jun 2, 2022

jbiseda Jun 2, 2022

behzadnouri Jun 2, 2022

jbiseda May 31, 2022

behzadnouri Jun 1, 2022

jbiseda May 31, 2022

behzadnouri Jun 1, 2022

jbiseda May 31, 2022

behzadnouri Jun 1, 2022

jbiseda May 31, 2022

behzadnouri Jun 1, 2022

jbiseda May 31, 2022

behzadnouri Jun 1, 2022

jbiseda May 31, 2022

behzadnouri Jun 1, 2022

ryoqun Oct 5, 2022

behzadnouri Oct 6, 2022

		const SIZE_OF_DATA_SHRED_HEADER: usize = 5;
		const SIZE_OF_CODING_SHRED_HEADER: usize = 6;

embeds versioning into shred binary #25631

embeds versioning into shred binary #25631

Conversation

behzadnouri commented May 29, 2022 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

behzadnouri May 29, 2022 • edited Loading

Choose a reason for hiding this comment

behzadnouri May 29, 2022 • edited Loading

Choose a reason for hiding this comment

behzadnouri May 30, 2022 • edited Loading

Choose a reason for hiding this comment

jbiseda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri commented May 29, 2022 •

edited

Loading

behzadnouri May 29, 2022 •

edited

Loading

behzadnouri May 29, 2022 •

edited

Loading

behzadnouri May 30, 2022 •

edited

Loading