Award one credit per dequeued vote when processing VoteStateUpdate in… #25743

bji · 2022-06-02T20:56:43Z

…struction,

to match vote rewards of Vote instruction.

Problem

VoteStateUpdate instruction processing does not award one credit per vote dequeued from the Tower. This is a difference in how vote credits are awarded and has not received community consensus nor governance permissions.

This change will bring the VoteStateUpdate instruction processing in line with how Vote instructions are processed with regards to credit award.

Summary of Changes

Modified VoteStateUpdate processing to count dequeued slots and then award that many credits. Added some unit tests.

Feature Gate issue:

#27091

carllin · 2022-06-03T00:07:54Z

programs/vote/src/vote_state/mod.rs

+    #[test]
+    #[allow(clippy::type_complexity)]
+    #[allow(clippy::field_reassign_with_default)]
+    fn test_vote_state_update_increment_credits() {


+1 for tests

AshwinSekar · 2022-06-03T00:13:31Z

is this in tandem with #25662 ? or are you going to split it out of there

bji · 2022-06-03T00:19:29Z

programs/vote/src/vote_state/mod.rs

+            ),
+            // Complex expiry: start with 1 - 31 in the tower, then locally vote for 35, 36, 37, 38, 42, 43, 46, 47,
+            // 48, 49, 50, 53.  None of the intermediate tower states land as tx, only the resulting complete state
+            // does.  This test very succinctly illustrates the way that "timely vote credits" are an approximation of


Much of this comment is out of date, it was relevant when this change included the "timely vote credits" stuff. I can remove it if you like. I hope that it's just prep work for a future change though ....

let's only keep the relevant comments for now

carllin · 2022-06-03T00:11:52Z

programs/vote/src/vote_state/mod.rs

+                .unwrap_or(false)
+            {
+                // For each finalized slot, there was one voted-on slot in the new vote state that was responsible
+                // for finalizing it.  Each of those votes is awarded 1 credit.


nit: Remove extra whitespace

I just let rustfmt do its thing. i.e. I don't know exactly what whitespace you're talking about or how you would prefer it to be formatted. I'm just using rustfmt.

extra whitespace before the word Each

carllin · 2022-06-03T00:16:02Z

programs/vote/src/vote_state/mod.rs

+                let mut credits = 0;
+
+                for _ in new_state.iter().rev() {
+                    credits += 1;
+                    finalized_slot_count -= 1;
+                    if finalized_slot_count == 0 {
+                        break;
+                    }
+                }


hmm, why is credits not just equivalent to finalized_slot_count, i.e. the number of slots in the current vote state less than the new root?

This is somewhat vestigial since it came from my other change where I was actually calculating vote credits based on latency.

But I left it in for paranoia about "what if for some reason the number of votes that have been calculated as retired is greater than the number of votes in the tower", which shouldn't be possible. But I didn't want to allow a bug to be exploited for additional credits with some weirdly crafted new tower state that somehow slipped through other checks.

I admit that it's just a protection against an impossible scenario. I will remove if you like and just use finalized_slot_count as the number of credits to award.

what if for some reason the number of votes that have been calculated as retired is greater than the number of votes in the tower"

Yeah this shouldn't be possible given we iterated through the tower earlier and found all the smaller slots. I think just using finalized_slot_count is cleaner and correct.

technically could be right? e.g
Old state: root 0, votes 1, 2, 3, ..., 30
New state: root 16, votes 17, 18, 19, 20, 21, 300
Technically this could be a valid state transition if vote transactions weren't landing + threshold checks and lockouts expiring. The 16 votes that contributed to moving the root could have all been popped off.

Either way seems like the correct option is to use finalized_slot_count, but might cause problems if you're using the top of the tower for computing latency based credits.

carllin · 2022-06-03T00:23:39Z

programs/vote/src/vote_state/mod.rs

+        // (vote_state_lockouts: Vec<(Slot, u32)>,
+        //  vote_state_root: Option<Slot>,
+        //  vote_state_update_lockouts: Vec<(Slot, u32)>,
+        //  vote_state_update_root: Option<Slot>,
+        //  expected_credits: u64)


large tuples should be converted to structs for the benefit of named fields, i.e.

struct TestVoteData {}

If you can show me how to then declare convenient data like I have here without tons of structural verbage that would be fine.

Honest question by the way. I am somewhat new to Rust. I don't know how these things are typically done. But I do not want to have to write:
let test_data : Vec =
vec![ TestVoteData { vote_state_lockouts : xxx, vote_state_root: yyy, ... },
TestVoteData { vote_state_lockouts: xxx, vote_state_root: yyy, ... },
...];

But I do not want to have to write:

Unfortunately, I don't know of a better way. Having the struct makes the code a lot more readable as it clearly delineates start and end of individual items in the list, and makes each field self documenting.

+1 on using the struct

carllin · 2022-06-03T00:38:27Z

programs/vote/src/vote_state/mod.rs

+            let mut vote_state = VoteState::default();
+
+            vote_state.votes = vote_state_votes.clone();
+
+            vote_state.root_slot = vote_state_root;


Here I think we should do two things:

Not directly assign the .votes and root_slot, instead call process_new_vote_state() to initialize the vote state and the root

Create a #[test] only constructor for initializing this a VoteState via suggestion 1. above called new_from_lockouts

But why? It's a test case and I think that the overly strict rules of your rust linter can just be relaxed for it ...

I guess one reason would be so that changes can be made to the VoteState struct without as much direct coupling with how this test is written ...

The reason is process_new_vote_state() is an safe state transition/way to modify the VoteState, and we want to ensure we only create/modify VoteStates in ways the API envisioned. Directly modifying individual fields breaks these guarantees.

carllin · 2022-06-03T00:40:49Z

programs/vote/src/vote_state/mod.rs

+            assert_eq!(
+                vote_state.process_new_vote_state(
+                    vote_state_update_votes.clone(),
+                    vote_state_update_root,
+                    None,
+                    100,
+                    None,
+                ),
+                Ok(())
+            );
+
+            assert_eq!(vote_state.epoch_credits[0].1, 1);
+
+            let mut vote_state = VoteState::default();
+
+            vote_state.votes = vote_state_votes;
+
+            vote_state.root_slot = vote_state_root;


This should be made redundant by the suggested constructor above.

Also there should be no need to reinitialize the vote state after construction, should directly be able to call process_new_vote_state on the vote_state_update_votes

But there are two things being tested:

Vote state update without the feature set

Vote state update with the feature set

The state being tested has to be reset between these two, which is what that code is doing.

I see, that's a subtetly I missed, thanks for the clarification!

Here we can more clearly condense this function by parameterizing on feature set, i.e.
do_test_vote_state_update_increment_credits_with_feature_set(feature_set: Option<>

and then two separate tests:
#[test] do_test_vote_state_update_increment_credits_with_feature_set(Some())

#[test] do_test_vote_state_update_increment_credits_with_feature_set(None)

carllin · 2022-06-03T00:46:22Z

programs/vote/src/vote_state/mod.rs

+            // Complex expiry: start with 1 - 31 in the tower, then locally vote for 35, 36, 37, 38, 42, 43, 46, 47,
+            // 48, 49, 50, 53.  None of the intermediate tower states land as tx, only the resulting complete state
+            // does.  This test very succinctly illustrates the way that "timely vote credits" are an approximation of
+            // the latencies involved, because vote expiry causes the votes immediately before expired votes to be
+            // charged with "worse" latency than they actually had (because expiry caused them to look like they had
+            // more latency when cast than they did).  The only easy solution to this is to track the voted-in slot
+            // along with each voted-on slot, and use those values when computing vote credits.  But this would
+            // require changes to the Lockout struct to add a new value ("credits to award for this vote") and that
+            // would then require changes to the serialized format of the tower, which seems like an extensive and
+            // risky undertaking.
+            (


Because these situations are quite complex, for correctness I would prefer if we used the existing build_vote_state() function present in this module to accurately recreate and verify the votes created the tower you anticipated.

In that function, you pass in

An ordered vector of votes that represents *every( vote you are making. These should include any expired votes as well.

slot_hashes: &[(Slot, Hash)] with a hash for each slot you input into 1.

This will simulate creating the tower and ultimately spit out a vote state. That way, you can actually verify that the output vote state matches your expectations (expired votes were actually expired for instance).

OK that's a good idea. I will do so.

OK would you also recommend asserting that the resulting lockouts queue is as the test case expects it to be?

yeah the assert would be nice to check your assumptions!

carllin · 2022-06-03T00:50:13Z

@AshwinSekar plan is to merge this one first, because the rest of #25662 requires a governance vote

bji · 2022-06-03T00:53:35Z

is this in tandem with #25662 ? or are you going to split it out of there

25662 is now obsolete; I was leaving it open just so that your comments are not lost. 25662 would be a follow-up to happen only if this change is accepted, and only after proper governance stuff is done.

bji · 2022-06-04T00:32:03Z

I hope that what I have done here is OK now. I have re-created the unit test to simply ensure that the new VoteStateUpdate counts credits the same way as the old Vote processing did.

AshwinSekar

LGTM. @carllin want me to own the feature gate for this?

programs/vote/src/vote_state/mod.rs

carllin · 2022-06-04T23:18:54Z

Change looks good, just one nit

…struction, to match vote rewards of Vote instruction.

Pull request has been modified.

bji · 2022-06-06T16:47:26Z

Hi, I've rebased to master, incorporated carllin's most recent feedback about process_vote_unchecked, and squashed down into a single commit.

bji · 2022-06-06T18:03:29Z

I can't tell - are these build failures real?

Pull request has been modified.

#25743) * Award one credit per dequeued vote when processing VoteStateUpdate instruction, to match vote rewards of Vote instruction. * Update feature pubkey to one owned by cc (ashwin) Co-authored-by: Ashwin Sekar <[email protected]> (cherry picked from commit cbb0f07) # Conflicts: # programs/stake/src/stake_instruction.rs # sdk/src/feature_set.rs

mergify bot added the community Community contribution label Jun 2, 2022

mergify bot requested a review from a team June 2, 2022 20:56

carllin requested a review from AshwinSekar June 3, 2022 00:04

carllin reviewed Jun 3, 2022

View reviewed changes

bji commented Jun 3, 2022

View reviewed changes

carllin reviewed Jun 3, 2022

View reviewed changes

AshwinSekar previously approved these changes Jun 4, 2022

View reviewed changes

carllin reviewed Jun 4, 2022

View reviewed changes

programs/vote/src/vote_state/mod.rs Outdated Show resolved Hide resolved

Award one credit per dequeued vote when processing VoteStateUpdate in…

003f24d

…struction, to match vote rewards of Vote instruction.

bji force-pushed the vote_state_update_one_credit_per_dequeue branch from 2efe202 to 003f24d Compare June 6, 2022 16:46

carllin previously approved these changes Jun 6, 2022

View reviewed changes

Update feature pubkey to one owned by cc (ashwin)

c4bfbe8

AshwinSekar added the v1.10 label Jun 6, 2022

AshwinSekar merged commit cbb0f07 into solana-labs:master Jun 6, 2022

mergify bot mentioned this pull request Jun 6, 2022

Award one credit per dequeued vote when processing VoteStateUpdate in… (backport #25743) #25808

Closed

AshwinSekar added the feature-gate Pull Request adds or modifies a runtime feature gate label Aug 11, 2022

bji deleted the vote_state_update_one_credit_per_dequeue branch September 27, 2022 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Award one credit per dequeued vote when processing VoteStateUpdate in… #25743

Award one credit per dequeued vote when processing VoteStateUpdate in… #25743

bji commented Jun 2, 2022 •

edited by AshwinSekar

Loading

carllin Jun 3, 2022

AshwinSekar commented Jun 3, 2022

bji Jun 3, 2022

carllin Jun 3, 2022

carllin Jun 3, 2022

bji Jun 3, 2022 •

edited

Loading

carllin Jun 3, 2022

carllin Jun 3, 2022

bji Jun 3, 2022

carllin Jun 3, 2022

AshwinSekar Jun 3, 2022

AshwinSekar Jun 3, 2022

carllin Jun 3, 2022

bji Jun 3, 2022 •

edited

Loading

carllin Jun 3, 2022

AshwinSekar Jun 3, 2022

carllin Jun 3, 2022

bji Jun 3, 2022 •

edited

Loading

carllin Jun 3, 2022

carllin Jun 3, 2022

bji Jun 3, 2022

carllin Jun 3, 2022 •

edited

Loading

carllin Jun 3, 2022

bji Jun 3, 2022

bji Jun 3, 2022 •

edited

Loading

carllin Jun 3, 2022 •

edited

Loading

carllin commented Jun 3, 2022

bji commented Jun 3, 2022

bji commented Jun 4, 2022

AshwinSekar left a comment

carllin commented Jun 4, 2022

bji commented Jun 6, 2022

bji commented Jun 6, 2022

Award one credit per dequeued vote when processing VoteStateUpdate in… #25743

Award one credit per dequeued vote when processing VoteStateUpdate in… #25743

Conversation

bji commented Jun 2, 2022 • edited by AshwinSekar Loading

Problem

Summary of Changes

Feature Gate issue:

Choose a reason for hiding this comment

AshwinSekar commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bji Jun 3, 2022 • edited Loading

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

bji Jun 3, 2022 • edited Loading

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

bji Jun 3, 2022 • edited Loading

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

carllin Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bji Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

carllin Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

carllin commented Jun 3, 2022

bji commented Jun 3, 2022

bji commented Jun 4, 2022

AshwinSekar left a comment

Choose a reason for hiding this comment

carllin commented Jun 4, 2022

bji commented Jun 6, 2022

bji commented Jun 6, 2022

bji commented Jun 2, 2022 •

edited by AshwinSekar

Loading

bji Jun 3, 2022 •

edited

Loading

bji Jun 3, 2022 •

edited

Loading

bji Jun 3, 2022 •

edited

Loading

carllin Jun 3, 2022 •

edited

Loading

bji Jun 3, 2022 •

edited

Loading

carllin Jun 3, 2022 •

edited

Loading