feat(vote-program): Authorize instructions #576

dadepo · 2025-02-24T20:39:13Z

Implements the authorize related instructions.

authorize
authorize_with_seed
authorize_checked
authorize_checked_with_seed

This also includes support for versioned state and ability to convert from previous states to the current vote state.

A bulk of the methods on the VoteState where also implemented as part of implementing the authorize instructions.

codecov · 2025-02-27T15:32:52Z

Codecov Report

Attention: Patch coverage is 90.43321% with 53 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/runtime/program/vote/state.zig	86.40%	42 Missing ⚠️
src/runtime/program/vote/execute.zig	94.95%	11 Missing ⚠️

Files with missing lines	Coverage Δ
src/utils/collections.zig	`94.28% <100.00%> (+0.29%)`	⬆️
src/runtime/program/vote/execute.zig	`93.98% <94.95%> (+3.78%)`	⬆️
src/runtime/program/vote/state.zig	`86.68% <86.40%> (-1.55%)`	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/runtime/program/vote/instruction.zig

src/runtime/program/vote/state.zig

src/runtime/program/vote/execute.zig

0xNineteen

lgtm

yewman

A couple of comments from an initial pass :)

yewman · 2025-03-17T18:27:24Z

src/runtime/program/vote/error.zig

+/// Agave https://github.com/anza-xyz/solana-sdk/blob/4e30766b8d327f0191df6490e48d9ef521956495/vote-interface/src/error.rs#L11
+///
+/// Reasons the vote might have had an error
+pub const VoteError = error{


Native program errors should be enums if possible, since when we set the custom error it need to match Agave's value which comes from an error enum. This is my bad as originally I had written system program like this, but have just fixed it in a new PR here #618

cbf8ea4 Although the return type of setNewAuthorizedVoter now looks a bit awkward. Not sure how that can be improved.

yewman · 2025-03-17T18:38:23Z

src/runtime/program/vote/execute.zig

+    switch (vote_authorize) {
+        .voter => {
+            const current_epoch = clock.epoch;
+            const target_epoch = std.math.add(u64, current_epoch, 1) catch {


Even though this should never really fail, it is out of sequence with respect to Agave and theoretically could cause an incorrect error return

yewman · 2025-03-17T18:53:04Z

src/runtime/program/vote/execute.zig

+    authorized: Pubkey,
+    vote_authorize: VoteAuthorize,
+    clock: Clock,
+    signers: ?std.AutoHashMap(Pubkey, void),


I think passing a slice would be simpler here, it would also make the subsequent control flow easier to understand and compare with agave. Then signature verification can just linearly search the slice. We would probably need a helper on the instruction context to pull out a slice of signers for of the call scenarios

Depends if searching through the signers map is time-critical. Moving to a slice would make it O(N). If that doesn't matter, I agree with Harold and would prefer a slice. Either way, it shouldn't be a managed type, the allocator is passed in just above.

yewman · 2025-03-17T18:56:28Z

src/runtime/program/vote/execute.zig

+
+                // current authorized withdrawer or epoch authorized voter must say "yay"
+                if (!authorized_withdrawer_signer) {
+                    const epoch_authorized_voter = try vote_state.getAndUpdateAuthorizedVoter(


Isn't this is called regardless of the value of authorized_withdrawer_signer?

yewman · 2025-03-17T19:00:48Z

src/runtime/program/vote/execute.zig

+                        allocator,
+                        current_epoch,
+                    );
+                    if (!ic.info.isPubkeySigner(epoch_authorized_voter)) {


This should check signers if it is non null right? (Would be redundant if we make signers a non-optional slice)

yewman · 2025-03-17T20:18:21Z

src/runtime/program/vote/execute.zig

+    var new_authority = try ic.borrowInstructionAccount(
+        @intFromEnum(vote_instruction.VoteAuthorize.AccountIndex.new_authority),
+    );
+    defer new_authority.release();
+
+    if (!ic.info.isPubkeySigner(new_authority.pubkey)) {
+        return InstructionError.MissingRequiredSignature;
+    }


For context, Agave stores pubkeys in the transaction context accounts. Sig, and Firedancer keep account pubkeys within the instruction context account meta so we don't need to re-index into the transaction context accounts every time we need a key. Instead of borrowing an account from the transaction context, just use ic.info.getAccountMetaAtIndex which gives you the pubkey along with some other meta data

yewman · 2025-03-17T20:23:08Z

src/runtime/program/vote/execute.zig

+    const authorize_pubkey = switch (vote_authorize) {
+        .voter => VoteAuthorize.voter,
+        .withdrawer => VoteAuthorize.withdrawer,
+    };
+


What is the purpose of this?

yewman · 2025-03-17T20:39:32Z

src/runtime/program/vote/instruction.zig

+};
+
+pub const VoteAuthorizeWithSeedArgs = struct {
+    authorization_type: sig.runtime.program.vote_program.state.VoteAuthorize,


Could probaby use an alias for these

yewman · 2025-03-17T20:50:50Z

src/runtime/program/vote/state.zig

+/// Agave https://github.com/anza-xyz/solana-sdk/blob/4e30766b8d327f0191df6490e48d9ef521956495/vote-interface/src/state/vote_state_0_23_5.rs#L11
+pub const VoteState0_23_5 = struct {


I don't mind how we format these permalinks, but I think it would be nice to be consistent. I went with /// [agave] ... arbitrarily and am happy to conform to any other suggestion but I do love consistency 😛

yewman · 2025-03-17T20:58:19Z

src/runtime/program/vote/state.zig

+        return self.authorized_voters.contains(epoch);
+    }
+
+    // TODO Add method that returns iterator over authorized_voters


Do we need an iterator other than authorized_voters.iterator()?

Rexicon226

Just gave it a quick read through, haven't compared to Agave yet.

Rexicon226 · 2025-03-19T02:24:42Z

src/runtime/program/vote/execute.zig

@@ -129,10 +163,268 @@ fn intializeAccount(
        clock,
    );
    defer vote_state.deinit();
-    try vote_account.serializeIntoAccountData(vote_state);
+    try vote_account.serializeIntoAccountData(VoteStateVersions{ .current = vote_state });


Suggested change

try vote_account.serializeIntoAccountData(VoteStateVersions{ .current = vote_state });

try vote_account.serializeIntoAccountData(.{ .current = vote_state });

Rexicon226 · 2025-03-19T02:26:15Z

src/runtime/program/vote/execute.zig

+    authorized: Pubkey,
+    vote_authorize: VoteAuthorize,
+    clock: Clock,
+    signers: ?std.AutoHashMap(Pubkey, void),


Depends if searching through the signers map is time-critical. Moving to a slice would make it O(N). If that doesn't matter, I agree with Harold and would prefer a slice. Either way, it shouldn't be a managed type, the allocator is passed in just above.

Rexicon226 · 2025-03-19T02:27:05Z

src/runtime/program/vote/execute.zig

+            // https://github.com/anza-xyz/agave/blob/49fb51295c1062b6b09e585b2fe0a4676c33d3d4/programs/vote/src/vote_state/mod.rs#L701-L707
+            // https://github.com/anza-xyz/solana-sdk/blob/4e30766b8d327f0191df6490e48d9ef521956495/vote-interface/src/state/mod.rs#L873
+            {
+                const authorized_withdrawer_signer = if (signers) |signers_|


Don't prepend or append identifiers with _. Call it maybe_signers and signers if it does need to be optional.

Rexicon226 · 2025-03-19T02:27:32Z

src/runtime/program/vote/execute.zig

+                target_epoch,
+            );
+
+            if (err) |err_| {


Same here, I'd like us to not use _ like this.

Rexicon226 · 2025-03-19T02:29:01Z

src/runtime/program/vote/execute.zig

+
+fn validateIsSigner(
+    authorized: Pubkey,
+    signers: std.AutoHashMap(Pubkey, void),


Shouldn't be managed.

Rexicon226 · 2025-03-19T02:30:03Z

src/runtime/program/vote/execute.zig

+    const ids = sig.runtime.ids;
+    const testing = sig.runtime.program.testing;
+    const PriorVote = sig.runtime.program.vote_program.state.PriorVote;
+    _ = &PriorVote;


Useless discard.

Rexicon226 · 2025-03-19T02:34:01Z

src/runtime/program/vote/state.zig

+
+    /// Upper limit on the size of the Vote State
+    /// when votes.len() is MAX_LOCKOUT_HISTORY.
+    pub fn sizeOf() usize {


This should be a constant.

Rexicon226 · 2025-03-19T02:34:40Z

src/runtime/program/vote/state.zig

+        self: *VoteState,
+        new_authorized_voter: Pubkey,
+        target_epoch: Epoch,
+    ) (error{OutOfMemory} || InstructionError)!struct { void, ?VoteError } {


I do not understand why this is returning a void tuple member.

Rexicon226 · 2025-03-19T02:35:30Z

src/runtime/program/vote/state.zig

+    return .{
+        .node_pubkey = node_pubkey,
+        .authorized_voters = if (authorized_voter) |authorized_voter_|
+            AuthorizedVoters.init(allocator, 0, authorized_voter_) catch unreachable


I see no reason not to use try here instead of catch unreachable.

0xNineteen assigned dadepo Feb 26, 2025

Base automatically changed from dade/vote-program to main March 3, 2025 15:45

dadepo marked this pull request as ready for review March 7, 2025 14:39

dadepo requested review from yewman and 0xNineteen March 7, 2025 14:40

0xNineteen requested changes Mar 10, 2025

View reviewed changes

dadepo commented Mar 11, 2025

View reviewed changes

src/runtime/program/vote/execute.zig Show resolved Hide resolved

0xNineteen previously approved these changes Mar 14, 2025

View reviewed changes

dadepo dismissed 0xNineteen’s stale review via a8f7261 March 17, 2025 19:25

dadepo force-pushed the dade/vote-program-authorize branch from 79d8ac1 to a8f7261 Compare March 17, 2025 19:25

dadepo requested a review from 0xNineteen March 17, 2025 19:56

yewman requested changes Mar 17, 2025

View reviewed changes

dadepo added 17 commits March 18, 2025 13:14

Scaffold implementation for authorize related instructions

e52e35c

Move some instruction data from state.zig to instructions.zig

0d41632

Added versioned states

e395121

Added AuthorizedVoters

0392513

implement other methods on AuthorizedVoters

b6313d3

Added vote error

e895647

Add impl for authorize

77a9487

Implemented authorize_with_seed

54c4f40

Support converting from previous state version to current

7ad7de9

Fixes after merge

3d8e211

Fix support for versioned vote state

dd0c608

Fixes after merge

402d6d4

Test conversion to current

f91a47d

Move tests to execute.zig

bc29fb1

Links to Agave

dbe1efc

Change oteError to error set

1cd872b

Documentation and improvement to Authorize

ce92fb6

dadepo added 15 commits March 18, 2025 13:14

Move checking authority of request to its own function

d2619df

Add tests for VoteState.setNewAuthorizedVoter

ebe1196

Test vote_state.isUninitialized

31728ae

More tests to increase coverage

fba1a40

Process some review comments

d7ebb67

Move state tests to test.zig

96f7b61

Remove unreachable

d52a29f

Dont convert oom

e4fd5fb

Remove serializedSize

2f58959

Added swapRemoveNoSort and used it

cd4b54b

Implement authorise instructions

a6003c4

Fixes after rebase

b751174

Fixes after rebase

ddc56c6

Fix entrypoint

0cad9c2

Turn VoteError to enum

cbf8ea4

dadepo force-pushed the dade/vote-program-authorize branch from 77610c1 to cbf8ea4 Compare March 18, 2025 09:14

Rexicon226 requested changes Mar 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vote-program): Authorize instructions #576

feat(vote-program): Authorize instructions #576

dadepo commented Feb 24, 2025 •

edited

Loading

codecov bot commented Feb 27, 2025 •

edited

Loading

0xNineteen left a comment

yewman left a comment

yewman Mar 17, 2025

dadepo Mar 18, 2025

yewman Mar 17, 2025

yewman Mar 17, 2025

Rexicon226 Mar 19, 2025

yewman Mar 17, 2025

yewman Mar 17, 2025

yewman Mar 17, 2025

yewman Mar 17, 2025

yewman Mar 17, 2025

yewman Mar 17, 2025

yewman Mar 17, 2025

Rexicon226 left a comment

Rexicon226 Mar 19, 2025

Rexicon226 Mar 19, 2025

Rexicon226 Mar 19, 2025

Rexicon226 Mar 19, 2025

Rexicon226 Mar 19, 2025

Rexicon226 Mar 19, 2025

Rexicon226 Mar 19, 2025

Rexicon226 Mar 19, 2025

Rexicon226 Mar 19, 2025

		/// Agave https://github.com/anza-xyz/solana-sdk/blob/4e30766b8d327f0191df6490e48d9ef521956495/vote-interface/src/state/vote_state_0_23_5.rs#L11
		pub const VoteState0_23_5 = struct {

	try vote_account.serializeIntoAccountData(VoteStateVersions{ .current = vote_state });
	try vote_account.serializeIntoAccountData(.{ .current = vote_state });

feat(vote-program): Authorize instructions #576

Are you sure you want to change the base?

feat(vote-program): Authorize instructions #576

Conversation

dadepo commented Feb 24, 2025 • edited Loading

codecov bot commented Feb 27, 2025 • edited Loading

Codecov Report

0xNineteen left a comment

Choose a reason for hiding this comment

yewman 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

Rexicon226 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

dadepo commented Feb 24, 2025 •

edited

Loading

codecov bot commented Feb 27, 2025 •

edited

Loading