feat: sigagg

[mskrzypkows]

No description provided.

[mskrzypkows]

@claude review

[claude]

Claude finished @mskrzypkows's task in 3m 58s —— View job

---

Claude PR Review — Sigagg

Reviewed the port of Charon's sigagg package against charon/core/sigagg/sigagg.go. Overall this is a clean and faithful port — structure, error cases, and dedup logic match Go closely. A few correctness deviations and several style/test gaps worth addressing before merging.

Todo

• Gather PR context (read changed files)
• Cross-check against Charon Go source (sigagg package)
• Review functional correctness vs Go
• Review Rust style / idioms
• Review tests for adequacy & edge cases
• Post review feedback

---

Functional correctness

1. Silent u64 → u8 truncation on share_idx — risky (crates/core/src/sigagg.rs:147-151)

#[allow(clippy::cast_possible_truncation)]
let bls_map: HashMap<u8, [u8; 96]> = bls_sigs
    .iter()
    .map(|(idx, sig)| (*idx as u8, *sig.as_ref()))
    .collect();

Dedup at line 133 happens in a HashMap<u64, _>, then the cast at line 150 silently truncates. If a malicious/malformed peer sends share_idx = 256, it collides with 0 after the dedup, and you may silently drop a legitimate share or accept an invalid one. The crypto crate (tbls.rs:85) documents that share IDs must be < 255, but the boundary should be enforced explicitly here rather than truncated. Suggest:

let idx_u8 = u8::try_from(*idx).map_err(|_| SigAggError::InvalidShareIndex)?;

Fix this →

2. Lost error context — pubkey not wrapped on per-validator failure (crates/core/src/sigagg.rs:110)
Go wraps each failure with z.Any("pubkey", pubkey):

return errors.Wrap(err, "threshold aggregate", z.Any("pubkey", pubkey))

Rust just propagates the inner error. With multi-validator batches, the caller can't tell which validator failed. Consider wrapping with the pubkey, or at least logging it on the error path.

3. Template-selection loop diverges from Go in mixed-type edge case (crates/core/src/sigagg.rs:162-172)
Go breaks out of the loop the first time parSig.SignedData is not a VersionedAttestation:

for _, parSig := range parSigs {
    attVidx, ok := parSig.SignedData.(core.VersionedAttestation)
    if !ok { break }
    if attVidx.ValidatorIndex != nil { fullSig = attVidx; break }
}

Rust uses find_map, which scans all parSigs and skips non-attestations. In practice all parSigs for a duty share one type, so this won't bite — but it's a deliberate behavioral deviation from the source. Worth either matching exactly or noting in a comment why the divergence is safe.

4. Subscriber semantics: no clone, immutable borrow (crates/core/src/sigagg.rs:116-118)
Go clones output before each subscriber call so each subscriber gets a mutable, owned copy. Rust passes &AggSignedDataSet. This is more efficient and arguably safer, but it's a real semantic change: Charon subscribers that need to retain or mutate the data must clone themselves. If parity with downstream subscriber expectations matters, document this. If not, this is fine.

5. new_verifier() is a no-op stub (crates/core/src/sigagg.rs:190-192)

pub fn new_verifier() -> VerifyFn {
    Arc::new(|_, _| Box::pin(async { Ok(()) }))
}

Acknowledged in the TODO above, but this is security-critical — without verification, an aggregator will happily emit a "valid" aggregated signature for any input. Should not be wired into the production path until Eth2SignedData and beacon-client verification are ported. A #[deprecated(note = "...")] or #[doc(hidden)] flag, or renaming to noop_verifier(), would reduce the chance of accidental use.

6. Missing tracing spans
Go uses an OTel span around tbls.ThresholdAggregate. Rust just has debug!("Threshold aggregated partial signatures"). Not a correctness bug, but worth filing as a follow-up so observability parity isn't silently dropped.

---

Rust style / idioms

7. AggSub future bound + Send + Sync is unusual (crates/core/src/sigagg.rs:52-57)

Pin<Box<dyn Future<Output = Result<()>> + Send + Sync>>

Futures rarely need Sync — the closure needs to be Sync to share across tasks, but the produced future is normally just Send. Adding Sync excludes a lot of valid futures (e.g., anything holding a RefCell internally) for no observable benefit, since the future isn't shared across threads in this code. Recommend dropping + Sync from the future bound.

8. aggregate(set: HashMap<...>) consumed but only borrowed (crates/core/src/sigagg.rs:98-112)
The function takes set by value but never moves out of it (for (pubkey, par_sigs) in &set). Either:

• accept &HashMap<...> (matches the iteration pattern), or
• consume and move out (for (pubkey, par_sigs) in set) so callers don't pay an unnecessary clone if they want to reuse the input.

9. Type alias Result<T> leaks into public types (crates/core/src/sigagg.rs:46, 52-65)
pub type AggSub = ... Future<Output = Result<()>> ... exposes the crate-local Result alias. Callers reading docs see Result<()> without the error type. Use std::result::Result<(), SigAggError> in the public type alias, or re-export with the full path.

10. Tbls brought in for a trait method (crates/core/src/sigagg.rs:5)
use pluto_crypto::{blst_impl::BlstImpl, tbls::Tbls}; — the Tbls import is needed so .threshold_aggregate(...) resolves. Worth use pluto_crypto::tbls::Tbls as _; to make the intent clear.

11. Doc comment style — top-of-file /// doc on line 1 attaches to the next item (use std::...), not the module. Convert to //!:

//! SigAgg aggregates threshold partial BLS signatures...

---

Tests

Good baseline coverage (12 tests), but a few important paths are uncovered.

12. No test for verify_fn returning an error. The whole point of the verify callback is to fail-closed if the aggregated sig doesn't verify; needs a test that proves the error propagates out of aggregate_one.

13. No test for subscriber returning an error. Currently sub(...).await? propagates, but no test guards against a future regression.

14. No test for SignatureFromCore / SetSignature error variants. Both error variants exist but aren't exercised. A failing MockSignedData (returning Err from signature() / set_signature_boxed) would cover them.

15. No test for the VersionedAttestation validator_index template preference. The fix-for-validator-index logic at lines 162-172 is non-obvious; add a test with two parSigs where one carries validator_index and one doesn't, asserting the right template is picked.

16. aggregate_one is called with a HashMap iteration order — non-deterministic. Multi-validator tests should not assume any particular pubkey order in the subscriber's view (the existing multiple_validators test handles this correctly by indexing).

---

Nitpicks

• SigAggError::SignatureFromCore / SetSignature wrap SignedDataError separately — fine, but consider adding #[source] or using #[from] on at least one if conversions are unambiguous.
• mock_par_sigs in tests creates count parsigs all with the same share_idx — name could be mock_par_sigs_same_share_idx to be self-documenting.
• Aggregator::aggregate doc doesn't mention that an error in any validator aborts the whole batch (matches Go, but worth stating).
• Empty par_sigs.is_empty() ⇒ RequireThresholdSignatures is correct (0 < threshold), but the error message reads slightly misleading when the actual cause is "no signatures at all." Not blocking.
• Per the project's AGENTS.md "Golden Rules": comparing against the Go source is mandatory — looks like the author did this carefully; only the deviations noted above are visible.

---

Verdict

Solid first pass. Main blockers before this can be considered production-ready:

1. Replace silent u64 → u8 cast with explicit try_from (feat: setup initial project structure #1).
2. Add tests for verify-fn failure, subscriber error propagation, and SignedData error paths (feat: Dockerize project #12-14).
3. Don't ship new_verifier() as-is — at minimum rename to noop_verifier() so nobody wires it up by accident (Add Nix support #5).

Style/observability points (#2, #3, #6, #7-11) are easy follow-ups and don't need to block.
• Branch: sigagg