Skip to content

fix devnet edge case#7828

Open
raduchis wants to merge 7 commits intomasterfrom
devnet-fix
Open

fix devnet edge case#7828
raduchis wants to merge 7 commits intomasterfrom
devnet-fix

Conversation

@raduchis
Copy link
Copy Markdown
Contributor

@raduchis raduchis commented Apr 8, 2026

epoch start trigger and force epoch start with minimum blocks per epoch

Reasoning behind the pull request

A 1-block epoch causes processIfFirstBlockAfterEpochStart to dirty the peer trie in memory, then createEpochStartBody fails to recreate it from DB because the root hash was never committed

Proposed changes

  • Added minimumBlocksPerEpoch = 2 guard in trigger.Update() — epoch start is deferred until nonce >= epochStartNonce + 2, ensuring at least 2 blocks per epoch

Testing procedure

  • ALLIN with rounds per epoch 0 or 1 maybe

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

@raduchis raduchis marked this pull request as ready for review April 8, 2026 10:21
sstanculeanu
sstanculeanu previously approved these changes Apr 8, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adjusts the metachain epoch-start trigger to avoid a devnet edge case where an epoch could effectively last only one block, leading to inconsistent in-memory vs DB state during epoch-start processing.

Changes:

  • Added a minimumBlocksPerEpoch = 2 guard to trigger.Update() so epoch start is deferred until at least 2 blocks exist in the current epoch.
  • Added unit tests covering both normal epoch-start triggering and forced epoch-start behavior under the new minimum-blocks constraint.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
epochStart/metachain/trigger.go Enforces a minimum number of blocks per epoch before allowing an epoch transition.
epochStart/metachain/trigger_test.go Adds regression tests ensuring epoch start/forced epoch start won’t happen with only 1 block in the current epoch.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread epochStart/metachain/trigger.go Outdated

isZeroEpochEdgeCase := nonce < minimumNonceToStartEpoch
epochStartNonce := t.epochStartMeta.GetNonce()
hasMinBlocksInEpoch := nonce >= epochStartNonce+minimumBlocksPerEpoch
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

hasMinBlocksInEpoch := nonce >= epochStartNonce+minimumBlocksPerEpoch can overflow when epochStartNonce is close to math.MaxUint64, potentially making the comparison unexpectedly true/false and triggering epoch start incorrectly. Please rewrite the check to avoid uint64 addition overflow (e.g., compare using nonce >= epochStartNonce and then nonce-epochStartNonce >= minimumBlocksPerEpoch, or guard the addition with an overflow-safe check).

Suggested change
hasMinBlocksInEpoch := nonce >= epochStartNonce+minimumBlocksPerEpoch
hasMinBlocksInEpoch := nonce >= epochStartNonce && nonce-epochStartNonce >= minimumBlocksPerEpoch

Copilot uses AI. Check for mistakes.
@raduchis raduchis dismissed stale reviews from BeniaminDrasovean and sstanculeanu via 9ace5b4 April 8, 2026 11:03
sstanculeanu
sstanculeanu previously approved these changes Apr 8, 2026
AdoAdoAdo
AdoAdoAdo previously approved these changes Apr 8, 2026
ssd04
ssd04 previously approved these changes Apr 8, 2026
miiu96
miiu96 previously approved these changes Apr 8, 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.

7 participants