fix: align QBFT, timer, dutydb & duty-type wire parity with charon v1.7.1#520
Open
varex83agent wants to merge 1 commit into
Open
fix: align QBFT, timer, dutydb & duty-type wire parity with charon v1.7.1#520varex83agent wants to merge 1 commit into
varex83agent wants to merge 1 commit into
Conversation
…y with charon v1.7.1 Implements the five parity fixes from the 07-consensus-and-core-parity plan: - dutydb: mirror charon storeAttestationUnsafe by dual-writing every attester duty's pubkey and attestation data under a hardcoded committee_index=0 key (post-Electra VC compat), with the relaxed source/target-only clash check on the commIdx=0 data entry; the real-index entries keep the full clash check. commIdx=0 keys are recorded in attestation_keys_by_slot so eviction removes them too. - consensus/timer: linear_subsequent_round_timeout now returns nanoseconds (Duration::from_nanos(200*(round-1)+200)) matching the pinned v1.7.1 roundtimer.go:243, which lacks the `* time.Millisecond` fixed later by charon#4537; comment documents the flip-back trigger. - consensus/qbft msg: relax value_hash/prepared_value_hash admission to Go newMsg semantics — absent/zero/non-32-byte hashes collapse to the nil hash with no error; drop the message-type and prepared-round gating and the now-unreachable InvalidValueHash / InvalidPreparedValueHash variants. - core/types: exhaustive DutyType<->i32 and Duty<->proto round-trip tests pinning the literal charon core/types.go enum numbers and the Unknown encode/decode asymmetry. - parity nits: sigagg template selection now breaks on the first non-attestation parSig (matching Go's loop); documented the inclusion.rs unreachable! as faithful panic parity, the analysis.rs missing cancellation tier as an accepted temporary divergence, the QBFT prepared-round bound as accepted hardening, and the Any::default() compare sentinel invariant (with a pinning test). Reference: charon v1.7.1 (749d2d7). Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the five behavioural-parity fixes from the 07-consensus-and-core-parity plan, all verified against the pinned Charon reference
v1.7.1(749d2d7).T01 — dutydb: committee_index=0 dual-storage (MAJOR)
Charon's
storeAttestationUnsafe(core/dutydb/memory.go:316-401) writes every attester duty twice: under the real committee index and under a hardcodedcommittee_index=0key — a deliberate post-Electra VC-compat shim (the beacon-APIs spec default forproduceAttestationDatais index 0). Pluto only stored under the real index, so a spec-correct post-Electra VC querying withcommittee_index=0would miss where Charon succeeds → silently missed attestation duties.store_attestationnow also writes the pubkey and attestation data under commIdx=0. The commIdx=0 data entry uses Charon's relaxed clash check (source/target only, ignoringbeacon_block_root— heads may legitimately differ mid-fetch-loop on a slow beacon node); the real-index entry keeps the full check. The commIdx=0PkKeyjoinsattestation_keys_by_slot, so the existing eviction loop removes both entries. When the real index is already 0 the dual write is a no-op (no duplicate key appended). 7 new tests cover lookups, both clash paths, eviction, and idempotency.T02 — timer: v1.7.1 nanosecond linear subsequent-round timeout (MAJOR)
v1.7.1roundtimer.go:243computestime.Duration(200*(round-1) + 200)— a bare integer, i.e. nanoseconds (the "400 milliseconds" comment is the upstream bug tracked in ObolNetwork/charon#4537, unfixed at the pin). Pluto usedDuration::from_millis, ~10⁶× longer, desynchronizing round boundaries in a mixed cluster under theLinearfeature. Switched toDuration::from_nanos, corrected the misleading comment (it previously claimed #4537 was already fixed), added a clock-independent literal-nanosecond test, and left an explicit note to flip back to milliseconds when the Charon pin moves past #4537.T03 — qbft/msg: relax value-hash admission to Go
newMsgsemantics (MINOR)Go
newMsg(core/consensus/qbft/msg.go:19-62) collapses absent / all-zero / non-32-byte hashes to the nil hash with no error, and errors only when a well-formed non-zero 32-byte hash is missing from the values map — no message-type or prepared-round gating. Pluto's wrapper was deliberately stricter, changing which peers' messages are admitted (consensus-sensitive).value_hash/prepared_value_hashnow mirrornewMsgexactly;value_hash_requiredand the unreachableInvalidValueHash/InvalidPreparedValueHashvariants are removed. Decision record: no hardening retained — the generic core's justification rules already reject values absent at decision time, so the wrapper strictness bought no safety Charon doesn't have. Rejection tests were replaced with admit-as-nil round-trips at both theMsg::newand componenthandlelevel.T04 — tests: exhaustive DutyType/Duty wire round-trips
Four new test-only additions in
types.rspin everyDutyType <-> i32mapping against the literalcore/types.gonumbers (0..=13; "MUST not change, it will break backwards compatibility"), bothTryFromdirections, the out-of-range error edges (14/15/99/-1/MAX/MIN), and theDuty <-> pbcore::Dutyround-trip including the asymmetry:Unknownencodes tor#type: 0but that proto is rejected on decode.T05 — parity nits: fix-or-document
sigagg.go:118-134) —breakon the first non-VersionedAttestationinstead offind_map's skip-and-continue. New test guards the break path.unreachable!(documented + tested): faithful analogue of Go'spanic("bug: unexpected type");#[should_panic]test pins it.is_cancelled_errorwhen the fetcher is ported). Lock-in tests for the default arm already existed (analyse_fetcher_failed_table).valid_round_change_prepared_roundcarries the full accepted-hardening parity note (Go omits the check); call sites carry back-references. Range boundary tests already existed.Any::default()sentinel (documented + tested): stated thenil-equivalence and non-empty-type_urlinvariant; new test pins that a marshalledUnsignedDataSetcan never collide with the sentinel.Testing
cargo +nightly fmt --all --check✅cargo clippy --workspace --all-targets --all-features -- -D warnings✅cargo test --workspace --all-features✅ (full suite, two consecutive clean runs)🤖 Generated with Claude Code