Aggregated block proof - devnet5#717
Conversation
anshalshukla
force-pushed
the
aggregated-block-proof
branch
from
2069744 to
c7c93d5
Compare
anshalshukla
force-pushed
the
aggregated-block-proof
branch
from
2c4babc to
4d0630b
Compare
g11tech
left a comment
g11tech
left a comment
There was a problem hiding this comment.
this is my major review of this PR:
https://github.com/leanEthereum/leanSpec/pull/717/changes#r3257938656
I think split and restitching belongs with proposer and not an aggregator, or atleast needs to be backed up by proposer as well incase it didn't see aggregator's restitched payload which could be a better solution I guess
so I recommend such split and restitching in block proposal build block as well
|
The only problem in restitching in block proposal as well is that it will further slow the block building process which will eventually lead to missed slots. Or we will have to bump up the spec for validators. |
we still need it as backup otherwise block proposal will fail if proposer can't repack attestations from a branch which moved justification/finalization so we can still have / expect aggregators to do this job and do heavy lifting but in case of anything that is missing, we still need proposer to be able to do this. expecting proposers to have a bit more compute itsn't too unrealistic imo, by 2028/2029 we might have more compute as well as optimizations available on a commodity hardware |
|
Okay, I've updated it to do deconstruction in onBlock even in case of validators and not just in case of aggregators. I think it is better to continue to do it at the same place and update the store than to look for blocks which can help justify in the build_block function |
There was a problem hiding this comment.
Hey @anshalshukla, thanks a lot for this. I've just went through a first careful review through a Claude discussion. I've noticed a couple of points that I think are valid. Please let me know what you think about them. I've let Claude pushed the comments since he is very clear and precise in his explanations.
the 8f9bdc1 move out of the aggregator-only branch is good, but there's a small bug in the new gate (see the inline at sync/service.py:743). Also that helper currently only fires on the gossip path — blocks that arrive via sync/backfill skip it — so a node catching up still doesn't end up with per-attestation Type-1s.
The verify_signatures binding — the proof binds (message_i, slot_i, pubkeys_i) per component internally, but the spec never compares those internal (message_i, slot_i) against hash_tree_root(att_i.data) / att_i.data.slot from the block body. The snark verifies that the supplied pubkeys signed the proof's internal messages — not that those messages match the body. Details inline at spec.py:913. Worth a sync with the lean-multisig folks on whether they'd rather take a verify_type_2_with_messages(...) upstream, or expose info after decompress_without_pubkeys so the spec can compare in Python.
The latest_known_aggregated_payloads seeding — on_block still inserts empty sets per attestation (spec.py:1296-1299) and the comment block above still describes the pre-PR behavior. The local write-back you added in _maybe_publish_reaggregated_attestations_from_block lands in latest_new_aggregated_payloads, not latest_known, so on import the block-imported votes still don't feed fork-choice weight on the receiver. Inline at spec.py:1292.
Test coverage — test_reaggregate_from_block.py's own header acknowledges the happy path can't be exercised because split_type_2_by_msg isn't in test-mode bindings yet. The three test bodies all assert published == [], so the ~50 lines that actually do the split/merge are uncovered. Might be worth a line in the PR description so it doesn't slip through as "tested".
CI — coverage-gate, fill-tests, interop-tests switched to macos-latest and Justfile dropped -n auto from fill-ci. GH Actions macos runners are roughly 10× the ubuntu cost — if the switch was working around a flake or OOM, would be great to capture the why in the PR description (xdist + setup_prover's global side-effects in lean_multisig_py is the likely suspect).
The rest are smaller — comments/docstrings that drifted and a couple of typos. Left inline so they're easy to scroll through.
|
@tcoratger you can take a look again |
Resolve conflicts in src/lean_spec/subspecs/sync/service.py against main's recent readability sweep: - Drop set_publish_agg_fn; reuse main's publish_aggregated_attestation as a direct dataclass field on SyncService. - Drop the standalone _check_sync_trigger; main inlined the SYNCING trigger into on_peer_status. - Drop ZERO_HASH and _ancestor_set imports; main inlined the ancestor walk in _default_process_block. - Keep _pending_block_aggregates, _deconstruct_block_into_store, and _publish_pending_block_aggregates from this branch; restore the publish-drain call after _replay_pending_attestations in on_gossip_block. While reconciling the deconstruction logic, take the opportunity to: - Remove the validator_id is None early-return so non-validator nodes also accrue block-imported attestation weight after the next acceptance tick. - Gate _pending_block_aggregates.extend on self.is_aggregator so non-aggregators no longer republish to gossip. - Hoist the per-message pubkey layout out of the per-attestation loop. - Narrow the post-decode except clause to (ValueError, IndexError). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Fix the broken sentence in LstarSpec.verify_signatures' docstring (dangling colon now reads as a coherent description of what the Type-2 proof binds). - Delete test_noop_when_not_a_validator: the validator-id early-return it exercised was removed so non-validator nodes also accrue block-imported attestation weight. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes flagged in the second round of consensus and Python architecture review: - sync/service.py: widen the decode_bytes catch to (SSZError, ValueError, IndexError). Pre-fix narrowing missed the SSZ exception family raised by Container deserialization, so a malformed proof would crash _process_block_wrapper instead of being demoted to a debug log. - sync/service.py: align the per-attestation loop variable name with the second loop. - validator/service.py: validate each participant index against the active validator set before indexing validators[vid] in _sign_block, raising a clear ValueError instead of a deep KeyError if a stale partial aggregate is passed in. - spec.py: document the one-slot deferral of fork-choice weight from block-imported attestations. The empty-set seeding plus the subsequent acceptance-tick migration is intentional and not a missed update_head call. - test_aggregation.py: add five Type-2 unit tests covering the aggregate-rejects paths, the verify round-trip, the message-swap rejection (pins the per-component binding security property), and the length-mismatch rejection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 participants
🗒️ Description
Implements multi-message recursive block signature aggregation. All the attestation and block signatures are coalesced into a single block proof.
The blocks are deconstructed for additional attestation_data when they can help move the target. These new payloads are only aggregated when the chain is in sync as catching up nodes should not spam the network.
coverage-gate, fill-tests, and interop-tests all switched ubuntu → macos in this PR, and Justfile dropped -n auto from fill-ci. This might be due to inapt compilation target of linux bindings via leanMultisig-py which lead to 4x CI runs in linux based systems. Will be looked upon in followup PR.
✅ Checklist
just check