feat(core): implement consensus/qbft/msg

[iamquang95]

Part of: #157

[iamquang95]

@claude review this for me

[claude]

Claude finished @iamquang95's task in 4m 38s —— View job

---

Claude PR Review — feat(core): implement consensus/qbft/msg

Todos:

• Read PR diff and changed files
• Read Charon Go source for parity reference
• Review functional correctness vs Go source
• Review Rust style and Pluto conventions
• Post review feedback

---

Overall this is a clean, well-documented port that mirrors charon/core/consensus/qbft/msg.go closely. The pinned hash and signature fixtures plus the value/prepared_value hash-mapping tests give solid parity coverage. Below are issues worth addressing, plus a couple of nits.

Functional correctness

1. MessageType::from_wire clamps unknown types to MSG_UNKNOWN — Go does not.
crates/core/src/qbft/mod.rs:208-217

In Go (charon/core/consensus/qbft/msg.go:76), Type() does a direct cast: qbft.MsgType(m.msg.GetType()). A wire value of 99 stays MsgType(99) and reaches the core's is_justified switch, which Pluto handles with a panic!("bug: invalid message type"). With from_wire mapping all unknowns to MSG_UNKNOWN(0), the same byzantine input still ends up at the _ => arm and still panics — but the failure mode shifts from "unknown 99" to "unknown 0".

This is fine iff receive-side validation rejects unknown types before they reach qbft::run. The doc comment on duty_from_proto_unchecked asserts this for duties, but no equivalent comment guards the message-type assumption. Please add one on Msg::type_() (or on the from_wire helper) so this precondition isn't lost when the transport layer lands.

2. duty_from_proto_unchecked is misleadingly named.
crates/core/src/consensus/qbft/msg.rs:318-328

The Go equivalent (core.DutyFromProto) does an unchecked DutyType(duty.GetType()) cast. The Rust version actually checks via DutyType::try_from(...).unwrap_or(DutyType::Unknown) — that's a behavior deviation (invalid 99 → DutyType::Unknown instead of DutyType(99)), which is unavoidable given Rust's enum, but the _unchecked suffix suggests we faithfully mirror Go. Either:

• rename to duty_from_proto (and update the doc to mention the silently-clamped path), or
• keep the name but explicitly note in the doc that "unchecked" refers to "we don't return an error", not "we don't validate".

Code quality

3. hash_proto Any-check allocates Strings on every call.
crates/core/src/consensus/qbft/msg.rs:260

M::full_name() == Any::full_name() materializes two heap Strings per hash. Since prost::Name exposes static NAME and PACKAGE, this is cheaper and equivalent:

if M::PACKAGE == "google.protobuf" && M::NAME == "Any" {
    return Err(Error::CannotHashAnyProto);
}

hash_proto runs on every signed/verified consensus message, so the allocation savings add up.

4. Msg::Debug prints raw i64 for message type.
crates/core/src/consensus/qbft/msg.rs:120

.field("type", &self.msg.r#type) produces type: 1 in logs. Wrapping in MessageType::from_wire(self.msg.r#type) would yield the readable pre_prepare/commit/etc. label that the Display impl already provides. Same for duty.type if you want the friendly form.

Tests

5. hash_proto_map_encoding_is_order_independent doesn't test what its name implies.
crates/core/src/consensus/qbft/msg.rs:394-407

BTreeMap is sorted by key, so inserting a then b vs. b then a produces the same iteration order regardless of any prost behavior. The test passes trivially because of BTreeMap, not because of deterministic protobuf encoding. The underlying determinism guarantee comes from build-proto/src/lib.rs:26 (.btree_map(["."])). Either:

• rename the test to e.g. hash_proto_uses_btree_map_for_deterministic_encoding, or
• exercise determinism more directly (e.g., serialize/hash a message containing a non-map repeated field whose protobuf encoding could vary, if any exist).

6. No round-trip test for re-signing.
crates/core/src/consensus/qbft/msg.rs:278-287

The doc on sign_msg says callers may pass an already-signed message to re-sign. There's a test that signs an unsigned fixed_qbft_msg, but no assertion that sign_msg(sign_msg(msg, k), k) == sign_msg(msg, k). Worth adding so regressions to the signature.clear() line are caught.

7. No tampering test for verify_msg_sig.
The wrong-key case is covered; a "right key, mutated payload → returns false" case would catch any future bug that hashes the wrong bytes.

Nits

• crates/core/src/consensus/qbft/msg.rs:54 — ValueMap is exported but used only as Arc<ValueMap> everywhere. Consider also exporting a SharedValueMap = Arc<ValueMap> alias for ergonomics (optional).
• crates/core/src/consensus/qbft/msg.rs:113 — justification_protos and justification are kept in lockstep but there is no invariant comment tying them together. A one-line note that "indices must match, even across recursion" would help.
• crates/core/src/qbft/mod.rs:209-217 — from_wire lives on MessageType but isn't covered by a unit test in the qbft module itself; the msg-adapter tests cover it transitively.

Verdict

LGTM after addressing 1, 2, and ideally 3. Items 4–7 are quality-of-life improvements that can land in a follow-up.
| Branch

[varex83agent]

Summary

This PR ports core/consensus/qbft/msg.go from Charon v1.7.1 to Pluto Rust as crates/core/src/consensus/qbft/msg.rs. The functional parity is solid: Msg::new, hash_proto, sign_msg, verify_msg_sig, and to_hash32 all map cleanly to their Go counterparts, golden-vector tests (UNSIGNED_DATASET_HASH, QBFT_MSG_HASH, QBFT_MSG_SIGNATURE) lock in cross-language byte parity, and the test surface meaningfully expands on Charon's (parametric hash cases, deterministic-encoding regression, sign/verify roundtrip with fixed keys). All 27 new tests pass locally.

Bugs (must-fix)

1. Debug-formatting a Msg with an unknown wire type panics the task (crates/core/src/consensus/qbft/msg.rs:119). Msg::fmt calls MessageType::from_wire(self.msg.r#type).to_string(), and the new public from_wire constructor preserves any i64 without clamping — but MessageType's Display impl panics for any value outside 0..=5. Since Msg::new does not validate r#type, a peer can construct a Msg whose Debug representation aborts the process. This is reachable from any tracing site or logger that touches a wrapped message, and Debug should never panic.

Major findings

None beyond the bug above.

Minor / nit findings

Closely related to the bug: MessageType::Display itself is the root cause (panic on the catch-all arm), and Msg::new should arguably reject !MessageType::valid() rather than relying on upstream filters. Other findings cover documented divergences from Charon (closed-enum DutyType projection, empty-vs-nil signature semantics), Rust-style polish (wider pub visibility than Go, missing #[source] on wrapped errors, double-wrap in trait value_source), an efficiency concern (#[derive(Clone)] deep-copies protobufs that Go shares by pointer), and a small test gap (no malformed-but-non-empty signature case).

Verdict

Request changes for the Debug/Display panic — easy fix (have Display fall back to unknown({n}) or branch on valid() in Msg::fmt). The rest is non-blocking and at author discretion.

[varex83agent]

nit: test-module imports are not grouped consistently with the production file (std::* / external / crate::* separated by blank lines on lines 23-33). Trivial to align: use super::*; and use crate::qbft::{...}; in one group, use prost::bytes::Bytes; use prost_types::Timestamp; use test_case::test_case; in another.

ai review

[mskrzypkows]

I wonder if we need optional msg here, if passing without it returns an error in the first line of the function. I might be better to not call new at all in this case.

[iamquang95]

Agree that we can change this to msg: pbconsensus::QbftMsg, the option is just charon compability API (they use pointer). I will change this in #448