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

TransactionState: add TransactionCost #34881

Conversation

apfitzge
Copy link
Contributor

@apfitzge apfitzge commented Jan 22, 2024

Problem

  • We currently do not track the TransactionCost in the TransactionState (currently holds some meta), but only the requested_cus.
  • For simple txs, such as transfers, the requested CUs massively over-estimates the CUs as they count towards limits - this has made throttling the scheduler unreliable for such txs.

Summary of Changes

  • Allow TransactionState to calculate and cache TransactionCost

This will allow 2 subsequent improvements:

  1. Scheduler can more reliably throttle number of CUs sent for execution
  2. Scheduler can prioritize by reward / cost

Fixes #

@apfitzge apfitzge force-pushed the transaction_state_container-transaction_cost branch from 329bc51 to 62d5ed2 Compare January 22, 2024 20:29
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a5c470d) 81.7% compared to head (62d5ed2) 81.7%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34881   +/-   ##
=======================================
  Coverage    81.7%    81.7%           
=======================================
  Files         826      826           
  Lines      223413   223441   +28     
=======================================
+ Hits       182614   182681   +67     
+ Misses      40799    40760   -39     

@apfitzge apfitzge marked this pull request as ready for review January 22, 2024 21:55
let cu_limit = transaction_state
.transaction_priority_details()
.compute_unit_limit;
let cost = transaction_state.transaction_cost().sum();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick check: for Vote, its cu_limit is always 0, but has value of 3428 for cost. By using different CUs, will it impact how vote's scheduled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vote's don't go through the scheduler - they have their own independent threads still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this PR realy has no impact on its' own. The CUs are tracked, but not used for load-balancing currently because they are so unreliable. This is intended to be used later on, but right now its' just tracked, so changing from cu_limit to cost shouldn't impact anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, good to know it is for the purpose of load-balancing, not counting towards the limits.

Also worth noting that transaction_cost includes requested_cu_limit, if used. So that unreliability also in transaction_cost atm.

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, a major difference here is that for simple txs which we know the cost of, it will not include "requested_cus". For example a simple transfer. So for purposes of prioritizing by reward/cu we want to use this number

Copy link
Contributor

Choose a reason for hiding this comment

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

that would de-prioritize txs with large requested_cu but actually not-so-largge_actual-cu, if signature fee is included in reward/cu calculation, would it?

Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm - it does not change existing function yet.

@apfitzge apfitzge merged commit 45a2a70 into solana-labs:master Jan 23, 2024
35 checks passed
@apfitzge apfitzge deleted the transaction_state_container-transaction_cost branch January 23, 2024 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants