devnet-4, recursive aggregation#426
Conversation
| if vid in gossip_id_set: | ||
| continue | ||
| key = SignatureKey(vid, data_root) | ||
| if (pls := children_payloads.get(key)) is not None: |
There was a problem hiding this comment.
we need to get the one with the maximum coverage with the remaining validator ids and consume them in the greedy manner
There was a problem hiding this comment.
infact gossips one should be last to be added
There was a problem hiding this comment.
infact you should now not store by vid-dataroot key, just store payloads by dataroot key i.e. datarootke => list of payloads , list of validator ids
because its simpler to just take them in a greedy fashion and then pick the non covered validator from validators and then do recursive aggregattion
| """ | ||
| Aggregate committee signatures for attestations in committee_signatures. | ||
| merged = dict(self.latest_known_aggregated_payloads) | ||
| for sig_key, proofs in self.latest_new_aggregated_payloads.items(): |
There was a problem hiding this comment.
the keys for aggregated payloads should now be just attestattion data right?
unnawut
left a comment
unnawut
left a comment
There was a problem hiding this comment.
A few small comments and just one substantial comment but I guess it's more for Emile I'll ping him for opinion.
I didn't look at dummy aggregate & verify just yet because that'll be replaced before we finalize devnet-4 specs but please correct me if I'm wrong.
There's also code comment clean up and updating tests, but I assume that'll also be sorted when leanMultisig is ready so I ignore them for now.
| for vid in validator_ids: | ||
| if vid in gossip_id_set: | ||
| continue |
There was a problem hiding this comment.
nit: Can be collapsed to this. Also more readable:
| for vid in validator_ids: | |
| if vid in gossip_id_set: | |
| continue | |
| for vid in set(validator_ids) - set(gossip_ids): |
| """SSZ-compliant list of sibling hashes, from bottom to top.""" | ||
|
|
||
|
|
||
| class BytecodePointOption(SSZUnion): |
There was a problem hiding this comment.
I remember vaguely that SSZUnion type is quite problematic(?, source needed but I think it was around determinism) and there's been proposals to remove it from specs (ethereum/consensus-specs#4763, ethereum/consensus-specs#3906). So I'm not sure we should consider this before adding SSZUnion type especially if we're trying to be more lean than current beacon specs.
Seeing this match clause in AggregatedXMSS::public_input() and with a bit of AI consultation it looks like replacing the optional with vec![F::ZERO; bytecode_claim_size] or similar could work? cc: @TomWambsgans
There was a problem hiding this comment.
Conclusion from pqinterop chat is we keep leanMultisig API the way it is (with Option) but work on leanSpec to avoid SSZUnion
3 participants
🗒️ Description
draft PR for devnet-4
updates aggregation API to support recursive aggregation
adds a temporary dummy flag which will be used to later fix the tests once the design is finalized
the bindings cannot be updated due to incompatibility between leanMultisig xmss and leanSig
INV_PROOF_SIZE should be a protocol level parameter imo, unlike a aggregator paramater as defined in high level doc
changes
aggregate_gossip_signaturestoaggregateas it will no longer just aggregates gossip signatures but also recursively aggregates children_payloads with themchanges
aggregate_gossip_signaturestoaggregate_committee_signatures_and_payloads, it includes both committee gossip signatures and payloads (no committee) and recursively aggregate them usingaggregate🔗 Related Issues or PRs
✅ Checklist
toxchecks to avoid unnecessary CI fails:uvx tox