-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
let minimum_size = | ||
serialized_size(vote_state).map_err(|_| InstructionError::InvalidAccountData)?; | ||
if (input.len() as u64) < minimum_size { | ||
return Err(InstructionError::InvalidAccountData); | ||
} | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
5d65d6d
to
de728f7
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #35036 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 830 830
Lines 224828 224867 +39
=======================================
+ Hits 183504 183561 +57
+ Misses 41324 41306 -18 |
546b76f
to
b9c5bec
Compare
b9c5bec
to
7e1464a
Compare
// 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> { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look great! Just a couple of small comments
// 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> { |
There was a problem hiding this comment.
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?
let minimum_size = | ||
serialized_size(vote_state).map_err(|_| InstructionError::InvalidAccountData)?; | ||
if (input.len() as u64) < minimum_size { | ||
return Err(InstructionError::InvalidAccountData); | ||
} | ||
|
There was a problem hiding this comment.
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
#[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 | ||
); | ||
} |
There was a problem hiding this comment.
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!
Problem
the custom
VoteState
parser can be much faster. also it now makes sense to expose this to non-bpf contexts so validators can benefit from faster vote parsing. however we still want to preserveV0_23_5
support for maximum safetySummary of Changes
previously, the logic went something like "if on bpf, use the new parser, otherwise use bincode," and the new parser did not support
V0_23_5
for the sake of code simplicity, as they should in theory never be encountered. now the logic goes "always use the new parser. inside the new parser, if V0_23_5 and on bpf, error. if V0_23_5 and not on bpf, fall back to bincode"also i made the parser appreciably faster. it is now 7x faster than bincode on average, can be up to 20x faster in the worst case, and is never slower
we also introduce a new function
VoteState::deserialize_with_bincode()
which is identical to the originalVoteState::deserialize()
. this is a temporary measure while we work on a (potentially feature gated) upgrade to use the new parser in the core runtime