Skip to content
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

Separate vote cost #33230

Merged
merged 10 commits into from
Sep 25, 2023
Merged

Conversation

tao-stones
Copy link
Contributor

@tao-stones tao-stones commented Sep 12, 2023

Problem

Usage cost of simple-vote transaction is static (#33269), due to restrictions of being a "simple vote transaction".

Assigning a static usage cost to simple-vote, instead of going to calculation that applies to normal transaction, would simplify code. Also preparing for enhancing normal transaction's usage cost categorization and calculation, without having to worry about accidentally changing vote's cost. Another side benefit is to slightly improve vote cost calculation performance.

Summary of Changes

  • Add enum cost type for Vote and Transaction.
  • Sets simple-vote cost to be statically 3428 CUs

Fixes #

@tao-stones tao-stones marked this pull request as ready for review September 13, 2023 16:09
@tao-stones tao-stones requested a review from apfitzge September 13, 2023 16:09
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #33230 (53a8deb) into master (997aa0a) will increase coverage by 0.0%.
Report is 5 commits behind head on master.
The diff coverage is 96.0%.

@@           Coverage Diff           @@
##           master   #33230   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         798      798           
  Lines      216579   216647   +68     
=======================================
+ Hits       177481   177540   +59     
- Misses      39098    39107    +9     

/// can be simpler, calculation quicker.
#[derive(Debug)]
pub enum TransactionCost {
Vote { writable_accounts: Vec<Pubkey> },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know upfront that we will only have 1 or 2 of these writable accounts.

Given the size of the other variant, it's probably better to use a size + array[2] here: { num: usize, writable_accounts: [Pubkey; 2] }.

That way we can avoid an allocation entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after #33269 , I am leaning to just give simple_vote a static cost, no need to track vote's writable accounts, their size etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible that we'd go over account-CU limits if non-vote txs can write to the vote-accounts, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, the whole non-vote tx's cu will be apply to the vote account in that scenario, just follow the usual logic. I bet there are some guards to prevent vote embedded in non-vote transaction. Will dig it between tasks ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So looks like it is always allowed to have vote ix included in normal transaction, where the vote shall be processed as usual. So the scenario you mentioned above is possible.

As for SimpleVote to use Vec<Pubkey> or array of two, imo, using vec is fine for current code since cost-model creates a vec of pubkey for writable accounts, then move to TransactionCost.

(Later when making TransactionCost as part of transaction meta, we can get ride of copying pubkey part, instead just holding write account's index.)

cost-model/src/transaction_cost.rs Show resolved Hide resolved

pub fn account_data_size(&self) -> u64 {
match self {
Self::Vote { .. } => 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably we want comments here. The actual size is not 0 for loaded accounts. It's I think we just don't want t include the costs for votes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once decided to give simple vote a static cost, maybe of this can be refactored away.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple vote tx has 8 cu for loaded_accounts_data_size_cost, which is different form this account_data_size that tracks allocating accounts size (eg CreateAccount, Allocate space etc)

apfitzge
apfitzge previously approved these changes Sep 25, 2023
@tao-stones tao-stones merged commit a41c15e into solana-labs:master Sep 25, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants