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

Don't use max cover on unaggregated atts nor check subgroup of validated signatures #12350

Merged
merged 5 commits into from
May 16, 2023

Conversation

potuz
Copy link
Contributor

@potuz potuz commented May 1, 2023

This PR does two things

  • It removes the check for subgroup inclusion when getting a signature from bytes, since the signature were already verified.
  • It aggregates attestations directly when they are a single bit attestation, there is no need to run the max coverage algo

These numbers were run on a NUC i5-8259U

On 293 aggregations, the average on mainnet were

  • at 7 seconds: 1905 ms. (standard deviation 310ms or 17%)
  • at 9 seconds: 142 ms. (standard deviation 119ms)
  • at 11 seconds: 17ms. (standard deviation 16ms)

variation happened up to 3500ms on the first batch regularly (hence the high standard deviation).

At 9 seconds it got regularly > 1000 ms and both second and third batches had standard deviation of 100%.

mean(field-1)	median(field-1)	perc:90(field-1)	perc:95(field-1)	perc:99(field-1)
1896.361774744	1892	2087.2	2292.8	3320

With the PR applied we obtain

  • 7 seconds: 789ms (std deviation 206ms)
  • 9 seconds: 86ms (std deviation 74 ms)
  • 11 seconds: 9ms (std deviation 7ms)

Variation happened up to 2256ms in a couple of places.

mean(field-1)	median(field-1)	perc:90(field-1)	perc:95(field-1)	perc:99(field-1)
794.21164021164	765	854	1055.5	2130.58

@potuz potuz force-pushed the aggregate_unnaggregated_without_max_cover branch from 9e7aaa4 to a932d9e Compare May 16, 2023 15:18
@potuz potuz added Ready For Review A pull request ready for code review Priority: High High priority item labels May 16, 2023
@potuz potuz marked this pull request as ready for review May 16, 2023 15:19
@potuz potuz requested a review from a team as a code owner May 16, 2023 15:19
@potuz potuz requested review from terencechain, rkapka and nisdas May 16, 2023 15:19
@potuz potuz changed the title Don't use max cover on unnaggregated atts Don't use max cover on unaggregated atts nor check subgroup of validated signatures May 16, 2023
@potuz potuz force-pushed the aggregate_unnaggregated_without_max_cover branch from a932d9e to b07032f Compare May 16, 2023 15:30
terencechain
terencechain previously approved these changes May 16, 2023
@potuz potuz force-pushed the aggregate_unnaggregated_without_max_cover branch from a36d487 to a64a6f9 Compare May 16, 2023 16:26
terencechain
terencechain previously approved these changes May 16, 2023
terencechain
terencechain previously approved these changes May 16, 2023
rauljordan
rauljordan previously approved these changes May 16, 2023
prestonvanloon
prestonvanloon previously approved these changes May 16, 2023
@potuz potuz dismissed stale reviews from prestonvanloon, terencechain, and rauljordan via d68a894 May 16, 2023 16:55
@prylabs-bulldozer prylabs-bulldozer bot merged commit be23773 into develop May 16, 2023
@prylabs-bulldozer prylabs-bulldozer bot deleted the aggregate_unnaggregated_without_max_cover branch May 16, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High High priority item Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants