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

Refactor to remove requested_cu from cost_trarcker #29015

Conversation

tao-stones
Copy link
Contributor

Problem

Forwarder uses requested_cu to track how many CUs are forwarded. Now cost_model has been updated to use requested_cu (#28281), forwarder should use cost_model for consistency.

Summary of Changes

  • refactor forwarder to use cost_model to calculate transaction cost (instead of use requested_cu as a rough measure)
  • remove requested_cu from cost_tracker.

Fixes #

apfitzge
apfitzge previously approved these changes Dec 1, 2022
Copy link
Contributor

@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.

Only minor nits in csome comments, otherwise lgtm

.unwrap();

// set limit ratio so each batch can only have one test transaction,
// eg ratio = floor(MAX_WRITABLE_ACCOUNT_UNITS- / tx_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.

either fix the comment (trailling -) or just remove it. I don't think it adds much since the equation is simply the coded line below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, removed the line

let requested_cu =
block_cost_limits::MAX_WRITABLE_ACCOUNT_UNITS.saturating_div(limit_ratio as u64);
// setup two transactions, one has high priority that writes to hot account, the
// other write to non-content account with no priority
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: non-contentious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tao-stones tao-stones force-pushed the refactor-remove-requested-cu-from-cost-tracker branch from eed7de4 to 6be50b6 Compare December 1, 2022 22:13
@tao-stones tao-stones added the automerge Merge this Pull Request automatically once CI passes label Dec 1, 2022
@mergify mergify bot dismissed apfitzge’s stale review December 1, 2022 22:14

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Dec 1, 2022

automerge label removed due to a CI failure

@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Dec 1, 2022
@tao-stones tao-stones added the automerge Merge this Pull Request automatically once CI passes label Dec 1, 2022
@mergify mergify bot merged commit 5850af5 into solana-labs:master Dec 2, 2022
@tao-stones tao-stones deleted the refactor-remove-requested-cu-from-cost-tracker branch December 2, 2022 00:35
gnapoli23 pushed a commit to gnapoli23/solana that referenced this pull request Dec 16, 2022
* refactor cost tracker by removing requested_cu from it, call sites to use cost_model forr consistency

* review fix
nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Jan 4, 2023
* refactor cost tracker by removing requested_cu from it, call sites to use cost_model forr consistency

* review fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants