Skip to content

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

Merged
prylabs-bulldozer[bot] merged 5 commits into
developfrom
aggregate_unnaggregated_without_max_cover
May 16, 2023
Merged

Don't use max cover on unaggregated atts nor check subgroup of validated signatures#12350
prylabs-bulldozer[bot] merged 5 commits into
developfrom
aggregate_unnaggregated_without_max_cover

Conversation

@potuz

@potuz potuz commented May 1, 2023

Copy link
Copy Markdown
Contributor

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

Comment thread beacon-chain/operations/attestations/kv/aggregated.go Outdated
@potuz potuz force-pushed the aggregate_unnaggregated_without_max_cover branch from 9e7aaa4 to a932d9e Compare May 16, 2023 15:18
@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 nisdas, rkapka and terencechain May 16, 2023 15:19
Comment thread proto/prysm/v1alpha1/attestation/aggregation/attestations/attestations.go Outdated
@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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants