Skip to content

test(consensus): fix TestIsYourTurnConsensusV2 timing flake - #3053

Open
gzliudan wants to merge 1 commit into
XinFinOrg:dev-upgradefrom
gzliudan:fix-yourturn-timing-flake
Open

gzliudan wants to merge 1 commit into
XinFinOrg:dev-upgradefrom
gzliudan:fix-yourturn-timing-flake

Conversation

@gzliudan

Copy link
Copy Markdown
Collaborator

Summary

 consensus/tests/engine_v2_tests/authorised_masternode_test.go
   currentBlockHeader := currentBlock.Header()
-  currentBlockHeader.Time = uint64(time.Now().Unix())
   err := blockchain.InsertBlock(currentBlock)
   assert.Nil(t, err)
   adaptor.Initial(blockchain, currentBlockHeader)
+  now := time.Now()
+  time.Sleep(now.Truncate(time.Second).Add(time.Second).Sub(now))
+  currentBlockHeader.Time = uint64(time.Now().Unix())

   // Less then Mine Period
   isYourTurn, err := adaptor.YourTurn(...)
   assert.False(t, isYourTurn)

YourTurn computes waitedTime := time.Now().Unix() - int64(parent.Time) and returns false only while waitedTime < minePeriod. The package helper shortenV2Timings shrinks round 0's MinePeriod to 1 second. Because parent.Time was stamped before InsertBlock/Initial, any I/O delay crossing a second boundary made waitedTime count as 1, so YourTurn skipped the mine-period guard and reached the round-leader check, returning true where the first assert expects false.

The timestamp is now stamped after all I/O and aligned to a fresh wall-clock second, leaving a whole second of slack. currentBlockHeader is a CopyHeader of the block; the block inserted into the chain keeps its original timestamp, so chain state and parent.Hash() are unchanged. leaderIndex depends only on round, epoch and masternode count, so every later assertion keeps the same expectation.

Evidence

  • Before: CI run 42948682501, job 130287604486 (race (engine_v2 5/5)) — --- FAIL: TestIsYourTurnConsensusV2 (4.25s), authorised_masternode_test.go:55: Should be false. Its log shows [yourturn] Yes, it's my turn based on parent block round=1 ... ParentBlockNumber=901, i.e. the mine-period guard was skipped. That shard repeatedly logged Failed to start filesystem watcher err="too many open files", which slows block insertion.
  • Reproduction: in a scratch worktree, injecting a 1.1s delay after the timestamp assignment reproduces the same failure; the unmodified tree passes go test -race -count=50 -run 'TestIsYourTurnConsensusV2$' (137.8s).
  • After: go test -race -count=30 -run 'TestIsYourTurnConsensusV2$' ./consensus/tests/engine_v2_tests/ → ok ... 91.360s. With a 500ms delay inserted after the assignment, the unaligned version failed 3 of 6 runs while the aligned version passed 6 of 6.
  • Branch verification: per-commit detached worktree run of make all, make generate, make tidy, clean-tree gate, make lint, go vet ./..., make test — all passed.

Merge Danger

Door: two-way

Test-only change; reverting restores the previous timing behaviour.

Blast Radius: test-only

Only TestIsYourTurnConsensusV2 is touched. No production code, no config, no API. The test gets up to one second slower per run.


Proposed changes

Types of changes

What types of changes does your code introduce to XDC network?
Put an ✅ in the boxes that apply

  • build: Changes that affect the build system or external dependencies
  • ci: Changes to CI configuration files and scripts
  • chore: Changes that don't change source code or tests
  • docs: Documentation only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that neither fixes a bug nor adds a feature
  • revert: Revert something
  • style: Changes that do not affect the meaning of the code
  • test: Adding missing tests or correcting existing tests

Impacted Components

Which parts of the codebase does this PR touch?
Put an ✅ in the boxes that apply

  • Consensus
  • Account
  • Network
  • Geth
  • Smart Contract
  • External components
  • Not sure (Please specify below)

Checklist

Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that

  • This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
  • Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
  • Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
  • Tested the backwards compatibility.
  • Tested with XDC nodes running this version co-exist with those running the previous version.
  • Relevant documentation has been updated as part of this PR
  • N/A

Not-checked items: this PR only reorders a timestamp assignment and adds a sub-second alignment inside an existing consensus unit test. It changes no node runtime, consensus rule, config, API or persisted data, so a private-network run from genesis, a devnet/testnet end-to-end plan, a backwards-compatibility test, mixed-version node coexistence and documentation updates are not applicable. Coverage is the corrected test itself, exercised 30 times under -race plus the full make test suite.

@coderabbitai

coderabbitai Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 4001230 and 23f6833.

Files selected for processing: 1

  • consensus/tests/engine_v2_tests/authorised_masternode_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@gzliudan
gzliudan force-pushed the fix-yourturn-timing-flake branch 2 times, most recently from 5774f3b to 253e62a Compare November 12, 2026 12:03
The test assigns parent.Time before InsertBlock and Initial, while YourTurn
compares waitedTime against a one-second mine period at second granularity.
Any delay crossing a second boundary makes waitedTime >= minePeriod, so
YourTurn reaches the round-leader check and returns true where the first
assert expects false (CI run 42948682501, job 130287604486).

Move the assignment after InsertBlock/Initial and align to a fresh
wall-clock second, leaving a full second of slack before YourTurn.
@gzliudan
gzliudan force-pushed the fix-yourturn-timing-flake branch from 253e62a to 23f6833 Compare November 12, 2026 23:50

This branch has not been deployed

No deployments
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.

2 participants