test(consensus,core): cut engine v2 test wall time from 219s to 67s by gzliudan · Pull Request #2944 · XinFinOrg/XDPoSChain

gzliudan · 2026-10-29T09:21:25Z

Summary

consensus/tests/engine_v2_tests/
├── helper.go
│   + shortenV2Timings()   countdown 4/4/5s -> 1/1/2s, mine 2/3s -> 1/2s (private clone)
│   + expectVote()         skip countdown chatter while a test waits for a vote
│   + expectNoVote()       fail only on *types.Vote, drain a parked sender at close
│   + drainBroadcast()     drop broadcasts emitted before the case starts
├── 20 *_test.go
│   + t.Parallel()         99 of 108 tests; 9 stay serial (reasons below)
└── (the reward-halving rewrite moved out: it lands as #2949)
core/blockchain.go
    lastWrite   package-level var -> BlockChain field (upstream geth: bc.lastWrite)
core/vm/privacy/
    + //go:build privacytests   ringct_test.go also requires cgo

Full-run time went to four places; three are addressed here:

Bucket	Time	Fix
Fixture building (~83k blocks at 0.9ms)	~72s	overlapped by `t.Parallel()`
Waiting on real countdown/mine periods	~70s	shortened test-only timing knobs
Fixed sleeps / negative windows	~40s	synchronous hygiene passes no longer slept on; windows cut to one period
`TestRewardHalvingVanishing` (11M epochs)	23.7s	not here - lands as #2949

Nine tests stay serial: two count timeout/sync-info messages across consecutive periods, two read the global skipped-proposed-block counter as a delta, two install a global logger, and three keep their skip-path siblings company in proposed_block_test.go.

The -race soak surfaced a second, pre-existing race on dev-upgrade: lastWrite was a package-level variable, so every chain in a process shared one flush high-water mark (writeBlockWithState). It is now a BlockChain field, matching upstream geth.

core/vm/privacy leaves the default suite: both precompiles are unreachable (#2895), the package is scheduled for deletion, and its 27 crypto cases cost 4s of make test and 328s of -race for code nothing reaches. The tests now sit behind -tags=privacytests; the package still builds because core/vm imports it. #2898 deletes the package outright - whichever lands second takes the other's hunk.

Evidence

Whole-repo make test (go test -timeout=45m -tags=integrationtests -p 1 ./...), test cache cleared before each run, 12 cores, both runs green:

	dev-upgrade `bb6eaace60`	this PR `d683aee2ec`
`make test`	661.2s	436.8s (-224.5s, -34%)
`consensus/tests/engine_v2_tests`	236.7s	64.7s (-172.0s, -73%)
`core/vm/privacy`	4.0s	no test files (-4.0s)

Packages run one at a time (-p 1), so those two rows account for -176s of the -224.5s. The rest is run-to-run spread - the other packages moved by ~20s in total, and build/per-package overhead accounts for the remainder. Read the total as roughly a three-minute cut.

Package consensus/tests/engine_v2_tests, -count=1, 12 cores:

	dev-upgrade	this PR
full	221.2s (236.7s on a loaded box)	67s (65-86s across runs)
`-short`	12.1s, 56 tests run	12.8s, same 56 tests
`-race -short`	40.2s	19-20s, 0 data races (3 runs)
`-race` full (not a CI lane)	-	538s, 0 data races

The -short lane no longer gains much: the forensics test's 5s sleep is back (see Merge Danger).

Also verified: 3 consecutive full runs, make all, golangci-lint over the touched packages (0 issues), go test ./core/ -run TestExecutedMarker.
go test ./core/vm/privacy reports [no test files]; go test -tags=privacytests ./core/vm/privacy runs all 27 in 3.9s.

Merge Danger

Door: two-way. Test-only changes plus one struct field; a revert is a clean revert.

Blast Radius: narrow. The tests live in one package. The lastWrite move changes flush throttling only for a process that hosts more than one BlockChain (tests, tooling) - a single-chain node behaves exactly as before, and the field now matches upstream geth.

Dependency: #2949 carries the reward-halving quarter of the original speedup (23.7s). #2946 - the Forensics.SetCommittedQCs serialisation tracked in #2945 - has landed in the base, and the test now reads through GetCommittedQCs() under the same lock, so its 5s sleep is a timing buffer rather than a race guard. This PR still leaves it at 5s because polling it was reverted while the race was still open; #2947 can turn it into a poll.

A flake the shortened periods exposed, and a hole it hid

TestTimeoutPeriodAndThreadholdConfigChange demanded exactly six timeouts and two syncInfos across eight broadcasts. That split is not something the engine guarantees - it depends on where the parameter update lands inside a countdown period - and it stopped holding once this PR cut the periods to one and two seconds.

Reproduced on this branch in the runner's shape (four cores pinned and saturated, -race, GOMAXPROCS=4): two of three runs failed with expected 6, actual 5. The failing runs carry no dropped message and no failed signature - zero authentication needed lines, zero unknown message types, the same nine timeout events. Only the mix moved.

The test now reads with a deadline (a message per period, ten seconds of slack before it calls a broadcast dropped) and asserts what the update is actually for: the engine's own current config has to be the round-900 one. Stubbing UpdateParams out drops the old split to five/three - exactly the bracket the relaxed reads would have accepted - so the config assertion is what still catches it. The engine keeps its own copy of the chain config, so GetCurrentConfigFaker() was added next to the other test fakers.

coderabbitai · 2026-10-29T09:21:33Z

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: 3ac7bd99-d564-4b8b-b661-5f6d50fceb15

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

Switching the race job to the full suite pushed race (bulk 1/4) past the 12-minute guard (run 40861875078): it was killed at 12m23s with engine_v2_tests.test still running as an orphan. The slice is the expensive one by construction - core, core/vm/privacy, consensus/tests/engine_v2_tests and common/countdown all land in it (401s of package time on twelve cores, against 112-241s for the other three). On this base, engine_v2_tests still carries the old 11M-epoch TestRewardHalvingVanishing: reproduced locally with GOMAXPROCS=4, the package hits go test's own 10-minute timeout with "running tests: TestRewardHalvingVanishing (5m35s)". XinFinOrg#2944 rewrites that test to a binary search plus a 9k-epoch sweep and makes the package's tests run in parallel, which takes engine_v2_tests from 86s to 68s under the detector and removes the five-minute serial block. Until it lands and this branch is rebased, 12 minutes is not a hang guard but a budget, so raise it to 20 and leave the note in place.

race (bulk 1/4) was killed at 12m23s (run 40861875078). That slice carried the tree's three slowest packages - core (125s), core/vm/privacy (97s) and consensus/tests/engine_v2_tests (86s) - on top of everything else, and -p 4 lets each of them starve the others; on this base engine_v2_tests alone hits go test's own 10-minute timeout with TestRewardHalvingVanishing (5m35s). Carve them out the way rpc was already carved out. RACE_ALONE lists the packages that get a lane each and RACE_PKGS excludes them, so the four bulk slices drop from 401/177/241/112s of package time to 124/58/232/210s (the largest slice goes from 401s to 232s). race-test-rpc / race-test-full-rpc become race-test-alone / race-test-full-alone, parameterised by RACE_ALONE, and the matrix grows a kind: pkg lane per package with its own cache key and watchdog. The engine_v2 lane is still about ten minutes on this base; XinFinOrg#2944 takes the package from 219s to 35s and the lane to a couple of minutes.

lastWrite was a package-level variable, so every BlockChain in a process shared one flush high-water mark. That is wrong whenever more than one chain is live in the same process - the test fixtures build one chain per test - and it is a plain data race once those chains insert blocks concurrently, which `go test -race` reports at writeBlockWithState. Move it onto BlockChain, matching upstream geth (`bc.lastWrite`). The flush throttle now compares against the high-water mark of the chain it is flushing, which is what the surrounding code already assumed (bc.gcproc, bc.cachedb, bc.triedb are all per-chain). The executed-marker fixtures used to save and restore the package variable. With the field there is nothing to restore - each case builds and drops its own chain - so those save/restore pairs, and the comments explaining them, are gone. The one pin a fixture still needs (lastWrite = TriesInMemory) stays, now on the chain that pins it.

`go test ./consensus/tests/engine_v2_tests` was the slowest package in the tree at 219s; everything else under consensus finishes in under two seconds. The time went to building fixtures (~72s for ~83k blocks at ~0.9ms each), waiting on real countdown and mine periods (~70s), and fixed sleeps (~40s). A fourth quarter - 24s walking 11M epochs of big.Float reward math - is not touched here; it lands separately as XinFinOrg#2949. - Timing knobs: the fixture now deep-clones the chain config and shortens the V2 periods (countdown 4/4/5s -> 1/1/2s, mine 2/3s -> 1/2s). Round 10 stays strictly above round 0 because TestIsYourTurnConsensusV2CrossConfig needs a larger post-switch period. Assertions read the fixture's own config instead of the package-level one. - Waits: the two pool-hygiene tests slept 5s each for a hygiene pass that is synchronous, and those sleeps are gone. The forensics test keeps its 5s sleep: the goroutine it waits for writes without synchronisation (XinFinOrg#2945), so polling would trade a hidden race for a reported one rather than removing either - XinFinOrg#2946 fixes the write itself. Negative windows shrink to one countdown period. - Reads: at one-second periods the countdown keeps broadcasting while a test waits for a vote, so the reads that follow a negative window - and every read whose message is then used as a vote - skip timeout and sync-info broadcasts, and expectNoVote drains whatever sender is already parked on the unbuffered channel when its window closes. Without that, a stale timeout reached a vote type assertion and panicked. - Parallelism: 99 of the 108 tests call t.Parallel(). Nine stay serial: two count messages across consecutive periods, two read the global skipped-proposed-block counter, two install a global logger, and three keep their skip-path siblings in proposed_block_test.go company. Verified: 219s -> ~67s on a 12-core box (the reward-halving quarter is the rest, see XinFinOrg#2949), three back-to-back runs and two -race runs clean (the -race soak is what surfaced the lastWrite race fixed in the previous commit).

TestTimeoutPeriodAndThreadholdConfigChange read three broadcasts, triggered a parameter update, read five more, and demanded exactly six timeouts and two syncInfos. That split is not a property of the engine: it depends on where the update lands inside a countdown period. It held at the old two-second period and stopped holding once this branch shortened the periods to one and two seconds - the same countdown produced five timeouts and three syncInfos, with no dropped message and no failed signature (zero "authentication needed" lines, zero unknown message types, the same nine timeouts and four threshold events). Read with a deadline instead. Each read must produce a message within messageTimeout (ten seconds, several periods), so a dropped broadcast still fails and names the counters, and the five reads after the update stay bounded by twenty seconds. What the update itself must do is now asserted where it is observable: the engine's own current config has to be the round-900 one (threshold 4, two-second period). The engine keeps its own copy of the chain config, so a GetCurrentConfigFaker() accessor sits next to the other test fakers. That is the assertion which catches an UpdateParams that stops applying the config - stubbing it out drops the old six/two split to five/three, exactly the bracket this rewrite replaces, and the relaxed reads alone would not notice. Verified on this branch with the runner's shape (four cores pinned and saturated, -race, GOMAXPROCS=4): before, two of three runs failed with "expected 6, actual 5"; after, ten of ten pass. Dropping a message still fails: draining the channel and reading past the deadline reports "no message in 10s (timeout=0 syncinfo=0)".

The privacy precompiles are unreachable - RING_SIG_VERIFY and BULLET_PROOF_VERIFY have never been called on mainnet or Apothem (XinFinOrg#2895) - and the package is scheduled for deletion, so its 27 crypto cases cost the default suite four seconds and the detector 328s for code nothing reaches. XinFinOrg#2912 drops the last CI job that ran them. They are now behind -tags=privacytests, so `go test ./...` reports the package as having no tests, and the cases stay runnable on demand: go test -tags=privacytests ./core/vm/privacy The package still builds, because core/vm imports it for the two precompiles.

The shared gate covers 52 of the package's 109 tests, but the gate exists for cost, and most of those tests are cheap now that the fixture rework (XinFinOrg#2944) shortens the V2 periods and lets the suite run its tests in parallel: 42 of the 52 cost under 4s, the cheapest 0.14s. Cut the carve-out at 4s, using the median per-test elapsed of three full runs of the package with -parallel 1. A -parallel 1 run is what a test costs on its own; the default parallel run charges it the contention of its neighbours, which is not what the gate is for. The rework is why the figures move at all: this branch was cut when the package ran one test at a time, and XinFinOrg#2944 landed in between. 11 tests keep the gate, 4.14s to 23.56s 42 tests lose it, 0.14s to 3.00s One test that dev-upgrade leaves ungated is over the line on this tree - TestProcessQcShallSetForensicsCommittedQc, 5.14s - so it gains the gate. -short runs 98 of the 109 tests and grows from 13.2s to 20.2s; the full run is unchanged. The 7s buys -short and -race -short coverage of 42 tests, and it is cheap because their fixtures overlap: on the pre-XinFinOrg#2944 tree the same 42 tests would have cost about 32s one at a time.

The shared gate covers 52 of the package's 109 tests, but the gate exists for cost, and most of those tests are cheap now that the fixture rework (XinFinOrg#2944) shortens the V2 periods and lets the suite run its tests in parallel: 42 of the 52 cost under 4s, the cheapest 0.14s. Cut the carve-out at 4s, using the median per-test elapsed of three full runs of the package with -parallel 1. That is what a test costs on its own; the default parallel run charges it the contention of its neighbours, which is not what the gate is for. The rework is also why the figures move: this branch was cut when the package ran one test at a time, and XinFinOrg#2944 landed in between. 11 tests keep the gate, 4.14s to 23.82s 42 tests lose it, 0.14s to 2.90s One test that dev-upgrade leaves ungated is over the line on this tree - TestProcessQcShallSetForensicsCommittedQc, 5.14s - so it gains the gate. -short runs 98 of the 109 tests and grows from 12.9s to 21.7s; the full run is unchanged. The 8.8s buys -short and -race -short coverage of 42 tests, and it is cheap because their fixtures overlap: on the pre-XinFinOrg#2944 tree the same 42 tests would have cost about 32s one at a time.

The shared gate covers 52 of the package's 109 tests, but the gate exists for cost, and most of those tests are cheap now that the fixture rework (XinFinOrg#2944) shortens the V2 periods and lets the suite run its tests in parallel: 42 of the 52 cost under 4s, the cheapest 0.14s. Cut the carve-out at 4s, using the median per-test elapsed of three full runs of the package with -parallel 1. That is what a test costs on its own; the default parallel run charges it the contention of its neighbours, which is not what the gate is for. The rework is also why the figures move: this branch was cut when the package ran one test at a time, and XinFinOrg#2944 landed in between. 10 tests keep the gate, 4.14s to 23.82s 42 tests lose it, 0.14s to 2.90s One test that dev-upgrade leaves ungated is over the line on this tree - TestProcessQcShallSetForensicsCommittedQc, 5.14s - and stays ungated here. Its cost is a literal five-second sleep rather than work; XinFinOrg#2984 replaces the sleep with a poll, which returns in 0.18s. Gating it would hide that fix and keep the test out of -race -short until it landed. -short runs 99 of the 109 tests and grows from 12.9s to 22.6s; the full run is unchanged. The 9.7s buys -short and -race -short coverage of 42 tests, and it is cheap because their fixtures overlap: on the pre-XinFinOrg#2944 tree the same 42 tests would have cost about 32s one at a time. The forensics sleep adds about a second of the rest, since it overlaps too.

…ng it TestProcessQcShallSetForensicsCommittedQc slept five seconds and only then asserted that three committed QCs had been recorded. XinFinOrg#2947 gates it at 4s for that cost, which is fair: the sleep is five seconds of nothing. The sleep was there because the forensics record had no lock. Polling it read the field while the committer goroutine wrote it, so the test would have traded a hidden race for a reported one rather than removing either - that is why XinFinOrg#2944 kept the sleep when it cut the package's other fixed waits. XinFinOrg#2946 then put the read and the write under the same mutex, and GetCommittedQCs returns a copy, so the poll is safe now: TestProcessQcShallSetForensicsCommittedQc 5.17s -> 0.176s The five seconds survive as a deadline, so the test keeps its teeth: dropping the vote that triggers the commit makes it fail after the same five seconds with "expected: 3, actual: 0". At 0.176s it is no longer a 4s test, so XinFinOrg#2947's gate comes off with it. -short runs it again, and so does -race -short.

* test(common,consensus,core,p2p,internal): share the -short skip skipLongInShortMode was copied into three packages: a byte-identical short_mode_test.go in p2p/discover, a definition inside common/countdown's test file, and consensus/tests/engine_v2_tests/helper.go. Go cannot import a symbol from another package's _test.go, which is why the copies grew - and the gates this series adds would each have brought another one. Move the five-line helper to internal/testshort and call testshort.Skip at the 82 call sites: 76 tests through testshort.Skip(t) and the six benchmark guards in core/bench_test.go through testshort.Skip(b), which is why Skip takes testing.TB rather than *testing.T. The skip string and the t.Helper() marking are unchanged, so a skipped test still reports the line that asked to be skipped; the benchmarks give up their own "Skipping in short-mode" wording for the shared one. No behaviour change: -short skips exactly the same tests and benchmarks, and the full runs are untouched. * test(p2p,accounts,rpc,ethclient,metrics,core,eth): gate tests of 4s+ behind -short Across the tree `go test ./... -count=1 -json -p 12` reports 7066 test entries, 76 of them at 4s or more. 22 had no short-mode carve-out; the rest already used the shared helper or testing.Short(). Add the carve-out where it was missing, through testshort.Skip: - p2p/discover: 10 tests, each waiting ~4s for a timeout - rpc: 3 teardown/unsubscribe tests (4-10s) - accounts/abi/abigen, ethclient/simulated, metrics, core/txpool/legacypool, eth: one each (5-12s) Four candidates were dropped after measuring them alone: their batch figures (4.6-4.8s) are contention, not cost (TestChainIndexerSingle: 4.79s in the batch, 1.47s alone), so they keep running in -short. No gated test turned out to be under 4s, so nothing lost its carve-out. Note the CI race job runs -race -short: these 18 tests no longer run there. * test(consensus): skip only tests at 4s or more in -short mode The shared gate covers 52 of the package's 109 tests, but the gate exists for cost, and most of those tests are cheap now that the fixture rework (#2944) shortens the V2 periods and lets the suite run its tests in parallel: 42 of the 52 cost under 4s, the cheapest 0.14s. Cut the carve-out at 4s, using the median per-test elapsed of three full runs of the package with -parallel 1. That is what a test costs on its own; the default parallel run charges it the contention of its neighbours, which is not what the gate is for. The rework is also why the figures move: this branch was cut when the package ran one test at a time, and #2944 landed in between. 11 tests keep the gate, 4.14s to 23.82s 42 tests lose it, 0.14s to 2.90s One test that dev-upgrade leaves ungated is over the line on this tree - TestProcessQcShallSetForensicsCommittedQc, 5.14s - so it gains the gate. -short runs 98 of the 109 tests and grows from 12.9s to 21.7s; the full run is unchanged. The 8.8s buys -short and -race -short coverage of 42 tests, and it is cheap because their fixtures overlap: on the pre-#2944 tree the same 42 tests would have cost about 32s one at a time.

…ng it TestProcessQcShallSetForensicsCommittedQc slept five seconds and only then asserted that three committed QCs had been recorded. XinFinOrg#2947 gates it at 4s for that cost, which is fair: the sleep is five seconds of nothing. The sleep was there because the forensics record had no lock. Polling it read the field while the committer goroutine wrote it, so the test would have traded a hidden race for a reported one rather than removing either - that is why XinFinOrg#2944 kept the sleep when it cut the package's other fixed waits. XinFinOrg#2946 then put the read and the write under the same mutex, and GetCommittedQCs returns a copy, so the poll is safe now: TestProcessQcShallSetForensicsCommittedQc 5.17s -> 0.176s The five seconds survive as a deadline, so the test keeps its teeth: dropping the vote that triggers the commit makes it fail after the same five seconds with "expected: 3, actual: 0". At 0.176s it is no longer a 4s test, so XinFinOrg#2947's gate comes off with it. -short runs it again, and so does -race -short.

gzliudan requested review from AnilChinchawale, anunay-xin, benjamin202410, liam-lai, wanwiset25 and wgr523 October 29, 2026 09:22

gzliudan force-pushed the test-speed-engine-v2 branch 4 times, most recently from c3563c8 to 089d683 Compare October 29, 2026 10:10

gzliudan changed the title ~~test(consensus,core): cut engine v2 test wall time from 219s to 36s~~ test(consensus,core): cut engine v2 test wall time from 219s to 35s Oct 29, 2026

gzliudan force-pushed the test-speed-engine-v2 branch 2 times, most recently from 825c9ed to 0fb656c Compare October 29, 2026 10:15

gzliudan mentioned this pull request Oct 29, 2026

ci: split engine_v2_tests out of the c[n-o] test job #2902

Merged

1 of 2 tasks

gzliudan mentioned this pull request Oct 29, 2026

ci: rebalance the test shards and run the race job in full #2912

Open

gzliudan force-pushed the test-speed-engine-v2 branch from 0fb656c to eabb1de Compare October 29, 2026 12:06

gzliudan mentioned this pull request Oct 29, 2026

test: cut the -short carve-out at 4s #2947

Merged

wanwiset25 approved these changes Oct 29, 2026

View reviewed changes

gzliudan force-pushed the test-speed-engine-v2 branch 4 times, most recently from 7960da7 to 2c21c85 Compare October 30, 2026 10:24

anunay-xin approved these changes Oct 30, 2026

View reviewed changes

gzliudan changed the title ~~test(consensus,core): cut engine v2 test wall time from 219s to 35s~~ test(consensus,core): cut engine v2 test wall time from 219s to 67s Nov 1, 2026

gzliudan force-pushed the test-speed-engine-v2 branch from 85b43ce to 1e09e1e Compare November 1, 2026 10:07

benjamin202410 approved these changes Nov 2, 2026

View reviewed changes

gzliudan added 3 commits November 3, 2026 04:22

gzliudan force-pushed the test-speed-engine-v2 branch from 1e09e1e to d683aee Compare November 2, 2026 20:24

gzliudan merged commit d381395 into XinFinOrg:dev-upgrade Nov 2, 2026
25 checks passed

gzliudan deleted the test-speed-engine-v2 branch November 2, 2026 21:01

gzliudan mentioned this pull request Nov 2, 2026

test(consensus): poll for the forensics committed QCs instead of sleeping #2984

Open

Navigation Menu

Repository navigation

test(consensus,core): cut engine v2 test wall time from 219s to 67s - #2944

test(consensus,core): cut engine v2 test wall time from 219s to 67s#2944
gzliudan merged 4 commits into
XinFinOrg:dev-upgradefrom
gzliudan:test-speed-engine-v2

gzliudan commented Oct 29, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 29, 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 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Evidence

Merge Danger

A flake the shortened periods exposed, and a hole it hid

Uh oh!

coderabbitai Bot commented Oct 29, 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 29, 2026 •

edited

Loading

coderabbitai Bot commented Oct 29, 2026 •

edited

Loading