Skip to content

feat(core): implement infosync#495

Open
iamquang95 wants to merge 19 commits into
mainfrom
feat/infosync
Open

feat(core): implement infosync#495
iamquang95 wants to merge 19 commits into
mainfrom
feat/infosync

Conversation

@iamquang95

@iamquang95 iamquang95 commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Fix #173

Stack on: #489

File Change
crates/infosync/src/lib.rs (+506) Component: new / trigger / protocols(slot) / proposals(slot), a ResultStore (dedup, history cap, slot-selection), and the priority subscribe callback.
crates/infosync/tests/infosync_integration.rs (+356) 3-host libp2p integration test (ports TestInfoSync): all nodes converge on the same agreed priorities.
crates/core/src/types.rs (+89) ProposalTypeopen enum (Unknown(String)), as_str / From / serde-via-String; drops Copy.
  1. crates/infosync/src/lib.rs — self-contained. Check the store dedup/cap (>= ⇒ caps at 99, matches charon), protocols/proposals slot-selection + defaults (local / [Full]), handle_results topic routing, build_request (3 topics + InfoSync duty).
  2. crates/core/src/types.rs — the only shared-type change. Design call to confirm: ProposalType open enum + loss of Copy, to preserve forward-compat values from newer peers (charon's type is an open string). Only users are core's def + infosync.
  3. integration test — note it runs at the infosync+priority layer, not app.Run (infosync isn't wired into the app yet).

@iamquang95 iamquang95 marked this pull request as ready for review June 22, 2026 09:09
Base automatically changed from feat/core-priority to main June 23, 2026 09:45
@iamquang95

Copy link
Copy Markdown
Collaborator Author

@claude review this PR

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

Claude finished @iamquang95's task in 2m 5s —— View job


Review: feat(core): implement infosync

I read the new infosync crate, the ProposalType change, and the integration test, and compared against charon/core/infosync/infosync.go. This is a faithful, well-documented port with strong test coverage. Findings below — mostly observations; nothing blocking.

Tasks

  • Read changed files
  • Compare against charon Go source
  • Review correctness (store dedup/cap, slot-selection, topic routing, request build)
  • Review ProposalType open-enum change
  • Review integration test

✅ Parity with charon — confirmed

  • add_result dedup-vs-last + >= MAX_RESULTS cap (effective 99) matches addResult exactly, including the off-by-one — and it's documented on MAX_RESULTS. 👍
  • protocols(slot) / proposals(slot) slot-selection + defaults (local / [Full]) match Protocols/Proposals.
  • handle_results topic routing + "store only if a version was agreed" (!res.versions.is_empty()) matches.
  • build_request (3 topics in version/protocol/proposal order + InfoSync duty) and the mock-alpha augmentation at new() match.
  • ProposalType open enum maps cleanly onto charon's type ProposalType stringfull/builder/synthetic known, everything else preserved as Unknown. Removing Copy is safe: the only users are core/types.rs + infosync (verified by grep).

Observations / minor suggestions

1. Slot-ordering assumption is now implicit (inherited from charon).
Both getters break on result.slot > slot and add_result dedups only against results.last() (lib.rs:109, 131, 150). Both rely on results being appended in non-decreasing slot order. If trigger is ever called out of slot order (a late/retried slot), selection silently returns the wrong entry and the dedup misses. Charon has the identical assumption, so this is parity-preserving, not a regression — but in Rust it'd be cheap to make the invariant explicit (a debug_assert!(results.last().map_or(true, |r| r.slot <= result.slot)) in add_result, or a doc note). Low severity since infosync is triggered once per epoch in order.

2. Poisoned-mutex behavior in the subscribe callback (lib.rs:103-119).
handle_resultsadd_result use .expect("...poisoned"). The priority fan-out invokes this callback; if any lock holder ever panics, the lock stays poisoned and every subsequent decided duty panics the fan-out path. Given the critical sections are tiny and panic-free this is unlikely, but consider lock().unwrap_or_else(|e| e.into_inner()) so one bad duty can't permanently wedge infosync. Minor.

3. Debug-log fidelity (lib.rs:93). Charon logs each topic separately (z.Any(result.Topic, result.Priorities)); the port logs ?results once. Cosmetic — the info is still there.

4. Forward note (not this PR). When infosync is wired into the app, downstream consumers of proposals(slot) must handle ProposalType::Unknown — that's the intended trade-off of the open enum, just worth tracking so an Unknown doesn't get silently treated as Full.

Tests

Unit tests cover dedup, cap, slot-selection, topic routing, unknown-proposal preservation, and the no-version-discard path — thorough. The 3-host integration test is well-constructed: the capture subscriber registered after infosync's guarantees the store is updated before assertion, and the TestConsensus claims the decision under-lock before fan-out to avoid double-notify. Nice.


Verdict: Looks good to merge. The slot-ordering assumption (#1) is the only item I'd consider hardening with a debug_assert, and it's optional given charon does the same.
· feat/infosync

@iamquang95

Copy link
Copy Markdown
Collaborator Author

For 1, keep the current implementation for parity, and it's low enhancement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement core/infosync

2 participants