feat(core): implement infosync by iamquang95 · Pull Request #495 · NethermindEth/pluto

iamquang95 · 2026-06-22T06:14:17Z

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)	`ProposalType` → open enum (`Unknown(String)`), `as_str` / `From` / serde-via-`String`; drops `Copy`.

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).
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.
integration test — note it runs at the infosync+priority layer, not app.Run (infosync isn't wired into the app yet).

iamquang95 · 2026-06-23T10:46:50Z

@claude review this PR

claude · 2026-06-23T10:47:10Z

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 string — full/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_results → add_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 · 2026-06-23T10:52:52Z

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

varex83

LGTM overall! Only a few improvements could be made

varex83 · 2026-06-24T16:40:20Z

+        results.push(result);
+
+        if results.len() >= MAX_RESULTS {
+            results.remove(0);


This is O(n) operation, better to use deque here

Agree for clarity mostly

varex83 · 2026-06-24T16:43:08Z

+            .expect("infosync results mutex poisoned");
+
+        let mut resp = self.local_protocols.clone();
+        for result in results.iter() {


Since slots in the results are in increasing order, we can make this search using binary search, which gives us O(n) -> O(log(n))

Agree on making the change just for the sake of clarity.

varex83 · 2026-06-24T16:43:21Z

+            .expect("infosync results mutex poisoned");
+
+        let mut resp = vec![ProposalType::Full];
+        for result in results.iter() {


varex83 · 2026-06-24T16:45:47Z

+}
+
+/// Returns the versions as their string representations.
+fn versions_to_strings(versions: &[SemVer]) -> Vec<String> {


I think we can inline it

Disagree, makes testing a bit easier having it separated.

varex83 · 2026-06-24T16:45:56Z

+}
+
+/// Returns the proposal types as their wire-format strings.
+fn proposals_to_strings(proposals: &[ProposalType]) -> Vec<String> {


Same comment as in versions_to_strings

emlautarom1

LGTM, minor things only

emlautarom1 · 2026-06-24T21:32:07Z

+
+/// Returns whether the `MockAlpha` feature is globally enabled.
+fn mock_alpha_enabled() -> bool {
+    GLOBAL_STATE


nit: prefer to use qualified names for variables/functions:

Suggested change

GLOBAL_STATE

pluto_featureset::GLOBAL_STATE

emlautarom1 · 2026-06-24T21:42:58Z

+            .expect("infosync results mutex poisoned");
+
+        let mut resp = self.local_protocols.clone();
+        for result in results.iter() {


Agree on making the change just for the sake of clarity.

emlautarom1 · 2026-06-24T21:44:18Z

+    fn local_protocols(&self) -> &[String] {
+        &self.local_protocols
+    }


Remove the method an inline it (single usage)

emlautarom1 · 2026-06-24T21:45:43Z

+}
+
+/// Returns the versions as their string representations.
+fn versions_to_strings(versions: &[SemVer]) -> Vec<String> {


Disagree, makes testing a bit easier having it separated.

emlautarom1 · 2026-06-24T21:46:14Z

+}
+
+/// Returns the proposal types as their wire-format strings.
+fn proposals_to_strings(proposals: &[ProposalType]) -> Vec<String> {


Same comment as in versions_to_strings

emlautarom1 · 2026-06-24T21:46:50Z

+        results.push(result);
+
+        if results.len() >= MAX_RESULTS {
+            results.remove(0);


Agree for clarity mostly

emlautarom1 · 2026-06-24T21:58:22Z

+    // Trigger infosync on every host for the same slot. `trigger` blocks until
+    // the duty deadline / cancellation, so it runs in the background while the
+    // decision is observed via the capture channel.
+    let mut triggers = Vec::with_capacity(N);


Can use a JoinSet here (less bookkeeping)

emlautarom1 · 2026-06-24T21:59:45Z

+                }
+            }
+        };
+        timeout(Duration::from_secs(30), mesh)


Do we need such long timeout? Can we make do with 10s or less?

emlautarom1 · 2026-06-24T22:01:43Z

+        peers.clone(),
+        i64::try_from(peers.len()).expect("peer count fits i64"),
+        consensus,
+        Duration::from_secs(3600),


1 hr timeout?

iamquang95 added 18 commits June 17, 2026 15:18

fix: public some ultilities

e7f1ef9

feat(core): implement prioritisier

a383698

fix: machete

df1fb27

fix: don't panic when duplicated duty

4d049e6

fix: don't flatten Deadline Error

bded871

fix: derive local_id from priv key

333c3db

fix: protocol_id start with slash

750d779

fix: surface deadline compute failed error

bdbdf95

Merge remote-tracking branch 'origin/main' into feat/core-priority

6aedb6c

fix: using HashSet to simplify code

d2123cc

fix: add debug on unexpected error

ae5b3c6

fix: protocol_id not start with slash

e1189dd

fix: priority test

587e53d

fix: use p2p_context

8de6e9e

Merge remote-tracking branch 'origin/main' into feat/core-priority

969a105

feat(core): implement infosync

e294412

fix: using enum instead of string ProposalType

f4dd367

test: add integ test for infosync

5d72613

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

Merge remote-tracking branch 'origin/main' into feat/infosync

a1c99f9

iamquang95 requested review from emlautarom1 and varex83 June 23, 2026 10:29

pinebit approved these changes Jun 23, 2026

View reviewed changes

varex83 approved these changes Jun 24, 2026

View reviewed changes

emlautarom1 approved these changes Jun 24, 2026

View reviewed changes

Uh oh!

Conversation

iamquang95 commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamquang95 commented Jun 23, 2026

Uh oh!

claude Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: feat(core): implement infosync

✅ Parity with charon — confirmed

Observations / minor suggestions

Tests

Uh oh!

iamquang95 commented Jun 23, 2026

Uh oh!

varex83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emlautarom1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iamquang95 commented Jun 22, 2026 •

edited

Loading

claude Bot commented Jun 23, 2026 •

edited

Loading

Review: `feat(core): implement infosync`