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

assert simple vote tx const cost #100

Merged

Conversation

tao-stones
Copy link

Problem

Simple Vote Transaction has static cost, which is sum of vote instruction cost, signature cost, 2 write lock cost, and 8CU for data size. Should have some way to make sure the const value are correct.

Summary of Changes

  • add assert on the const values

Fixes #

@tao-stones tao-stones requested a review from apfitzge March 6, 2024 01:11
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (ce34f3f) to head (eb6db5c).

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #100     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         838      838             
  Lines      225941   225941             
=========================================
- Hits       184945   184894     -51     
- Misses      40996    41047     +51     

Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

What I meant in the previous PR was that if it's a simple vote, we should still have consistency. The PR description cost says the vote cost is derived from ix, signature, etc costs. But that's not consistent with what the struct returns me in terms of signature_cost (and others) since it just returns 0 for everything that's not sum

@diman-io
Copy link

diman-io commented Mar 6, 2024

It can have 2 or even 3 signatures (although the current code, for some reason, does not treat the case with three signatures as a simple_vote; I hope because it was written at a time when there were no other validators)

@tao-stones
Copy link
Author

It can have 2 or even 3 signatures (although the current code, for some reason, does not treat the case with three signatures as a simple_vote; I hope because it was written at a time when there were no other validators)

Do you have examples of vote tx have more than one sig? afaik, simple vote txs practically always have one sig, due to the cost of additional sigverify.

@tao-stones
Copy link
Author

What I meant in the previous PR was that if it's a simple vote, we should still have consistency. The PR description cost says the vote cost is derived from ix, signature, etc costs. But that's not consistent with what the struct returns me in terms of signature_cost (and others) since it just returns 0 for everything that's not sum

Can you explain more what you mean? SIMPLE_VOTE_USAGE_COST is consistent with its components costs, the assert is to ensure that. For example, signature_cost for vote is SIGNATURE_COST:

    pub fn signature_cost(&self) -> u64 {
        match self {
            Self::SimpleVote { .. } => block_cost_limits::SIGNATURE_COST,
            Self::Transaction(usage_cost) => usage_cost.signature_cost,
        }
    }

Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Huh. Idk which number I looked at last night, but you're right...all the component costs are there.

@diman-io
Copy link

diman-io commented Mar 6, 2024

It can have 2 or even 3 signatures (although the current code, for some reason, does not treat the case with three signatures as a simple_vote; I hope because it was written at a time when there were no other validators)

Do you have examples of vote tx have more than one sig? afaik, simple vote txs practically always have one sig, due to the cost of additional sigverify.

Yes, on-chain there are many transactions with two signatures. When you change a validator identity, you usually change the authorize voter (to reduce the cost, as you correctly noted). But the authorize voter cannot be changed instantly; it only changes through an epoch. Thus, practically, you pay 10K lamports for each voting transaction for two epochs. One of the most famous examples is probably Laine. And indeed, all validators who had their identity key leaked (at some point, as far as I remember, there was a discussion in Discord about a problematic provider) went through this.

Yes, probably, you can first switch the vote authority and then change the validator identity at the epoch boundary. But this does not cover the case of a leaked private key of validator identity.

Three signatures are needed when the payer for the transaction is separate (this case is not implementable on a vanilla validator). I understand that it is not economically feasible. But making a code that knowingly will not cover some cases, in my opinion, is also not right.

But overall, I thought, why not simplify the simple vote and include all transactions with a single instruction and a vote program in it. This will simply separate the change of vote accounts parameters from ordinary user transactions. Generally, this is not bad. After all, when central scheduling starts working, such transactions will be difficult to land without a priority fee. And it would be desirable to avoid that.

@tao-stones tao-stones merged commit c161351 into anza-xyz:master Mar 6, 2024
35 checks passed
@tao-stones tao-stones deleted the const-assert-simple-vote-cost branch March 6, 2024 17:08
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
* assert simple vote tx const cost
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.

4 participants