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

loaded accounts data size cost does not apply to vote transaction #33235

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

tao-stones
Copy link
Contributor

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

Problem

It is a bug to include Loaded_accounts_data_size_cost to Simple Vote transaction; if feature gate #30657 is activated, each vote transaction will have ~16K more CUs (vote tx does not have compute_budget instructions), therefore resulting less votes in block. Ideally vote should have stable cost, if not static.

It needs to be BP

Summary of Changes

  • sum vote cost differently.

Fixes #

@tao-stones tao-stones added v1.14 v1.16 PRs that should be backported to v1.16 labels Sep 13, 2023
Copy link
Contributor

@AshwinSekar AshwinSekar left a comment

Choose a reason for hiding this comment

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

lgtm, but i don't have enough context on the loaded accounts data size cost to approve.

cost-model/src/transaction_cost.rs Show resolved Hide resolved
@apfitzge
Copy link
Contributor

Does this need to be feature-gated since we're changing to cost of votes on testnet where the feature is already active?

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #33235 (5401a22) into master (525e59f) will increase coverage by 0.0%.
Report is 4 commits behind head on master.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #33235   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         790      790           
  Lines      214213   214246   +33     
=======================================
+ Hits       175589   175646   +57     
+ Misses      38624    38600   -24     

@tao-stones
Copy link
Contributor Author

Does this need to be feature-gated since we're changing to cost of votes on testnet where the feature is already active?

No that feature gate #30657 was blocked, not activated anyway yet. Hope to bp this fix to everywhere before that feature is unblocked.

@tao-stones tao-stones requested review from apfitzge and removed request for apfitzge September 13, 2023 23:27
@tao-stones tao-stones requested a review from t-nelson September 14, 2023 03:40
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

sgtm! :shipit:

@tao-stones tao-stones merged commit dfaec78 into solana-labs:master Sep 14, 2023
mergify bot pushed a commit that referenced this pull request Sep 14, 2023
…3235)

* loaded accounts data size cost does not apply to vote transaction

* add a test for vote cost

(cherry picked from commit dfaec78)

# Conflicts:
#	cost-model/src/transaction_cost.rs
mergify bot pushed a commit that referenced this pull request Sep 14, 2023
…3235)

* loaded accounts data size cost does not apply to vote transaction

* add a test for vote cost

(cherry picked from commit dfaec78)

# Conflicts:
#	cost-model/src/transaction_cost.rs
@tao-stones
Copy link
Contributor Author

turns out no need to back port it - relevant logics it fixes were not back ported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants