-
Notifications
You must be signed in to change notification settings - Fork 27
feat: add signature aggregation using python bindings #238
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
base: main
Are you sure you want to change the base?
Conversation
db09205 to
d5c1566
Compare
|
In my bindings of I've commented out testcases related to invalid signatures as current the aggregation & verification is in test mode. Once we have working |
packages/testing/src/consensus_testing/test_fixtures/fork_choice.py
Outdated
Show resolved
Hide resolved
5caa25a to
a8af346
Compare
6661a38 to
a763fc4
Compare
| ) | ||
|
|
||
| attestation_root = aggregated_attestation.data.data_root_bytes() | ||
| attestation_data_root = aggregated_attestation.data.data_root_bytes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite minor and I should've raised in the previous PR. But just want to flag this for quick opinion @tcoratger spec-writing wise...
Since data_root_bytes() is doing just bytes(hash_tree_root(self)), should we do this instead and remove data_root_bytes()?
| attestation_data_root = aggregated_attestation.data.data_root_bytes() | |
| attestation_data_root = hash_tree_root(aggregated_attestation.data) |
also the bytes(...) can be handled inside verify_aggregated_payload() so we don't lose type information early
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to both; I don't have a preference. It seems to me that this pattern is used a lot in the code, so perhaps having the utility is simpler and less verbose.
Of course, if there are only one or two uses across the entire codebase, that is useless from a spec point of view.
|
I wonder for the process of If I understand these parts correctly (and I have low confidence I do 😅 ), I think these are all to do with re-using/merging/splitting/deduplicating aggregated attestations that are the complex ones:
But can we say that:
If this all makes sense then maybe we can drop all the aggregation merging and splitting from the specs and only need 2 features around aggregation?
|
| else: | ||
| signature = Signature( | ||
| path=HashTreeOpening(siblings=HashDigestList(data=[])), | ||
| rho=Randomness(data=[Fp(0) for _ in range(PROD_CONFIG.RAND_LEN_FE)]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we should have any PROD_CONFIG in the codebase, we should use the env variable right?
| For each aggregated attestation, collect the participating validators' public keys and | ||
| signatures, then produce a single leanVM aggregated signature proof blob using | ||
| `xmss_aggregate_signatures` (via `aggregate_signatures`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These implementation details should change in the future (especially when we will not rely on the bindings anylire) so no need to add them here in the doc.
| For each aggregated attestation, collect the participating validators' public keys and | |
| signatures, then produce a single leanVM aggregated signature proof blob using | |
| `xmss_aggregate_signatures` (via `aggregate_signatures`). | |
| For each aggregated attestation, collect the participating validators' public keys and | |
| signatures, then produce a single leanVM aggregated signature proof. |
| ) | ||
|
|
||
| attestation_root = aggregated_attestation.data.data_root_bytes() | ||
| attestation_data_root = aggregated_attestation.data.data_root_bytes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to both; I don't have a preference. It seems to me that this pattern is used a lot in the code, so perhaps having the utility is simpler and less verbose.
Of course, if there are only one or two uses across the entire codebase, that is useless from a spec point of view.
| return True | ||
| seen.add(attestation.data) | ||
| return False | ||
| def each_duplicate_attestation_has_unique_participant(self) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here maybe we could have a less verbose name such as validate_unique_contributions ? Or maybe another idea?
| groups: dict[bytes, list[AggregationBits]] = defaultdict(list) | ||
|
|
||
| for att in self: | ||
| groups[att.data.data_root_bytes()].append(att.aggregation_bits) | ||
|
|
||
| for bits_list in groups.values(): | ||
| if len(bits_list) <= 1: | ||
| continue | ||
|
|
||
| counts: Counter[int] = Counter() | ||
|
|
||
| # Pass 1: count participants across the group | ||
| for bits in bits_list: | ||
| for i, bit in enumerate(bits.data): | ||
| if bit: | ||
| counts[i] += 1 | ||
|
|
||
| # Pass 2: each attestation must have a participant that appears exactly once | ||
| for bits in bits_list: | ||
| unique = False | ||
| for i, bit in enumerate(bits.data): | ||
| if bit and counts[i] == 1: | ||
| unique = True | ||
| break | ||
| if not unique: | ||
| return False | ||
|
|
||
| return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this should be more pythonic and compact no?
groups = defaultdict(list)
for att in self:
groups[att.data.data_root_bytes()].append(att.aggregation_bits)
for bits_list in groups.values():
if len(bits_list) <= 1:
continue
# 1. Convert bitfields to sets of active indices
sets = [{i for i, bit in enumerate(bits.data) if bit} for bits in bits_list]
# 2. Count index occurrences across the entire group
counts = Counter(idx for s in sets for idx in s)
# 3. Verify EVERY attestation has ANY index that appears EXACTLY once
if not all(any(counts[i] == 1 for i in s) for s in sets):
return False
return TrueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes looks simple to read
| def make_attestation(validator_index: int, data: AttestationData) -> Attestation: | ||
| """Create an attestation for the provided validator.""" | ||
| return Attestation(validator_id=Uint64(validator_index), data=data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same I think that is useless
| # ============================================================================ | ||
| # Additional edge case tests for _aggregate_signatures_from_gossip | ||
| # ============================================================================ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this we can directly put the other tests without this comment.
| # ============================================================================ | |
| # Additional edge case tests for _aggregate_signatures_from_gossip | |
| # ============================================================================ |
| # ============================================================================ | ||
| # Additional edge case tests for _aggregate_signatures_from_block_payload | ||
| # ============================================================================ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
| # ============================================================================ | |
| # Additional edge case tests for _aggregate_signatures_from_block_payload | |
| # ============================================================================ |
| # ============================================================================ | ||
| # Additional edge case tests for split_aggregated_attestations | ||
| # ============================================================================ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # ============================================================================ | |
| # Additional edge case tests for split_aggregated_attestations | |
| # ============================================================================ |
| # ============================================================================ | ||
| # Additional edge case tests for compute_aggregated_signatures | ||
| # ============================================================================ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # ============================================================================ | |
| # Additional edge case tests for compute_aggregated_signatures | |
| # ============================================================================ |
| aggregated_payloads and aggregated_payloads.get(attestation_key) | ||
| ) | ||
|
|
||
| if has_gossip_sig or has_block_payload: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this condition should always be true, we just need to track if we have gossip sig for it or not
| aggregated_payloads: dict[AttestationSignatureKey, AggregatedSignaturePayloads] | ||
| | None = None, | ||
| ) -> tuple[list[AggregatedAttestation], list[LeanAggregatedSignature]]: | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this function is better written as to give an aggregate signature of whatever validator indices it can find it gossip for that data root and also returning pending validator ids which can be used in the next segments.
since the validators are voting every slot, most of the signatures will be covered in this, and only small remaining validator ids might need to be covered from already aggregated signatures.
this is the aggregation flow we need :
completely_aggregated_attestations = AggregatedAttestation.aggregate_by_data(attestations)- for completely_aggregated_attestation in completely_aggregated_attestations:
i) gossip_aggregated_signature, gossip_aggregated_signature_bit_list, remaining_validator_ids = self._aggregate_signatures_from_gossip
most of the times remaining_validator_ids should be almost empty or null and clients can choose to not further find aggregated attestations for theremaining_validator_idssince in the next round of voting they will get the gossip signatures if those validators are on the same fork of the chain.
ii) If client choose to go further, then from the aggregated signatures, one by one pick where aggregated signature with most remaining validators ids, will no remaining validator ids set is zero
i.e.aggregates_signatures_list = [ ] aggregated_signature_bitlist_list = [] # add previously aggregated signature from gossip into this list we are maintaining aggregates_signatures_list.append(gossip_aggregated_signature) aggregated_signature_bitlist_list.append(gossip_aggregated_signature_bit_list) # pick and collect other aggregated signatures to cover all the validator ids while(remaining_validator_ids != []): # we should be able to find a previously aggregated signature because a vote is only imported if we have recieved from gossip or from a previously aggregated signature aggregated_signature, aggregated_signature_bitlist,remaining_validators_ids = pick_from_aggregated_signatures(remaining_validator_ids) aggregates_signatures_list.append(aggregated_signature) aggregated_signature_bitlist_list.append(aggregated_signature_bitlist) # now we can aggregate the aggregated signature list recurively into just one aggregated signature # but since for now we can't so we will just create multiple aggregated attestations with the same data, but using the # signature and bit list from here
```
🗒️ Description
Supports aggregation as per our discussion for devnet2.
The current implementation of aggregation bindings don't actually aggregate the signatures and instead return a single 0 byte, same for the verification API. This will be updated when we have aggregation support for current signature encoding scheme on TEST_CONFIG.
✅ Checklist
toxchecks to avoid unnecessary CI fails:uvx tox