fix(tbtc): wire reservation actions into the coordination checklist - #4278
Draft
piotr-roslaniec wants to merge 3 commits into
Draft
fix(tbtc): wire reservation actions into the coordination checklist#4278piotr-roslaniec wants to merge 3 commits into
piotr-roslaniec wants to merge 3 commits into
Conversation
Blocker gap found while preparing the M3 multi-signer integration test: pkg/tbtc/coordination.go's getActionsChecklist decides which WalletActionTypes a coordination round even considers, and it never emitted ActionReservationAnchor or ActionReservationReanchor. pkg/tbtcpg.ProposalGenerator.Generate only runs a task whose ActionType() appears in the checklist it's handed (tbtcpg.go:124-135) - it never iterates pg.tasks directly. NewReservationAcceptanceTask and NewReservationReanchorTask are registered when config.Reservations.Enabled=true (tbtcpg.go:88-92), but with no checklist entry, Generate's per-window loop never selected them. Both tasks were structurally unreachable in production regardless of PR #4276/#4277's fixes. Every existing unit test for these tasks (reservation_acceptance_test.go, reservation_reanchor_test.go) calls task.Run(request) directly, bypassing getActionsChecklist/Generate entirely - which is why this was never caught by any prior PR's test suite. Fix: ActionReservationAnchor and ActionReservationReanchor are now appended unconditionally, checked on every coordination window like ActionRedemption (both are custody-critical - an unaccepted reservation or a stale re-anchor risks stranding, not just reduced throughput - unlike the frequency-gated sweep/moving-funds actions). A node with reservations disabled safely no-ops on these: Generate already treats a checklist action with no matching registered task as 'unsupported' and skips it without error (tbtcpg.go:131-135), the same mechanism that already gates every other optional per-node task. Testing: - Updated TestCoordinationExecutor_GetActionsChecklist and its _PostActivation sibling: every non-nil expected checklist now includes both new actions right after ActionRedemption, matching the real append order. Extended assertChecklistOrdering's priority map accordingly (Redemption=0, ReservationAnchor=1, ReservationReanchor=2, then the existing sweep/moving-funds/ heartbeat priorities shifted). - Added TestCoordinationExecutor_GetActionsChecklist_ReservationActionsAlwaysPresent, a dedicated regression guard asserting both actions are present across pre/post-activation and 4th/non-4th windows, decoupled from the large table-driven test - would fail on its own if this wiring regresses. - go test ./pkg/tbtc/... ./pkg/tbtcpg/...: 476/476 pass. - go build ./... && go test ./...: full repo, 49 packages, zero FAIL. - gofmt/vet clean on both changed files.
|
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 |
…-coordination-checklist
…-coordination-checklist Reconciles this branch's original coordination-checklist wiring with the review-fix commit that independently re-fixed the same wiring on the base branch (757c6d8: 'gate reservation checklist entries solely on the activation block, not the local per-operator config flag'). - coordination.go: the auto-merge silently duplicated the reservation checklist append (this branch's original unconditional append, right after Redemption, plus the base's corrected activation-block-and-frequency-gated append later in the function) - ActionReservationAnchor/ActionReservationReanchor would have been listed twice whenever both conditions held, and listed even before activation the rest of the time. Removed this branch's unconditional append; kept only the base's gated one. - coordination_test.go: adopted the base's TestCoordinationExecutor_GetActionsChecklist_Reservations (activation + frequency gating) and its updated priority ordering (Redemption < DepositSweep < MovedFundsSweep < MovingFunds < ReservationAnchor < ReservationReanchor < Heartbeat, reservations now gated like the sweep actions rather than unconditional). Rewrote this branch's two pre-existing checklist tests (TestCoordinationExecutor_GetActionsChecklist, _PostActivation) to match: expected values recomputed by running the corrected production getActionsChecklist against each test's exact block/window inputs, not hand-derived - 24 subtests total, all independently verified against the same fixed implementation the regression guard exists to catch drift in. 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
Blocker gap, found while preparing PR (M3 multi-signer integration test),
not while doing the ask. Adds
docs/spec/reservations/m1-keep-core-readiness/01-gap-analysis.mdBlocker row 2:
pkg/tbtc/coordination.go'sgetActionsChecklistnever includedActionReservationAnchor/ActionReservationReanchor, so the reservationacceptance and re-anchor proposal tasks were structurally unreachable by the
live coordination loop regardless of #4276/#4277 landing.
Stacked on
m1/reservation-protobuf-marshaling(#4277),tip
72373fcad.The gap
pkg/tbtcpg.ProposalGenerator.Generateonly runs a task if itsActionType()appears in
request.ActionsChecklist(tbtcpg.go:124-135) - it neveriterates
pg.tasksdirectly. That checklist comes from exactly one place:coordinationExecutor.getActionsChecklistinpkg/tbtc/coordination.go.That function builds the list from scratch every call and never mentioned
reservations.
NewReservationAcceptanceTask/NewReservationReanchorTaskwere registered into
pg.taskswheneverconfig.Reservations.Enabled=true(
tbtcpg.go:88-92), but with no matching checklist entry,Generate'sper-window loop never selected them - both tasks were dead code in
production.
Every existing unit test for these tasks (
reservation_acceptance_test.go,reservation_reanchor_test.go) callstask.Run(request)directly, bypassinggetActionsChecklist/Generateentirely - which is exactly why no priorPR's test suite caught this. It only became visible while building the
real multi-signer coordination-round test the M1 plan's Milestone 3 calls
for, which is the first test in this repo to exercise the actual production
call path (
coordinationExecutor.coordinate->getActionsChecklist->ProposalGenerator.Generate) for these action types.Fix
ActionReservationAnchor/ActionReservationReanchorare now appendedunconditionally in
getActionsChecklist, checked on every coordinationwindow - like
ActionRedemption, not frequency-gated likeDepositSweep/MovedFundsSweep/MovingFunds - since both are custody-critical
(an unaccepted reservation or a stale re-anchor risks reservation stranding,
not just reduced throughput).
A node with reservations disabled is unaffected:
Generatealready treats achecklist action with no matching registered task as "unsupported" and skips
it without error (
tbtcpg.go:131-135) - the same mechanism every otheroptional per-node task already relies on. No new config plumbing needed in
pkg/tbtc.Testing
TestCoordinationExecutor_GetActionsChecklistand its_PostActivationsibling: every non-nil expected checklist now includesboth new actions right after
ActionRedemption. ExtendedassertChecklistOrdering's priority map (Redemption=0, ReservationAnchor=1, ReservationReanchor=2, existing priorities shifted).TestCoordinationExecutor_GetActionsChecklist_ReservationActionsAlwaysPresent,a dedicated regression guard (pre/post-activation x 4th/non-4th window)
decoupled from the large table-driven test.
go test ./pkg/tbtc/... ./pkg/tbtcpg/...: 476/476 pass.go build ./...&&go test ./...: full repo, 49 packages, zeroFAIL.gofmt -l/go vet: clean on both changed files.Not in this PR
test itself - both tracked as separate follow-up PRs; the M3 test will be
built on top of this branch now that the path it needs to exercise is
actually reachable.