Check overflow on vote tx compaction boundary (#27185)

* Check overflow on vote tx compaction boundary

Check for overflow during the conversion between VoteStateUpdate and
CompactVoteStateUpdate.

* Try removing clippy supress
This commit is contained in:
Ashwin Sekar 2022-08-23 22:29:03 -07:00 committed by GitHub
parent 804dfe0f1a
commit efa6201eda
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 137 additions and 76 deletions

View File

@ -70,7 +70,7 @@ use {
timing::timestamp,
transaction::Transaction,
},
solana_vote_program::vote_state::{CompactVoteStateUpdate, VoteTransaction},
solana_vote_program::vote_state::VoteTransaction,
std::{
collections::{HashMap, HashSet},
result,
@ -2012,7 +2012,13 @@ impl ReplayStage {
.is_active(&feature_set::compact_vote_state_updates::id());
let vote = match (should_compact, vote) {
(true, VoteTransaction::VoteStateUpdate(vote_state_update)) => {
VoteTransaction::from(CompactVoteStateUpdate::from(vote_state_update))
if let Some(compact_vote_state_update) = vote_state_update.compact() {
VoteTransaction::from(compact_vote_state_update)
} else {
// Compaction failed
warn!("Compaction failed when generating vote tx for vote account {}. Unable to vote", vote_account_pubkey);
return None;
}
}
(_, vote) => vote,
};

View File

@ -3,11 +3,7 @@
use {
crate::vote_state,
log::*,
solana_program::vote::{
instruction::VoteInstruction,
program::id,
state::{VoteAuthorize, VoteStateUpdate},
},
solana_program::vote::{instruction::VoteInstruction, program::id, state::VoteAuthorize},
solana_program_runtime::{
invoke_context::InvokeContext, sysvar_cache::get_sysvar_with_account_check,
},
@ -186,11 +182,12 @@ pub fn process_instruction(
let sysvar_cache = invoke_context.get_sysvar_cache();
let slot_hashes = sysvar_cache.get_slot_hashes()?;
let clock = sysvar_cache.get_clock()?;
let vote_state_update = compact_vote_state_update.uncompact()?;
vote_state::process_vote_state_update(
&mut me,
slot_hashes.slot_hashes(),
&clock,
VoteStateUpdate::from(compact_vote_state_update),
vote_state_update,
&signers,
&invoke_context.feature_set,
)

View File

@ -2631,13 +2631,13 @@ mod tests {
vote_state_update.hash = Hash::new_unique();
vote_state_update.root = Some(1);
let compact_vote_state_update = CompactVoteStateUpdate::from(vote_state_update.clone());
let compact_vote_state_update = vote_state_update.clone().compact().unwrap();
assert_eq!(vote_state_update.slots(), compact_vote_state_update.slots());
assert_eq!(vote_state_update.hash, compact_vote_state_update.hash);
assert_eq!(vote_state_update.root, compact_vote_state_update.root());
let vote_state_update_new = VoteStateUpdate::from(compact_vote_state_update);
let vote_state_update_new = compact_vote_state_update.uncompact().unwrap();
assert_eq!(vote_state_update, vote_state_update_new);
}
@ -2651,11 +2651,11 @@ mod tests {
(u64::pow(2, 28), 17),
(u64::pow(2, 28) + u64::pow(2, 16), 1),
]);
let compact_vote_state_update = CompactVoteStateUpdate::from(vote_state_update.clone());
let compact_vote_state_update = vote_state_update.clone().compact().unwrap();
assert_eq!(vote_state_update.slots(), compact_vote_state_update.slots());
let vote_state_update_new = VoteStateUpdate::from(compact_vote_state_update);
let vote_state_update_new = compact_vote_state_update.uncompact().unwrap();
assert_eq!(vote_state_update, vote_state_update_new);
}
@ -2671,11 +2671,11 @@ mod tests {
(two_31 + two_15 + 1, 6),
(two_31 + two_15 + 1 + 64, 1),
]);
let compact_vote_state_update = CompactVoteStateUpdate::from(vote_state_update.clone());
let compact_vote_state_update = vote_state_update.clone().compact().unwrap();
assert_eq!(vote_state_update.slots(), compact_vote_state_update.slots());
let vote_state_update_new = VoteStateUpdate::from(compact_vote_state_update);
let vote_state_update_new = compact_vote_state_update.uncompact().unwrap();
assert_eq!(vote_state_update, vote_state_update_new);
}
@ -2685,11 +2685,42 @@ mod tests {
let two_31 = u64::pow(2, 31);
let mut vote_state_update = VoteStateUpdate::from(vec![(two_58, 31), (two_58 + two_31, 1)]);
vote_state_update.root = Some(two_31);
let compact_vote_state_update = CompactVoteStateUpdate::from(vote_state_update.clone());
let compact_vote_state_update = vote_state_update.clone().compact().unwrap();
assert_eq!(vote_state_update.slots(), compact_vote_state_update.slots());
let vote_state_update_new = VoteStateUpdate::from(compact_vote_state_update);
let vote_state_update_new = compact_vote_state_update.uncompact().unwrap();
assert_eq!(vote_state_update, vote_state_update_new);
}
#[test]
fn test_compact_vote_state_update_overflow() {
let compact_vote_state_update = CompactVoteStateUpdate {
root: u64::MAX - 1,
root_to_first_vote_offset: 10,
lockouts_32: vec![],
lockouts_16: vec![],
lockouts_8: vec![CompactLockout::new(10)],
hash: Hash::new_unique(),
timestamp: None,
};
assert_eq!(
Err(InstructionError::ArithmeticOverflow),
compact_vote_state_update.uncompact()
);
let compact_vote_state_update = CompactVoteStateUpdate {
root: u64::MAX - u32::MAX as u64,
root_to_first_vote_offset: 10,
lockouts_32: vec![CompactLockout::new(u32::MAX)],
lockouts_16: vec![],
lockouts_8: vec![CompactLockout::new(10)],
hash: Hash::new_unique(),
timestamp: None,
};
assert_eq!(
Err(InstructionError::ArithmeticOverflow),
compact_vote_state_update.uncompact()
);
}
}

View File

@ -7,7 +7,7 @@ use {
signature::Signature,
transaction::{SanitizedTransaction, Transaction},
},
solana_vote_program::{vote_instruction::VoteInstruction, vote_state::VoteStateUpdate},
solana_vote_program::vote_instruction::VoteInstruction,
};
pub type ParsedVote = (Pubkey, VoteTransaction, Option<Hash>, Signature);
@ -82,14 +82,18 @@ fn parse_vote_instruction_data(
VoteInstruction::UpdateVoteStateSwitch(vote_state_update, hash) => {
Some((VoteTransaction::from(vote_state_update), Some(hash)))
}
VoteInstruction::CompactUpdateVoteState(compact_vote_state_update) => Some((
VoteTransaction::from(VoteStateUpdate::from(compact_vote_state_update)),
None,
)),
VoteInstruction::CompactUpdateVoteStateSwitch(compact_vote_state_update, hash) => Some((
VoteTransaction::from(VoteStateUpdate::from(compact_vote_state_update)),
Some(hash),
)),
VoteInstruction::CompactUpdateVoteState(compact_vote_state_update) => {
compact_vote_state_update
.uncompact()
.ok()
.map(|vote_state_update| (VoteTransaction::from(vote_state_update), None))
}
VoteInstruction::CompactUpdateVoteStateSwitch(compact_vote_state_update, hash) => {
compact_vote_state_update
.uncompact()
.ok()
.map(|vote_state_update| (VoteTransaction::from(vote_state_update), Some(hash)))
}
VoteInstruction::Authorize(_, _)
| VoteInstruction::AuthorizeChecked(_)
| VoteInstruction::AuthorizeWithSeed(_)

View File

@ -1,4 +1,3 @@
#![allow(clippy::integer_arithmetic)]
//! Vote state
#[cfg(test)]
@ -76,6 +75,7 @@ impl Lockout {
// The last slot at which a vote is still locked out. Validators should not
// vote on a slot in another fork which is less than or equal to this slot
// to avoid having their stake slashed.
#[allow(clippy::integer_arithmetic)]
pub fn last_locked_out_slot(&self) -> Slot {
self.slot + self.lockout()
}
@ -151,6 +151,10 @@ impl VoteStateUpdate {
pub fn slots(&self) -> Vec<Slot> {
self.lockouts.iter().map(|lockout| lockout.slot).collect()
}
pub fn compact(self) -> Option<CompactVoteStateUpdate> {
CompactVoteStateUpdate::new(self.lockouts, self.root, self.hash, self.timestamp)
}
}
/// Ignoring overhead, in a full `VoteStateUpdate` the lockouts take up
@ -194,14 +198,19 @@ impl From<Vec<(Slot, u32)>> for CompactVoteStateUpdate {
confirmation_count,
})
.collect();
Self::new(lockouts, None, Hash::default())
Self::new(lockouts, None, Hash::default(), None).unwrap()
}
}
impl CompactVoteStateUpdate {
pub fn new(mut lockouts: VecDeque<Lockout>, root: Option<Slot>, hash: Hash) -> Self {
pub fn new(
mut lockouts: VecDeque<Lockout>,
root: Option<Slot>,
hash: Hash,
timestamp: Option<UnixTimestamp>,
) -> Option<Self> {
if lockouts.is_empty() {
return Self::default();
return Some(Self::default());
}
let mut cur_slot = root.unwrap_or(0u64);
let mut cur_confirmation_count = 0;
@ -214,13 +223,14 @@ impl CompactVoteStateUpdate {
}| {
assert!(confirmation_count < 32);
let offset = slot - cur_slot;
cur_slot = slot;
cur_confirmation_count = confirmation_count;
offset
slot.checked_sub(cur_slot).map(|offset| {
cur_slot = slot;
cur_confirmation_count = confirmation_count;
offset
})
},
)
.expect("Tower should not be empty");
.expect("Tower should not be empty")?;
let mut lockouts_32 = Vec::new();
let mut lockouts_16 = Vec::new();
let mut lockouts_8 = Vec::new();
@ -231,7 +241,7 @@ impl CompactVoteStateUpdate {
} in lockouts
{
assert!(confirmation_count < 32);
let offset = slot - cur_slot;
let offset = slot.checked_sub(cur_slot)?;
if cur_confirmation_count > 15 {
lockouts_32.push(CompactLockout {
offset: offset.try_into().unwrap(),
@ -254,15 +264,15 @@ impl CompactVoteStateUpdate {
}
// Last vote should be at the top of tower, so we don't have to explicitly store it
assert!(cur_confirmation_count == 1);
Self {
Some(Self {
root: root.unwrap_or(u64::MAX),
root_to_first_vote_offset: offset,
lockouts_32,
lockouts_16,
lockouts_8,
hash,
timestamp: None,
}
timestamp,
})
}
pub fn root(&self) -> Option<Slot> {
@ -279,29 +289,32 @@ impl CompactVoteStateUpdate {
.chain(self.lockouts_16.iter().map(|lockout| lockout.offset.into()))
.chain(self.lockouts_8.iter().map(|lockout| lockout.offset.into()))
.scan(self.root().unwrap_or(0), |prev_slot, offset| {
let slot = *prev_slot + offset;
*prev_slot = slot;
Some(slot)
prev_slot.checked_add(offset).map(|slot| {
*prev_slot = slot;
slot
})
})
.collect()
}
}
impl From<CompactVoteStateUpdate> for VoteStateUpdate {
fn from(vote_state_update: CompactVoteStateUpdate) -> Self {
let lockouts = vote_state_update
pub fn uncompact(self) -> Result<VoteStateUpdate, InstructionError> {
let first_slot = self
.root()
.unwrap_or(0)
.checked_add(self.root_to_first_vote_offset)
.ok_or(InstructionError::ArithmeticOverflow)?;
let mut arithmetic_overflow_occured = false;
let lockouts = self
.lockouts_32
.iter()
.map(|lockout| (lockout.offset.into(), lockout.confirmation_count))
.chain(
vote_state_update
.lockouts_16
self.lockouts_16
.iter()
.map(|lockout| (lockout.offset.into(), lockout.confirmation_count)),
)
.chain(
vote_state_update
.lockouts_8
self.lockouts_8
.iter()
.map(|lockout| (lockout.offset.into(), lockout.confirmation_count)),
)
@ -310,36 +323,35 @@ impl From<CompactVoteStateUpdate> for VoteStateUpdate {
std::iter::once((0, 1)),
)
.scan(
vote_state_update.root().unwrap_or(0) + vote_state_update.root_to_first_vote_offset,
first_slot,
|slot, (offset, confirmation_count): (u64, u8)| {
let cur_slot = *slot;
*slot += offset;
Some(Lockout {
slot: cur_slot,
confirmation_count: confirmation_count.into(),
})
if let Some(new_slot) = slot.checked_add(offset) {
*slot = new_slot;
Some(Lockout {
slot: cur_slot,
confirmation_count: confirmation_count.into(),
})
} else {
arithmetic_overflow_occured = true;
None
}
},
)
.collect();
Self {
lockouts,
root: vote_state_update.root(),
hash: vote_state_update.hash,
timestamp: vote_state_update.timestamp,
if arithmetic_overflow_occured {
Err(InstructionError::ArithmeticOverflow)
} else {
Ok(VoteStateUpdate {
lockouts,
root: self.root(),
hash: self.hash,
timestamp: self.timestamp,
})
}
}
}
impl From<VoteStateUpdate> for CompactVoteStateUpdate {
fn from(vote_state_update: VoteStateUpdate) -> Self {
CompactVoteStateUpdate::new(
vote_state_update.lockouts,
vote_state_update.root,
vote_state_update.hash,
)
}
}
#[derive(Default, Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Copy)]
pub struct VoteInit {
pub node_pubkey: Pubkey,
@ -387,6 +399,7 @@ pub struct CircBuf<I> {
}
impl<I: Default + Copy> Default for CircBuf<I> {
#[allow(clippy::integer_arithmetic)]
fn default() -> Self {
Self {
buf: [I::default(); MAX_ITEMS],
@ -397,6 +410,7 @@ impl<I: Default + Copy> Default for CircBuf<I> {
}
impl<I> CircBuf<I> {
#[allow(clippy::integer_arithmetic)]
pub fn append(&mut self, item: I) {
// remember prior delegate and when we switched, to support later slashing
self.idx += 1;
@ -506,6 +520,7 @@ impl VoteState {
///
/// if commission calculation is 100% one way or other,
/// indicate with false for was_split
#[allow(clippy::integer_arithmetic)]
pub fn commission_split(&self, on: u64) -> (u64, u64, bool) {
match self.commission.min(100) {
0 => (0, on, false),
@ -573,6 +588,7 @@ impl VoteState {
}
/// increment credits, record credits for last epoch if new epoch
#[allow(clippy::integer_arithmetic)]
pub fn increment_credits(&mut self, epoch: Epoch, credits: u64) {
// increment credits, record by epoch
@ -600,6 +616,7 @@ impl VoteState {
self.epoch_credits.last_mut().unwrap().1 += credits;
}
#[allow(clippy::integer_arithmetic)]
pub fn nth_recent_vote(&self, position: usize) -> Option<&Lockout> {
if position < self.votes.len() {
let pos = self.votes.len() - 1 - position;
@ -735,6 +752,7 @@ impl VoteState {
}
}
#[allow(clippy::integer_arithmetic)]
pub fn double_lockouts(&mut self) {
let stack_depth = self.votes.len();
for (i, v) in self.votes.iter_mut().enumerate() {
@ -762,6 +780,7 @@ impl VoteState {
Ok(())
}
#[allow(clippy::integer_arithmetic)]
pub fn is_correct_size_and_initialized(data: &[u8]) -> bool {
const VERSION_OFFSET: usize = 4;
data.len() == VoteState::size_of()

View File

@ -5,7 +5,7 @@ use {
bincode::deserialize,
serde_json::json,
solana_sdk::{instruction::CompiledInstruction, message::AccountKeys},
solana_vote_program::{vote_instruction::VoteInstruction, vote_state::VoteStateUpdate},
solana_vote_program::vote_instruction::VoteInstruction,
};
pub fn parse_vote(
@ -136,7 +136,9 @@ pub fn parse_vote(
})
}
VoteInstruction::CompactUpdateVoteState(compact_vote_state_update) => {
let vote_state_update = VoteStateUpdate::from(compact_vote_state_update);
let vote_state_update = compact_vote_state_update.uncompact().map_err(|_| {
ParseInstructionError::InstructionNotParsable(ParsableProgram::Vote)
})?;
check_num_vote_accounts(&instruction.accounts, 2)?;
let vote_state_update = json!({
"lockouts": vote_state_update.lockouts,
@ -154,7 +156,9 @@ pub fn parse_vote(
})
}
VoteInstruction::CompactUpdateVoteStateSwitch(compact_vote_state_update, hash) => {
let vote_state_update = VoteStateUpdate::from(compact_vote_state_update);
let vote_state_update = compact_vote_state_update.uncompact().map_err(|_| {
ParseInstructionError::InstructionNotParsable(ParsableProgram::Vote)
})?;
check_num_vote_accounts(&instruction.accounts, 2)?;
let vote_state_update = json!({
"lockouts": vote_state_update.lockouts,
@ -252,7 +256,7 @@ mod test {
solana_sdk::{hash::Hash, message::Message, pubkey::Pubkey, sysvar},
solana_vote_program::{
vote_instruction,
vote_state::{CompactVoteStateUpdate, Vote, VoteAuthorize, VoteInit},
vote_state::{Vote, VoteAuthorize, VoteInit, VoteStateUpdate},
},
};
@ -766,7 +770,7 @@ mod test {
#[test]
fn test_parse_compact_vote_state_update_ix() {
let vote_state_update = VoteStateUpdate::from(vec![(0, 3), (1, 2), (2, 1)]);
let compact_vote_state_update = CompactVoteStateUpdate::from(vote_state_update.clone());
let compact_vote_state_update = vote_state_update.clone().compact().unwrap();
let vote_pubkey = Pubkey::new_unique();
let authorized_voter_pubkey = Pubkey::new_unique();
@ -809,7 +813,7 @@ mod test {
#[test]
fn test_parse_compact_vote_state_update_switch_ix() {
let vote_state_update = VoteStateUpdate::from(vec![(0, 3), (1, 2), (2, 1)]);
let compact_vote_state_update = CompactVoteStateUpdate::from(vote_state_update.clone());
let compact_vote_state_update = vote_state_update.clone().compact().unwrap();
let vote_pubkey = Pubkey::new_unique();
let authorized_voter_pubkey = Pubkey::new_unique();