From 505cc641899602fa531dc22b0c7d5e719672b35f Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Tue, 18 Oct 2022 18:17:12 +0000 Subject: [PATCH 1/2] enforces hash domain for ping-pong protocol (#28433) https://github.com/solana-labs/solana/pull/27193 added hash domain to ping-pong protocol. For backward compatibility responses both with and without domain were generated and accepted. Now that all clusters are upgraded, this commit enforces the hash domain by removing the response without the domain. (cherry picked from commit e283461d9901ccd3c845c21521aae7a3ff23695d) # Conflicts: # gossip/src/cluster_info.rs --- core/src/ancestor_hashes_service.rs | 13 ++++--------- core/src/serve_repair.rs | 15 +++++---------- gossip/src/cluster_info.rs | 22 ++++++++++++++++------ gossip/src/ping_pong.rs | 29 ++++------------------------- 4 files changed, 29 insertions(+), 50 deletions(-) diff --git a/core/src/ancestor_hashes_service.rs b/core/src/ancestor_hashes_service.rs index ec802decc30..015e602794f 100644 --- a/core/src/ancestor_hashes_service.rs +++ b/core/src/ancestor_hashes_service.rs @@ -430,15 +430,10 @@ impl AncestorHashesService { return None; } stats.ping_count += 1; - // Respond both with and without domain so that the other node - // will accept the response regardless of its upgrade status. - // TODO: remove domain = false once cluster is upgraded. - for domain in [false, true] { - if let Ok(pong) = Pong::new(domain, &ping, keypair) { - let pong = RepairProtocol::Pong(pong); - if let Ok(pong_bytes) = serialize(&pong) { - let _ignore = ancestor_socket.send_to(&pong_bytes[..], from_addr); - } + if let Ok(pong) = Pong::new(&ping, keypair) { + let pong = RepairProtocol::Pong(pong); + if let Ok(pong_bytes) = serialize(&pong) { + let _ignore = ancestor_socket.send_to(&pong_bytes[..], from_addr); } } None diff --git a/core/src/serve_repair.rs b/core/src/serve_repair.rs index 7d34af093f5..9fd96f92308 100644 --- a/core/src/serve_repair.rs +++ b/core/src/serve_repair.rs @@ -946,16 +946,11 @@ impl ServeRepair { } packet.meta.set_discard(true); stats.ping_count += 1; - // Respond both with and without domain so that the other node - // will accept the response regardless of its upgrade status. - // TODO: remove domain = false once cluster is upgraded. - for domain in [false, true] { - if let Ok(pong) = Pong::new(domain, &ping, keypair) { - let pong = RepairProtocol::Pong(pong); - if let Ok(pong_bytes) = serialize(&pong) { - let from_addr = packet.meta.socket_addr(); - pending_pongs.push((pong_bytes, from_addr)); - } + if let Ok(pong) = Pong::new(&ping, keypair) { + let pong = RepairProtocol::Pong(pong); + if let Ok(pong_bytes) = serialize(&pong) { + let from_addr = packet.meta.socket_addr(); + pending_pongs.push((pong_bytes, from_addr)); } } } diff --git a/gossip/src/cluster_info.rs b/gossip/src/cluster_info.rs index 39be4162fa7..c95a5e6c7e5 100644 --- a/gossip/src/cluster_info.rs +++ b/gossip/src/cluster_info.rs @@ -2163,6 +2163,7 @@ impl ClusterInfo { I: IntoIterator, { let keypair = self.keypair(); +<<<<<<< HEAD let mut packets = Vec::new(); for (addr, ping) in pings { // Respond both with and without domain so that the other node will @@ -2181,6 +2182,17 @@ impl ClusterInfo { } } if packets.is_empty() { +======= + let pongs_and_dests: Vec<_> = pings + .into_iter() + .filter_map(|(addr, ping)| { + let pong = Pong::new(&ping, &keypair).ok()?; + let pong = Protocol::PongMessage(pong); + Some((addr, pong)) + }) + .collect(); + if pongs_and_dests.is_empty() { +>>>>>>> e283461d9 (enforces hash domain for ping-pong protocol (#28433)) None } else { let packet_batch = PacketBatch::new_unpinned_with_recycler_data( @@ -3214,9 +3226,7 @@ mod tests { let pongs: Vec<(SocketAddr, Pong)> = pings .iter() .zip(&remote_nodes) - .map(|(ping, (keypair, socket))| { - (*socket, Pong::new(/*domain:*/ true, ping, keypair).unwrap()) - }) + .map(|(ping, (keypair, socket))| (*socket, Pong::new(ping, keypair).unwrap())) .collect(); let now = now + Duration::from_millis(1); cluster_info.handle_batch_pong_messages(pongs, now); @@ -3259,7 +3269,7 @@ mod tests { .collect(); let pongs: Vec<_> = pings .iter() - .map(|ping| Pong::new(/*domain:*/ false, ping, &this_node).unwrap()) + .map(|ping| Pong::new(ping, &this_node).unwrap()) .collect(); let recycler = PacketBatchRecycler::default(); let packets = cluster_info @@ -3271,9 +3281,9 @@ mod tests { &recycler, ) .unwrap(); - assert_eq!(remote_nodes.len() * 2, packets.len()); + assert_eq!(remote_nodes.len(), packets.len()); for (packet, (_, socket), pong) in izip!( - packets.into_iter().step_by(2), + packets.into_iter(), remote_nodes.into_iter(), pongs.into_iter() ) { diff --git a/gossip/src/ping_pong.rs b/gossip/src/ping_pong.rs index f6e47c6907e..6c7399281e9 100644 --- a/gossip/src/ping_pong.rs +++ b/gossip/src/ping_pong.rs @@ -104,17 +104,9 @@ impl Signable for Ping { } impl Pong { - pub fn new( - domain: bool, - ping: &Ping, - keypair: &Keypair, - ) -> Result { + pub fn new(ping: &Ping, keypair: &Keypair) -> Result { let token = serialize(&ping.token)?; - let hash = if domain { - hash::hashv(&[PING_PONG_HASH_PREFIX, &token]) - } else { - hash::hash(&token) - }; + let hash = hash::hashv(&[PING_PONG_HASH_PREFIX, &token]); let pong = Pong { from: keypair.pubkey(), hash, @@ -203,11 +195,6 @@ impl PingCache { _ => { let ping = pingf()?; let token = serialize(&ping.token).ok()?; - // For backward compatibility, for now responses both with and - // without domain are accepted. - // TODO: remove no domain case once cluster is upgraded. - let hash = hash::hash(&token); - self.pending_cache.put(hash, node); let hash = hash::hashv(&[PING_PONG_HASH_PREFIX, &token]); self.pending_cache.put(hash, node); self.pings.put(node, now); @@ -303,12 +290,7 @@ mod tests { assert!(ping.verify()); assert!(ping.sanitize().is_ok()); - let pong = Pong::new(/*domain:*/ false, &ping, &keypair).unwrap(); - assert!(pong.verify()); - assert!(pong.sanitize().is_ok()); - assert_eq!(hash::hash(&ping.token), pong.hash); - - let pong = Pong::new(/*domian:*/ true, &ping, &keypair).unwrap(); + let pong = Pong::new(&ping, &keypair).unwrap(); assert!(pong.verify()); assert!(pong.sanitize().is_ok()); assert_eq!( @@ -370,10 +352,7 @@ mod tests { assert!(ping.is_none()); } Some(ping) => { - let domain = rng.gen_ratio(1, 2); - let pong = Pong::new(domain, ping, keypair).unwrap(); - assert!(cache.add(&pong, *socket, now)); - let pong = Pong::new(!domain, ping, keypair).unwrap(); + let pong = Pong::new(ping, keypair).unwrap(); assert!(cache.add(&pong, *socket, now)); } } From c8964daabf43a17b894eff040427ce342f7d8ad8 Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Mon, 28 Nov 2022 12:46:54 -0500 Subject: [PATCH 2/2] removes mergify merge conflicts --- gossip/src/cluster_info.rs | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/gossip/src/cluster_info.rs b/gossip/src/cluster_info.rs index c95a5e6c7e5..4dfe01e5a11 100644 --- a/gossip/src/cluster_info.rs +++ b/gossip/src/cluster_info.rs @@ -2163,36 +2163,21 @@ impl ClusterInfo { I: IntoIterator, { let keypair = self.keypair(); -<<<<<<< HEAD - let mut packets = Vec::new(); - for (addr, ping) in pings { - // Respond both with and without domain so that the other node will - // accept the response regardless of its upgrade status. - // TODO: remove domain = false once cluster is upgraded. - for domain in [false, true] { - if let Ok(pong) = Pong::new(domain, &ping, &keypair) { - let pong = Protocol::PongMessage(pong); - match Packet::from_data(Some(&addr), pong) { - Ok(packet) => packets.push(packet), - Err(err) => { - error!("failed to write pong packet: {:?}", err); - } - } - } - } - } - if packets.is_empty() { -======= - let pongs_and_dests: Vec<_> = pings + let packets: Vec<_> = pings .into_iter() .filter_map(|(addr, ping)| { let pong = Pong::new(&ping, &keypair).ok()?; let pong = Protocol::PongMessage(pong); - Some((addr, pong)) + match Packet::from_data(Some(&addr), pong) { + Ok(packet) => Some(packet), + Err(err) => { + error!("failed to write pong packet: {:?}", err); + None + } + } }) .collect(); - if pongs_and_dests.is_empty() { ->>>>>>> e283461d9 (enforces hash domain for ping-pong protocol (#28433)) + if packets.is_empty() { None } else { let packet_batch = PacketBatch::new_unpinned_with_recycler_data(