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

Use the latest computed power table instead of always ranging certs #882

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

masih
Copy link
Member

@masih masih commented Feb 7, 2025

Certificate store already keeps track of the latest power table, i.e., the power table of the next expected certificate. Use this pre-computed table whenever the next power table is asked for instead of re-computing it every time.

This results in a much faster response to GetPowerTable for the most common case: getting the committee as part of monotonic progress of GPBFT instances.

Fixes #881

Certificate store already keeps track of the latest power table, i.e.,
the power table of the next expected certificate. Use this pre-computed
table whenever the next power table is asked for instead of re-computing
it every time.

This results in a much faster response to `GetPowerTable` for the most
common case: getting the committee as part of monotonic progress of
GPBFT instances.

Fixes #881
@masih masih requested a review from Stebalien February 7, 2025 11:02
@masih masih self-assigned this Feb 7, 2025
@masih masih added this to the M2: Mainnet Passive Testing milestone Feb 7, 2025
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.24%. Comparing base (ac4d25d) to head (a2b3c66).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #882      +/-   ##
==========================================
+ Coverage   67.11%   67.24%   +0.12%     
==========================================
  Files          85       85              
  Lines        9175     9185      +10     
==========================================
+ Hits         6158     6176      +18     
+ Misses       2468     2464       -4     
+ Partials      549      545       -4     
Files with missing lines Coverage Δ
certstore/certstore.go 65.42% <100.00%> (+1.01%) ⬆️

... and 2 files with indirect coverage changes

@masih
Copy link
Member Author

masih commented Feb 7, 2025

Has been running on butterflynet since ~1200 UTC. Get committee times looking much better:
image

if instance > nextCertInstance {
return nil, fmt.Errorf("cannot return future power table for instance %d > %d", instance, nextCertInstance)
}
if instance == nextCertInstance && len(latestPowerTable) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking len(latestPowerTable), can we check latestCert != nil? I guess that won't work for instance 0... meh, this works fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it would be problematic for firstInstance. An alternative is to explicitly check if the instance is the same as firstInstance. But that makes up a more complicated logic.

@masih masih added this pull request to the merge queue Feb 7, 2025
Merged via the queue into main with commit 423eb5a Feb 7, 2025
14 checks passed
@masih masih deleted the masih/certstore-latest-optim branch February 7, 2025 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Cache the latest known power table in certstore
2 participants