x/bor: fix off-by-one in producer downtime span#573
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review once |
|
Claude finished @kamuikatsurgi's task in 1m 54s —— View job Code Review: x/bor: fix off-by-one in producer downtime span
CorrectnessThe fix is correct. The codebase consistently uses inclusive spans where I also verified the other two Consensus SafetyThe fork-height gating is the right approach for a post-handler state change, per the consensus-critical rules — changing how past blocks are processed without a gate would cause app hash mismatch. The implementation correctly preserves pre-fork behavior. Issues1. TBD fork heights — must be set before merge All three chain-specific heights are
With 2. Pre-fork test uses block height -1
3. Missing boundary test case There's no test for SummaryThe bug fix is sound and well-motivated by on-chain evidence. The fork-height gating is appropriate for consensus safety. Main action items before merge: (1) resolve the TBD fork heights, and (2) consider improving test boundary coverage. |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (83.3%) is below the target coverage (90.0%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #573 +/- ##
=======================================
Coverage 45.1% 45.1%
=======================================
Files 172 172
Lines 18655 18660 +5
=======================================
+ Hits 8421 8428 +7
+ Misses 9026 9025 -1
+ Partials 1208 1207 -1
... and 1 file with indirect coverage changes
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an off-by-one error when creating veBlop spans in PostHandleSetProducerDowntime by computing an inclusive endBlock (start + spanDuration - 1) and gating the behavior behind a new fork height (producerDowntimeSpanFixHeight) to preserve historical behavior.
Changes:
- Adjust veBlop span
endBlockcalculation to be inclusive (- 1) after a configured fix height. - Add test coverage to assert pre-/post-fork veBlop span duration behavior.
- Introduce
producerDowntimeSpanFixHeightconfig plumbing and getter inhelper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| x/bor/keeper/side_msg_server.go | Implements inclusive end-block calculation for veBlop spans, gated by producerDowntimeSpanFixHeight. |
| x/bor/keeper/side_msg_server_test.go | Adds a targeted test verifying span duration behavior before/after the fix height. |
| helper/config.go | Adds producerDowntimeSpanFixHeight and accessor, with chain-specific initialization placeholders. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There was a problem hiding this comment.
The fix is correct in intent, but producerDowntimeSpanFixHeight = 0 for mainnet and amoy makes the fork gate permanently inactive — the pre-fork else-branch is dead code from block 0. Since the producer downtime feature is already live on both networks, a rolling upgrade with height=0 will cause validators on old and new code to compute different spanEndBlock values, producing a consensus split. Real activation heights need to be chosen and filled in before this is safe to deploy.
Extended reasoning...
Overview
This PR adds a producerDowntimeSpanFixHeight fork gate in helper/config.go and updates PostHandleSetProducerDowntime in x/bor/keeper/side_msg_server.go to use startBlock + spanDuration - 1 (inclusive) above the gate height, preserving the old startBlock + spanDuration behavior below it. A new test (TestPostHandleSetProducerDowntime_VeBlopSpanDuration) exercises both branches.
Security / Consensus Risk
All four chain entries in InitHeimdallConfigWith set producerDowntimeSpanFixHeight = 0 (with TBD comments for mainnet, mumbai, and amoy). Because valid block heights are always >= 1, the guard ctx.BlockHeight() >= helper.GetProducerDowntimeSpanFixHeight() is always true, and the else-branch (old formula) is permanently unreachable. Mainnet has had producer downtime active since block ~34,966,593 and amoy since ~20,457,139; with height=0 a rolling upgrade causes nodes on old code to commit spanEndBlock = start + spanDuration while nodes on new code commit start + spanDuration - 1, diverging state roots and halting or forking the chain. The same divergence affects full-history replay.
Level of Scrutiny
This is a production-critical consensus change touching the span calculation that drives Bor chain block production. The correct activation heights for mainnet and amoy must be decided in coordination with the validator set and cannot be placeholder zeros. This requires human review and sign-off before merge.
Other Factors
The test pre-fork case uses blockHeight = fixHeight - 1 = -1, an impossible production height that does not validate the gate in any realistic scenario. The fixHeight +/- 1 pattern is intentionally future-proof, but until real heights are filled in the test provides false confidence. SonarQube passes because it cannot reason about consensus semantics.
Code review1 issue found. Checked for bugs and CLAUDE.md compliance. Issue: Consensus-critical fork gate is ineffective with height = 0 File:
On Mainnet (downtime spans from block This violates Suggested fix: Use producerDowntimeSpanFixHeight = math.MaxInt64 // TBD: set to actual fork height before deployment |
|
This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Code review1 issue found. Checked for bugs and CLAUDE.md compliance. Issue: Hardfork gate is a no-op when v080HardforkHeight = 0 File:
The codebase has From .claude/rules/hardfork-rollout.md: A network is not silently left at 0 or TODO when the behavior should activate at a specific height. Check each helper's zero semantics. Note: Other Suggested fix: Replace |
Description
The veBlop span created by PostHandleSetProducerDowntime used endBlock = startBlock + spanDuration, but spans are inclusive, so the correct formula is startBlock + spanDuration - 1. This made every downtime span one block longer than intended (6401 instead of 6400), causing overlaps with adjacent spans.
Confirmed on-chain:
Gated behind producerDowntimeSpanFixHeight to preserve pre-fork behaviour.