From 54aa57b705613c0f5bfe46e34c7d7be77b14d694 Mon Sep 17 00:00:00 2001 From: Ashwin Sekar Date: Tue, 14 Nov 2023 20:53:50 +0000 Subject: [PATCH 1/2] gossip: process duplicate proofs for merkle root conflicts --- gossip/src/cluster_info.rs | 2 +- gossip/src/duplicate_shred.rs | 244 ++++++++++++++++++++++++++++++---- 2 files changed, 220 insertions(+), 26 deletions(-) diff --git a/gossip/src/cluster_info.rs b/gossip/src/cluster_info.rs index 353d1e13ddc278..4d8e9f7f47ebc5 100644 --- a/gossip/src/cluster_info.rs +++ b/gossip/src/cluster_info.rs @@ -268,7 +268,7 @@ pub fn make_accounts_hashes_message( pub(crate) type Ping = ping_pong::Ping<[u8; GOSSIP_PING_TOKEN_SIZE]>; // TODO These messages should go through the gpu pipeline for spam filtering -#[frozen_abi(digest = "HvA9JnnQrJnmkcGxrp8SmTB1b4iSyQ4VK2p6LpSBaoWR")] +#[frozen_abi(digest = "CroFF8MTW2fxatwv7ALz9Wde4e9L9yE4L59yebM3XuWe")] #[derive(Serialize, Deserialize, Debug, AbiEnumVisitor, AbiExample)] #[allow(clippy::large_enum_variant)] pub(crate) enum Protocol { diff --git a/gossip/src/duplicate_shred.rs b/gossip/src/duplicate_shred.rs index b1ceab79b26949..5512ef9adafcd0 100644 --- a/gossip/src/duplicate_shred.rs +++ b/gossip/src/duplicate_shred.rs @@ -30,7 +30,7 @@ pub struct DuplicateShred { pub(crate) wallclock: u64, pub(crate) slot: Slot, _unused: u32, - shred_type: ShredType, + _unused_shred_type: ShredType, // Serialized DuplicateSlotProof split into chunks. num_chunks: u8, chunk_index: u8, @@ -66,6 +66,8 @@ pub enum Error { InvalidErasureMetaConflict, #[error("invalid last index conflict")] InvalidLastIndexConflict, + #[error("invalid merkle root conflict")] + InvalidMerkleRootConflict, #[error("invalid signature")] InvalidSignature, #[error("invalid size limit")] @@ -78,8 +80,6 @@ pub enum Error { MissingDataChunk, #[error("(de)serialization error")] SerializationError(#[from] bincode::Error), - #[error("shred type mismatch")] - ShredTypeMismatch, #[error("slot mismatch")] SlotMismatch, #[error("type conversion error")] @@ -90,8 +90,8 @@ pub enum Error { /// Check that `shred1` and `shred2` indicate a valid duplicate proof /// - Must be for the same slot -/// - Must have the same `shred_type` /// - 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` do not share the same index and are data shreds /// verify that they indicate an index conflict. One of them must be the @@ -106,10 +106,6 @@ where return Err(Error::SlotMismatch); } - if shred1.shred_type() != shred2.shred_type() { - return Err(Error::ShredTypeMismatch); - } - if let Some(leader_schedule) = leader_schedule { let slot_leader = leader_schedule(shred1.slot()).ok_or(Error::UnknownSlotLeader(shred1.slot()))?; @@ -118,6 +114,31 @@ where } } + // Merkle root conflict check + if shred1.fec_set_index() == shred2.fec_set_index() { + let mr1 = shred1.merkle_root().ok(); + let mr2 = shred2.merkle_root().ok(); + + if (mr1.is_some() && mr2.is_none()) || (mr2.is_some() && mr1.is_none()) { + // A mixture of legacy and merkle shreds in the same fec set + // constitutes a valid duplicate proof + return Ok(()); + } + + if let Some((mr1, mr2)) = mr1.zip(mr2) { + if mr1 != mr2 { + // Conflicting merkle roots for the same fec set is a + // valid duplicate proof + return Ok(()); + } + } + } + + if shred1.shred_type() != shred2.shred_type() { + // Only valid proof here is a merkle conflict which was checked above + return Err(Error::InvalidMerkleRootConflict); + } + if shred1.index() == shred2.index() { if shred1.payload() != shred2.payload() { return Ok(()); @@ -164,7 +185,7 @@ where } let other_shred = Shred::new_from_serialized_shred(other_payload)?; check_shreds(leader_schedule, &shred, &other_shred)?; - let (slot, shred_type) = (shred.slot(), shred.shred_type()); + let slot = shred.slot(); let proof = DuplicateSlotProof { shred1: shred.into_payload(), shred2: other_shred.into_payload(), @@ -184,27 +205,21 @@ where from: self_pubkey, wallclock, slot, - shred_type, num_chunks, chunk_index: i as u8, chunk, _unused: 0, + _unused_shred_type: ShredType::Code, }); Ok(chunks) } // Returns a predicate checking if a duplicate-shred chunk matches -// (slot, shred_type) and has valid chunk_index. -fn check_chunk( - slot: Slot, - shred_type: ShredType, - num_chunks: u8, -) -> impl Fn(&DuplicateShred) -> Result<(), Error> { +// the slot and has valid chunk_index. +fn check_chunk(slot: Slot, num_chunks: u8) -> impl Fn(&DuplicateShred) -> Result<(), Error> { move |dup| { if dup.slot != slot { Err(Error::SlotMismatch) - } else if dup.shred_type != shred_type { - Err(Error::ShredTypeMismatch) } else if dup.num_chunks != num_chunks { Err(Error::NumChunksMismatch) } else if dup.chunk_index >= num_chunks { @@ -226,13 +241,12 @@ pub(crate) fn into_shreds( let mut chunks = chunks.into_iter(); let DuplicateShred { slot, - shred_type, num_chunks, chunk_index, chunk, .. } = chunks.next().ok_or(Error::InvalidDuplicateShreds)?; - let check_chunk = check_chunk(slot, shred_type, num_chunks); + let check_chunk = check_chunk(slot, num_chunks); let mut data = HashMap::new(); data.insert(chunk_index, chunk); for chunk in chunks { @@ -260,8 +274,6 @@ pub(crate) fn into_shreds( let shred2 = Shred::new_from_serialized_shred(proof.shred2)?; if shred1.slot() != slot || shred2.slot() != slot { Err(Error::SlotMismatch) - } else if shred1.shred_type() != shred_type || shred2.shred_type() != shred_type { - Err(Error::ShredTypeMismatch) } else { check_shreds(Some(|_| Some(slot_leader).copied()), &shred1, &shred2)?; Ok((shred1, shred2)) @@ -300,7 +312,7 @@ pub(crate) mod tests { from: Pubkey::new_unique(), wallclock: u64::MAX, slot: Slot::MAX, - shred_type: ShredType::Data, + _unused_shred_type: ShredType::Data, num_chunks: u8::MAX, chunk_index: u8::MAX, chunk: Vec::default(), @@ -421,7 +433,7 @@ pub(crate) mod tests { wallclock: u64, max_size: usize, // Maximum serialized size of each DuplicateShred. ) -> Result, Error> { - let (slot, shred_type) = (shred.slot(), shred.shred_type()); + let slot = shred.slot(); let proof = DuplicateSlotProof { shred1: shred.into_payload(), shred2: other_shred.into_payload(), @@ -437,11 +449,11 @@ pub(crate) mod tests { from: self_pubkey, wallclock, slot, - shred_type, num_chunks, chunk_index: i as u8, chunk, _unused: 0, + _unused_shred_type: ShredType::Code, }); Ok(chunks) } @@ -949,4 +961,186 @@ pub(crate) mod tests { ); } } + + #[test] + fn test_merkle_root_conflict_round_trip() { + 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..31_000); + let leader_schedule = |s| { + if s == slot { + Some(leader.pubkey()) + } else { + None + } + }; + + let (data_shreds, coding_shreds) = new_rand_shreds( + &mut rng, + next_shred_index, + next_shred_index, + 10, + true, /* merkle_variant */ + &shredder, + &leader, + false, + ); + + let (legacy_data_shreds, legacy_coding_shreds) = new_rand_shreds( + &mut rng, + next_shred_index, + next_shred_index, + 10, + false, /* merkle_variant */ + &shredder, + &leader, + true, + ); + + let (diff_data_shreds, diff_coding_shreds) = new_rand_shreds( + &mut rng, + next_shred_index, + next_shred_index, + 10, + true, /* merkle_variant */ + &shredder, + &leader, + false, + ); + + let test_cases = vec![ + (data_shreds[0].clone(), diff_data_shreds[1].clone()), + (coding_shreds[0].clone(), diff_coding_shreds[1].clone()), + (data_shreds[0].clone(), diff_coding_shreds[0].clone()), + (coding_shreds[0].clone(), diff_data_shreds[0].clone()), + // Mix of legacy and merkle for same fec set + (legacy_coding_shreds[0].clone(), data_shreds[0].clone()), + (coding_shreds[0].clone(), legacy_data_shreds[0].clone()), + (legacy_data_shreds[0].clone(), coding_shreds[0].clone()), + (data_shreds[0].clone(), legacy_coding_shreds[0].clone()), + ]; + for (shred1, shred2) in test_cases.into_iter() { + let chunks: Vec<_> = from_shred( + shred1.clone(), + Pubkey::new_unique(), // self_pubkey + shred2.payload().clone(), + Some(leader_schedule), + rng.gen(), // wallclock + 512, // max_size + ) + .unwrap() + .collect(); + assert!(chunks.len() > 4); + let (shred3, shred4) = into_shreds(&leader.pubkey(), chunks).unwrap(); + assert_eq!(shred1, shred3); + assert_eq!(shred2, shred4); + } + } + + #[test] + fn test_merkle_root_conflict_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..31_000); + let leader_schedule = |s| { + if s == slot { + Some(leader.pubkey()) + } else { + None + } + }; + + let (data_shreds, coding_shreds) = new_rand_shreds( + &mut rng, + next_shred_index, + next_shred_index, + 10, + true, + &shredder, + &leader, + true, + ); + + let (next_data_shreds, next_coding_shreds) = new_rand_shreds( + &mut rng, + next_shred_index + 1, + next_shred_index + 1, + 10, + true, + &shredder, + &leader, + true, + ); + + let (legacy_data_shreds, legacy_coding_shreds) = new_rand_shreds( + &mut rng, + next_shred_index, + next_shred_index, + 10, + false, + &shredder, + &leader, + true, + ); + + let test_cases = vec![ + // Same fec set same merkle root + (coding_shreds[0].clone(), data_shreds[0].clone()), + (data_shreds[0].clone(), coding_shreds[0].clone()), + // Different FEC set different merkle root + (coding_shreds[0].clone(), next_data_shreds[0].clone()), + (next_coding_shreds[0].clone(), data_shreds[0].clone()), + (data_shreds[0].clone(), next_coding_shreds[0].clone()), + (next_data_shreds[0].clone(), coding_shreds[0].clone()), + // Legacy shreds + ( + legacy_coding_shreds[0].clone(), + legacy_data_shreds[0].clone(), + ), + ( + legacy_data_shreds[0].clone(), + legacy_coding_shreds[0].clone(), + ), + // Mix of legacy and merkle with different fec index + (legacy_coding_shreds[0].clone(), next_data_shreds[0].clone()), + (next_coding_shreds[0].clone(), legacy_data_shreds[0].clone()), + (legacy_data_shreds[0].clone(), next_coding_shreds[0].clone()), + (next_data_shreds[0].clone(), legacy_coding_shreds[0].clone()), + ]; + for (shred1, shred2) in test_cases.into_iter() { + assert_matches!( + from_shred( + shred1.clone(), + Pubkey::new_unique(), // self_pubkey + shred2.payload().clone(), + Some(leader_schedule), + rng.gen(), // wallclock + 512, // max_size + ) + .err() + .unwrap(), + Error::InvalidMerkleRootConflict + ); + + 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).err().unwrap(), + Error::InvalidMerkleRootConflict + ); + } + } } From 85f29cd2306a1637b4ed0b47473e97b86c78bcd6 Mon Sep 17 00:00:00 2001 From: Ashwin Sekar Date: Thu, 16 Nov 2023 21:17:49 +0000 Subject: [PATCH 2/2] pr comments + abi --- gossip/src/cluster_info.rs | 2 +- gossip/src/duplicate_shred.rs | 35 ++++++++++++----------------------- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/gossip/src/cluster_info.rs b/gossip/src/cluster_info.rs index 4d8e9f7f47ebc5..a3a1edc504a6a9 100644 --- a/gossip/src/cluster_info.rs +++ b/gossip/src/cluster_info.rs @@ -268,7 +268,7 @@ pub fn make_accounts_hashes_message( pub(crate) type Ping = ping_pong::Ping<[u8; GOSSIP_PING_TOKEN_SIZE]>; // TODO These messages should go through the gpu pipeline for spam filtering -#[frozen_abi(digest = "CroFF8MTW2fxatwv7ALz9Wde4e9L9yE4L59yebM3XuWe")] +#[frozen_abi(digest = "FW5Ycg6GXPsY5Ek9b2VjP69toxRb95bSNQRRWLSdKv2Y")] #[derive(Serialize, Deserialize, Debug, AbiEnumVisitor, AbiExample)] #[allow(clippy::large_enum_variant)] pub(crate) enum Protocol { diff --git a/gossip/src/duplicate_shred.rs b/gossip/src/duplicate_shred.rs index 5512ef9adafcd0..70e56d35e82334 100644 --- a/gossip/src/duplicate_shred.rs +++ b/gossip/src/duplicate_shred.rs @@ -66,8 +66,6 @@ pub enum Error { InvalidErasureMetaConflict, #[error("invalid last index conflict")] InvalidLastIndexConflict, - #[error("invalid merkle root conflict")] - InvalidMerkleRootConflict, #[error("invalid signature")] InvalidSignature, #[error("invalid size limit")] @@ -80,6 +78,8 @@ pub enum Error { MissingDataChunk, #[error("(de)serialization error")] SerializationError(#[from] bincode::Error), + #[error("shred type mismatch")] + ShredTypeMismatch, #[error("slot mismatch")] SlotMismatch, #[error("type conversion error")] @@ -115,28 +115,17 @@ where } // Merkle root conflict check - if shred1.fec_set_index() == shred2.fec_set_index() { - let mr1 = shred1.merkle_root().ok(); - let mr2 = shred2.merkle_root().ok(); - - if (mr1.is_some() && mr2.is_none()) || (mr2.is_some() && mr1.is_none()) { - // A mixture of legacy and merkle shreds in the same fec set - // constitutes a valid duplicate proof - return Ok(()); - } - - if let Some((mr1, mr2)) = mr1.zip(mr2) { - if mr1 != mr2 { - // Conflicting merkle roots for the same fec set is a - // valid duplicate proof - return Ok(()); - } - } + if shred1.fec_set_index() == shred2.fec_set_index() + && shred1.merkle_root().ok() != shred2.merkle_root().ok() + { + // This catches a mixture of legacy and merkle shreds + // as well as merkle shreds with different roots in the + // same fec set + return Ok(()); } if shred1.shred_type() != shred2.shred_type() { - // Only valid proof here is a merkle conflict which was checked above - return Err(Error::InvalidMerkleRootConflict); + return Err(Error::ShredTypeMismatch); } if shred1.index() == shred2.index() { @@ -1123,7 +1112,7 @@ pub(crate) mod tests { ) .err() .unwrap(), - Error::InvalidMerkleRootConflict + Error::ShredTypeMismatch ); let chunks: Vec<_> = from_shred_bypass_checks( @@ -1139,7 +1128,7 @@ pub(crate) mod tests { assert_matches!( into_shreds(&leader.pubkey(), chunks).err().unwrap(), - Error::InvalidMerkleRootConflict + Error::ShredTypeMismatch ); } }