Optimized sync committee signature verification

[AbolareRoheemah]

Enhanced sync committee signature verification to leverage precomputed aggregate pubkey for improved efficiency in high-participation scenarios, particularly for light clients, while ensuring compatibility with existing BLS operations and maintaining spec clarity with added explanatory notes.

Fixes #4195

• Provide optimized method that uses SyncCommittee.aggregate_pubkey #4195

[jtraglia]

Nice work, I think this looks great! I have confirmed that there is an existing test case (test_sync_committee_rewards_not_full_participants) which tests the middle part too. I would appreciate reviews from @mkalinin, @etan-status, @b-wagn, and @arnetheduck.

[etan-status]

I find it a bit weird to have this arbitrary optimization as part of the spec function, as it reduces readability.

Other parts of the spec generally focus on concepts. e.g., the fork choice specs are far from practical implementations where caching every possible intermediate state is a non-option. It is typically up to the client teams to figure out how to optimize the logic to be viable.

The optimization itself looks correct though, and is what Nimbus also uses.

• https://github.com/status-im/nim-blscurve/blob/f4d0de2eece20380541fbf73d4b8bf57dc214b3b/blscurve/bls_sig_min_pubkey.nim#L260
• https://github.com/status-im/nim-blscurve/blob/f4d0de2eece20380541fbf73d4b8bf57dc214b3b/blscurve/blst/blst_min_pubkey_sig_core.nim#L197

[b-wagn]

LGTM, modulo the two minor comments below.

[b-wagn]

I find it a bit weird to have this arbitrary optimization as part of the spec function, as it reduces readability.

Other parts of the spec generally focus on concepts. e.g., the fork choice specs are far from practical implementations where caching every possible intermediate state is a non-option. It is typically up to the client teams to figure out how to optimize the logic to be viable.

I totally agree that in general the specs should describe concepts, and implementations should do the optimizations.

However, I think there are two arguments for having this in the spec:

1. if we don't have it, then the aggregate pub key is never used, which is really odd. This is the reason why all of this came up (recall this PR)
2. I think that there is a point in having cryptographically relevant optimizations in the spec, and this is one, as it deals with how the key for verification is computed. If this is done incorrectly, we introduce vulnerabilities, so having a spec that explains how to do it is valuable. This could of course be marked as an alternative instead of just replacing the old non-optimized method. A similar thing has been done in the recent KZG specs for Fulu (see this function). We could've just put the naive verification for KZG in the spec, but we put the optimized batch verification there, as it is cryptographically relevant, and if implementations mess it up, this leads to big issues.

There is also another argument against it: the second optimization that you described here would also not be in spec now. So we would have one optimization but not the other.

[etan-status]

For (2), a new, reusable helper function that solves this generically would at least allow the sync aggregate spec function to be clean of these random details. Also a bit more future proof if we have additional situations where the full aggregate pub key exists.

[b-wagn]

reusable helper function

yeah, having a function that separates this would be useful. But I wouldn't insist on it.

[jtraglia]

reusable helper function

yeah, having a function that separates this would be useful. But I wouldn't insist on it.

I agree. We could pull this chunk out & put it in a helper function, but it's not necessary. I would lean towards going with what we have now & splitting it into multiple functions if/when it becomes necessary.

[ralexstokes]

i'd echo others about not including the optimization in the spec like this

could instead have a note at the bottom here with some (psuedo)code to demonstrate

[ralexstokes]

we could also move this function to the pytest files, although that would be harder to find

[ralexstokes]

would also be good to have explicit tests for each of the variants here (1 key, more than half, less than half), although we may have them already

[jtraglia]

i'd echo others about not including the optimization in the spec like this

could instead have a note at the bottom here with some (psuedo)code to demonstrate

We've already tried this & came to the conclusion that codifying it would be better.

• Add note about aggregate key in SyncCommittee #4185

FWIW, I agree that code is better than a note. It's not that much more complex either.

[jtraglia]

would also be good to have explicit tests for each of the variants here (1 key, more than half, less than half), although we may have them already

They aren't explicit, but yes there are many existing tests which use each variant. Eg:

• For the first variant: test_sync_committee_committee__full
• For the second variant: test_sync_committee_rewards_not_full_participants
• For the third variant: test_sync_committee_committee__half

[jtraglia]

There are opposing views on whether or not this should be a spec function or a note. I feel that the "spec function" side has a stronger argument. Part of the rationale for this change is so that the aggregate_pubkey field gets used in the specifications; currently it's not used anywhere which could be confusing for implementers. For these reasons, I've decided to merge this PR. In my opinion, this is a special situation and should not open the door to hyper-optimized specifications.