feat(core): implement consensus/instance

[iamquang95]

Fix: #48
This PR:

• Adds core::consensus::instance::InstanceIo for per-instance consensus I/O.
• Uses prost_types::Any for proposal/verify values as a concrete protobuf wire container.
• Receivers are taken once via take_*_rx; senders stay crate-visible for later consensus wiring.

[iamquang95]

@claude review this PR

[claude]

Claude finished @iamquang95's task in 3m 49s —— View job

---

Claude PR Review

• Read CLAUDE.md / AGENTS.md guidelines
• Read changed files (crates/core/src/consensus/instance.rs, crates/core/src/consensus/mod.rs)
• Compare against Charon's Go counterpart for parity (charon/core/consensus/instance/instance_io.go)
• Analyze code for correctness, style, and test coverage
• Post review feedback

Summary

Clean and faithful port of instance.IO[T] from Charon. The atomic-flag semantics map 1:1, and Tokio's single-consumer mpsc constraint is handled correctly via the take_*_rx ownership-transfer pattern. Test coverage is better than the Go original: channel-capacity checks, receiver-ownership tests, and a concurrent maybe_start race test. Only minor design/style notes below; nothing blocking.

Parity Matrix

Component	Go	Rust	Match	Notes
RecvBufferSize = 100	instance_io.go:15	instance.rs:38	yes
RecvBuffer channel (chan T, cap 100)	yes	mpsc::channel(100)	yes	split tx/rx pair
HashCh chan [32]byte cap 1	yes	mpsc::channel(1)	yes
ValueCh chan proto.Message cap 1	yes	mpsc::channel(1) of prost_types::Any	partial	type representation differs (see F1)
VerifyCh chan proto.Message cap 1	yes	mpsc::channel(1) of prost_types::Any	partial	same as ValueCh
ErrCh chan error cap 1	yes	mpsc::channel(1) of RunnerResult	yes	Box<dyn Error+Send+Sync> consistent with repo idiom
DecidedAtCh chan time.Time cap 1	yes	mpsc::channel(1) of tokio::time::Instant	yes
Participated/Proposed/Running atomic.Bool	yes	AtomicBool	yes	ordering choice noted (F3)
MarkParticipated / "already participated"	yes	mark_participated / Error::AlreadyParticipated ("already participated")	yes	error string matches
MarkProposed / "already proposed"	yes	mark_proposed / Error::AlreadyProposed ("already proposed")	yes	error string matches
MaybeStart	yes	maybe_start	yes

Findings

• [Low] prost_types::Any vs Go's proto.Message is a real type-level deviation
  Impact: In Charon, qbft pushes a concrete proto.Message directly into ValueCh/VerifyCh (charon/core/consensus/qbft/qbft.go:413,426). Using Any forces a type_url + value envelope at this boundary, so consumers will need to pack/unpack instead of accepting any prost-decoded type. This is acknowledged in the PR description and is a reasonable choice given Rust's static typing, but a generic value parameter (e.g. InstanceIo<M, V>) would be a closer parity match. Worth confirming the wiring PR can ergonomically pack the proposed value into Any (e.g. via Any::from_msg) without extra allocations on the hot path.
  Evidence: crates/core/src/consensus/instance.rs:103,110
  Go reference: charon/core/consensus/instance/instance_io.go:38-39
  Recommendation: Document in a doc-comment the convention/codec for packing into Any (which type_url is used), so the runner-side wiring is unambiguous. Optionally, add a typed wrapper newtype to keep callers from mixing payloads. Fix this →

• [Low] Error::ReceiverStatePoisoned is practically unreachable
  Impact: The mutex is only held to perform an infallible Option::take(). A poison can only occur if a panic happens while the lock is held, which is impossible here. The variant adds API surface (and a string in error displays) for a state that cannot occur. Not harmful, but technically dead.
  Evidence: crates/core/src/consensus/instance.rs:58-63,217-226
  Recommendation: Either (a) keep it but add // defensive: not reachable today comment; or (b) drop the variant and use .lock().unwrap_or_else(PoisonError::into_inner) to recover unconditionally. Option (b) is more honest about the actual state machine.

• [Low] Ordering::Relaxed is fine here, but document the contract
  Impact: For these three flags taken in isolation, Relaxed is correct and matches the Pluto style guide's preference for standalone flags. However, in the runner-wiring PR the caller that wins maybe_start() will spawn the runner task; the spawn itself carries the necessary memory-ordering. If a future caller ever relies on "data published before the winning CAS must be visible to other observers after a false is returned," Relaxed would not give that — AcqRel/Release+Acquire would. Worth a one-line note that the flag is a pure mutual-exclusion guard, not a synchronization point for other data.
  Evidence: crates/core/src/consensus/instance.rs:156,166,175-177

• [Low] Box<dyn Error + Send + Sync> in RunnerResult mirrors Go's chan error
  Impact: Loses typed-error guarantees the project otherwise prefers (thiserror enums). Consistent with the existing repo idiom (e.g. parsigdb/memory.rs:150, p2p.rs:159+), and matches Charon's chan error. Just flagging that downstream consumers should convert to typed errors when handling.
  Evidence: crates/core/src/consensus/instance.rs:70,75,114
  Recommendation: No change required; ensure runner-side code defines a typed error and boxes only at the channel boundary.

• [Nit] Redundant test alias type TestIo<M> = InstanceIo<M>;
  Evidence: crates/core/src/consensus/instance.rs:240
  Recommendation: Drop the alias and use InstanceIo directly in tests.

• [Nit] Test name is broader than its assertions
  recv_buffering_before_start_does_not_start_instance only verifies that calling recv_tx.try_send(_) then maybe_start() returns true the first time. The "does not start instance" claim is implicit (the runner has nothing to do with recv_tx). The test is fine, but the name oversells what's being demonstrated.
  Evidence: crates/core/src/consensus/instance.rs:318-324
  Recommendation: Rename to something like recv_tx_send_does_not_consume_start_token or add an explicit precondition assertion.

• [Nit] TODO + #[allow(dead_code)] on senders
  Acceptable for a stacked-PR sequence; just make sure the follow-up runner PR removes both rather than leaving them in tree.
  Evidence: crates/core/src/consensus/instance.rs:82-83

Things Done Well

• The Mutex<Option<Receiver<T>>> + one-shot take_*_rx pattern is the right way to bridge Go's "channels are dup-readable values" with Tokio's single-consumer mpsc constraint.
• Sender capacities (100 for recv, 1 for the rest) match the Go original exactly and the test asserts each capacity.
• concurrent_maybe_start_returns_true_once (32-thread race test) is a stronger guarantee than Charon's tests provide.
• Error strings ("already participated", "already proposed") match Charon verbatim per parity rules.
• Module docs explain the buffering rationale, capacities, and the receiver-once contract — exactly the non-obvious details a reviewer needs.

Tests

Not run locally (review-only); the test set added in the PR appears correct and complete based on reading.

Open Questions / Assumptions

1. Is there a plan to wrap Any in a typed alias (e.g. pub type ConsensusValue = Any;) once the qbft runner is wired, or will the bare Any remain the public boundary?
2. The runner-wiring PR will need to drop #[allow(dead_code)] and the TODO — assumed to be tracked.
   • Branch: iamquang95/instanceio

[mskrzypkows]

Looks good