Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

solana-program: use VoteState::deserialize_into on non-bpf #35036

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion account-decoder/src/parse_vote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ use {
};

pub fn parse_vote(data: &[u8]) -> Result<VoteAccountType, ParseAccountError> {
let mut vote_state = VoteState::deserialize(data).map_err(ParseAccountError::from)?;
let mut vote_state =
VoteState::deserialize_with_bincode(data).map_err(ParseAccountError::from)?;
let epoch_credits = vote_state
.epoch_credits()
.iter()
Expand Down
2 changes: 1 addition & 1 deletion programs/vote/src/vote_state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ impl From<VoteStateUpdate> for VoteTransaction {

// utility function, used by Stakes, tests
pub fn from<T: ReadableAccount>(account: &T) -> Option<VoteState> {
VoteState::deserialize(account.data()).ok()
VoteState::deserialize_with_bincode(account.data()).ok()
}

// utility function, used by Stakes, tests
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2680,7 +2680,7 @@ impl Bank {
// vote_accounts_cache_miss_count is shown to be always zero.
let account = self.get_account_with_fixed_root(vote_pubkey)?;
if account.owner() == &solana_vote_program
&& VoteState::deserialize(account.data()).is_ok()
&& VoteState::deserialize_with_bincode(account.data()).is_ok()
{
vote_accounts_cache_miss_count.fetch_add(1, Relaxed);
}
Expand Down
74 changes: 41 additions & 33 deletions sdk/program/src/vote/state/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
//! Vote state

#[cfg(not(target_os = "solana"))]
use bincode::deserialize;
#[cfg(test)]
use {
crate::epoch_schedule::MAX_LEADER_SCHEDULE_EPOCH_OFFSET,
Expand All @@ -18,7 +16,7 @@ use {
sysvar::clock::Clock,
vote::{authorized_voters::AuthorizedVoters, error::VoteError},
},
bincode::{serialize_into, serialized_size, ErrorKind},
bincode::{serialize_into, ErrorKind},
serde_derive::{Deserialize, Serialize},
std::{collections::VecDeque, fmt::Debug, io::Cursor},
};
Expand Down Expand Up @@ -374,50 +372,64 @@ impl VoteState {
3762 // see test_vote_state_size_of.
}

// we retain bincode deserialize for not(target_os = "solana")
// because the hand-written parser does not support V0_23_5
/// Deserializes the input buffer into a newly allocated `VoteState`
///
/// This function is intended as a drop-in replacement for `bincode::deserialize()`.
/// V0_23_5 is not supported in a BPF context, but all versions are supported on non-BPF.
pub fn deserialize(input: &[u8]) -> Result<Self, InstructionError> {
let mut vote_state = Self::default();
Self::deserialize_into(input, &mut vote_state)?;
Ok(vote_state)
}

// this only exists for the sake of the feature gated upgrade to the new parser; do not use it
#[doc(hidden)]
#[allow(clippy::used_underscore_binding)]
pub fn deserialize_with_bincode(_input: &[u8]) -> Result<Self, InstructionError> {
Comment on lines +385 to +388
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i waffled a lot on whether to try and do the feature gate and vote program/runtime parser upgrade all in this pr or split into two, but the fact that some functions that want to use VoteState::deserialize() dont have access to the featureset struct made me decide the changes are too complicated and should be done separately. this function is identical to the original VoteState::deserialize() and i replace calls to that with this in a few places

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.

Rather than having this temporarily-private function though, how about just having the call-sites do bincode::deserialize themselves?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

figured this was cleaner because other callsites would need to use VoteStateVersions and InstructionError. i could make it a pub(crate) function instead of a struct method

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah pub(crate) doesnt work because multiple crates and inlining bincode would mean i need to add it to and then remove it from Cargo.tomls. i think ill just do all this in one shot in #35079 rather than create an uncomfortable intermediate state

#[cfg(not(target_os = "solana"))]
{
deserialize::<VoteStateVersions>(input)
bincode::deserialize::<VoteStateVersions>(_input)
.map(|versioned| versioned.convert_to_current())
.map_err(|_| InstructionError::InvalidAccountData)
}
#[cfg(target_os = "solana")]
{
let mut vote_state = Self::default();
Self::deserialize_into(input, &mut vote_state)?;
Ok(vote_state)
}
unimplemented!()
}

/// Deserializes the input buffer into the provided `VoteState`
///
/// This function exists to deserialize `VoteState` in a BPF context without going above
/// the compute limit, and must be kept up to date with `bincode::deserialize`.
/// This function is exposed to allow deserialization in a BPF context directly into boxed memory.
pub fn deserialize_into(
input: &[u8],
vote_state: &mut VoteState,
) -> Result<(), InstructionError> {
let minimum_size =
serialized_size(vote_state).map_err(|_| InstructionError::InvalidAccountData)?;
if (input.len() as u64) < minimum_size {
return Err(InstructionError::InvalidAccountData);
}

Comment on lines -402 to -407
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check is unnecessary, the cursor position check on line 421 covers all cases of too-short buffers. in fact all it ends up doing is imposing a compute cost on correct inputs while saving compute for malformed inputs. presuming an account owner check precedes deserialization, we should never encounter them

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming this is correct -- any one of the reads will fail properly if the buffer isn't big enough

let mut cursor = Cursor::new(input);

let variant = read_u32(&mut cursor)?;
match variant {
// V0_23_5. not supported; these should not exist on mainnet
0 => Err(InstructionError::InvalidAccountData),
// V0_23_5. not supported for bpf targets; these should not exist on mainnet
// supported for non-bpf targets for backwards compatibility
0 => {
#[cfg(not(target_os = "solana"))]
{
*vote_state = bincode::deserialize::<VoteStateVersions>(input)
.map(|versioned| versioned.convert_to_current())
.map_err(|_| InstructionError::InvalidAccountData)?;

Ok(())
}
#[cfg(target_os = "solana")]
Err(InstructionError::InvalidAccountData)
}
// V1_14_11. substantially different layout and data from V0_23_5
1 => deserialize_vote_state_into(&mut cursor, vote_state, false),
// Current. the only difference from V1_14_11 is the addition of a slot-latency to each vote
2 => deserialize_vote_state_into(&mut cursor, vote_state, true),
_ => Err(InstructionError::InvalidAccountData),
}?;

// if cursor overruns the input, it produces 0 values and continues to advance `position`
// this check ensures we do not accept such a malformed input erroneously
if cursor.position() > input.len() as u64 {
return Err(InstructionError::InvalidAccountData);
}
Expand Down Expand Up @@ -865,7 +877,7 @@ pub mod serde_compact_vote_state_update {

#[cfg(test)]
mod tests {
use {super::*, itertools::Itertools, rand::Rng};
use {super::*, bincode::serialized_size, itertools::Itertools, rand::Rng};

#[test]
fn test_vote_serialize() {
Expand All @@ -885,14 +897,13 @@ mod tests {
}

#[test]
fn test_vote_deserialize_into() {
fn test_vote_deserialize() {
// base case
let target_vote_state = VoteState::default();
let vote_state_buf =
bincode::serialize(&VoteStateVersions::new_current(target_vote_state.clone())).unwrap();

let mut test_vote_state = VoteState::default();
VoteState::deserialize_into(&vote_state_buf, &mut test_vote_state).unwrap();
let test_vote_state = VoteState::deserialize(&vote_state_buf).unwrap();

assert_eq!(target_vote_state, test_vote_state);

Expand All @@ -908,31 +919,28 @@ mod tests {
let vote_state_buf = bincode::serialize(&target_vote_state_versions).unwrap();
let target_vote_state = target_vote_state_versions.convert_to_current();

let mut test_vote_state = VoteState::default();
VoteState::deserialize_into(&vote_state_buf, &mut test_vote_state).unwrap();
let test_vote_state = VoteState::deserialize(&vote_state_buf).unwrap();

assert_eq!(target_vote_state, test_vote_state);
}
}

#[test]
fn test_vote_deserialize_into_nopanic() {
fn test_vote_deserialize_nopanic() {
// base case
let mut test_vote_state = VoteState::default();
let e = VoteState::deserialize_into(&[], &mut test_vote_state).unwrap_err();
let e = VoteState::deserialize(&[]).unwrap_err();
assert_eq!(e, InstructionError::InvalidAccountData);

// variant
let serialized_len_x4 = serialized_size(&test_vote_state).unwrap() * 4;
let serialized_len_x4 = serialized_size(&VoteState::default()).unwrap() * 4;
let mut rng = rand::thread_rng();
for _ in 0..1000 {
let raw_data_length = rng.gen_range(1..serialized_len_x4);
let raw_data: Vec<u8> = (0..raw_data_length).map(|_| rng.gen::<u8>()).collect();

// it is extremely improbable, though theoretically possible, for random bytes to be syntactically valid
// so we only check that the deserialize function does not panic
let mut test_vote_state = VoteState::default();
let _ = VoteState::deserialize_into(&raw_data, &mut test_vote_state);
let _ = VoteState::deserialize(&raw_data);
}
}

Expand Down
59 changes: 59 additions & 0 deletions sdk/program/src/vote/state/vote_state_0_23_5.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
#![allow(clippy::arithmetic_side_effects)]
use super::*;
#[cfg(test)]
use arbitrary::{Arbitrary, Unstructured};

const MAX_ITEMS: usize = 32;

#[derive(Debug, Default, Serialize, Deserialize, PartialEq, Eq, Clone)]
#[cfg_attr(test, derive(Arbitrary))]
pub struct VoteState0_23_5 {
/// the node that votes in this account
pub node_pubkey: Pubkey,
Expand Down Expand Up @@ -59,3 +62,59 @@ impl<I> CircBuf<I> {
self.buf[self.idx] = item;
}
}

#[cfg(test)]
impl<'a, I: Default + Copy> Arbitrary<'a> for CircBuf<I>
where
I: Arbitrary<'a>,
{
fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
let mut circbuf = Self::default();

let len = u.arbitrary_len::<I>()?;
for _ in 0..len {
circbuf.append(I::arbitrary(u)?);
}

Ok(circbuf)
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_vote_deserialize_0_23_5() {
// base case
let target_vote_state = VoteState0_23_5::default();
let target_vote_state_versions = VoteStateVersions::V0_23_5(Box::new(target_vote_state));
let vote_state_buf = bincode::serialize(&target_vote_state_versions).unwrap();

let test_vote_state = VoteState::deserialize(&vote_state_buf).unwrap();

assert_eq!(
target_vote_state_versions.convert_to_current(),
test_vote_state
);

// variant
// provide 4x the minimum struct size in bytes to ensure we typically touch every field
let struct_bytes_x4 = std::mem::size_of::<VoteState0_23_5>() * 4;
for _ in 0..100 {
let raw_data: Vec<u8> = (0..struct_bytes_x4).map(|_| rand::random::<u8>()).collect();
let mut unstructured = Unstructured::new(&raw_data);

let arbitrary_vote_state = VoteState0_23_5::arbitrary(&mut unstructured).unwrap();
let target_vote_state_versions =
VoteStateVersions::V0_23_5(Box::new(arbitrary_vote_state));

let vote_state_buf = bincode::serialize(&target_vote_state_versions).unwrap();
let target_vote_state = target_vote_state_versions.convert_to_current();

let test_vote_state = VoteState::deserialize(&vote_state_buf).unwrap();

assert_eq!(target_vote_state, test_vote_state);
}
}
}
25 changes: 20 additions & 5 deletions sdk/program/src/vote/state/vote_state_deserialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@ use {
serialize_utils::cursor::*,
vote::state::{BlockTimestamp, LandedVote, Lockout, VoteState, MAX_ITEMS},
},
bincode::serialized_size,
std::io::Cursor,
};

// hardcode this number to avoid calculating onchain; this is a fixed-size ringbuffer
// `serialized_size()` must be used over `mem::size_of()` because of alignment
const PRIOR_VOTERS_SERIALIZED_SIZE: u64 = 1545;

pub(super) fn deserialize_vote_state_into(
cursor: &mut Cursor<&[u8]>,
vote_state: &mut VoteState,
Expand Down Expand Up @@ -70,10 +73,8 @@ fn read_prior_voters_into<T: AsRef<[u8]>>(
// record our position at the start of the struct
let prior_voters_position = cursor.position();

// `serialized_size()` must be used over `mem::size_of()` because of alignment
let is_empty_position = serialized_size(&vote_state.prior_voters)
.ok()
.and_then(|v| v.checked_add(prior_voters_position))
let is_empty_position = PRIOR_VOTERS_SERIALIZED_SIZE
.checked_add(prior_voters_position)
.and_then(|v| v.checked_sub(1))
.ok_or(InstructionError::InvalidAccountData)?;

Expand Down Expand Up @@ -140,3 +141,17 @@ fn read_last_timestamp_into<T: AsRef<[u8]>>(

Ok(())
}

#[cfg(test)]
mod tests {
use {super::*, bincode::serialized_size};

#[test]
fn test_prior_voters_serialized_size() {
let vote_state = VoteState::default();
assert_eq!(
serialized_size(&vote_state.prior_voters).unwrap(),
PRIOR_VOTERS_SERIALIZED_SIZE
);
}
Comment on lines +149 to +156
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this test!

}
6 changes: 4 additions & 2 deletions vote/src/vote_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,13 @@ impl VoteAccount {
}

pub fn vote_state(&self) -> Result<&VoteState, &Error> {
// VoteState::deserialize deserializes a VoteStateVersions and then
// VoteState::deserialize_with_bincode deserializes a VoteStateVersions and then
// calls VoteStateVersions::convert_to_current.
self.0
.vote_state
.get_or_init(|| VoteState::deserialize(self.0.account.data()).map_err(Error::from))
.get_or_init(|| {
VoteState::deserialize_with_bincode(self.0.account.data()).map_err(Error::from)
})
.as_ref()
}

Expand Down
Loading