Repository navigation
fix(common/countdown,params): correct the exponential timeout arithmetic and validate its bounds - #2836
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
c82852f to
825c714
Compare
10f450e to
8561cbe
Compare
22d66c0 to
131cc18
Compare
131cc18 to
bcba4bb
Compare
…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
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 (XinFinOrg#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.
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.
bcba4bb to
0063916
Compare
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the timeout arithmetic and duration-bound validation changes.
No open issues 🎉
What changed in this PR
Fixes XDPoS v2 exponential timeout arithmetic and rejects configurations whose maximum timeout cannot fit in time.Duration. This addresses #2835 while preserving the locking introduced in #2831; built-in network configurations remain unaffected.
Changes:
- Compute round differences without
uint8truncation and saturate overflowing timeout products. - Validate period conversion and maximum exponential timeout bounds at startup.
- Add regression tests for wraparound and overflow boundaries, and correct the documented timeout unit.
| File | Description |
|---|---|
params/config_xdpos.go |
Corrects the TimeoutPeriod comment from milliseconds to seconds. |
params/config_test.go |
Tests duration bounds for current and scheduled configurations. |
params/config_backfill.go |
Rejects timeout configurations that exceed duration limits. |
common/countdown/exp_duration.go |
Fixes exponent truncation and saturates overflowing products. |
common/countdown/exp_duration_test.go |
Adds round-difference and overflow-boundary regression tests. |
Review effort: Balanced
Summary
Three defects in the XDPoS v2 exponential timeout. All of them are dormant on every network shipped in this repository (
base=1, maxExponent=0), and all are covered by unit tests.ExpTimeoutDuration.GetTimeoutDurationderived the exponent of the exponential timeout from auint8conversion of the round difference, which truncates it modulo 256 (types.Roundisuint64). For everyroundDiffof 257 or more whose low 8 bits fall in[1, maxExponent]the exponent came out too small; atroundDiff = 257it became 0 and the timeout collapsed toduration(the minimum) instead of saturating atmaxExponent. The window recurs every 256 rounds.The same expression multiplied
duration * base^expinint64without a range check, so a large base overflowed and wrapped around to a negative timeout — which masked the truncation above once the difference outgrew the int64 budget, and would have retriggered the timer immediately.validateV2ExpTimeoutConfigboundedmax_exponentandbase^max_exponentbut never checked that the resulting timeout fits in atime.Duration. The engine converts the period withtime.Duration(TimeoutPeriod) * time.Secondandcommon/countdownthen multiplies bybase^exp; when either step overflows, the timer saturates atmath.MaxInt64(~292 years) and never fires, so the pacemaker silently stops.Changes
uint64and clamp the exponent afterwards;math.MaxInt64instead of wrapping around;base^max_exponent— and reject an unrepresentable config at startup;TimeoutPeriodcomment, which claimed milliseconds while every consumer multiplies bytime.Second.Reachability today
All built-in configurations (
params/config_networks.go) useExpTimeoutConfig{Base: 1.0, MaxExponent: 0}, so the exponent is always 0 and none of the defects can manifest on mainnet, testnet or devnet. They become observable only on a custom chain that enables the exponential backoff (Base >= 2,MaxExponent >= 1).roundDiffonly grows while the engine advances rounds through timeout certificates without applying a newer QC (for example whenprocessQCaborts with "Block not found using the QC"), so reaching 257 requires a long stretch of that failure mode.The config check only affects a custom genesis whose own maximum exponential timeout does not fit in a
time.Duration; such a chain used to start and then stall silently, and now fails at startup.Tests
TestExpDurationRoundDiff: a table of(maxExponent, roundDiff)pairs overmaxExponent = 0, 3, 31, withroundDiffspanning below and beyond the uint8 wrap point (1, 2, 4, 5, 32, 256, 257, 258, 259, 513), asserting the intended saturated value everywhere. It fails against the previous implementation wherever a round difference of 257 or more has low 8 bits in[1, maxExponent](e.g. themaxExponent = 3rows atroundDiff = 257, 258, 259, 513) and passes now.TestExpDurationOverflowSaturates:59s * 2^27stays exact,59s * 2^28and beyond saturate atmath.MaxInt64(previously negative), including theroundDiff = 257case where both defects combine. At the exact int64 boundary (MaxInt64/3period multiplier) the product stays exact instead of saturating.TestValidateV2ExpTimeoutConfigDurationRange: walks the productionCheckConfigForkOrder()path and asserts both field paths (XDPoS.V2.CurrentConfig.ExpTimeoutConfigandXDPoS.V2.AllConfigs[10].ExpTimeoutConfig); it covers the shrinking-base case (a combined check used to let an overflowing period through whenbase < 1), the exact seconds boundary (9,223,372,036 s fits, 9,223,372,037 s does not), the exact product boundary (3 s * 3074457345fits) and theAllConfigsloop.Verification:
make all,gofmt/goimports,golangci-lint run --new-from-rev=eae4c6a7f4 ./common/countdown/... ./params/...(0 issues),go test -count=1 ./common/countdown/... ./params/... ./core/... ./consensus/...(all pass).No private-network run: the fix is dormant for every built-in configuration (see Reachability above), and the defects are fully covered by the unit tests.
Upstream
go-ethereumhas nocommon/countdownpackage and no equivalent chain-config check (git grepandgit log -Sagainstorigin/masterat8817f99f9both return nothing), so there is no upstream fix to port. The closest 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 branch rejects the config at startup and saturates the runtime value as a backstop.Merge order
#2831 (
fix-countdown-race) is already merged intodev-upgrade; this branch is based on top of it and keeps the locked parameter snapshot together with theuint64/saturation arithmetic.close #2835