solana-program: VoteState::deserialize() #34829

2501babe · 2024-01-18T10:36:39Z

Problem

presently, VoteState::deserialize() is not implemented for bpf compile targets because the bincode call blows the 200k compute limit. being able to deserialize this struct in a program context would likely be useful for many downstream projects. but in particular, we need this to port the stake program to bpf

Summary of Changes

implement a parser by hand. this implementation can deserialize a real devnet VoteState (empty prior_voters, several dozen votes and epoch_credits) in 7.4k CUs. it supports Current and V1_14_11 versions of the struct and returns a Current without a call to convert_to_current()

(this will remain a draft until there is sufficient test coverage, im planning on using proptest to compare it to bincode thoroughly)

sdk/program/src/vote/state/mod.rs

codecov · 2024-01-18T12:57:08Z

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (8ff511e) 81.6% compared to head (872d39d) 81.6%.
Report is 20 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #34829     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         827      829      +2     
  Lines      223884   224092    +208     
=========================================
+ Hits       182841   183008    +167     
- Misses      41043    41084     +41

…ariants

2501babe · 2024-01-23T20:59:18Z

@CriesofCarrots i put the more general deserialization helpers in a new submodule of solana_program::serialize_utils since you suggested it, note theyre not marked for bincode because the binary layout is the same as borsh. other than moving some helpers and the addition of tests, the code is the same as when you looked last

joncinque

Mostly tiny nits, this looks great and easy to follow!

sdk/program/src/vote/state/mod.rs

joncinque · 2024-01-24T15:58:32Z

sdk/program/src/vote/state/mod.rs

+
+        // variant
+        for _ in 0..1000 {
+            let size_hint = VoteStateVersions::size_hint(0).0 * 4;


Sorry, I looked up what size_hint does in the arbitrary lib, but I still don't understand what the * 4 bit is for. Why is it needed?

size_hint returns a tuple with a lower bound and possibly an upper bound (here None because the struct can be any size). i just gave arbitrary 4x the minimum size to work with so it typically touches all the fields

...and it turns out this always returns (0, None) anyway so this is actually wrong

joncinque · 2024-01-24T16:02:29Z

sdk/program/src/vote/state/mod.rs

+    pub fn deserialize_into(
+        input: &[u8],
+        vote_state: &mut VoteState,
+    ) -> Result<(), InstructionError> {


micro-nit: Could you add a doc comment to this function explaining when to use it? Something like:

Deserializes the input buffer into the provided `VoteState` This function exists to deserialize `VoteState` in a BPF context without going above the stack frame limit, and must be kept up to date with `bincode::deserialize`.

Or feel free to update that as you wish!

sdk/program/src/vote/state/mod.rs

joncinque · 2024-01-24T16:04:43Z

sdk/program/src/vote/state/mod.rs

+        let mut r = StdRng::seed_from_u64(seed);
+
+        for _ in 0..1000 {
+            let raw_data_length = r.gen_range(1..1024);


Where does this number come from?

its arbitrary, i could note this in a comment?

going to use the serialized size of the default struct instead

joncinque · 2024-01-24T16:05:03Z

sdk/program/src/vote/state/mod.rs

+            r.fill(&mut raw_data[..]);
+
+            let mut test_vote_state = VoteState::default();
+            let e = VoteState::deserialize_into(&raw_data, &mut test_vote_state).unwrap_err();


If the first 4 bytes give 1u32 or 2u32, is it possible for this to succeed and give a false negative? If so, maybe we shouldn't check that it's InvalidAccountData and just that it doesn't panic

previously commented this cannot false negative because the input is too short but it would be better to do this and make the input long to hit all possible cases. its probably impossible to actually false negative tho because the is_empty byte would have to be correct and all of the u32 vector lengths would need to be below a few dozen or so

joncinque · 2024-01-24T16:28:18Z

sdk/program/src/vote/state/vote_state_deserialize.rs

+    let is_empty_idx = position_after_prior_voters
+        .checked_sub(1)
+        .ok_or(InstructionError::InvalidAccountData)?;
+
+    match input[is_empty_idx as usize] {
+        0 => read_prior_voters_into(cursor, vote_state)?,
+        1 => cursor.set_position(position_after_prior_voters),
+        _ => return Err(InstructionError::InvalidAccountData),
+    }


nit: It's a bit strange and potentially error-prone to take in the input bytes and also a cursor over the same input bytes.

Although it might make the code slightly uglier, I'd prefer to see it all operate on the cursor, so this would need to change to move the cursor to is_empty_idx, read the u8 there, and optionally move it back to the previous position if it's not empty. Does that sound reasonable?

we could do that. honestly we could also just remove this logic and have it step over the bytes normally, it turns out to not be that much of an optimization since the final version of read_prior_voters_into doesnt memcpy the null pubkeys

went ahead and did that

CriesofCarrots

Beyond Jon comments (all of which I think you've already handled), just nits from me

CriesofCarrots · 2024-01-24T17:57:04Z

sdk/program/src/vote/state/mod.rs

-    use {super::*, itertools::Itertools, rand::Rng};
+    use {
+        super::*,
+        arbitrary::{Arbitrary, Unstructured},


Do you actually need to reimport these? I would think super::* would get them.

CriesofCarrots · 2024-01-24T18:01:22Z

sdk/program/src/vote/state/mod.rs

+        match variant {
+            // V0_23_5. not supported; these should not exist on mainnet
+            0 => Err(InstructionError::InvalidAccountData),
+            // V1_14_11. substantially different layout and data


Substantially different from what? I think you mean from V0_23_5, but it is not clear.

CriesofCarrots · 2024-01-24T18:01:35Z

sdk/program/src/vote/state/mod.rs

+            0 => Err(InstructionError::InvalidAccountData),
+            // V1_14_11. substantially different layout and data
+            1 => deserialize_vote_state_into(input, &mut cursor, vote_state, false),
+            // Current. the only difference is the addition of a slot-latency to each vote


The only difference from what?

CriesofCarrots

lgtm, but would be good to ensure Jon's satisfied :)

joncinque

Looks great!

2501babe force-pushed the 20240116_votedeser branch from fbe67a2 to dcba7a0 Compare January 18, 2024 11:01

2501babe commented Jan 18, 2024

View reviewed changes

sdk/program/src/vote/state/mod.rs Outdated Show resolved Hide resolved

2501babe added 22 commits January 23, 2024 07:45

initial parser

a01e875

split out an into version, deserialize circbuf, get ready to handle v…

438d323

…ariants

abstract abstract

6e440b2

support all variants

8c8ccf7

remove prior_voters parsing for 0.23

8971c74

remove V0_23_5 support

10f146f

mostly happily cleaned up

90cf126

rename file

08ecf2a

some last notes

7f96097

couple style things

d01bdb3

conditional import was correct

edba69c

remove comments that interrupt testing

99aca45

fix ringbuffer deser

9a44ff6

silly

6f03e54

test deserialize

35a8729

rustfmt mistake? weird

d2c5d39

change maybe name

9bb63c4

test default case, test we never panic

45a7aa2

mistake in tests

6527dd9

make read helpers more general to maybe expose wider

162e9c9

move read helpers into central location

70f934c

dont use print

25f1b73

2501babe force-pushed the 20240116_votedeser branch from aff0b52 to 25f1b73 Compare January 23, 2024 15:45

2501babe added 2 commits January 23, 2024 09:39

update comment

c8e7cd9

fussy

9e4bccd

2501babe marked this pull request as ready for review January 23, 2024 20:55

2501babe requested a review from joncinque January 23, 2024 20:55

2501babe requested a review from CriesofCarrots January 23, 2024 20:55

joncinque reviewed Jan 24, 2024

View reviewed changes

2501babe added 2 commits January 24, 2024 09:48

fix most code review nits

077299d

remove too-clever cursor logic

45e3e53

CriesofCarrots reviewed Jan 24, 2024

View reviewed changes

more nits

872d39d

CriesofCarrots approved these changes Jan 24, 2024

View reviewed changes

joncinque approved these changes Jan 24, 2024

View reviewed changes

2501babe merged commit 0c2d9d2 into solana-labs:master Jan 25, 2024

This was referenced Feb 6, 2024

use VoteState::deserialize_into() in place of bincode everywhere #35101

Open

use VoteState::deserialize() everywhere #35079

Closed

apfitzge mentioned this pull request Feb 28, 2025

perf: vote state view anza-xyz/agave#4834

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solana-program: VoteState::deserialize() #34829

solana-program: VoteState::deserialize() #34829

2501babe commented Jan 18, 2024

codecov bot commented Jan 18, 2024 •

edited

Loading

2501babe commented Jan 23, 2024

joncinque left a comment

joncinque Jan 24, 2024

2501babe Jan 24, 2024

2501babe Jan 24, 2024

joncinque Jan 24, 2024

joncinque Jan 24, 2024

2501babe Jan 24, 2024

2501babe Jan 24, 2024

joncinque Jan 24, 2024

2501babe Jan 24, 2024

joncinque Jan 24, 2024

2501babe Jan 24, 2024

2501babe Jan 24, 2024

CriesofCarrots left a comment

CriesofCarrots Jan 24, 2024

CriesofCarrots Jan 24, 2024

CriesofCarrots Jan 24, 2024

CriesofCarrots left a comment

joncinque left a comment

solana-program: VoteState::deserialize() #34829

solana-program: VoteState::deserialize() #34829

Conversation

2501babe commented Jan 18, 2024

Problem

Summary of Changes

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

2501babe commented Jan 23, 2024

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 18, 2024 •

edited

Loading