fix(common/countdown): fix data race between SetParams and GetTimeoutDuration by gzliudan · Pull Request #2831 · XinFinOrg/XDPoSChain

gzliudan · 2026-10-20T09:29:42Z

Proposed changes

ExpTimeoutDuration in common/countdown stored its three parameters (duration, base, maxExponent) without synchronisation while the countdown goroutine read them through GetTimeoutDuration. In production the two sides run concurrently: the checkpoint loop calls engine.UpdateParams (cmd/XDC/main.go:389) → XDPoS_v2.UpdateParams (consensus/XDPoS/engines/engine_v2/engine.go:167) → SetParams, while the timer goroutine reads the same fields in startTimer (common/countdown/countdown.go:67/87/91).

SetParams wrote the three fields one by one and rolled them back one by one when the sanity check failed, so a concurrent reader could observe a mix of the old and the new set and compute a timeout that belongs to neither configuration. That timeout drives XDPoS v2 round progression, so a wrong value makes a node time out earlier or later than the rest of the network.

This PR guards the three fields with a sync.RWMutex:

GetTimeoutDuration takes the read lock and computes from a consistent snapshot of the three fields.
SetParams validates the candidate set first, then publishes all three fields at once under the write lock. An invalid set is never written, so the per-field rollback path is gone and a rejected set can no longer become partially visible.

The signatures are unchanged. NewExpTimeoutDuration now returns a nil helper together with the error instead of a helper holding the rejected values; its only caller (NewExpCountDown) already drops the helper on error.

The upstream go-ethereum has no countdown package, so there is no upstream fix to port.

Types of changes

Put an ✅ in the boxes that apply

Impacted Components

Which parts of the codebase does this PR touch?

Verification

make all
go run build/ci.go lint ./common/countdown/... → 0 issues
go test -race -count=1 ./common/countdown/... → ok
gofmt -l / goimports -l on the touched files → clean
Regression proof: the new TestExpTimeoutDurationConcurrentSetParams, run against the pre-fix exp_duration.go through go test -overlay, reports WARNING: DATA RACE — write at exp_duration.go:56 (SetParams) and read at exp_duration.go:48 (GetTimeoutDuration) — the same pair the issue reports. Against the fixed file the same test passes under -race.
Regression protection relies on -race: the CI test job runs go run build/ci.go test -coverage without -race, so a plain CI run would not catch this race. The race job comes from ci: run the short suite under the race detector, close #2700 #2797 (ci: run the short suite under the race detector); this test is not skipped in -short, so it is covered once that PR lands.
Engine-level proof on the original reproduction: go test -race -count=1 -run 'TestIsYourTurnConsensusV2CrossConfig|TestTimeoutPeriodAndThreadholdConfigChange' ./consensus/tests/engine_v2_tests/ passes on this branch (33.6s). With exp_duration.go replaced by the pre-fix version through -overlay, both tests fail with WARNING: DATA RACE: the write at exp_duration.go:56 through countdown.go:52 (engine.UpdateParams, engine.go:167) races the read at exp_duration.go:48 through countdown.go:67 and countdown.go:87.
Review follow-ups (squashed into this commit): the concurrent test comments now state that only some torn combinations produce an unrecognised value (a torn maxExponent alone is invisible with exp = min(1, maxExponent)), the rejected-set test covers both validation branches and makes all three parameters observable, the nil-on-error contract of NewExpTimeoutDuration is asserted, the pre-existing )= >= typo in both copies of the validation message is fixed, and TestExpDuration no longer discards SetParams errors.
Riding along as its own commit: CountdownTimer.isInitilised (common/countdown/countdown.go) now reads initilised under RLock instead of the write lock, matching the read path of ExpTimeoutDuration. No behaviour change: the field is only written by setInitilised, and neither helper is called while the other lock is held. This is pre-existing (blame 521b703207) and unrelated to the SetParams/GetTimeoutDuration race.

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. (Not applicable: the change is confined to the in-process timeout parameter of the countdown timer; the package test above covers both the read and the write path.)
Tested the backwards compatibility. (Not applicable: no wire format or state layout change; the accepted parameter set produces the same timeout values as before.)
Tested with XDC nodes running this version co-exist with those running the previous version. (Not applicable: same reason as above.)
Relevant documentation has been updated as part of this PR
N/A

Fixes #2828

coderabbitai · 2026-10-20T09:29:50Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 992956a9-0139-4c33-a58d-0c704b3c619b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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

_{Comment @coderabbitai help to get the list of available commands.}

…exponent The exponent was derived from a uint8 conversion of the round difference, which truncates it modulo 256. A difference of 257 therefore collapsed the timeout to its minimum instead of saturating at maxExponent, and the window recurs every 256 rounds. Compute the difference in uint64 instead. Also saturate the duration*base^exp product at the maximum time.Duration: with a large base it already overflowed int64 and wrapped around to a negative timeout, which masked the truncation above once the difference outgrew the int64 budget. Both defects are pre-existing since the package was introduced (XinFinOrg#764) and are independent of XinFinOrg#2831. close XinFinOrg#2835

…exponent The exponent was derived from a uint8 conversion of the round difference, which truncates it modulo 256. A difference of 257 therefore collapsed the timeout to its minimum instead of saturating at maxExponent, and the window recurs every 256 rounds. Compute the difference in uint64 instead. Also saturate the duration*base^exp product at the maximum time.Duration: with a large base the product already overflowed int64 and wrapped around to a negative timeout, so the timer retriggered immediately. This is a separate defect from the truncation above: truncation recurs every 256 rounds, while the overflow depends on the exponent, and each can occur without the other. Both defects are pre-existing since the package was introduced (XinFinOrg#764) and are independent of XinFinOrg#2831. close XinFinOrg#2835

…Duration ExpTimeoutDuration stored duration/base/maxExponent without synchronisation while the countdown goroutine read them, so a concurrent SetParams could be observed torn and a rejected parameter set could be partially visible. Guard the three fields with a sync.RWMutex: GetTimeoutDuration copies them under the read lock, SetParams validates the candidate set before taking the write lock and publishes all three at once. Rollback is no longer needed because an invalid set is never written. The signatures are unchanged. NewExpTimeoutDuration now returns a nil helper along with the error instead of a helper holding the rejected values; its only caller already drops the helper on error. Add a concurrent SetParams/GetTimeoutDuration test that reports the data race on the old code under -race, plus a test pinning that a rejected set leaves the timeout unchanged. The latter covers both rejection branches and keeps all three parameters observable, so a partial write cannot slip through. Also correct the pre-existing ")= >=" typo in the validation message now owned by checkExpParams, and assert the previously discarded SetParams errors in TestExpDuration. Fixes XinFinOrg#2828

validateV2ExpTimeoutConfig printed "max_exponent (%d)= >= max_exponent_upperbound (%d)". Drop the stray "=" so the message matches the one emitted by checkExpParams in common/countdown for the same bound. Message text only; the validation behaviour is unchanged.

CountdownTimer.isInitilised took the write lock to read a single bool, while the read path of ExpTimeoutDuration takes the read lock. Use RLock for the read so both helpers in the package follow the same rule. No behaviour change: initilised is only written by setInitilised, and the two helpers are never called while the other's lock is held. Pre-existing (blame 521b703), not introduced by the SetParams and GetTimeoutDuration race fix.

…exponent The exponent was derived from a uint8 conversion of the round difference, which truncates it modulo 256. A difference of 257 therefore collapsed the timeout to its minimum instead of saturating at maxExponent, and the window recurs every 256 rounds. Compute the difference in uint64 instead. Also saturate the duration*base^exp product at the maximum time.Duration: with a large base the product already overflowed int64 and wrapped around to a negative timeout, so the timer retriggered immediately. This is a separate defect from the truncation above: truncation recurs every 256 rounds, while the overflow depends on the exponent, and each can occur without the other. Both defects are pre-existing since the package was introduced (XinFinOrg#764) and are independent of XinFinOrg#2831. close XinFinOrg#2835

…exponent The exponent was derived from a uint8 conversion of the round difference, which truncates it modulo 256. A difference of 257 therefore collapsed the timeout to its minimum instead of saturating at maxExponent, and the window recurs every 256 rounds. Compute the difference in uint64 instead. This defect is pre-existing since the package was introduced (XinFinOrg#764) and is independent of XinFinOrg#2831. close XinFinOrg#2835

…tic and validate its bounds (#2836) * fix(common/countdown): keep the full round difference in the timeout exponent The exponent was derived from a uint8 conversion of the round difference, which truncates it modulo 256. A difference of 257 therefore collapsed the timeout to its minimum instead of saturating at maxExponent, and the window recurs every 256 rounds. Compute the difference in uint64 instead. This defect is pre-existing since the package was introduced (#764) and is independent of #2831. close #2835 * fix(common/countdown): saturate the timeout at the maximum time.Duration duration*base^exp can overflow int64 for a long round difference and a large base, wrapping around to a negative timeout; the timer then retriggers immediately. Saturate at math.MaxInt64 instead. This is a separate defect from the truncation in the previous commit: truncation recurs every 256 rounds, while the overflow depends on the exponent, and each can occur without the other. It is pre-existing since the package was introduced (#764). The saturated value never fires in practice; a chain whose own maximum exponential timeout is unrepresentable is rejected by the chain config validation at startup. * fix(params): reject XDPoS v2 exp timeouts that overflow time.Duration common/countdown arms the pacemaker timer with timeout_period * base^exp. The engine converts the period with time.Duration(TimeoutPeriod) * time.Second first, then common/countdown multiplies by base^exp; when either step leaves the int64 range the timer saturates at math.MaxInt64 (~292 years) and never fires, so the node silently stops producing timeout messages. validateV2ExpTimeoutConfig only bounded max_exponent and base^max_exponent, which admits such a genesis: with base=2, maxExponent=30 and the 30s mainnet timeout period the product already overflows from round difference 30 on. Bound both steps at the time.Duration range in whole seconds, so a chain whose own maximum exponential timeout is unrepresentable fails at startup instead of running until the round difference grows. The per-step bound matters: with base < 1, base^max_exponent shrinks the multiplier, so a single combined check would let an overflowing period through. No network shipped in this repository is affected: all 19 built-in V2Config entries and every genesis file use base=1 and maxExponent=0, where the product is just timeout_period seconds. Also correct the TimeoutPeriod comment, which claims milliseconds while every consumer multiplies by time.Second. go-ethereum has no common/countdown package and no equivalent chain-config check. The closest upstream precedent is geth ethereum#31407 (eth/catalyst: sanitize simulated beacon period to avoid overflowing time.Duration), which clamps the period with an integer bound instead; this commit rejects the config instead, because a saturated pacemaker stalls silently while a rejected config fails loudly at startup. Checking the largest exponent covers every smaller one only while base >= 1; with base < 1 the largest timeout is at exponent 0, which the period conversion check above bounds on its own.

gzliudan requested review from AnilChinchawale, anunay-xin, benjamin202410, liam-lai and wanwiset25 October 20, 2026 09:32

gzliudan force-pushed the fix-countdown-race branch from e19e2ba to d2cc2d1 Compare October 20, 2026 09:43

benjamin202410 approved these changes Oct 20, 2026

View reviewed changes

gzliudan force-pushed the fix-countdown-race branch 2 times, most recently from a637636 to d1b3fce Compare October 20, 2026 10:30

This was referenced Oct 20, 2026

common/countdown: GetTimeoutDuration truncates the round difference to uint8 and can collapse the timeout to its minimum #2835

Open

fix(common/countdown,params): correct the exponential timeout arithmetic and validate its bounds #2836

Merged

gzliudan force-pushed the fix-countdown-race branch from d1b3fce to 6fa4a4a Compare October 20, 2026 12:07

gzliudan force-pushed the fix-countdown-race branch from 6fa4a4a to 50987f5 Compare October 20, 2026 15:09

gzliudan force-pushed the fix-countdown-race branch from 50987f5 to 8636b24 Compare October 21, 2026 00:11

gzliudan force-pushed the fix-countdown-race branch from 8636b24 to 30c3b4e Compare October 22, 2026 09:05

gzliudan force-pushed the fix-countdown-race branch from 30c3b4e to 93b0673 Compare October 22, 2026 14:59

gzliudan mentioned this pull request Oct 24, 2026

common/countdown,params: deduplicate exp-timeout parameter validation #2874

Closed

wanwiset25 approved these changes Oct 26, 2026

View reviewed changes

gzliudan force-pushed the fix-countdown-race branch from 38720cf to 1069a32 Compare October 26, 2026 04:10

anunay-xin approved these changes Oct 26, 2026

View reviewed changes

gzliudan added 3 commits October 26, 2026 13:26

gzliudan force-pushed the fix-countdown-race branch from 1069a32 to a0c266e Compare October 26, 2026 05:27

gzliudan merged commit 61569b8 into XinFinOrg:dev-upgrade Oct 26, 2026
10 checks passed

gzliudan deleted the fix-countdown-race branch October 26, 2026 05:27

gzliudan mentioned this pull request Oct 26, 2026

common/countdown: ExpTimeoutDuration.SetParams races with the countdown goroutine #2828

Closed

benjamin202410 mentioned this pull request Oct 26, 2026

chore: merge dev-upgrade into main #2903

Merged

13 of 25 tasks

Navigation Menu

Repository navigation

fix(common/countdown): fix data race between SetParams and GetTimeoutDuration - #2831

fix(common/countdown): fix data race between SetParams and GetTimeoutDuration#2831
gzliudan merged 3 commits into
XinFinOrg:dev-upgradefrom
gzliudan:fix-countdown-race

gzliudan commented Oct 20, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 20, 2026 •

edited

Loading

Review skipped

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

gzliudan commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Impacted Components

Verification

Checklist

Uh oh!

coderabbitai Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gzliudan commented Oct 20, 2026 •

edited

Loading

coderabbitai Bot commented Oct 20, 2026 •

edited

Loading