Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Comments

re-calibrate limit based on mainnet data#21995

Merged
tao-stones merged 2 commits intosolana-labs:masterfrom
tao-stones:recalibrate-cost-limit
Dec 31, 2021
Merged

re-calibrate limit based on mainnet data#21995
tao-stones merged 2 commits intosolana-labs:masterfrom
tao-stones:recalibrate-cost-limit

Conversation

@tao-stones
Copy link
Contributor

Problem

Adjust cost limit to better reflect mainnet data collected via dashboard

Summary of Changes

numbers collected

Fixes #
#21917

@tao-stones
Copy link
Contributor Author

The adjustment is based on mainnet data collected since 1.8.10. The best way to check out if it strikes a better balance between block's fullness and parallelism is perhaps to test it in testnet, with transaction-dos test proposed at https://solanalabs.slack.com/archives/C02LG8Y1KNV/p1639782027077200

@jstarry
Copy link
Contributor

jstarry commented Dec 20, 2021

@taozhu-chicago the metrics aren't loading for me, do you have another link or screenshot? It'd also be nice to have a comment explaining where each of these constants came from

pub const MAX_BLOCK_REPLAY_TIME_US: u64 = 400_000;
/// number of concurrent processes,
pub const MAX_CONCURRENCY: u64 = 10;
pub const MAX_CONCURRENCY: u64 = 4;
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean? Max Block = 4* max compute per account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally, banking threads run concurrently without being blocked by each other (on accounts). MAX_CONCURRENCY represents number of threads under such condition. Therefore the block_max = account_max * MAX_CONCURRENCY

(secp256k1_program::id(), COMPUTE_UNIT_TO_US_RATIO * 4),
(system_program::id(), COMPUTE_UNIT_TO_US_RATIO * 10),
(solana_vote_program::id(), COMPUTE_UNIT_TO_US_RATIO * 70),
(secp256k1_program::id(), COMPUTE_UNIT_TO_US_RATIO * 2),
Copy link
Contributor

Choose a reason for hiding this comment

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

what timing is this based on? Because the actual work of the secp256k1 program is not in the instruction execution part, it's executed in banking stage before it hands it to the message processor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 2us is collected from per-program-timing metrics for secp256K1 (the dashboard linked in the issue #21917). So its actual number should come from banking stage?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, the message processor part of it is like a no-op. We can cost it similar to a regular signature verify I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, changed it

@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #21995 (bf24909) into master (29edb13) will decrease coverage by 0.0%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #21995     +/-   ##
=========================================
- Coverage    81.1%    81.1%   -0.1%     
=========================================
  Files         523      523             
  Lines      146370   146370             
=========================================
- Hits       118747   118735     -12     
- Misses      27623    27635     +12     

@tao-stones tao-stones merged commit a2a7e91 into solana-labs:master Dec 31, 2021
@tao-stones tao-stones deleted the recalibrate-cost-limit branch December 31, 2021 23:45
mergify bot added a commit that referenced this pull request Jan 3, 2022
* re-calibrate limit based on mainnet data, see issue #21917

(cherry picked from commit d743c29)

# Conflicts:
#	runtime/src/block_cost_limits.rs

* set secp256k1 cost similar to sigverify

(cherry picked from commit a2a7e91)

* removes backport merge conflicts

Co-authored-by: Tao Zhu <tao@solana.com>
mergify bot added a commit that referenced this pull request Jan 3, 2022
* re-calibrate limit based on mainnet data, see issue #21917

(cherry picked from commit d743c29)

# Conflicts:
#	runtime/src/block_cost_limits.rs

* set secp256k1 cost similar to sigverify

(cherry picked from commit a2a7e91)

* removes backport merge conflicts

Co-authored-by: Tao Zhu <tao@solana.com>
@brooksprumo brooksprumo mentioned this pull request Jan 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants