From ff87ed9187ab5e4967ad6cfa61bb951fe54fb68b Mon Sep 17 00:00:00 2001 From: Ashwin Sekar Date: Thu, 22 Aug 2024 12:21:39 -0400 Subject: [PATCH] gossip: ignore retransmitter signatures when comparing duplicate shreds (#2673) * gossip: ignore retransmitter signatures when comparing duplicate shreds * pr feedback: compare rest of payload instead of setting sig * pr feedback: remove dcou, pub(super) --- gossip/src/duplicate_shred.rs | 85 +++++++++++++++++++++++++++++++++-- ledger/src/shred.rs | 33 +++++++++++++- ledger/src/shred/merkle.rs | 2 +- 3 files changed, 115 insertions(+), 5 deletions(-) diff --git a/gossip/src/duplicate_shred.rs b/gossip/src/duplicate_shred.rs index 3b4448c968414c..ad8c8cc6eb484c 100644 --- a/gossip/src/duplicate_shred.rs +++ b/gossip/src/duplicate_shred.rs @@ -95,7 +95,8 @@ pub enum Error { /// - Must match the expected shred version /// - Must both sigverify for the correct leader /// - Must have a merkle root conflict, otherwise `shred1` and `shred2` must have the same `shred_type` -/// - If `shred1` and `shred2` share the same index they must be not equal +/// - If `shred1` and `shred2` share the same index they must be not have equal payloads excluding the +/// retransmitter signature /// - If `shred1` and `shred2` do not share the same index and are data shreds /// verify that they indicate an index conflict. One of them must be the /// LAST_SHRED_IN_SLOT, however the other shred must have a higher index. @@ -144,7 +145,7 @@ where } if shred1.index() == shred2.index() { - if shred1.payload() != shred2.payload() { + if shred1.is_shred_duplicate(shred2) { return Ok(()); } return Err(Error::InvalidDuplicateShreds); @@ -311,7 +312,7 @@ pub(crate) mod tests { solana_ledger::shred::{ProcessShredsStats, ReedSolomonCache, Shredder}, solana_sdk::{ hash::Hash, - signature::{Keypair, Signer}, + signature::{Keypair, Signature, Signer}, system_transaction, }, std::sync::Arc, @@ -1252,4 +1253,82 @@ pub(crate) mod tests { ); } } + + #[test] + fn test_retransmitter_signature_invalid() { + let mut rng = rand::thread_rng(); + let leader = Arc::new(Keypair::new()); + let (slot, parent_slot, reference_tick, version) = (53084024, 53084023, 0, 0); + let shredder = Shredder::new(slot, parent_slot, reference_tick, version).unwrap(); + let next_shred_index = rng.gen_range(0..32_000); + let leader_schedule = |s| { + if s == slot { + Some(leader.pubkey()) + } else { + None + } + }; + let data_shred = + new_rand_data_shred(&mut rng, next_shred_index, &shredder, &leader, true, true); + let coding_shred = + new_rand_coding_shreds(&mut rng, next_shred_index, 10, &shredder, &leader, true)[0] + .clone(); + let mut data_shred_different_retransmitter_payload = data_shred.clone().into_payload(); + shred::layout::set_retransmitter_signature( + &mut data_shred_different_retransmitter_payload, + &Signature::new_unique(), + ) + .unwrap(); + let data_shred_different_retransmitter = + Shred::new_from_serialized_shred(data_shred_different_retransmitter_payload).unwrap(); + let mut coding_shred_different_retransmitter_payload = coding_shred.clone().into_payload(); + shred::layout::set_retransmitter_signature( + &mut coding_shred_different_retransmitter_payload, + &Signature::new_unique(), + ) + .unwrap(); + let coding_shred_different_retransmitter = + Shred::new_from_serialized_shred(coding_shred_different_retransmitter_payload).unwrap(); + + let test_cases = vec![ + // Same data shred from different retransmitter + (data_shred, data_shred_different_retransmitter), + // Same coding shred from different retransmitter + (coding_shred, coding_shred_different_retransmitter), + ]; + for (shred1, shred2) in test_cases.iter().flat_map(|(a, b)| [(a, b), (b, a)]) { + assert_matches!( + from_shred( + shred1.clone(), + Pubkey::new_unique(), // self_pubkey + shred2.payload().clone(), + Some(leader_schedule), + rng.gen(), // wallclock + 512, // max_size + version, + ) + .err() + .unwrap(), + Error::InvalidDuplicateShreds + ); + + let chunks: Vec<_> = from_shred_bypass_checks( + shred1.clone(), + Pubkey::new_unique(), // self_pubkey + shred2.clone(), + rng.gen(), // wallclock + 512, // max_size + ) + .unwrap() + .collect(); + assert!(chunks.len() > 4); + + assert_matches!( + into_shreds(&leader.pubkey(), chunks, version) + .err() + .unwrap(), + Error::InvalidDuplicateShreds + ); + } + } } diff --git a/ledger/src/shred.rs b/ledger/src/shred.rs index 7525fd5258e442..5ece826ce5369b 100644 --- a/ledger/src/shred.rs +++ b/ledger/src/shred.rs @@ -576,6 +576,37 @@ impl Shred { Self::ShredData(_) => Err(Error::InvalidShredType), } } + + /// Returns true if the other shred has the same ShredId, i.e. (slot, index, + /// shred-type), but different payload. + /// Retransmitter's signature is ignored when comparing payloads. + pub fn is_shred_duplicate(&self, other: &Shred) -> bool { + if self.id() != other.id() { + return false; + } + fn get_payload(shred: &Shred) -> &[u8] { + let Ok(offset) = shred.retransmitter_signature_offset() else { + return shred.payload(); + }; + // Assert that the retransmitter's signature is at the very end of + // the shred payload. + debug_assert_eq!(offset + SIZE_OF_SIGNATURE, shred.payload().len()); + shred + .payload() + .get(..offset) + .unwrap_or_else(|| shred.payload()) + } + get_payload(self) != get_payload(other) + } + + fn retransmitter_signature_offset(&self) -> Result { + match self { + Self::ShredCode(ShredCode::Merkle(shred)) => shred.retransmitter_signature_offset(), + Self::ShredData(ShredData::Merkle(shred)) => shred.retransmitter_signature_offset(), + Self::ShredCode(ShredCode::Legacy(_)) => Err(Error::InvalidShredVariant), + Self::ShredData(ShredData::Legacy(_)) => Err(Error::InvalidShredVariant), + } + } } // Helper methods to extract pieces of the shred from the payload @@ -802,7 +833,7 @@ pub mod layout { } } - pub(crate) fn set_retransmitter_signature( + pub fn set_retransmitter_signature( shred: &mut [u8], signature: &Signature, ) -> Result<(), Error> { diff --git a/ledger/src/shred/merkle.rs b/ledger/src/shred/merkle.rs index e569999ba3d82f..3b9e29934f3e5c 100644 --- a/ledger/src/shred/merkle.rs +++ b/ledger/src/shred/merkle.rs @@ -445,7 +445,7 @@ macro_rules! impl_merkle_shred { Ok(()) } - fn retransmitter_signature_offset(&self) -> Result { + pub(super) fn retransmitter_signature_offset(&self) -> Result { let ShredVariant::$variant { proof_size, chained,