Skip to content

feat(qbft_manager): wire EnvelopeProposer per-proposer QBFT instances - #1188

Merged
mergify[bot] merged 3 commits into
sigp:epbsfrom
jnhsigmap:feat/1122/qbft-manager
Jul 30, 2026
Merged

feat(qbft_manager): wire EnvelopeProposer per-proposer QBFT instances#1188
mergify[bot] merged 3 commits into
sigp:epbsfrom
jnhsigmap:feat/1122/qbft-manager

Conversation

@jnhsigmap

@jnhsigmap jnhsigmap commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Problem, Evidence, and Context (Required)

Change Overview (Required)

  • Change was to single crate: qbft_manager.
  • Adds EnvelopeProposerInstanceId with validator and instance height. No ValidatorDutyKind, which differs to ProposerInstanceId.
    • There is one envelope duty per slot.
  • Adds 5th instance map envelope_consensus_data_instances to QbftManager.
  • Replaces TODO in receive_data that rejected validator duty executor EnvelopeProposer messages.
    • Adds fork gate to the EnvelopeProposer match arm that rejects before Gloas and routes the new map to the qbft instance at/after Gloas.
  • Cheap refactor - adds a gloas_enabled_at_slot helper to handle duplicated call. This reads from Ethereum consensus spec and not the SSV fork schedule. Existing Gloas beacon-vote gate also adopts it.
  • Adds the fifth cleaner .retain (same QBFT_RETAIN_SLOTS threshold as siblings).
  • New QbftDecidable for EnvelopeConsensusData mirroring peers' structure.

Risks, Trade-offs, and Mitigations (Required)

  • Blast radius is one file plus tests; pre-Gloas behaviour is unchanged (RoleNotActive still thrown).
  • The Gloas gate is defence in depth behind message_validator's validate_role_for_fork; both read the same Arc<ChainSpec>, so they cannot disagree. Reaching the gate logs at warn! (parity with the AggregatorCommittee Boole gate).
  • Post-Gloas map growth is bounded upstream by message validation and the interest filter, and downstream by the cleaner TTL, identical to the four existing maps.

Validation (Required)

  • New envelope_dispatch_tests module: pre-Gloas rejection for both duty-executor spellings with map-emptiness assertions; post-Gloas routing into the envelope map without touching the proposer map; Gloas activation boundary (last pre-fork slot rejects, first fork slot routes); message_id is validator-scoped and distinct from the block-proposal id for the same validator.
  • Existing test envelope_proposer_any_executor_decodes_as_validator_transient_role_not_active is to envelope_proposer_rejected_before_gloas and moved into new module anchor/qbft_manager/src/tests/envelope_dispatch_tests.rs, preserving its record of why a committee-executor EnvelopeProposer id is unconstructable.
  • cargo test -p qbft_manager: 38 passed. make test, make lint, make cargo-fmt-check: clean.

Rollback (Required for behavior or runtime changes; optional otherwise)

  • Revert the commit; the stub reject returns. No wire-format, database, or config surface is touched.

Blockers / Dependencies (Optional)

@codecov-commenter

codecov-commenter commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (epbs@59005f5). Learn more about missing BASE report.

Files with missing lines Patch % Lines
anchor/qbft_manager/src/lib.rs 85.29% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             epbs    #1188   +/-   ##
=======================================
  Coverage        ?   69.44%           
=======================================
  Files           ?      164           
  Lines           ?    32173           
  Branches        ?        0           
=======================================
  Hits            ?    22342           
  Misses          ?     9831           
  Partials        ?        0           
Flag Coverage Δ
rust 69.44% <85.29%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jnhsigmap

Copy link
Copy Markdown
Contributor Author

@claude, review this PR

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude finished @jnhsigmap's task in 2m 58s —— View job


PR Review: wire EnvelopeProposer per-proposer QBFT instances

  • Gather context (read changed files + surrounding code)
  • Analyze lib.rs changes (instance map, routing, fork gate, cleaner)
  • Analyze test changes
  • Post review feedback

Reviewed against origin/epbs. This is a clean, well-scoped change that mirrors the four existing instance-map patterns faithfully. I verified the load-bearing invariants below. No blocking issues found.

What I verified

  • Round cap won't panic the new path. decide_instance (lib.rs:254-258) requires role().max_round() to be Some; Role::EnvelopeProposer.max_round() returns Some(2) (msgid.rs:84), so envelope instances build a valid config. ✅
  • debug import removal is correct. The only remaining tracing uses are debug_span!, error, warn — the removed debug! was the deleted TODO line. No other debug! callers in the file. ✅
  • Message-id disjointness. EnvelopeConsensusData::message_id (lib.rs:610) uses Role::EnvelopeProposer, distinct from ProposerConsensusData's Role::Proposer, so envelope and block duties never share a map key for the same validator — pinned by envelope_proposer_message_id_is_validator_scoped. ✅
  • Cleaner parity. The fifth .retain (lib.rs:449) uses the identical QBFT_RETAIN_SLOTS cutoff as its siblings, so post-Gloas map growth is bounded the same way. ✅
  • Fork gate reads the Ethereum spec, not the SSV schedule — consistent with the committee GloasBeaconVote gate it was refactored alongside. ✅

Observations (non-blocking)

  1. warn! on the pre-Gloas reject path (lib.rs:307) is a potential log-spam surface. Because this returns before inserting into any map, a peer flooding pre-Gloas EnvelopeProposer messages would emit one warn! per message. This is defense in depth behind message_validator::validate_role_for_fork, so in practice these shouldn't arrive — and it matches the existing AggregatorCommittee/Boole gate precedent (lib.rs:372), so I'm not flagging it as a defect. Worth noting that the sibling committee gate (GloasBeaconVote vs BeaconVote) instead routes silently pre-fork, so the codebase is inconsistent on whether reaching a fork gate is warn-worthy. If reaching this gate is genuinely a "should never happen" condition, warn! is the right call; just flagging the trade-off.

  2. Refactor dropped the "beacon-vote shape" rationale comment. The old committee gate carried an inline comment explaining why it reads the consensus spec rather than the SSV fork. The new gloas_enabled_at_slot doc-comment (lib.rs:216-217) preserves the "distinct from the SSV fork_schedule" point, so the key rationale survives — noting only that the beacon-vote-specific framing is gone.

  3. Test coverage is thorough. The new envelope_dispatch_tests module covers both duty-executor spellings pre-Gloas, the activation boundary (last pre-fork slot rejects / first fork slot routes), map isolation from the proposer map, and validator-scoped id distinctness. The migrated envelope_proposer_rejected_before_gloas correctly preserves the byte-identity reasoning for why a committee-executor EnvelopeProposer id is unconstructable. Nicely done.

Note on verification

I could not re-run cargo test -p qbft_manager / make lint / make cargo-fmt-check in this environment (build commands are blocked by the sandbox), so I'm relying on the PR's stated results (38 passed, clean lint/fmt) for those. The Codecov note (5 missing lines in lib.rs, 85% patch coverage) is the early-return warn/reject branches and the get_map/instance_height trait glue — low-risk, hard-to-hit lines consistent with the sibling impls.
• branch feat/1122/qbft-manager

@jnhsigmap

Copy link
Copy Markdown
Contributor Author

my justification forwarn!: it highlights a case where messages get through the validator check and are caught here. validation and routing disagree. Probably worth attention.

I added the comment about BeaconVote back where it was in the Committee branch as it was a slight semantic divergence from "this is a fork gate check".

@jnhsigmap
jnhsigmap marked this pull request as ready for review July 29, 2026 23:40

@shane-moore shane-moore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm man!

@mergify

mergify Bot commented Jul 30, 2026

Copy link
Copy Markdown

Queued — the merge queue status continues in this comment ↓.

@mergify

mergify Bot commented Jul 30, 2026

Copy link
Copy Markdown

Merge Queue Status

  • Entered queue2026-07-30 02:17 UTC · Rule: default · triggered by rule Add outdated, ready-to-merge PRs to merge queue
  • Checks passed · on draft merge queue: checking epbs (59005f5) and #1188 together #1194
  • Merged2026-07-30 02:46 UTC · at 480dea8cd1be6b79de8af06f410114bda9e19427 · squash

This pull request spent 28 minutes 20 seconds in the queue, including 27 minutes 3 seconds running CI.

Required conditions to merge
  • check-success=cli-reference-check
  • check-success=run-local-testnet
  • check-success=test-suite-success

@mergify
mergify Bot merged commit 7bad23f into sigp:epbs Jul 30, 2026
23 checks passed
@mergify mergify Bot removed the queued label Jul 30, 2026
@jnhsigmap
jnhsigmap deleted the feat/1122/qbft-manager branch July 30, 2026 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants