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.
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.
Confirming this is correct -- any one of the reads will fail properly if the buffer isn't big enough
2501babe
force-pushed
the
20240201_bettervsde
branch
2 times, most recently
from
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 |
2501babe
force-pushed
the
20240201_bettervsde
branch
3 times, most recently
from
546b76f
to
b9c5bec
Compare
2501babe
force-pushed
the
20240201_bettervsde
branch
from
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.
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.
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.
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.
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
| // 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.
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.
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.
Thanks for adding this test!
Problem
the custom
VoteStateparser 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_5support 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_5for 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