From c9c78622a842ea795c7d37dc0f1b42b9e9ce3e20 Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Wed, 29 Dec 2021 19:31:26 +0000 Subject: [PATCH] discards serialized gossip crds votes if cannot parse tx (#22129) --- gossip/src/cluster_info.rs | 5 +-- gossip/src/crds_gossip_pull.rs | 7 ++-- gossip/src/crds_value.rs | 55 ++++++++++++------------------ local-cluster/src/cluster_tests.rs | 2 +- perf/src/sigverify.rs | 9 +++-- perf/src/test_tx.rs | 26 +++++++++----- 6 files changed, 53 insertions(+), 51 deletions(-) diff --git a/gossip/src/cluster_info.rs b/gossip/src/cluster_info.rs index 4d2399363..860e553f5 100644 --- a/gossip/src/cluster_info.rs +++ b/gossip/src/cluster_info.rs @@ -983,7 +983,7 @@ impl ClusterInfo { assert!((vote_index as usize) < MAX_LOCKOUT_HISTORY); let self_pubkey = self.id(); let now = timestamp(); - let vote = Vote::new(self_pubkey, vote, now); + let vote = Vote::new(self_pubkey, vote, now).unwrap(); let vote = CrdsData::Vote(vote_index, vote); let vote = CrdsValue::new_signed(vote, &self.keypair()); let mut gossip_crds = self.gossip.crds.write().unwrap(); @@ -4219,7 +4219,8 @@ mod tests { keypair.pubkey(), vote_tx, 0, // wallclock - ); + ) + .unwrap(); let vote = CrdsValue::new_signed(CrdsData::Vote(1, vote), &Keypair::new()); assert!(bincode::serialized_size(&vote).unwrap() <= PUSH_MESSAGE_MAX_PAYLOAD_SIZE as u64); } diff --git a/gossip/src/crds_gossip_pull.rs b/gossip/src/crds_gossip_pull.rs index c3860a4cc..5ce1559f1 100644 --- a/gossip/src/crds_gossip_pull.rs +++ b/gossip/src/crds_gossip_pull.rs @@ -675,7 +675,7 @@ pub(crate) mod tests { rand::{seq::SliceRandom, thread_rng, SeedableRng}, rand_chacha::ChaChaRng, rayon::ThreadPoolBuilder, - solana_perf::test_tx::test_tx, + solana_perf::test_tx::new_test_vote_tx, solana_sdk::{ hash::{hash, HASH_BYTES}, packet::PACKET_DATA_SIZE, @@ -1623,6 +1623,7 @@ pub(crate) mod tests { #[test] fn test_process_pull_response() { + let mut rng = rand::thread_rng(); let node_crds = RwLock::::default(); let node = CrdsGossipPull::default(); @@ -1678,8 +1679,8 @@ pub(crate) mod tests { ); // construct something that's not a contact info - let peer_vote = - CrdsValue::new_unsigned(CrdsData::Vote(0, Vote::new(peer_pubkey, test_tx(), 0))); + let peer_vote = Vote::new(peer_pubkey, new_test_vote_tx(&mut rng), 0).unwrap(); + let peer_vote = CrdsValue::new_unsigned(CrdsData::Vote(0, peer_vote)); // check that older CrdsValues (non-ContactInfos) infos pass even if are too old, // but a recent contact info (inserted above) exists assert_eq!( diff --git a/gossip/src/crds_value.rs b/gossip/src/crds_value.rs index 50b3675b8..20906a63d 100644 --- a/gossip/src/crds_value.rs +++ b/gossip/src/crds_value.rs @@ -305,15 +305,14 @@ impl Sanitize for Vote { } impl Vote { - pub fn new(from: Pubkey, transaction: Transaction, wallclock: u64) -> Self { - let slot = - parse_vote_transaction(&transaction).and_then(|(_, vote, _)| vote.last_voted_slot()); - Self { + // Returns None if cannot parse transaction into a vote. + pub fn new(from: Pubkey, transaction: Transaction, wallclock: u64) -> Option { + parse_vote_transaction(&transaction).map(|(_, vote, _)| Self { from, transaction, wallclock, - slot, - } + slot: vote.last_voted_slot(), + }) } /// New random Vote for tests and benchmarks. @@ -347,16 +346,11 @@ impl<'de> Deserialize<'de> for Vote { wallclock: u64, } let vote = Vote::deserialize(deserializer)?; - let vote = match vote.transaction.sanitize() { - Ok(_) => Self::new(vote.from, vote.transaction, vote.wallclock), - Err(_) => Self { - from: vote.from, - transaction: vote.transaction, - wallclock: vote.wallclock, - slot: None, - }, - }; - Ok(vote) + vote.transaction + .sanitize() + .map_err(serde::de::Error::custom)?; + Self::new(vote.from, vote.transaction, vote.wallclock) + .ok_or_else(|| serde::de::Error::custom("invalid vote tx")) } } @@ -692,7 +686,7 @@ mod test { bincode::{deserialize, Options}, rand::SeedableRng, rand_chacha::ChaChaRng, - solana_perf::test_tx::test_tx, + solana_perf::test_tx::new_test_vote_tx, solana_sdk::{ signature::{Keypair, Signer}, timing::timestamp, @@ -703,15 +697,14 @@ mod test { #[test] fn test_keys_and_values() { + let mut rng = rand::thread_rng(); let v = CrdsValue::new_unsigned(CrdsData::ContactInfo(ContactInfo::default())); assert_eq!(v.wallclock(), 0); let key = v.contact_info().unwrap().id; assert_eq!(v.label(), CrdsValueLabel::ContactInfo(key)); - let v = CrdsValue::new_unsigned(CrdsData::Vote( - 0, - Vote::new(Pubkey::default(), test_tx(), 0), - )); + let v = Vote::new(Pubkey::default(), new_test_vote_tx(&mut rng), 0).unwrap(); + let v = CrdsValue::new_unsigned(CrdsData::Vote(0, v)); assert_eq!(v.wallclock(), 0); let key = match &v.data { CrdsData::Vote(_, vote) => vote.from, @@ -759,6 +752,7 @@ mod test { #[test] fn test_signature() { + let mut rng = rand::thread_rng(); let keypair = Keypair::new(); let wrong_keypair = Keypair::new(); let mut v = CrdsValue::new_unsigned(CrdsData::ContactInfo(ContactInfo::new_localhost( @@ -766,10 +760,8 @@ mod test { timestamp(), ))); verify_signatures(&mut v, &keypair, &wrong_keypair); - v = CrdsValue::new_unsigned(CrdsData::Vote( - 0, - Vote::new(keypair.pubkey(), test_tx(), timestamp()), - )); + let v = Vote::new(keypair.pubkey(), new_test_vote_tx(&mut rng), timestamp()).unwrap(); + let mut v = CrdsValue::new_unsigned(CrdsData::Vote(0, v)); verify_signatures(&mut v, &keypair, &wrong_keypair); v = CrdsValue::new_unsigned(CrdsData::LowestSlot( 0, @@ -780,14 +772,10 @@ mod test { #[test] fn test_max_vote_index() { + let mut rng = rand::thread_rng(); let keypair = Keypair::new(); - let vote = CrdsValue::new_signed( - CrdsData::Vote( - MAX_VOTES, - Vote::new(keypair.pubkey(), test_tx(), timestamp()), - ), - &keypair, - ); + let vote = Vote::new(keypair.pubkey(), new_test_vote_tx(&mut rng), timestamp()).unwrap(); + let vote = CrdsValue::new_signed(CrdsData::Vote(MAX_VOTES, vote), &keypair); assert!(vote.sanitize().is_err()); } @@ -811,7 +799,8 @@ mod test { Pubkey::new_unique(), // from tx, rng.gen(), // wallclock - ); + ) + .unwrap(); assert_eq!(vote.slot, Some(7)); let bytes = bincode::serialize(&vote).unwrap(); let other = bincode::deserialize(&bytes[..]).unwrap(); diff --git a/local-cluster/src/cluster_tests.rs b/local-cluster/src/cluster_tests.rs index 1d7892a99..8c393e2f2 100644 --- a/local-cluster/src/cluster_tests.rs +++ b/local-cluster/src/cluster_tests.rs @@ -441,7 +441,7 @@ pub fn submit_vote_to_cluster_gossip( vec![CrdsValue::new_signed( CrdsData::Vote( 0, - crds_value::Vote::new(node_keypair.pubkey(), vote_tx, timestamp()), + crds_value::Vote::new(node_keypair.pubkey(), vote_tx, timestamp()).unwrap(), ), node_keypair, )], diff --git a/perf/src/sigverify.rs b/perf/src/sigverify.rs index e7e47c59b..3da88f9dd 100644 --- a/perf/src/sigverify.rs +++ b/perf/src/sigverify.rs @@ -599,7 +599,7 @@ mod tests { crate::{ packet::{Packet, PacketBatch}, sigverify::{self, PacketOffsets}, - test_tx::{test_multisig_tx, test_tx, vote_tx}, + test_tx::{new_test_vote_tx, test_multisig_tx, test_tx}, }, bincode::{deserialize, serialize}, solana_sdk::{ @@ -1187,6 +1187,7 @@ mod tests { #[test] fn test_is_simple_vote_transaction() { solana_logger::setup(); + let mut rng = rand::thread_rng(); // tansfer tx is not { @@ -1200,7 +1201,7 @@ mod tests { // single vote tx is { - let mut tx = vote_tx(); + let mut tx = new_test_vote_tx(&mut rng); tx.message.instructions[0].data = vec![1, 2, 3]; let mut packet = sigverify::make_packet_from_transaction(tx); let packet_offsets = do_get_packet_offsets(&packet, 0).unwrap(); @@ -1233,15 +1234,17 @@ mod tests { #[test] fn test_is_simple_vote_transaction_with_offsets() { solana_logger::setup(); + let mut rng = rand::thread_rng(); let mut current_offset = 0usize; let mut batch = PacketBatch::default(); batch .packets .push(sigverify::make_packet_from_transaction(test_tx())); + let tx = new_test_vote_tx(&mut rng); batch .packets - .push(sigverify::make_packet_from_transaction(vote_tx())); + .push(sigverify::make_packet_from_transaction(tx)); batch .packets .iter_mut() diff --git a/perf/src/test_tx.rs b/perf/src/test_tx.rs index 14ce3ea0f..4e896b547 100644 --- a/perf/src/test_tx.rs +++ b/perf/src/test_tx.rs @@ -1,5 +1,7 @@ use { + rand::{CryptoRng, Rng, RngCore}, solana_sdk::{ + clock::Slot, hash::Hash, instruction::CompiledInstruction, signature::{Keypair, Signer}, @@ -50,15 +52,21 @@ pub fn test_multisig_tx() -> Transaction { ) } -pub fn vote_tx() -> Transaction { - let keypair = Keypair::new(); +pub fn new_test_vote_tx(rng: &mut R) -> Transaction +where + R: CryptoRng + RngCore, +{ + let mut slots: Vec = std::iter::repeat_with(|| rng.gen()).take(5).collect(); + slots.sort_unstable(); + slots.dedup(); + let switch_proof_hash = rng.gen_bool(0.5).then(|| solana_sdk::hash::new_rand(rng)); vote_transaction::new_vote_transaction( - vec![2], - Hash::default(), - Hash::default(), - &keypair, - &keypair, - &keypair, - None, + slots, + solana_sdk::hash::new_rand(rng), // bank_hash + solana_sdk::hash::new_rand(rng), // blockhash + &Keypair::generate(rng), // node_keypair + &Keypair::generate(rng), // vote_keypair + &Keypair::generate(rng), // authorized_voter_keypair + switch_proof_hash, ) }