blockworks-foundation
/
solana
mirror of https://github.com/blockworks-foundation/solana.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Eliminate unchecked arithmetic in vote program (#29885)

This commit is contained in:
Ashwin Sekar 2023-01-25 21:55:52 -08:00 committed by GitHub
parent ac5b2ffb39
commit a5213d41f3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 111 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
programs/vote/src/lib.rs
View File

@ -1,5 +1,4 @@
#![cfg_attr(RUSTC_WITH_SPECIALIZATION, feature(min_specialization))]
#![allow(clippy::integer_arithmetic)]
pub mod vote_processor;
pub mod vote_state;

4
programs/vote/src/vote_processor.rs
View File

@ -483,10 +483,10 @@ mod tests {
vote_state.authorized_withdrawer = vote_pubkey;
vote_state.epoch_credits = Vec::new();
let mut current_epoch_credits = 0;
let mut current_epoch_credits: u64 = 0;
let mut previous_epoch_credits = 0;
for (epoch, credits) in credits_to_append.iter().enumerate() {
current_epoch_credits += credits;
current_epoch_credits = current_epoch_credits.saturating_add(*credits);
vote_state.epoch_credits.push((
u64::try_from(epoch).unwrap(),
current_epoch_credits,

98
programs/vote/src/vote_state/mod.rs
View File

@ -240,11 +240,18 @@ fn check_update_vote_state_slots_are_valid(
};
if root_to_check.is_none()
&& vote_state_update_index > 0
&& proposed_vote_slot <= vote_state_update.lockouts[vote_state_update_index - 1].slot()
&& proposed_vote_slot
<= vote_state_update.lockouts[vote_state_update_index.checked_sub(1).expect(
"`vote_state_update_index` is positive when checking `SlotsNotOrdered`",
)]
.slot()
{
return Err(VoteError::SlotsNotOrdered);
}
let ancestor_slot = slot_hashes[slot_hashes_index - 1].0;
let ancestor_slot = slot_hashes[slot_hashes_index
.checked_sub(1)
.expect("`slot_hashes_index` is positive when computing `ancestor_slot`")]
.0;
// Find if this slot in the proposed vote state exists in the SlotHashes history
// to confirm if it was a valid ancestor on this fork
@ -280,7 +287,9 @@ fn check_update_vote_state_slots_are_valid(
}
root_to_check = None;
} else {
vote_state_update_index += 1;
vote_state_update_index = vote_state_update_index.checked_add(1).expect(
"`vote_state_update_index` is bounded by `MAX_LOCKOUT_HISTORY` when `proposed_vote_slot` is too old to be in SlotHashes history",
);
}
continue;
} else {
@ -296,7 +305,9 @@ fn check_update_vote_state_slots_are_valid(
}
Ordering::Greater => {
// Decrement `slot_hashes_index` to find newer slots in the SlotHashes history
slot_hashes_index -= 1;
slot_hashes_index = slot_hashes_index
.checked_sub(1)
.expect("`slot_hashes_index` is positive when finding newer slots in SlotHashes history");
continue;
}
Ordering::Equal => {
@ -306,8 +317,12 @@ fn check_update_vote_state_slots_are_valid(
if root_to_check.is_some() {
root_to_check = None;
} else {
vote_state_update_index += 1;
slot_hashes_index -= 1;
vote_state_update_index = vote_state_update_index
.checked_add(1)
.expect("`vote_state_update_index` is bounded by `MAX_LOCKOUT_HISTORY` when match is found in SlotHashes history");
slot_hashes_index = slot_hashes_index.checked_sub(1).expect(
"`slot_hashes_index` is positive when match is found in SlotHashes history",
);
}
}
}
@ -368,13 +383,15 @@ fn check_update_vote_state_slots_are_valid(
true
} else if vote_state_update_index == vote_state_update_indexes_to_filter[filter_votes_index]
{
filter_votes_index += 1;
filter_votes_index = filter_votes_index.checked_add(1).unwrap();
false
} else {
true
};
vote_state_update_index += 1;
vote_state_update_index = vote_state_update_index
.checked_add(1)
.expect("`vote_state_update_index` is bounded by `MAX_LOCKOUT_HISTORY` when filtering out irrelevant votes");
should_retain
});
@ -410,21 +427,29 @@ fn check_slots_are_valid(
.last_voted_slot()
.map_or(false, |last_voted_slot| vote_slots[i] <= last_voted_slot)
{
i += 1;
i = i
.checked_add(1)
.expect("`i` is bounded by `MAX_LOCKOUT_HISTORY` when finding larger slots");
continue;
}
// 2) Find the hash for this slot `s`.
if vote_slots[i] != slot_hashes[j - 1].0 {
if vote_slots[i] != slot_hashes[j.checked_sub(1).expect("`j` is positive")].0 {
// Decrement `j` to find newer slots
j -= 1;
j = j
.checked_sub(1)
.expect("`j` is positive when finding newer slots");
continue;
}
// 3) Once the hash for `s` is found, bump `s` to the next slot
// in `vote_slots` and continue.
i += 1;
j -= 1;
i = i
.checked_add(1)
.expect("`i` is bounded by `MAX_LOCKOUT_HISTORY` when hash is found");
j = j
.checked_sub(1)
.expect("`j` is positive when hash is found");
}
if j == slot_hashes.len() {
@ -561,7 +586,7 @@ pub fn process_new_vote_state(
// Find the first vote in the current vote state for a slot greater
// than the new proposed root
let mut current_vote_state_index = 0;
let mut current_vote_state_index: usize = 0;
let mut new_vote_state_index = 0;
// Count the number of slots at and before the new root within the current vote state lockouts. Start with 1
@ -577,9 +602,13 @@ pub fn process_new_vote_state(
// Find the first vote in the current vote state for a slot greater
// than the new proposed root
if current_vote.slot() <= new_root {
current_vote_state_index += 1;
current_vote_state_index = current_vote_state_index
.checked_add(1)
.expect("`current_vote_state_index` is bounded by `MAX_LOCKOUT_HISTORY` when processing new root");
if current_vote.slot() != new_root {
finalized_slot_count += 1;
finalized_slot_count = finalized_slot_count
.checked_add(1)
.expect("`finalized_slot_count` is bounded by `MAX_LOCKOUT_HISTORY` when processing new root");
}
continue;
}
@ -604,7 +633,9 @@ pub fn process_new_vote_state(
if current_vote.last_locked_out_slot() >= new_vote.slot() {
return Err(VoteError::LockoutConflict);
}
current_vote_state_index += 1;
current_vote_state_index = current_vote_state_index
.checked_add(1)
.expect("`current_vote_state_index` is bounded by `MAX_LOCKOUT_HISTORY` when slot is less than proposed");
}
Ordering::Equal => {
// The new vote state should never have less lockout than
@ -613,11 +644,17 @@ pub fn process_new_vote_state(
return Err(VoteError::ConfirmationRollBack);
}
current_vote_state_index += 1;
new_vote_state_index += 1;
current_vote_state_index = current_vote_state_index
.checked_add(1)
.expect("`current_vote_state_index` is bounded by `MAX_LOCKOUT_HISTORY` when slot is equal to proposed");
new_vote_state_index = new_vote_state_index
.checked_add(1)
.expect("`new_vote_state_index` is bounded by `MAX_LOCKOUT_HISTORY` when slot is equal to proposed");
}
Ordering::Greater => {
new_vote_state_index += 1;
new_vote_state_index = new_vote_state_index
.checked_add(1)
.expect("`new_vote_state_index` is bounded by `MAX_LOCKOUT_HISTORY` when slot is greater than proposed");
}
}
}
@ -738,7 +775,10 @@ pub fn authorize<S: std::hash::BuildHasher>(
vote_state.set_new_authorized_voter(
authorized,
clock.epoch,
clock.leader_schedule_epoch + 1,
clock
.leader_schedule_epoch
.checked_add(1)
.expect("epoch should be much less than u64::MAX"),
|epoch_authorized_voter| {
// current authorized withdrawer or authorized voter must say "yay"
if authorized_withdrawer_signer {
@ -1215,7 +1255,11 @@ mod tests {
fn check_lockouts(vote_state: &VoteState) {
for (i, vote) in vote_state.votes.iter().enumerate() {
let num_votes = vote_state.votes.len() - i;
let num_votes = vote_state
.votes
.len()
.checked_sub(i)
.expect("`i` is less than `vote_state.votes.len()`");
assert_eq!(vote.lockout(), INITIAL_LOCKOUT.pow(num_votes as u32) as u64);
}
}
@ -2833,8 +2877,8 @@ mod tests {
#[test_case(0, true; "first slot")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH / 2, true; "halfway through epoch")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH / 2 + 1, false; "halfway through epoch plus one")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH - 1, false; "last slot in epoch")]
#[test_case((DEFAULT_SLOTS_PER_EPOCH / 2).saturating_add(1), false; "halfway through epoch plus one")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH.saturating_sub(1), false; "last slot in epoch")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH, true; "first slot in second epoch")]
fn test_epoch_half_check(slot: Slot, expected_allowed: bool) {
let epoch_schedule = EpochSchedule::without_warmup();
@ -2860,14 +2904,14 @@ mod tests {
#[test_case(0, true; "first slot")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH / 2, true; "halfway through epoch")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH / 2 + 1, false; "halfway through epoch plus one")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH - 1, false; "last slot in epoch")]
#[test_case((DEFAULT_SLOTS_PER_EPOCH / 2).saturating_add(1), false; "halfway through epoch plus one")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH.saturating_sub(1), false; "last slot in epoch")]
#[test_case(DEFAULT_SLOTS_PER_EPOCH, true; "first slot in second epoch")]
fn test_epoch_half_check_with_warmup(slot: Slot, expected_allowed: bool) {
let epoch_schedule = EpochSchedule::default();
let first_normal_slot = epoch_schedule.first_normal_slot;
assert_eq!(
is_commission_update_allowed(first_normal_slot + slot, &epoch_schedule),
is_commission_update_allowed(first_normal_slot.saturating_add(slot), &epoch_schedule),
expected_allowed
);
}

62
sdk/program/src/vote/state/mod.rs
View File

@ -82,9 +82,8 @@ 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()
self.slot.saturating_add(self.lockout())
}
pub fn is_locked_out_at_slot(&self, slot: Slot) -> bool {
@ -200,22 +199,25 @@ 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],
idx: MAX_ITEMS - 1,
idx: MAX_ITEMS
.checked_sub(1)
.expect("`MAX_ITEMS` should be positive"),
is_empty: true,
}
}
}
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;
self.idx %= MAX_ITEMS;
self.idx = self
.idx
.checked_add(1)
.and_then(|idx| idx.checked_rem(MAX_ITEMS))
.expect("`self.idx` should be < `MAX_ITEMS` which should be non-zero");
self.buf[self.idx] = item;
self.is_empty = false;
@ -321,7 +323,6 @@ 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),
@ -333,8 +334,18 @@ impl VoteState {
// This is also to cancel the rewarding if either of the parties
// should receive only fractional lamports, resulting in not being rewarded at all.
// Thus, note that we intentionally discard any residual fractional lamports.
let mine = on * u128::from(split) / 100u128;
let theirs = on * u128::from(100 - split) / 100u128;
let mine = on
.checked_mul(u128::from(split))
.expect("multiplication of a u64 and u8 should not overflow")
/ 100u128;
let theirs = on
.checked_mul(u128::from(
100u8
.checked_sub(split)
.expect("commission cannot be greater than 100"),
))
.expect("multiplication of a u64 and u8 should not overflow")
/ 100u128;
(mine as u64, theirs as u64, true)
}
@ -389,7 +400,6 @@ 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
@ -414,13 +424,17 @@ impl VoteState {
}
}
self.epoch_credits.last_mut().unwrap().1 += credits;
self.epoch_credits.last_mut().unwrap().1 =
self.epoch_credits.last().unwrap().1.saturating_add(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;
let pos = self
.votes
.len()
.checked_sub(position)
.and_then(|pos| pos.checked_sub(1))?;
self.votes.get(pos)
} else {
None
@ -553,13 +567,12 @@ 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() {
// Don't increase the lockout for this vote until we get more confirmations
// than the max number of confirmations this vote has seen
if stack_depth > i + v.confirmation_count() as usize {
if stack_depth > i.checked_add(v.confirmation_count() as usize).expect("`confirmation_count` and tower_size should be bounded by `MAX_LOCKOUT_HISTORY`") {
v.increase_confirmation_count(1);
}
}
@ -581,12 +594,11 @@ impl VoteState {
Ok(())
}
#[allow(clippy::integer_arithmetic)]
pub fn is_correct_size_and_initialized(data: &[u8]) -> bool {
const VERSION_OFFSET: usize = 4;
const DEFAULT_PRIOR_VOTERS_END: usize = VERSION_OFFSET + DEFAULT_PRIOR_VOTERS_OFFSET;
data.len() == VoteState::size_of()
&& data[VERSION_OFFSET..VERSION_OFFSET + DEFAULT_PRIOR_VOTERS_OFFSET]
!= [0; DEFAULT_PRIOR_VOTERS_OFFSET]
&& data[VERSION_OFFSET..DEFAULT_PRIOR_VOTERS_END] != [0; DEFAULT_PRIOR_VOTERS_OFFSET]
}
}
@ -1145,19 +1157,21 @@ mod tests {
}
}
#[allow(clippy::integer_arithmetic)]
fn run_serde_compact_vote_state_update<R: Rng>(rng: &mut R) {
let lockouts: VecDeque<_> = std::iter::repeat_with(|| {
let slot = 149_303_885 + rng.gen_range(0, 10_000);
let slot = 149_303_885_u64.saturating_add(rng.gen_range(0, 10_000));
let confirmation_count = rng.gen_range(0, 33);
Lockout::new_with_confirmation_count(slot, confirmation_count)
})
.take(32)
.sorted_by_key(|lockout| lockout.slot())
.collect();
let root = rng
.gen_ratio(1, 2)
.then(|| lockouts[0].slot() - rng.gen_range(0, 1_000));
let root = rng.gen_ratio(1, 2).then(|| {
lockouts[0]
.slot()
.checked_sub(rng.gen_range(0, 1_000))
.expect("All slots should be greater than 1_000")
});
let timestamp = rng.gen_ratio(1, 2).then(|| rng.gen());
let hash = Hash::from(rng.gen::<[u8; 32]>());
let vote_state_update = VoteStateUpdate {