test(tbtc): multi-signer simulated integration test for reservation coordination - #4279
Draft
piotr-roslaniec wants to merge 3 commits into
Draft
Conversation
…oordination Implementation-plan.md Milestone 3, 'multi-signer simulated integration test' item (per user decision: build the test, leave the testnet-drill item as an agent-not-actionable tracked item since it needs live infra and calendar time, not code). Scales TestCoordinationExecutor_Coordinate's existing 3-operator harness - deterministic keypairs, real per-operator localChain fakes, a real shared netlocal.BroadcastChannel, one goroutine per operator running coordinationExecutor.coordinate concurrently - to ReservationAnchorProposal and ReservationReanchorProposal. This exercises the real leader/follower coordination round-trip (checklist generation -> leader election -> broadcast -> follower validation -> convergence) that no mocked pkg/tbtcpg unit test can cover, since those call task.Run(request) directly and never go through coordinationExecutor.coordinate. It also exercises PR #4277's protobuf marshaling of both proposal types over a real wire round-trip, since every follower unmarshals the leader's broadcast coordinationMessage. Depends on PR #4278 (this branch's parent): before that fix, ActionReservationAnchor/ActionReservationReanchor never appeared in getActionsChecklist's output, so every operator's checklist search in these tests would fall through to NoopProposal and fail - confirmed by temporarily reverting the checklist fix and re-running (both new tests failed with the expected NoopProposal mismatch), then restoring it. Found and fixed one bug in this test's own harness during verification: both new tests initially shared one netlocal broadcast channel name. getBroadcastChannel's registry is keyed by name and never releases old channels, so under -race (which changed goroutine/channel-delivery timing enough to surface it in ~every run), the reanchor test's follower sometimes received a stale broadcast left over from the anchor test's leader. Fixed by giving each test its own channel name; re-verified stable across 10 repeated -race runs plus the full non-race and race suites. Testing: - go test ./pkg/tbtc/...: 365/365 pass. - go test -race ./pkg/tbtc/...: clean, no data races, including -count=10 on just the two new tests. - go build ./... && go test ./...: full repo, 49 packages, zero FAIL. - gofmt -l / go vet: clean.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ion-multisigner-integration-test
…ion-multisigner-integration-test Auto-merged cleanly on coordination_test.go (this branch's own additions - the multi-signer integration tests and their fixtures - land purely after the base's content with zero line overlap), but the merge exposed a real breakage in this branch's own new tests: both TestCoordinationExecutor_Coordinate_ReservationAnchor and _ReservationReanchor used coordinationBlock = 900, which predates ReservationsActivationBlock (24559289) fixed on the base branch (757c6d8). With the base's activation-block gate now in effect, ActionReservationAnchor/ActionReservationReanchor never appear in the checklist at block 900, so both tests' mock proposal generators fell through to NoopProposal and failed. Bumped both tests' coordinationBlock to 24559289+3511=24562800 (a post-activation, 4th-coordination-window block, verified against the same production getActionsChecklist this session already confirmed correct on the base branch). Leader election is unaffected by this change and stays at operator2: the coordination seed depends only on the wallet public key hash and the injected safe-block hash - both fixed regardless of which raw block number the test passes in - not on the coordination block number itself. Corrected newReservationCoordinationWallet's doc comment, which had claimed leader election was tied to block 900 specifically. Full repo: zero FAIL.
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.
Summary
Implements
implementation-plan.mdMilestone 3's "multi-signer simulatedintegration test" item - the last piece of the full M1 keep-core-readiness
implementation plan (M0 is an external release-coordination gate, not a code
task; M1's four rows and M2's test-coverage backfill are covered by
#4276,
#4277,
#4278, and a
separate M2 follow-up PR).
Stacked on
m1/reservation-coordination-checklist(#4278), tip
c8eb5419f.Scope note (per explicit decision this session): Milestone 3 has two
items - this test, and a "testnet round with a forced liveness/stranding
drill" (~2 weeks, needs a live testnet deployment and real multi-operator
wall-clock timing). Only the former is code; the latter is tracked as an
agent-not-actionable item in the plan doc, unchanged by this PR.
Change
Scales
TestCoordinationExecutor_Coordinate's existing 3-operator harness -deterministic keypairs, real per-operator
localChainfakes, a real sharednetlocal.BroadcastChannel, one goroutine per operator runningcoordinationExecutor.coordinateconcurrently - toReservationAnchorProposaland
ReservationReanchorProposal:TestCoordinationExecutor_Coordinate_ReservationAnchorTestCoordinationExecutor_Coordinate_ReservationReanchorThis exercises the real leader/follower coordination round-trip (checklist
generation -> leader election -> broadcast -> follower validation ->
convergence) that no mocked
pkg/tbtcpgunit test can cover, since thosecall
task.Run(request)directly and never go throughcoordinationExecutor.coordinate. It also exercises #4277's protobufmarshaling of both proposal types over a real wire round-trip, since every
follower unmarshals the leader's broadcast
coordinationMessage.Depends on #4278 (this branch's parent): before that fix,
ActionReservationAnchor/ActionReservationReanchornever appeared ingetActionsChecklist's output, so every operator's checklist search inthese tests fell through to
NoopProposaland failed. Verified directly:temporarily reverted #4278's checklist change, re-ran both new tests (both
failed with the expected
NoopProposalmismatch), then restored it.A bug found in this test's own harness
Both new tests initially shared one
netlocalbroadcast channel name.getBroadcastChannel's registry is keyed by name and never releases oldchannels, so under
-race(which changed goroutine/channel-delivery timingenough to surface it in ~every run), the reanchor test's follower sometimes
received a stale broadcast left over from the anchor test's leader - a
cross-test data race in the test harness itself, not in the production code
under test. Fixed by giving each test its own channel name; re-verified
stable across 10 repeated
-raceruns plus the full non-race and racesuites.
Testing
go test ./pkg/tbtc/...: 365/365 pass.go test -race ./pkg/tbtc/...: clean, no data races, including-count=10targeted at just the two new tests.go build ./...&&go test ./...: full repo, 49 packages, zeroFAIL.gofmt -l/go vet: clean.Not in this PR
operational, not code; tracked separately.