tapgarden: hardening phase two (batch invariants)#2203
Conversation
|
A note on the singleton invariant: that invariant was previously assumed, but only implicitly, so the refactoring makes it explicit. It does not preclude extending to non-singletons in the future (it extends gracefully to such). |
jtobin
force-pushed
the
tapgarden-batch-invariant
branch
from
85fb3a2 to
19b9926
Compare
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
MintingBatch.batchState (in-memory) and the asset_minting_batches.state DB column were written by separate code paths and could diverge on failure. Several store methods that transitioned state as a side-effect (AddSproutsToBatch, CommitSignedGenesisTx, MarkBatchConfirmed) updated the DB while the in-memory mirror was only patched up later by advanceStateUntil's blanket post-step write -- or, in places, *before* the DB write was issued. Funnel every state mutation through MintingStore methods that take a *MintingBatch and advance the in-memory field only after the DB write succeeds. The public UpdateState mutator is removed (renamed to unexported setState); an exported SetStateOnDBSuccess remains as the bridge for store implementations. Two redundant in-memory writes in BatchCaretaker that either re-mirrored state already updated by a store call or got ahead of the DB are dropped.
fundBatch did one of two things depending on whether its workingBatch arg was nil: create a fresh batch and persist via CommitMintingBatch, or attach funding to an existing on-disk batch via CommitBatchFunding. This overload was the shape of bug that caused lightninglabs#2136 -- the wrong in-memory reference silently flipped which path ran, and downstream code that thought it was modifying batch A was in fact creating a fresh batch B. Split into three pieces with non-overlapping responsibilities. createFundedBatch derives, funds, and persists a new batch and never mutates planter state. applyFundingToBatch funds an existing on-disk batch atomically and rejects nil up front. fundPendingBatch is a thin nil-dispatch wrapper used by callers that genuinely have the create-or-update ambiguity. Start()'s recovery path calls applyFundingToBatch directly. Shared funding prep (sibling persistence and the computeFunding closure) is factored into prepareFunding.
fetchDelegationKey and fetchPreCommitGroupKey each iterated pendingBatch.Seedlings and broke after the first entry, picking either the anchor directly or one that referenced it. Go's map iteration is non-deterministic, so correctness depended on an invariant ( <= 1 anchor per universe-commitment batch) enforced by validateUniCommitment elsewhere; break that invariant by one line and the loops silently picked at random. Replace both call sites with MintingBatch.uniqueAnchorSeedling, a deterministic scan that returns the single seedling with GroupAnchor == nil and errors loudly on zero or more than one match. The invariant is now asserted at the site that depends on it rather than presumed.
The planter has long treated a single "current" pre-broadcast batch as a singleton in code, but the schema admitted plurality. Older versions of the planter could in some failure scenarios desync the in-memory slot from disk and leave two pre-broadcast rows behind -- making the in-code invariant unenforceable on a recovering daemon. Add migration 000061, a partial unique index on asset_minting_batches that forbids more than one row in BatchStatePending or BatchStateFrozen at any time. The "(1) WHERE batch_state IN (0, 1)" idiom indexes a constant expression filtered by the active-set predicate; both modernc SQLite >= 3.9 and Postgres accept it. Legacy databases that already hold duplicates fail on startup with the migration error; the operator recovery path arrives in a follow-up commit. Supply-commit test fixtures that don't exercise the planter state machine advance each batch past Frozen immediately after insert so they no longer trip the constraint. LatestMigrationVersion bumps from 60 to 61.
jtobin
force-pushed
the
tapgarden-batch-invariant
branch
from
19b9926 to
cec1069
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a database migration to enforce a singleton pre-broadcast batch invariant (at most one batch in Pending or Frozen state) and adds a one-shot repair tool to resolve duplicate batches. It also refactors MintingStore and MintingBatch to ensure in-memory states are only updated upon successful database writes. The review feedback highlights critical issues, including a bug in uniqueAnchorSeedling that breaks multi-asset non-grouped minting, and a dangerous reference to c.pendingBatch when cancelling failed caretaker batches. Additionally, several potential nil pointer dereference panics were identified across the repair tool, delegation key fetching, and database store methods, which require defensive checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
This is the next chunk of the large-scale tapgarden hardening work originally contained in #2153 (following #2190). It handles the following bullets from the original PR description:
It contains the following changes (summary ctsy Opus):
Two-truth collapse.
MintingBatch.batchState(in-memory) and theasset_minting_batches.statecolumn (on-disk) were maintained by separate code paths and could diverge under failure -- store methods sometimes updated the DB while the in-memory mirror was patched up later, or in places got ahead of the DB entirely. Every state mutation now flows through aMintingStoremethod that advances the in-memory field only after the DB write commits; the two views can no longer drift.FundBatch split.
fundBatchwas overloaded: same call either attached funding to a pending batch or silently minted a new one, depending on hidden state. It splits into acreatehalf and anapplyhalf with disjoint signatures, so the caller's intent is carried by the function chosen rather than by an implicit side condition.Deterministic anchor lookup. Two call sites that fetched the "anchor seedling" iterated
pendingBatch.Seedlingsand took the first match, relying on Go map iteration order and an unenforced <=1-anchor invariant validated elsewhere. Replaced with a deterministic scan that returns the unique seedling withGroupAnchor == niland errors loudly on zero or multiple matches -- the invariant is now asserted at the point it's depended on.Singleton invariant, enforced. The planter has always treated the pre-broadcast batch as a singleton in memory, but the schema admitted plurality; older planter versions could desync the in-memory slot from disk under certain failure paths and leave two pre-broadcast rows behind. Migration 000060 adds a partial unique index on
asset_minting_batchesforbidding more than one row inPendingorFrozenat a time. Legacy databases holding duplicates fail startup with the migration error; the operator recovery path arrives in a follow-up commit.