Repository navigation
tests: follow the fork's epoch rules, fix the intermittent tests, run all of beacon-chain/blockchain in CI - #14
Merged
Merged
Conversation
…anicking DeepEqual built its diff with cmp.AllowUnexported(expected) and cmp.AllowUnexported(actual). AllowUnexported accepts struct values only and panics with "invalid struct type" on a slice, a map or a pointer, so a failed comparison of anything else did not report a difference: it ended the test binary, and the rest of the package never ran. Use an exporter that allows every type, which has no such restriction, and always add protocmp.Transform, which only acts on protobuf messages. Nested structs with unexported fields and protobuf messages inside slices now produce a diff as well. The new table case compares two slices that differ and panics without the fix.
TestProcessEpoch_CanProcess and TestProcessEpoch_CanProcessBellatrix expected the upstream balance of a validator that attested to nothing for an epoch during an inactivity leak. This fork does not take the inactivity penalty from the balance (ProcessRewardsAndPenaltiesPrecompute subtracts the source and target penalties only), so the balance is 31999841900 in both forks: base reward = 32 * (64e9 / isqrt(2048 * 32e9)) = 32 * 7905 = 252960 source penalty = 252960 * 14 / 64 = 55335 target penalty = 252960 * 26 / 64 = 102765 balance = 32e9 - 55335 - 102765 = 31999841900 Upstream is lower by the inactivity penalty for a score of 4: 635 in Altair (31999841265) and 1907 in Bellatrix (31999839993). The expectation is one constant with the derivation beside it, so the tests fail if the penalty rule changes.
TestService_HeadSyncCommitteeIndices and TestService_headNextSyncCommitteeIndices advance a genesis state over one to three sync committee periods of 256 epochs with no attestations and then ask for validator 0's positions. Upstream only drains the balance of a validator that does not attest. This fork exits it once its inactivity score reaches the ejection threshold, which under the default config is at epoch 61, so validator 0 has left the set (exit epoch 66) before the first period ends and is in no later committee. The first test then compared "no positions" with "no positions", and failed because the helper that looks the positions up returns a nil slice when it reads the state and an empty one when it reads its cache. The second test expects positions and got none. Both tests now run with 8 epochs per sync committee period, so the periods they look at end before any validator is exited, and the first test checks that the validator has positions at all. The skip slot cache is disabled for these two tests: it is keyed by the starting state and not by the config, and would hand a state advanced under the short period to the next test.
…ultiple The three attestations for slot 1 aggregate into two, and the pool returns its attestations in map order. The test sorted what it received by slot only, so the two attestations of slot 1 came out in either order and the comparison with the expected list failed on some runs (26 of 200 here, 19 of 200 on the upstream commit this fork is based on). Sort the expected and the received list by slot and then by aggregation bits, and compare their lengths first. This is the upstream fix from OffchainLabs/prysm#17571; the file is now the same as upstream's.
The simulated backend adds a transaction to its pool asynchronously, so right after one is sent PendingNonceAt can still return that transaction's nonce. A test that sends several deposits without committing a block in between then signs the next one with a nonce already in use, and the pool rejects it with "replacement transaction underpriced". That is the intermittent failure of TestProcessLogs_DepositRequestsStarted, and under load TestProcessETH2GenesisLog_CorrectNumOfDeposits fails the same way. The deposit contract is now bound to a client that remembers the next nonce of each sender and returns the higher of that and the pool's answer. This is the upstream fix from OffchainLabs/prysm#17571, taken unchanged. The package is a test helper: it is testonly in Bazel, only test files import it, and no binary links it.
…tionRequest The test table stamped the new registration with the current second when the table was built, and stamped the registration already in the cache with the current second minus one when the subtest ran. When a second boundary fell between the two readings both were the same second, and the check that the timestamp changed failed (13 of 3000 runs here; it also failed one full run of the CI test command). Read the clock once and derive both timestamps from that reading.
The two epoch tests, the two intermittent upstream tests and the sync committee tests are fixed, and a failed DeepEqual no longer takes a package down, so the skip list is back to the tests that read their data from a Bazel runfiles directory and the whole blockchain package runs with the others. testing/assertions joins the list because the fork now changes it.
… hashes requestBodiesByHash builds its list of hashes by ranging over a map, so the order of the hashes in the engine request changes from run to run, and it matches the bodies it gets back to the hashes by position, which is what an execution client does. The mock engine in two tests returned the bodies in a fixed order whatever was asked. With two hashes the map comes out in the other order one time in eight, the bodies were then matched to the wrong headers, and TestPayloadBodiesViaUnblinder failed with "payload ... and header ... roots do not match" (251 of 2000 runs here; it also failed one full run of the CI test command). The mock now looks the bodies up by the requested hashes. This is the upstream fix from OffchainLabs/prysm#17571, with the body type under the name it has in this tree.
The test asked GenerateFullBlock for one proposer slashing, one attester
slashing and one voluntary exit in the same block. The generator picks the
validator of each operation independently at random, out of 64 here. When the
exit falls on a slashed validator, or both slashings fall on the same one,
processing the block to compute its state root fails ("validator with index N
has already submitted an exit", "unable to slash any validator"), and the
test then dereferences the nil block and takes the package down. It failed 17
of 300 runs here and one full run of the CI test command.
The test now builds the three operations itself, with the helpers the
generator uses, for validators 1, 2 and 3. The generator is unchanged and can
still produce such a block for other callers.
…ationTasks
executePostFinalizationTasks starts one goroutine that sends the finalization
event and another that inserts the finalized deposits and logs "Finalized
deposit insertion completed". The test waited for the event and then looked
for the log line straight away, so it failed whenever the second goroutine
had not got that far yet ("Expected log not found"). It failed one full run
of the CI test command; with the insertion delayed by 200 ms it fails every
time.
The test now waits for the log line the same way it waits for the event,
with require.Eventually. executePostFinalizationTasks returns nothing to wait
on, so the log line is the only signal that the insertion has finished.
…ing it
MockStateNotifier records the events sent on its feed in a goroutine: it
receives from an unbuffered channel and then appends under a lock. A send
returns as soon as the receive has happened, which can be before the append,
so a test that reads ReceivedEvents() right after the call that sends the
event can still see none. Test_notifyNewHeadEvent failed that way on the
first Linux run of the check ("head_test.go:239 Values are not equal, want:
1, got: 0"). With the append delayed by 50 ms all three tests that read the
recorded events fail; without the delay, 40 concurrent copies at GOMAXPROCS=2
failed 27 of 1000 runs of Test_notifyNewHeadEvent and 3 of 1000 of
Test_sendNewFinalizedEvent.
Test_notifyNewHeadEvent now waits for the event with require.Eventually in
its three subtests, which is the upstream fix from OffchainLabs/prysm#17361.
Test_sendNewFinalizedEvent gets the same wait, and
Test_postBlockProcess_EventSending waits for the event it expects instead of
relying on its 10 ms pause; upstream still has those two as they were.
TestRunnerPushesProposerSettings_ValidContext checks that the mocked client calls get a live context. It counted any context error as a failure, including an exceeded deadline, and the deadline of those contexts is the end of the slot, which a loaded machine can pass during a call. The checks now fail on a canceled context only. This is the upstream change from OffchainLabs/prysm#17571, unchanged. It was not seen failing here; it is taken with the rest of that pull request's fixes for the packages the check runs.
setup-go restores the Go build cache, and that cache holds test results. On the second run of the check on the same commit, eight of the ten packages were reported as "ok (cached)" without running; only beacon-chain/execution and validator/client ran. A test that passes on one run and fails on the next cannot show up that way, and a re-run of a green check proves nothing. -count=1 turns the result cache off, so every run executes every test.
…cksParallel The test runs ten rounds on one service and replaces the service's forkchoice store after every round. Since forkchoice updates are sent in the background, postBlockProcess can leave a goroutine behind (forkchoiceUpdateWithExecution) that is still running when the round ends. It asks the service for its store once to lock it and once more to unlock it; when the test replaces the store in between, it locks the old store and unlocks the new one, which nobody holds, and the test binary dies with "fatal error: sync: Unlock of unlocked RWMutex". That ended the third run of the check on the previous commit. With a 20 ms pause between the two calls it happens within a few rounds. A running node sets the store once, when the service is built, so this cannot happen outside a test that swaps it. The service in this test now reports that it is not synced, which is the condition under which postBlockProcess sends no forkchoice update, so nothing outlives a round. The test predates the background update and does not look at it.
TestNoViableHead_Reboot and TestStore_NoViableHead_NewPayload import a chain of blocks through postBlockProcess with an engine that answers "syncing", and then replace the engine of the service with one that answers INVALID to NewPayload and, as it happens, valid to a forkchoice update. Since forkchoice updates are sent in the background, the update for the last imported block can still be waiting at that point. It is then answered by the new engine, the head chain is marked valid, and the INVALID payload that follows cannot invalidate it: validateExecutionOnBlock returns "invalid optimistic status" where the test expects "received an INVALID payload from execution engine". That failed the second run of the check on the previous commit. With the update delayed by 20 ms and the test paused after the engine swap it fails every time; TestNoViableHead_Reboot also replaces the forkchoice store, and with the delay alone it dies with "Unlock of unlocked RWMutex". Both tests now build their service as not synced, which is what they describe and is the condition under which postBlockProcess sends no forkchoice update, so nothing is left running when they replace the engine or the store. The answers of the first engine left the imported blocks optimistic either way. TestStore_NoViableHead_Liveness has the same shape and is skipped upstream; it is left as it is.
lightchainaidev
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
The check skipped by name two tests that fail on this fork and two that fail now and then, and ran only five tests of
beacon-chain/blockchain, because one failing comparison there panicked inside the assertion helper and ended the package's run.Nothing a beacon node or validator runs changes here: test files, the test-only deposit mock (
contracts/deposit/mock,testonlyin Bazel, imported by test files only), the assertion helper and the workflow.The tests in the ticket
TestProcessEpoch_CanProcess,TestProcessEpoch_CanProcessBellatrix. This fork does not take the inactivity penalty from the balance, so a validator that attested to nothing loses the source and target penalties only:That is the value both tests reported. Upstream is lower by the inactivity penalty for a score of 4: 635 in Altair (31999841265) and 1907 in Bellatrix (31999839993), the two old expectations. The tests now share one constant with this derivation beside it.
TestService_HeadSyncCommitteeIndices. The test advances a state over sync committee periods of 256 epochs with no attestations. This fork exits a validator whose inactivity score reaches the ejection threshold, at epoch 61 under the default config, so validator 0 has left before the first period ends and is in no later committee. The test then compared "no positions" with "no positions" and failed on a nil slice against an empty one. It andTestService_headNextSyncCommitteeIndicesnow run with 8 epochs per period, so the periods they look at end before anyone is exited, and the first asserts that the validator has positions at all.DeepEqualpanic.cmp.AllowUnexportedaccepts structs only and panics on a slice, a map or a pointer. The helper now uses an exporter that allows every type. A new table case compares two different slices and panics without the fix.TestAggregateAndSaveForkChoiceAtts_Multiple. Two attestations share a slot, the pool returns them in map order, and the test sorted by slot only (26 failures in 200 runs; 19 in 200 on the upstream base, so not fork-specific). Fixed as upstream did in OffchainLabs/prysm#17571; the file is now identical to upstream's.TestProcessLogs_DepositRequestsStarted. The simulated backend adds a transaction to its pool asynchronously, so the next deposit could be signed with a nonce already in use. The deposit mock now tracks each sender's next nonce. Also from prysm#17571, and identical to upstream's file. The mock istestonlyin Bazel and only test files import it.Found by running the whole check repeatedly
TestValidator_SignValidatorRegistrationRequestTestPayloadBodiesViaUnblinderTestSaveOrphanedOpsTest_executePostFinalizationTasksTest_notifyNewHeadEvent(and two more readers of the same mock)TestOnBlock_ProcessBlocksParallelTestNoViableHead_Reboot,TestStore_NoViableHead_NewPayloadEach is fixed at its cause, none by a sleep or a retry. Where a test has to wait for a goroutine that returns no handle, it waits on the condition with
require.Eventually. The last two rows can only happen in a test that swaps the store or the engine: a node sets them once when the service is built.validator/client/runner_test.goalso takes upstream's change from prysm#17571 (only a canceled context counts as invalid). It was not seen failing here.CI
beacon-chain/blockchainpackage runs, andtesting/assertionsjoins the list.-count=1is new.setup-gorestores the Go build cache, which carries test results: on a re-run of the same commit eight of the ten packages were reportedok (cached)without running. Every run now executes every test, about seven minutes.Ten consecutive green runs
Run 38460143779 on head
451a4c7: attempts 1 to 10 all green, each with all ten packages executed and none cached. Three earlier attempts on older heads failed, one test each (the last three rows of the table); none has failed again since its fix.Known and left alone
TestRetry_On_ConnectionErrorstill sleeps 600 ms and then reads counters. Not seen failing; the likeliest next intermittent test.TestProcessDepositLog_InsertsPendingDepositfailed about 1 in 400 to 2000 runs, only under 40 concurrent processes on one machine. Cause not established; never seen in the check.TestStore_NoViableHead_Livenessis skipped upstream and untouched.Linear: LC-203