From 9c9e2ccf4abe30f6c8b9755c274c03240826969c Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Fri, 23 Aug 2024 12:22:34 +0000 Subject: [PATCH] VoteAccounts: discard invalid accounts on deserialization Before switching to eager vote account deserialization we were (accidentally) able to load snapshots with invalid vote accounts (because account parsing was deferred). This change ensures that we're still able to parse such snapshots. --- core/src/consensus/progress_map.rs | 4 +- ledger/src/blockstore_processor.rs | 6 +- programs/bpf_loader/src/syscalls/mod.rs | 6 +- runtime/src/bank/tests.rs | 2 +- vote/src/vote_account.rs | 112 +++++++++++++++++++++++- 5 files changed, 120 insertions(+), 10 deletions(-) diff --git a/core/src/consensus/progress_map.rs b/core/src/consensus/progress_map.rs index 447ebff0f8361e..3a3ef31150e79c 100644 --- a/core/src/consensus/progress_map.rs +++ b/core/src/consensus/progress_map.rs @@ -452,7 +452,7 @@ mod test { let vote_account_pubkeys: Vec<_> = std::iter::repeat_with(solana_sdk::pubkey::new_rand) .take(num_vote_accounts) .collect(); - let epoch_vote_accounts: HashMap<_, _> = vote_account_pubkeys + let epoch_vote_accounts = vote_account_pubkeys .iter() .skip(num_vote_accounts - staked_vote_accounts) .map(|pubkey| (*pubkey, (1, VoteAccount::new_random()))) @@ -494,7 +494,7 @@ mod test { let vote_account_pubkeys: Vec<_> = std::iter::repeat_with(solana_sdk::pubkey::new_rand) .take(num_vote_accounts) .collect(); - let epoch_vote_accounts: HashMap<_, _> = vote_account_pubkeys + let epoch_vote_accounts = vote_account_pubkeys .iter() .skip(num_vote_accounts - staked_vote_accounts) .map(|pubkey| (*pubkey, (1, VoteAccount::new_random()))) diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 4af5663f78c515..94acabf1acf400 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -4570,7 +4570,11 @@ pub mod tests { let total_stake = 10; // Supermajority root should be None - assert!(supermajority_root_from_vote_accounts(total_stake, &HashMap::default()).is_none()); + assert!(supermajority_root_from_vote_accounts( + total_stake, + &VoteAccountsHashMap::default() + ) + .is_none()); // Supermajority root should be None let roots_stakes = vec![(8, 1), (3, 1), (4, 1), (8, 1)]; diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index 876b734af19156..e749d813a7737a 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -2121,8 +2121,8 @@ mod tests { last_restart_slot::LastRestartSlot, }, }, - solana_vote::vote_account::VoteAccount, - std::{collections::HashMap, mem, str::FromStr}, + solana_vote::vote_account::{VoteAccount, VoteAccountsHashMap}, + std::{mem, str::FromStr}, test_case::test_case, }; @@ -4817,7 +4817,7 @@ mod tests { compute_budget.compute_unit_limit = expected_cus; let vote_address = Pubkey::new_unique(); - let mut vote_accounts_map = HashMap::new(); + let mut vote_accounts_map = VoteAccountsHashMap::default(); vote_accounts_map.insert( vote_address, (expected_epoch_stake, VoteAccount::new_random()), diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 2647ede9ec783d..3b3d5aef3f351c 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -13037,7 +13037,7 @@ fn test_bank_epoch_stakes() { .unwrap(); (authorized_voter, (100_u64, vote_account)) }) - .collect::>(), + .collect(), 1, ); bank1.set_epoch_stakes_for_test(1, new_epoch_stakes); diff --git a/vote/src/vote_account.rs b/vote/src/vote_account.rs index e1fd6b51987c56..560d159c0c2d85 100644 --- a/vote/src/vote_account.rs +++ b/vote/src/vote_account.rs @@ -1,6 +1,9 @@ use { itertools::Itertools, - serde::ser::{Serialize, Serializer}, + serde::{ + de::{MapAccess, Visitor}, + ser::{Serialize, Serializer}, + }, solana_sdk::{ account::{AccountSharedData, ReadableAccount}, instruction::InstructionError, @@ -10,8 +13,10 @@ use { std::{ cmp::Ordering, collections::{hash_map::Entry, HashMap}, + fmt, iter::FromIterator, mem, + ops::{Deref, DerefMut}, sync::{Arc, OnceLock}, }, thiserror::Error, @@ -37,7 +42,78 @@ struct VoteAccountInner { vote_state: VoteState, } -pub type VoteAccountsHashMap = HashMap; +#[derive(Default, Clone, Debug, PartialEq, Serialize)] +#[repr(transparent)] +pub struct VoteAccountsHashMap(HashMap); + +impl Deref for VoteAccountsHashMap { + type Target = HashMap; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for VoteAccountsHashMap { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl FromIterator<(Pubkey, (/*stake:*/ u64, VoteAccount))> for VoteAccountsHashMap { + fn from_iter(iter: I) -> Self + where + I: IntoIterator, + { + Self(HashMap::from_iter(iter)) + } +} + +// This custom deserializer is needed to ensure compatibility at snapshot loading with versions +// before https://github.com/anza-xyz/agave/pull/2659 which would teorethically allow invalid vote +// accounts in VoteAccounts. +// +// In the (near) future we should remove this custom deserializer and make it a hard error when we +// find invalid vote accounts in snapshots. +impl<'de> serde::Deserialize<'de> for VoteAccountsHashMap { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + struct VoteAccountsVisitor; + + impl<'de> Visitor<'de> for VoteAccountsVisitor { + type Value = VoteAccountsHashMap; + + fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + formatter.write_str("a map of vote accounts") + } + + fn visit_map(self, mut access: M) -> Result + where + M: MapAccess<'de>, + { + let mut accounts = HashMap::new(); + + while let Some((pubkey, (stake, account))) = + access.next_entry::()? + { + match VoteAccount::try_from(account) { + Ok(vote_account) => { + accounts.insert(pubkey, (stake, vote_account)); + } + Err(e) => { + log::warn!("failed to deserialize vote account: {e}"); + } + } + } + + Ok(VoteAccountsHashMap(accounts)) + } + } + + deserializer.deserialize_map(VoteAccountsVisitor) + } +} #[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Clone, Debug, Deserialize)] @@ -381,7 +457,7 @@ impl FromIterator<(Pubkey, (/*stake:*/ u64, VoteAccount))> for VoteAccounts { where I: IntoIterator, { - Self::from(Arc::new(HashMap::from_iter(iter))) + Self::from(Arc::new(VoteAccountsHashMap::from_iter(iter))) } } @@ -549,6 +625,36 @@ mod tests { assert_eq!(*vote_accounts.vote_accounts, vote_accounts_hash_map); } + #[test] + fn test_vote_accounts_deserialize_invalid_account() { + let mut rng = rand::thread_rng(); + let mut vote_accounts_hash_map = HashMap::::new(); + + let (valid_account, _) = new_rand_vote_account(&mut rng, None); + vote_accounts_hash_map.insert(Pubkey::new_unique(), (0xAA, valid_account.clone())); + + // bad data + let invalid_account_data = + AccountSharedData::new_data(42, &vec![0xFF; 42], &solana_sdk::vote::program::id()) + .unwrap(); + vote_accounts_hash_map.insert(Pubkey::new_unique(), (0xBB, invalid_account_data)); + + // wrong owner + let invalid_account_key = + AccountSharedData::new_data(42, &valid_account.data().to_vec(), &Pubkey::new_unique()) + .unwrap(); + vote_accounts_hash_map.insert(Pubkey::new_unique(), (0xCC, invalid_account_key)); + + // populate the map with 1 valid and 2 invalid accounts, ensure that we only get the valid + // one after deserialiation + let data = bincode::serialize(&vote_accounts_hash_map).unwrap(); + let vote_accounts: VoteAccountsHashMap = bincode::deserialize(&data).unwrap(); + + assert_eq!(vote_accounts.len(), 1); + let (stake, _account) = vote_accounts.values().next().unwrap(); + assert_eq!(*stake, 0xAA); + } + #[test] fn test_staked_nodes() { let mut rng = rand::thread_rng();