-
Notifications
You must be signed in to change notification settings - Fork 220
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: improve VoteState::deserialize_into() #2146
Conversation
6b33566
to
c4c07e0
Compare
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.
Looks great! A couple of nits and see the CircBuf comment
@@ -86,21 +86,12 @@ fn read_prior_voters_into<T: AsRef<[u8]>>( | |||
if !is_empty { |
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.
if is_empty=true and prior_voters.buf is filled, we diverge from bincode
i think? One could maliciously create such a vote state.
(I think our Arbitrary impl for CircBuf should probably additionally generate this)
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.
its true that it would diverge. how could one create such a vote state tho? i wrote this based on the fact that it seemed there was no code path to do that, all changes to circbuf go through the insert function (hence also why the Arbitrary
impl works that way)
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’m a hacker, I see this commit get deployed to mainnet, as soon as it is I craft a special payload to split the cluster
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 was gonna say some stuff about how such a payload cant be delivered because it would fail vote program ownership, but it turns out theres negligible perf benefit to the special logic anyway. so, onwards!
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.
In the call site where I'm looking to use this (Bank::update_stakes_cache), I'm definitely seeing votes fail to deserialize, so surely this must be possible somehow?
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.
not sure what itd be withput seeing an example. in any event, should be the same as bincode now
1f39b48
to
8dab344
Compare
we also relax some constraints which bring it in line with bincode::deserialize
10a4593
to
2807669
Compare
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.
great work!
Problem
earlier this year we implemented
VoteState::deserialize_into()
, a custom parser intended to be suitable for usage in a bpf context. we want to use the new parser everywhere because, with some optimizations, it is 7-20x faster than bincode depending on the inputit also uses half as much memory for
V1_14_11
because conversion toCurrent
happens during parsingSummary of Changes
implement those optimizations, and also remove some constraints that make this parser behave differently from
bincode::deserialize
. this allowsVoteState::deserialize_into
to be used in optimized vote processingin the future we would like to replace
VoteState::deserialize
to usedeserialize_into
for everything, but this requires testing on mainnet-beta because it affects sensitive parts of the codebase which cannot be feature-gatedpart of solana-labs#35101