Skip to content

fix(grey): remove dead raw EquivocationEvidence broadcast#653

Closed
sorpaas wants to merge 1 commit into
masterfrom
fix/issue-645-remove-dead-evidence-broadcast
Closed

fix(grey): remove dead raw EquivocationEvidence broadcast#653
sorpaas wants to merge 1 commit into
masterfrom
fix/issue-645-remove-dead-evidence-broadcast

Conversation

@sorpaas

@sorpaas sorpaas commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Remove the raw EquivocationEvidence broadcast that peers silently discard (they only decode EquivocationCountersig)
  • The countersig already contains the full evidence, so no information is lost

Fixes #645.

Test plan

  • cargo check -p grey passes
  • cargo clippy -p grey -- -D warnings passes
  • cargo fmt --all -- --check passes
  • The removed broadcast was dead code — peers never successfully decoded it

The raw evidence broadcast is dead code — peers only decode
EquivocationCountersig, so the raw evidence is silently discarded.
The countersig already contains the full evidence, so removing the
raw broadcast loses no functionality.

Fixes #645.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Genesis Review

Comparison targets:

How to review

Post a comment with the following format (rank from best to worst):

/review
difficulty: <commit1>, <commit2>, ..., <commitN>, currentPR
novelty: <commit1>, <commit2>, ..., <commitN>, currentPR
design: <commit1>, <commit2>, ..., <commitN>, currentPR
verdict: merge

Use the short commit hashes above and currentPR for this PR.
Each line ranks all comparison targets + this PR from best to worst.

To meta-review another reviewer's comment, react with 👍 or 👎.

@sorpaas

sorpaas commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

/review
difficulty: 8a89b50, 33a5d48, 8c441e0, b33622c, currentPR, cf701ea, 3de0dde, c80c727
novelty: 8a89b50, 8c441e0, b33622c, 33a5d48, currentPR, 3de0dde, cf701ea, c80c727
design: 8a89b50, 33a5d48, b33622c, 8c441e0, currentPR, 3de0dde, cf701ea, c80c727
verdict: merge

The PR removes a redundant BroadcastEquivocation call in two symmetric locations in node.rs — the raw evidence broadcast is unnecessary since the countersig already embeds the evidence, so peers receive everything they need from the countersig alone. The change is correct, minimal, and the rationale in the updated comment is clear. All CI checks pass and no existing tests are modified.

@github-actions

Copy link
Copy Markdown
Contributor

JAR Bot: Quorum reached — triggering merge.
Reviews: 1, meta-reviews: 0.
Merge weight: 31178/34272 (>50%).

@luzanikita

Copy link
Copy Markdown
Contributor

/review
difficulty: 8a89b50, 33a5d48, 8c441e0, 3de0dde, b33622c, currentPR, cf701ea, c80c727
novelty: 8a89b50, 3de0dde, currentPR, 8c441e0, b33622c, 33a5d48, cf701ea, c80c727
design: 8a89b50, 33a5d48, currentPR, 3de0dde, 8c441e0, b33622c, c80c727, cf701ea
verdict: merge

The raw EquivocationEvidence broadcast was dead code: the EquivocationReceived handler always decodes bytes as EquivocationCountersig and never attempts to parse raw evidence, so peers received it, failed to decode, logged a warning, and dropped it. Since EquivocationCountersig embeds the full evidence, removing the broadcast loses zero information. Fix applied correctly at both the authored-block and imported-block sites, with a comment that explains the invariant. Ranked mid-field on difficulty (requires reading the decode path to confirm the type mismatch) and above mechanical fixes on novelty (genuine protocol-level dead-code diagnosis).

@github-actions

Copy link
Copy Markdown
Contributor

JAR Bot: Quorum reached — triggering merge.
Reviews: 2, meta-reviews: 0.
Merge weight: 34653/37177 (>50%).

@olanod

olanod commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

/review
difficulty: 8a89b50, 8c441e0, 33a5d48, b33622c, 3de0dde, currentPR, c80c727, cf701ea
novelty: 8a89b50, 8c441e0, b33622c, 33a5d48, 3de0dde, currentPR, c80c727, cf701ea
design: 8a89b50, 8c441e0, 33a5d48, b33622c, currentPR, 3de0dde, c80c727, cf701ea
verdict: merge

Removes redundant BroadcastEquivocation calls in two places — the countersig already contains the evidence, so broadcasting raw evidence separately is dead code. Small, correct cleanup with a clear comment explaining the rationale. Ranked below feature additions like the SIGHUP handler (8c441e0) and genesis comparison target selection (8a89b50), but above trivial fixes like removing debug eprintln (cf701ea).

@github-actions

Copy link
Copy Markdown
Contributor

JAR Bot: Quorum reached — triggering merge.
Reviews: 3, meta-reviews: 0.
Merge weight: 36581/37274 (>50%).

@luzanikita

Copy link
Copy Markdown
Contributor

/review
difficulty: 8a89b50, 8c441e0, 33a5d48, b33622c, currentPR, c80c727, 3de0dde, cf701ea
novelty: 8a89b50, 8c441e0, 33a5d48, b33622c, currentPR, c80c727, 3de0dde, cf701ea
design: 8a89b50, 33a5d48, 8c441e0, b33622c, c80c727, 3de0dde, currentPR, cf701ea
verdict: notMerge

Superseded by #718, which takes the correct approach. The premise here — "raw evidence is redundant because countersig contains it" — is wrong. Raw evidence broadcast is the trigger for the relay path: a peer that missed both conflicting blocks learns of the equivocation via the raw evidence message and can then sign and broadcast its own countersig. PR #718 keeps the raw broadcast and adds a fallback decoder in EquivocationReceived to handle it. If this PR merged, #718 would need to re-add the broadcast it removes. Design ranked near-last because the architectural reasoning points in the wrong direction.

@github-actions

Copy link
Copy Markdown
Contributor

JAR Bot: Quorum reached — triggering merge.
Reviews: 3, meta-reviews: 0.
Merge weight: 35990/37353 (>50%).

@sorpaas

sorpaas commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #718, which addresses #645 the opposite way: instead of removing the raw EquivocationEvidence broadcast, it makes the receiver handle it properly (signing and relaying its own countersig). The raw broadcast is no longer dead code — it's the entry point for validators that missed one of the conflicting blocks locally to still contribute to §17 quorum. Closing this PR as the premise no longer holds.

@sorpaas sorpaas closed this Apr 20, 2026
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.

fix(grey): raw EquivocationEvidence broadcast is dead code

3 participants