feat(app): wire the duty tracker into the core workflow - #570
feat(app): wire the duty tracker into the core workflow#570varex83agent wants to merge 10 commits into
Conversation
`Transaction` is a one-field struct deriving `Encode`/`Decode`, so `ssz_derive` gave it container framing: a spurious 4-byte offset prefix per transaction. The spec type is `ByteList[MAX_BYTES_PER_TRANSACTION]` with no framing (charon models it as a plain `type Transaction []byte`), so pluto could not SSZ-decode any block carrying even one transaction. The validator client publishes signed blocks as SSZ, so every non-empty block came back `400 invalid submitted block`. With no proposer partial signature from the pluto nodes, a 4-node 2-charon/2-pluto cluster lost the whole duty: threshold 3 needs at least one pluto signature. In a kurtosis mixed cluster only 27 of 87 proposer duties were broadcast, and all of pluto's successful proposals contained zero transactions. Adding `struct_behaviour = "transparent"` restores the bare-list encoding. `TreeHash` deliberately keeps the derived container behaviour: merkleizing a one-field container yields the single leaf unchanged, so the root already matched the bare list and the `*_beacon_block_body_root` spec vectors (whose fixtures carry transactions) are unaffected. The gap that hid this was test coverage: the payload fixtures only exercised `TreeHash` and JSON, never an SSZ round trip. Both are now covered. Measured on a kurtosis 2-charon/2-pluto cluster, proposer duties broadcast cluster-wide went from 27/87 to 57/71, with charon (57) and pluto (58) at parity and `400 invalid submitted block` eliminated. Pluto now proposes blocks with up to 606 transactions and 12 blobs. Also fixes two things that made this hard to diagnose: - `ApiError::into_response` dropped the `source` field, so the real decode error was never logged despite the doc comment claiming it was "surfaced in debug logs only". The cause had to be read off the validator client instead. It is now logged with its `source()` chain. - `map_dutydb_error` is shared by `attestation_data` and `await_proposal` but hardcoded attestation wording, so a timed-out block proposal reported "attestation duty expired before data was stored". It now names the duty awaited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tracker was fully implemented in `pluto-core` — trait, service, analysis, reason/step mapping, reporters, metrics and the inclusion core — but never instantiated. Nothing constructed `MetricsDutyReporter`, so its zero-initialisation never ran, and nothing called `report`, so no label set was ever touched. Because every duty metric is a `LabeledFamily` (which emits only once touched), pluto exposed exactly one of charon's eleven `core_tracker_*` metrics: `inclusion_delay`, the sole unlabeled gauge. Dashboards computing `success_duties_total / expect_duties_total` got no series at all, and the `Duty failed` logging was silent. This wires it up: * Starts `TrackerService` with two deadliners derived from the shared duty deadline, offset by `INCL_MISSED_LAG + INCL_CHECK_LAG` slots (deleter a further minute) so a duty is analysed only once its inclusion verdict can have arrived. Parity: charon `app.go` `newTracker`. * Ports `calculateTrackerDelay` (at most 10s, never fewer than 2 slots) to suppress noisy startup failures from a validator client still coming up. * Adds the networked `InclusionChecker` that drives the existing I/O-free `InclusionCore` — the follow-up the module's own TODO called for. One check per due slot at head minus `INCL_CHECK_LAG`, `404` treated as "no block proposed" rather than an error (charon's `is404Error` distinction), then trimming submissions older than `INCL_MISSED_LAG`. * Wires all ten events across the nine core-workflow stitch points, mirroring `core/tracking.go`: each calls through to the real component and then reports that step's error, returning it unchanged so control flow is untouched. `inclusion.submitted` runs before the broadcast and regardless of its outcome, since peers may still succeed. * Exports `INCL_MISSED_LAG` and adds `INCL_CHECK_LAG`, both needed to derive the analyser offset. Step errors are shared rather than stringified. The tracker needs an owned `StepError` while the stitch point must still return its own error, and those types are not `Clone`; `SharedStepError` moves the error into an `Arc` and exposes it via `source()`. That matters because reason inference walks `source()` looking for an `EthBeaconNodeApiClientError` — flattening to a string would silently downgrade beacon-node failures to an unknown reason. Measured on a kurtosis mixed 2-charon/2-pluto cluster, pluto now exposes 9 of the 11 `core_tracker_*` metrics (the remaining two appear on first occurrence), and its per-duty success/expect ratios match charon's on every duty type except proposer: attester 48/71 on both, aggregator 42/42, randao 10/10, sync_message 49/49, sync_contribution 49/49, prepare_* identical, and the `no_local_vc_signature` failure count identical at 23. The inclusion checker reports 23 on-chain inclusions against charon's 25. Known gap, not addressed here: charon additionally reports 6 `proposer_zero_randaos` and 2 `not_included_onchain` proposer failures that pluto does not, because it tracked 16 proposer duty instances against pluto's 10. Pluto's fetcher awaits the aggregated randao rather than erroring when it is absent, so a zero-randao proposer duty emits no event and is never analysed. That is fetcher behaviour rather than tracker wiring; filed as follow-up. The attestation-inclusion path (`Feature::AttestationInclusion`, Alpha and off by default) still needs `getBlockAttestationsV2` + `getEpochCommittees` and a port of charon's `conjugateAggregationBits`. Left as a follow-up so this stays reviewable; the default proposer-inclusion path is complete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
||
| { | ||
| let mut core = self.core.lock().expect("inclusion core mutex poisoned"); | ||
| core.check_block(slot, found); |
There was a problem hiding this comment.
With AttestationInclusion enabled, submitted() stores attester/aggregator duties, but run() always calls proposer-only check_block(), which panics on those entries. This poisons the mutex and blocks later beacon broadcasts.
There was a problem hiding this comment.
Good catch. The networked InclusionChecker only implements the proposer path (the attestation-inclusion driver is the documented follow-up), so I've masked the alpha, off-by-default AttestationInclusion feature off across the whole tracker subsystem in wire_core_workflow (new tracker_feature_set helper). With it disabled, incl_supported returns proposer-only, so the core never stores attester/aggregator submissions and check_block can never hit the unreachable!. Masking it for both the TrackerService and the checker keeps the analyser consistent too (no stalled ChainInclusion steps), and a warn! fires at startup if someone enables it. Added unit tests for the masking. Fixed in 5822bb9.
| .map_err(AppError::BeaconApi)?, | ||
| ) | ||
| }; | ||
| tokio::spawn(Arc::clone(&inclusion).run(ct.clone())); |
There was a problem hiding this comment.
supervise it in run_lifecycle
There was a problem hiding this comment.
Done — the run loop is now spawned into the run_lifecycle JoinSet (via a new inclusion_checker field on WiredComponents) instead of a detached tokio::spawn, so its exit triggers node shutdown like the scheduler/parsigdb tasks. Fixed in 5822bb9.
emlautarom1
left a comment
There was a problem hiding this comment.
+1 to @iamquang95 comments. Rest looks good to me. I'd reduce the comments/inline docs (overly verbose with little to no value)
…testation-inclusion panic - Supervise the networked InclusionChecker's run loop in `run_lifecycle` (JoinSet) instead of a detached `tokio::spawn`, so its exit triggers node shutdown like the other long-lived tasks. - Mask the alpha, off-by-default `AttestationInclusion` feature off across the whole tracker subsystem until the attestation-inclusion path lands. Previously, enabling it made the core store attester/aggregator submissions that the proposer-only `check_block` panics on, poisoning the mutex and blocking later beacon broadcasts. Masking keeps the analyser and checker consistent (proposer-only) and adds a startup warn. - Trim overly verbose inline docs per review. Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
|
Addressed the review in 5822bb9:
|
Problem
The tracker is fully implemented in
pluto-core— trait, service, analysis, reason/step mapping, reporters, metrics, inclusion core, ~4.4k lines with tests — but nothing ever instantiated it.grepforMetricsDutyReporterortracker::across the workspace, excluding the tracker module itself, returns nothing.Two consequences:
MetricsDutyReporter::new()never ran, so its zero-initialisation never happened, andreport()never ran, so no label set was ever touched. Every duty metric is aLabeledFamily, which emits nothing until touched — so pluto exposed 1 of charon's 11core_tracker_*metrics:inclusion_delay, the only unlabeled gauge.Duty failed/Not all peers participatedlogging was silent. Over one run charon logged 250 tracker lines; pluto logged 0.So a dashboard panel computing
success_duties_total / expect_duties_totalreturned no series at all — not zero, nothing.What this wires
TrackerServicewith two deadliners derived from the shared duty deadline, offset byINCL_MISSED_LAG + INCL_CHECK_LAGslots (deleter a further minute), so a duty is analysed only once its inclusion verdict can have arrived. Parity:app.gonewTracker.calculate_tracker_delay— port ofcalculateTrackerDelay: at most 10s, never fewer than 2 slots, suppressing startup noise from a VC still coming up.InclusionCheckerdriving the existing I/O-freeInclusionCore— the follow-up the module's own TODO called for. One check per due slot at head minusINCL_CHECK_LAG;404means "no block proposed", not an error (charon'sis404Errordistinction); then trims submissions older thanINCL_MISSED_LAG.core/tracking.go: call through to the real component, then report that step's error, returning it unchanged so control flow is untouched.inclusion.submittedruns before the broadcast and regardless of its outcome — peers may still succeed.INCL_MISSED_LAGexported andINCL_CHECK_LAGadded, both needed for the analyser offset.Step errors are shared, not stringified
The tracker needs an owned
StepError(Arc<dyn Error>) while the stitch point must still return its own error, and those error types are notClone.SharedStepErrormoves the error into anArcand exposes it throughsource().This is not cosmetic:
analysis::is_eth2_api_errorwalks thesource()chain looking for anEthBeaconNodeApiClientError. Flattening a step error to a string would silently downgrade every beacon-node failure to an unknown reason code.Results
Measured on a kurtosis mixed 2-charon/2-pluto cluster (lighthouse VCs, Fulu).
Metric coverage: 1 → 9 of 11
core_tracker_*metrics. The remaining two (participation_total,inclusion_missed_total) are labeled families that appear on first occurrence.Per-duty
success/expect, charon vs pluto:Failure reasons match too —
attester no_local_vc_signature = 23on both.Duty failedlogging now works, in charon's shape:Inclusion checking is confirmed live — 23
Broadcasted block included on-chainreports with correct slots, pubkeys and broadcast delays, against charon's 25, with zero beacon-node errors.Known gaps
Proposer row diverges. Charon additionally reports 6
proposer_zero_randaosand 2not_included_onchainfailures that pluto does not, because charon tracked 16 proposer duty instances against pluto's 10. Pluto's fetcher awaits the aggregated randao rather than erroring when it is absent (fetch_proposer_data→query_agg_sig_db), so a zero-randao proposer duty emits no event and is never analysed. That is fetcher behaviour, not tracker wiring — worth a follow-up.Attestation-inclusion path.
Feature::AttestationInclusionis Alpha andmin_statusdefaults to Stable, so it is off by default in pluto exactly as in charon. Enabling it needsgetBlockAttestationsV2+getEpochCommitteesand a port ofconjugateAggregationBits. Left as a follow-up to keep this reviewable; the default proposer-inclusion path is complete. Worth noting for whoever picks it up: charon flattensCommitteeAggregationsback into a single concatenated bitlist before building the block map (inclusion.go:796-808), so pluto's existingBlocktype already has the right shape — the conjugation is purely driver-side and needs no core type changes.Checks
cargo test --workspace --all-features: all pass (4integration::*fail in this environment only — they need a Docker socket for testcontainers)cargo clippy --workspace --all-targets --all-features -- -D warnings: cleancargo +nightly fmt --all --check: cleancargo deny check: advisories ok, bans ok, licenses ok, sources ok🤖 Generated with Claude Code