Skip to content

feat(consensus): implement consensus debugger + wrapper + controller#471

Open
iamquang95 wants to merge 3 commits into
mainfrom
iamquang95/consensus-debugger
Open

feat(consensus): implement consensus debugger + wrapper + controller#471
iamquang95 wants to merge 3 commits into
mainfrom
iamquang95/consensus-debugger

Conversation

@iamquang95

Copy link
Copy Markdown
Collaborator

Part of #398

Stack on consensus/qbft

Base automatically changed from iamquang95/consensus-qbft to main June 12, 2026 07:15
@iamquang95 iamquang95 changed the title feat(consensus): implement consensus debugger feat(consensus): implement consensus debugger + wrapper + controller Jun 12, 2026
* feat(consensus): implement consensus wrapper

* feat(consensus): implement consensus controller
@iamquang95 iamquang95 force-pushed the iamquang95/consensus-debugger branch from 025f6a5 to c3864c9 Compare June 12, 2026 08:21
@iamquang95

Copy link
Copy Markdown
Collaborator Author

@claude review this PR

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

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


PR Review: consensus debugger + wrapper + controller

  • Gather context (read changed files & Go source)
  • Review controller.rs
  • Review debugger.rs
  • Review wrapper.rs
  • Review component.rs changes
  • Post review feedback

Reviewed against the Charon Go sources (debugger.go, wrapper.go, controller.go). Overall this is a clean, faithful port with good test coverage. No blocking bugs found. CI: linter ✅, build in progress.


Parity assessment

debugger.rs — faithful ✅

  • DEFAULT_MAX_BUFFER_SIZE = 52_428_800 matches Go's 50 * (1 << 20). ✓
  • Compression::fast() == zlib level 1 == Go's gzip.BestSpeed. ✓
  • FIFO eviction, content headers, and error body ("...see logs\n" + 500) match ServeHTTP/http.Error. ✓
  • Rust handles the empty-buffer edge in the eviction loop (pop_front() … else break) more safely than the Go slice indexing. Nice.

wrapper.rs — correct, with one semantic deviation worth knowing

  • Go holds the RLock for the entire duration of Participate/Propose/Start, so SetImpl (write lock) blocks until in-flight calls complete. The Rust current() clones the Arc and releases the read lock immediately, so set_impl can swap the implementation while a participate/propose future is still running on the old impl.
  • This is arguably the better Rust idiom (no long-held lock across .await) and is harmless for the only current caller (swapping back to the default impl). Just flagging it as a behavioral difference from Go in case future protocol-switching logic relies on the Go blocking semantics.

controller.rs — matches current Go behavior; two notes

  1. The Go controller owns mutable.cancelWrappedCtx and spawns a goroutine in Start to cancel the wrapped consensus when the parent context is done. The Rust port omits this entirely. Since that machinery is only exercised by the commented-out non-default-protocol path in Go (SetCurrentConsensusForProtocol's TODO), the omission is functionally equivalent to Go's active behavior — but it does mean the future "stop the previous non-default impl" hook isn't carried over. Worth a one-line comment noting this is deferred until additional protocols exist, mirroring the Go TODO.
  2. Deadliner ownership deviation: Go's controller builds its own deadliner internally — core.NewDeadliner(ctx, "consensus.qbft", deadlineFunc) — whereas the Rust Config requires the caller to pass a pre-built deadliner + expired_rx pair. This pushes the "consensus.qbft" naming/lifecycle responsibility onto the caller. Probably intentional given DeadlinerTask's design, but please confirm — losing the fixed "consensus.qbft" deadliner name could matter for metrics/observability parity.

component.rs changes

  • Moving expired_rx into Config + Mutex<Option<…>> and changing start from self: Arc<Self> to &self is the right move to satisfy the &self trait signature. ✓
  • start() is now silently single-shot: the second call take()s None, spawns a task that immediately returns, and the cleanup loop never runs. The wrapper's start also drops the JoinHandle. This is fine for the intended once-only call, but it's a quiet footgun. Consider a debug-log/assert on the second call, or a doc note that start must be called exactly once. Fix this →
  • delete_instance_io is now #[cfg(test)]-only and start inlines the instances.lock().remove(&duty) logic. Minor duplication — you could keep a small non-test private helper (fn remove_instance(&self, duty)) and reuse it from both start and tests, avoiding the test-only carve-out. Trivial.
  • Dropping the JoinHandle in the trait start detaches (does not abort) the task, so the loop runs until ct is cancelled — correct, and consistent with the CancellationToken-driven lifetime.

Minor / optional

  • debugger.rs error path: Go's http.Error also sets Content-Type: text/plain; charset=utf-8 and X-Content-Type-Options: nosniff. error_response() sets only the body + 500 status. Cosmetic; not worth changing unless you want byte-for-byte HTTP parity.
  • Test coverage is solid: FIFO eviction, git-hash, clone-shares-buffer, sniffer, router for the debugger; forward+swap for the wrapper; default/current + unsupported-protocol error for the controller. 👍

Summary

No correctness blockers. Recommend: (1) confirm the deadliner-name ownership deviation is intended, (2) add a note/log that start is single-shot, and (3) optionally a one-line comment mirroring Go's deferred wrapped-context-cancellation TODO.
· branch iamquang95/consensus-debugger

@iamquang95 iamquang95 marked this pull request as ready for review June 12, 2026 09:55
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.

1 participant