Sync aggregation v2: bitmap acks, partitioned block delivery, coordinated activation - #984
Sync aggregation v2: bitmap acks, partitioned block delivery, coordinated activation#984hackobi wants to merge 3 commits into
Conversation
…ctivation height Encode acknowledgements as a bitmap over the hash-committed peerlist (34.4 KB -> ~183 B at 500 peers), partition block delivery across signing committee members with block-hash-rotated slices, publish per-member partial aggregates, buffer next-block aggregates that race delivery, guard sync-hint monotonicity, and gate the send path behind a coordinated activation height. Receivers admit both wire versions; legacy behaviour is byte-identical below the activation height or with the flag off.
23 new bun tests (bitmap codec canonicality, canonical index, v2 admission, partition determinism/coverage/rotation, v2 traffic model, activation gating, 20/50/500-node partial-aggregate convergence). Emulator gains --aggregate-version=2 with partitioned delivery, per-member partials, v2 safety cases and byte accounting; VPS wrapper forwards AGGREGATE_VERSION.
Document the bitmap format and why the index excludes the non-hash-covered signature map, partitioned dissemination and race buffering, the formal liveness-only relay trust rule with its fail-closed analysis, the activation-height rollout invariant, measured 100/250/500-peer emulator results (18.1 MB -> 0.54 MB per block at 500 peers), eight recorded maintainer findings, and the detached-signature appendix incl. the domain-separation pitfall.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@greptileai review |
|
| Filename | Overview |
|---|---|
| src/libs/communications/syncAggregation.ts | Adds canonical bitmap codecs, fail-closed v2 admission, deterministic delivery partitioning, activation gating, and traffic modeling without an identified defect. |
| src/libs/communications/broadcastManager.ts | Adds mode-aware publication, partitioned delivery, buffering, and monotonic updates, but buffered aggregates are not replayed when blocks arrive through fastSync. |
| src/libs/consensus/v2/PoRBFT.ts | Delegates post-consensus publication to the mode-aware BroadcastManager without changing voting, validation, construction, or finality. |
| src/libs/network/manageGCRRoutines.ts | Makes duplicate-block responses include the standard syncData response shape for aggregation. |
| src/config/loader.ts | Loads and validates aggregation version and activation-height settings with safe defaults. |
| src/libs/communications/syncAggregation.test.ts | Thoroughly covers bitmap canonicality, admission, partitioning, activation, and convergence, but does not cover buffer draining through alternate block-ingestion paths. |
Sequence Diagram
sequenceDiagram
participant P as Committee publisher
participant R as Receiving node
participant B as Pending aggregate buffer
participant F as fastSync
P->>R: Partial aggregate for height N+1
R->>B: Buffer until block arrives
F->>R: Download and insert block N+1
Note over R,B: fastSync bypasses handleNewBlock drain
R->>B: Later prune by height or TTL
Note over R: Buffered acknowledgements are never applied
Prompt To Fix All With AI
### Issue 1
src/libs/communications/broadcastManager.ts:310-312
**Fast-sync bypasses aggregate replay**
When a next-height aggregate is buffered but the block subsequently arrives through `fastSync` or another direct insertion path, `drainPendingSyncAggregates` is never called, causing the valid acknowledgements to remain unused until they are pruned or expire.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "docs(sync-aggregation): v2 design, trust..." | Re-trigger Greptile
| // buffered; replay them now through the same admission path. | ||
| this.drainPendingSyncAggregates(block) | ||
| } |
There was a problem hiding this comment.
Fast-sync bypasses aggregate replay
When a next-height aggregate is buffered but the block subsequently arrives through fastSync or another direct insertion path, drainPendingSyncAggregates is never called, causing the valid acknowledgements to remain unused until they are pruned or expire.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/libs/communications/broadcastManager.ts
Line: 310-312
Comment:
**Fast-sync bypasses aggregate replay**
When a next-height aggregate is buffered but the block subsequently arrives through `fastSync` or another direct insertion path, `drainPendingSyncAggregates` is never called, causing the valid acknowledgements to remain unused until they are pruned or expire.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Stacks on #983 and completes the three follow-ups its review named: aggregate byte size, the relay trust rule, and mixed-version activation. No consensus rule changes — voting, transaction validation, block construction and finality are untouched, and everything stays behind
BLOCK_SYNC_AGGREGATION_ENABLED=false.At 500 peers, the post-block burst goes from 251,984 calls (legacy) / 995 calls + 18.1 MB (v1 POC) to 2,492 calls + 0.54 MB.
Why v1 needed a v2
committee[0]is view-dependent (liveness-filtered draw, mutates on failover), so rounds can end with zero or duplicate publishersHow it works
1. Bitmap wire format (v2)
{ "version": 2, "blockNumber": 12345, "blockHash": "…", "peerlistSize": 500, // cross-checked against the locally derived index "ackBits": "…" // base64; bit i = index entry i acked the block }The bitmap indexes the committed peerlist (normalized, deduped, sorted) because it sits inside the block-hash preimage — every node holding the block derives the identical index, so no identities travel on the wire. It deliberately does not index
validation_data.signatures: that map merges incrementally per node and is not hash-covered, so a bitmap over it would decode differently on different nodes.Decoding fails closed: wrong byte length, non-canonical base64,
peerlistSizemismatch, or any bit set beyond the index ⇒ 400. Receivers admit both wire versions indefinitely, so mixed v1/v2 fleets converge.2. Partitioned block delivery
Peer
pbelongs to sliceH(p, blockHash) mod S, owned by that position of the sorted signing committee. Properties:handleNewBlock) or missed ones (repaired by fastSync, now ~1/S of peers instead of all).N − S; per-sender block bytes divide byS.3. Partial aggregates + race buffering
Each member builds one partial bitmap from its own slice's responses, applies it locally, and broadcasts it. Receivers union partials through the same fail-closed admission — idempotent and order-independent, so there is no merge protocol, collection window, or new message type.
Because a member with a fast slice publishes while slower slices are still delivering, a partial routinely reaches a peer moments before its own block does. Receivers buffer aggregates addressed to
lastBlockNumber + 1(64 entries, 60 s TTL) and replay them through admission once the block lands, instead of dropping them.4. Correctness fixes picked up along the way
applySyncAggregatecould regress a peer'ssynchint to an older block (the legacy path gets this guard fromPeerManager.addPeer; the aggregate path bypassed it). Those hints feed sync-source selection, the forge-quorum pre-check and the network-ahead veto.syncNewBlockduplicate-block short-circuit returned a bare string withoutsyncData, so already-synced peers could never be counted in an aggregate. It now returns the standard response shape.Measured results
Same emulator, knobs and pass criteria as the #983 VPS run (five bursts per size, 20–100 ms jitter, 5% slow peers, 5% transient failures, bounded retries). Observed calls matched
estimatePostBlockTraffic(n, s, true, 2)exactly at every size; every partial admitted everywhere; every non-signer delivered exactly once; every receiver's accepted union exactlypeerlist − self.Call model for a committee of
SinNnodes:v2 spends ~2.5× v1's requests to buy: aggregate bytes down two orders of magnitude, per-sender block load ÷ S, and no single required publisher. A v1-mode regression run confirms unchanged v1 numbers, and all safety cases (v1's three plus four v2-specific rejections) fail closed.
Trust rule (review gate 1 of #983)
Sound because admission is fail-closed on every axis that could touch consensus: an aggregate is only accepted for a block the receiver itself already verified and stored; it can only reference identities committed in that block's hash-covered peerlist (or its signers) that are already known locally; it never marks a peer online, never regresses a hint, never adds a peer. The residual power of a malicious signer is a stale hint — bounded to liveness (a retried sync-source pick, or a forge round that starts and then fails to gather real signatures). The upgrade path to per-ack detached signatures is designed in the doc's Appendix A, including the hard requirement for domain separation (block signatures sign the bare block hash — an un-separated ack signature would be a block signature).
Rollout (review gate 2 of #983)
BLOCK_SYNC_AGGREGATION_ENABLEDfalseBLOCK_SYNC_AGGREGATION_VERSION21= secretary/JSON POC,2= this PR; invalid values warn and fall backBLOCK_SYNC_AGGREGATION_ACTIVATION_HEIGHT0Invariant: any change to
ENABLEDorVERSIONon a live fleet ships with a fresh future activation height, identical on every node, so the fleet flips in the same round. Receivers admit both versions regardless of height; laggards degrade to anti-entropy, never diverge. Rollback = unset the flag; there is no persistent state — aggregates only touch in-memory sync hints.Reviewing this PR
Three commits, reviewable independently:
feat(sync-aggregation)— all production changes:syncAggregation.ts(pure functions: codec, index, admission, partition, model),broadcastManager.ts(modes, partitioned publish, buffer), thin hooks inPoRBFT.ts/manageGCRRoutines.ts, config plumbing.test(sync-aggregation)— 23 new bun tests (codec canonicality, admission rejections, partition determinism/coverage/rotation, 20/50/500-node convergence) and the emulator's--aggregate-version=2mode.docs(sync-aggregation)—docs/poc/block-sync-aggregation.mdcarries the full design, the trust-rule analysis, the activation protocol, and eight recorded findings for maintainers.Verification: 38/38 tests green; no new type or lint errors versus the base branch; emulator green in both modes.
Still required before enabling anywhere public
updateSyncAggregate.