tapgarden: large scale hardening#2153
Closed
jtobin wants to merge 56 commits into
Closed
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a large-scale hardening of the tapgarden minting subsystem. The changes focus on ensuring structural correctness and restart-idempotence for minting batches. By enforcing strict singleton invariants on pending batches, ensuring atomic persistence, and implementing deterministic state transitions, the system is now significantly more resilient to crashes and re-orgs. The refactor also improves code modularity by extracting node-level service interfaces and dedicated subsystems into their own packages. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 refactors the tapd codebase by modularizing substrate dependencies into the tapnode package and separating concerns into tapreorg, tapcustody, and mintpublish. It also introduces database constraints to enforce a singleton pre-broadcast batch invariant, adds a repair tool to resolve legacy database violations, and implements deduplication for supply update events. The review feedback highlights a critical migration failure risk where the programmatic backfill in migration 62 could fail on legacy databases containing duplicate events. Additionally, feedback points out a shallow copy issue in GroupKey.Copy(), potential nil pointer dereferences in UpdateBatchState and Cultivator.Cancel, and a potential infinite loop in PublishMintBatch if batchSize is non-positive.
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.
jtobin
force-pushed
the
tapgarden-hardening
branch
from
7f01e15 to
f21a6d7
Compare
|
@jtobin, remember to re-request review from reviewers when ready |
jtobin
force-pushed
the
tapgarden-hardening
branch
from
f21a6d7 to
caf0b4f
Compare
jtobin
force-pushed
the
tapgarden-hardening
branch
2 times, most recently
from
080e146 to
f30771f
Compare
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.
prepAssetSeedling assigned or mutated c.pendingBatch before the DB write that justified the mutation. A failed commit left the in-memory state ahead of disk: either pointing at a batch that did not exist, or holding a seedling that the DB had never seen. The next call to QueueNewSeedling would then either trip a uniqueness check or wedge the failed seedling in memory indefinitely. Reorder both branches to persist first and mirror into memory only on success. To make the existing-batch branch separable, MintingBatch grows validateSeedling (pure) and commitSeedling (mutation); AddSeedling remains as the combined wrapper for callers that have no persistence boundary between the two. The fresh-batch branch builds entirely on a local newBatch and only assigns c.pendingBatch after CommitMintingBatch returns.
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.
MintingOutputKey memoized its result on first call; subsequent calls silently ignored the sibling argument. The caretaker's BatchStateCommitted branch was the only caller that exploited this, passing nil and relying on the Frozen branch having cached the real sibling. Any path that reached Committed without going through Frozen first -- a future RBF retry, a partial restart, a refactor -- would have silently computed and cached the wrong output key. Drop the cache. MintingBatch loses its memoized mintingPubKey and taprootAssetScriptRoot fields; MintingOutputKey recomputes from (BatchKey, RootAssetCommitment, sibling) on every call. The caretaker's Committed branch now loads the sibling preimage before calling MintingOutputKey and passes it explicitly; the same preimage is already used a few lines below for siblingBytes, so this is a reorder rather than a new I/O cost. The work is a single tapscript hash plus an ECC tweak -- not expensive enough to justify a memoizing getter that introduces correctness footguns.
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 000060, 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 59 to 60.
The DB-level singleton invariant from migration 000060 still leaves the planter with two operational gaps: at startup, against a legacy DB that survived migration but otherwise has duplicates, and on caretaker failure, where the gardener used to leave the orphaned (typically Frozen) batch behind. Two cancel-related error messages also predate the invariant and were ambiguous. Add a defensive pre-Start check that refuses to come up if more than one non-final batch is Pending or Frozen, naming the offending batch keys and pointing the operator at --repair.cancel-duplicate-batches. The gardener's BroadcastErrChan handler now cancels the failed batch in the same step, mirroring caretaker.Cancel()'s state rule (Pending/Frozen -> SeedlingCancelled, Committed -> SproutCancelled). The cancel-ambiguity error wording is rewritten to reflect the actual source (the cancel API has no batch-key parameter), and an obsolete TODO becomes an invariant statement referencing migration 000060. Tests follow: TestCheckSingletonInvariant covers the relevant shapes; finalize_with_tapscript_tree expects SeedlingCancelled on caretaker error; fund_seal_on_restart drops the now-unrepresentable two-pending scenario.
A legacy database populated by an older version of tapd may hold more than one pre-broadcast batch, in which case migration 000060 fails on startup with a unique-constraint violation and blocks the daemon indefinitely with no documented recovery path. Add a one-shot operator flag that opens the database with migrations skipped, finds all batches in BatchStatePending or BatchStateFrozen, preserves the most recent (by CreationTime), and cancels the rest by transitioning them to BatchStateSeedlingCancelled. On exit (status 0) the operator restarts tapd normally and migration 000060 applies against the now-clean database. The default posture is still "refuse to start" -- the daemon never silently mutates state on its own; the operator must explicitly invoke the repair flag. Cancellations are state transitions rather than row deletions, so cancelled batches remain on disk for later inspection.
A re-run of the minting caretaker's Confirmed branch fires SendMintEvent again for each minted asset; until now this produced a fresh supply_update_events row per re-fire because the table held no uniqueness constraint on event content. The downstream supply-commit state machine then saw the same logical mint twice and double-counted the update log. Add a deterministic 32-byte content hash column event_key on supply_update_events with hash = sha256(group_key || be32(update_type_id) || event_data), guarded by a unique index. The InsertSupplyUpdateEvent query becomes an ON CONFLICT DO NOTHING upsert so a re-insert of the same logical event is silently absorbed. Two migrations: 000061 adds the column nullable plus the unique index (NULLs are distinct in both backends, so pre-backfill rows don't collide); 000062 is a programmatic step that computes hashes for pre-existing rows (SQLite lacks a native SHA-256). After 000062 the index enforces dedup across the whole table. LatestMigrationVersion bumps from 60 to 62.
In the Committed branch of BatchCaretaker.stateStep, Wallet.ImportTaprootOutput ran after CommitSignedGenesisTx, which is the state-transition write that flips the batch from Committed to Broadcast. A crash between the two left the genesis transaction persisted (state=Broadcast) but the minting taproot output never imported into lnd's wallet. On restart, the Broadcast branch executes, which does not import the output, so lnd remained unaware of it indefinitely. Move the import to before the state-transition write. The import is already idempotent (the "already exists" error is caught and swallowed), so a crash anywhere in this branch now resumes from Committed and re-runs the sign-import-commit sequence cleanly.
The minting caretaker's Committed branch reloads an unsigned PSBT from disk on restart and re-signs it; the resulting signed bytes are implicitly compared with the prior attempt through downstream side effects. lnd uses BIP-340 RFC-6979 deterministic Schnorr nonces, so the property should hold by construction -- but it is load-bearing for restart semantics and worth checking in CI rather than assuming. Add an itest that calls Wallet.SignAndFinalizePsbt twice on the same unsigned PSBT and asserts byte-identical output.
testBasicAssetCreation pins the "restart at every observable boundary" mint case in one fixed order. A regression that only fires under a particular interleaving of restart points would slip past it and would not shrink to a small reproducer. Add a rapid property test that models the legal BatchState transition graph and samples every subset of two well-synchronized restart points (after disk-state Committed; after the Broadcast branch's publish call), asserting the mint flow always ends in BatchStateFinalized with no errors and no leftover caretakers. The harness is structured so a third restart point can be added trivially.
MintingBatch.Copy and the FundedMintAnchorPsbt.Copy it delegated to were "deep" in name only. Seedlings, AssetMetas, GenesisPacket's nested slices, and RootAssetCommitment all shared substructure with the source. RPC payloads and subscriber notifications used Copy believing they had a snapshot, when they actually held a smaller window into the same data -- callers reasoned as if they had isolation, with no actual barrier. Walk every reachable pointer, slice, map, and byte buffer and duplicate it. Helpers cover the leaf types (Genesis, GroupKey, ScriptKey, MetaReveal, etc.); RootAssetCommitment goes through commitment.TapCommitment.Copy (already exercised by TestTapCommitmentDeepCopy); FundedMintAnchorPsbt.Copy round-trips the psbt.Packet through Serialize/NewFromRawBytes, which duplicates every nested PInput / POutput / Unknown without hand-rolling per-type copies. TestMintingBatchCopyIsDeep builds a fully-populated batch, snapshots it, mutates every reachable substructure on the source, and asserts the snapshot is unmoved. Impossible error paths panic rather than fall back to shared pointers.
The cancel protocol used two shared per-caretaker channels (CancelReqChan + CancelRespChan). Matching a response to its specific request relied on the gardener serializing all cancel calls -- a discipline, not a property of the protocol itself. Replace the pair with a single CancelReqChan carrying cancelReq values, each embedding its own per-call reply channel. The binding is now intrinsic and no longer depends on caller serialization. CancelRespChan is dropped from BatchCaretakerConfig as a side-effect.
The finalize-batch state-request handler returned the caretaker's live MintingBatch pointer once BroadcastCompleteChan fired. The caretaker goroutine continues to mutate the batch after broadcast (Broadcast -> Confirmed -> Finalized writes GenesisPacket and RootAssetCommitment), so the caller's reads raced those writes. Take a deep snapshot via Batch.Copy before resolving, matching what every other state-request handler in the same select arm already does. Document the ownership invariant on BatchCaretakerConfig.Batch: the live pointer is owned by the caretaker goroutine; external observers must Copy first.
Two latent issues in the caretaker. BatchCaretaker.anchorOutputIndex was a uint32 field populated only by the Frozen state step; on any restart that re-entered the state machine at Committed, Broadcast, or Confirmed, the Frozen step never ran and downstream reads used the zero value. The bug was latent because FundPsbt(..., -1) typically leaves the asset anchor at output 0. Separately, the confirmation-watching goroutine launched in the Broadcast step read CancelReqChan in parallel with the cultivator's main loop, giving each per-call request two potential receivers; today every post-broadcast Cancel() rejects, but the per-call respCh contract is undermined if that ever changes. Remove the cached field and read AssetAnchorOutIdx directly from GenesisPacket at each use site; the value is always present once the batch has progressed past Frozen. Drop the inner CancelReqChan reads so the cultivator loop is the sole reader post-broadcast, and document the single-reader invariant on the field.
ChainPlanter was the only implementation; no mocks exist anywhere in the repo, and tests already hold *ChainPlanter directly. The interface added abstraction without any second implementation to abstract over. Replace the two config-field types that held the interface with *tapgarden.ChainPlanter, delete the interface and its compile-time assertion, and strip the stale "NOTE: This is part of the Planter interface" comments. If a fake minter is ever needed, the interface can be reintroduced from the concrete type's actual surface.
tapdb's persistence path for the supply pre-commit row was coupled to tapgarden's FundedMintAnchorPsbt.PreCommitmentOutput. Extracting the supply-commit code into its own package would require breaking that coupling first. Introduce tapgarden.PreCommitBindData, a typed persistence payload. The five BatchStore methods that bind a funded genesis transaction or seal a batch -- CommitMintingBatch, CommitBatchTx, CommitBatchFunding, AddSproutsToBatch, SealBatch -- gain an fn.Option[PreCommitBindData] parameter that tapdb writes alongside its usual columns. For now the planter constructs the payload by lifting it off FundedMintAnchorPsbt.PreCommitmentOutput via a new PreCommitBindData helper; the same SQL writes happen in the same transactions. Two inner helpers (upsertPreCommit + insertMintAnchorTx) merge into a single upsertPreCommitRow that takes the typed payload plus the genesis tx hash.
The supply-commit code needs to read mint_supply_pre_commits rows (for intake checks and verifier lookups) but importing all of AssetMintingStore just for that surface drags every other minting concern along with it. Create a dedicated SupplyPreCommitStore backed by the same db handle as AssetMintingStore, exposing the mint_supply_pre_commits reads (currently just FetchDelegationKey) that the supply-commit code needs. AssetMintingStore.FetchDelegationKey becomes a thin shim that delegates to it; the shim survives for now because the MintingRefReader interface in tapgarden still has the method, and that method is removed in a later commit once the augmenter takes over delegation-key prep. The write side stays on AssetMintingStore because pre-commit rows must land atomically with the batch's chain update, which only the binding tx in AssetMintingStore can offer.
tapgarden's minting flow has supply-commit logic hardcoded throughout (seedling delegation-key prep, universe-commitment intake validation, pre-commitment output stamping, PreCommitBindData production, post-confirmation events). The two concerns are entangled across the planter and cultivator, and the supply-commit code cannot move to its own package until tapgarden's call sites stop reaching directly into it. Introduce tapgarden.GenesisTxAugmenter, an interface tapgarden will invoke at well-defined lifecycle moments to let an external participant act on batch minting without tapgarden knowing the details. Six hooks: PrepareSeedling and ValidateSeedling for intake; ExtraOutputs and PostFund for genesis-tx contributions; BindData for the typed persistence payload; OnBatchConfirmed for downstream events. A NoOpAugmenter is provided so call sites don't nil-check. universe/supplycommit gains a concrete GenesisAugmenter that captures every prior supply-commit-specific tapgarden path in one place. The augmenter is not yet wired into GardenKit; that happens in the following commit.
With GenesisTxAugmenter defined but unwired, tapgarden still
executed the legacy supply-commit code paths directly. The
indirection only earns its keep once production paths route through
it.
Wire the augmenter through GardenKit and replace the planter's
direct supply-commit references with augmenter calls at every
lifecycle moment: seedling intake goes through PrepareSeedling
instead of prepSeedlingDelegationKey; the intake gate through
ValidateSeedling instead of validateUniCommitment plus
validateDelegationKey; funding consults ExtraOutputs to splice
additional anchor outputs and PostFund to stamp PSBT metadata;
persistence payloads at funding and seal come from BindData;
post-confirmation events go through OnBatchConfirmed. The planter
and cultivator each gain an augmenter() helper that returns the
configured augmenter or NoOpAugmenter{}, so call sites never
nil-check. MintingBatch.validateGroupAnchor is exported as
ValidateGroupAnchor so the augmenter can invoke it.
tapgarden/mock.go gets a local mockSupplyCommitAugmenter that
mirrors the real one without the dependency cycle, so existing
tests still get fully-formed funded batches.
With GenesisTxAugmenter wired through and tapgarden's production paths routed through it, the legacy supply-commit types and helpers in tapgarden are dead code. Delete them: MintSupplyCommitter and the matching GardenKit fields (MintSupplyCommitter, DelegationKeyChecker); PreCommitmentOutput, its Copy method, NewPreCommitmentOutput, and the PreCommitmentOutput field on FundedMintAnchorPsbt; PreCommitTxOut (moved to supplycommit and exported there); the DelegationKey alias; fetchDelegationKey / fetchPreCommitGroupKey; prepSeedlingDelegationKey; sealBatchPreCommit; the validateUniCommitment / validateDelegationKey methods on MintingBatch; the sendSupplyCommitEvents Cultivator method; FetchDelegationKey on MintingRefReader; the AssetMintingStore.FetchDelegationKey shim and PreCommitmentOutput reconstruction in marshalMintingBatch; the PreCommitOutIdx tracking in AnchorTxOutputIndexes. tapconfig wires a real supplycommit.GenesisAugmenter into GardenKit. The obsolete supply_commit_test.go is replaced by a unit test on the augmenter at universe/supplycommit/genesis_augmenter_test.go; TestValidateUniCommitment is removed since the invariants now live on the augmenter. tapgarden/mock.go gains a MockBindDataForBatch helper so tapdb tests can derive a typed payload without going through the deleted method.
GenGroupVerifier, GenGroupAnchorVerifier, and GenRawGroupAnchorVerifier construct closures over tapnode.GroupFetcher that feed proof.VerifierCtx; they sit naturally alongside tapnode.GenHeaderVerifier rather than inside the minting state machine. Verifier consumption is downstream of tapgarden's end (a verifiable asset in the local store); the generators belong to the proof-verifier code, not the minting one. Move the three generators to tapnode along with ErrGroupKeyUnknown, ErrGenesisNotGroupAnchor, and the LRU cache helpers. Call sites are updated accordingly.
The cultivator's batch-confirmation handler assembled universe.Item values inline (storeMintingProof returned both the proof blob and the item) and streamed them to a local universe via UpsertProofLeafBatch. The planter's re-org handler had its own UpsertProofLeaf loop with the same wire-shape construction. Both sites encode universe publication, which is downstream of tapgarden's natural end (a verifiable asset in the local store). Introduce tapgarden.MintProofPublisher, owned by the consumer, with PublishMintBatch and PublishMintProofUpdates. The universe/mintpublish package provides the BatchRegistrar-backed implementation. tapgarden now passes raw (asset, proof, mintTxHash, outIdx) inputs and never constructs a universe.Item, BaseLeafKey, Identifier, Leaf, or GenesisWithGroup itself. GardenKit loses the Universe and UniversePushBatchSize fields in favor of a single MintProofPublisher field; NoOpMintProofPublisher covers test configurations with no downstream universe.
The bug this PR closes -- restart-triggered re-fires of mint events inserting duplicate supply_update_events rows -- could have already fired against an unupgraded database. Two rows with identical content hash to the same event_key, so the second SetSupplyUpdateEventKey in the migration 62 backfill would violate the unique index added in migration 61 and abort the migration. Track hashes seen during the backfill loop and delete any row whose hash a prior row already claimed. The deleted rows are by definition the same logical event as the one that survives. A new DeleteSupplyUpdateEvent sqlc query supports the delete-by-event_id that the dedupe needs. TestMigration62BackfillDedupesLegacyDuplicates seeds three identical rows pre-backfill and asserts the migration leaves exactly one with the expected hash, never erroring out.
Address golangci-lint findings (lll, gofmt, whitespace, exhaustive, gosec, govet copylocks) and two reviewer comments: use uint for mintpublish's batchSize so the negative-value case is ruled out at the type boundary, and use the already-computed batchKey in the Cultivator.Cancel default arm instead of re-deriving it from the live batch. The copylocks fixes switch the augmenter staging-batch construction from `*batch` to `batch.Copy()` so the atomic state field isn't copied by value. The exhaustive cases get explicit default arms on the BatchState switches in checkSingletonInvariant and RunRepairTool. Rename Migration62BackfillSupplyUpdateEventKeys to Migration62BackfillEventKeys so the aligned map entry fits the 80-column limit. Make TestMigration62BackfillSupplyUpdateEventKeys and TestMigration62BackfillDedupesLegacyDuplicates portable to Postgres: the first one used the SQLite-only `?` placeholder, the second used MAX(bytea) which isn't defined in Postgres. Switch to sqlc's InsertSupplyUpdateEvent and split COUNT + key lookup into two queries. Regenerate sqlc bindings whose source-comment references the event_key column's migration number. Add a [repair] section to sample-tapd.conf with an example for repair.cancel-duplicate-batches; the sample-conf check requires every tapd flag to have a default or example in the file.
Main introduced 000060_asset_transfers_superseded, colliding with this branch's 000060_unique_pending_or_frozen_batch. Renumber this branch's migrations and all their narrative references: 60 unique_pending_or_frozen_batch -> 61 61 dedupe_supply_update_events -> 62 62 backfill_supply_update_event_keys -> 63 LatestMigrationVersion bumps to 63 and the Migration62BackfillEventKeys constant becomes Migration63BackfillEventKeys.
Legacy databases affected by the pre-hardening planter can hold two
kinds of state that trip the invariants this branch introduces:
1. Duplicate rows in {Pending, Frozen} (from a startup race the
planter no longer allows). Migration 61's naive `CREATE UNIQUE
INDEX` would fail on these DBs with an opaque SQL error and
force the operator into a manual `tapd --repair.cancel-duplicate-batches`
step before the daemon would start again.
2. Multiple `supply_update_events` rows with identical content,
split between rows attached to a finalized transition and
dangling rows (from restart re-fires of the same logical
event). Migration 63's backfill dedup kept the first row by
physical order, which could delete the attached row and leave a
finalized transition without its events.
Migration 61 now cancels all but the most recent pre-broadcast batch
inside the same migration transaction before creating the index, so
affected DBs migrate cleanly without operator intervention.
`tapd --repair.cancel-duplicate-batches` is retained as a diagnostic
that surfaces the same repair outside the migration stream.
Migration 63's fetch query now orders attached rows before dangling
ones (tie-broken by event_id ASC), so the backfill's first-wins dedup
is guaranteed to preserve any attached row when duplicates exist.
Migration 62's down.sql picks up a warning that down-migrating past
62 is destructive because 63 physically removed duplicate rows.
The universe_commitments column on asset_minting_batches is set at batch creation from the seedling's SupplyCommitments intent. Under the previous BindMintingBatchWithTx query the funding step overwrote it with `preCommit.IsSome()`; a NoOpAugmenter (or any deployment where the augmenter chose not to emit a bind payload for a batch that legitimately requested supply commitments) would silently flip the flag off at funding, and the mint would proceed without pre-commit anchoring even though the operator had asked for it. Drop the column from the SET clause so the flag is authoritative from NewMintingBatch onward and cannot regress at funding time. Also add ORDER BY precommits.id ASC to FetchMintSupplyPreCommits so SupplyPreCommitStore.FetchDelegationKey's fetchRow[0] pick is deterministic when a legacy DB somehow holds divergent pre-commit rows for a single group.
RunRepairTool ran under context.Background(), so a Ctrl+C mid-repair would leave partial state on disk (some duplicate batches cancelled, others not). Take the shutdownInterceptor as a parameter and derive a cancellable context from its shutdown channel. Also switch from sort.Slice to sort.SliceStable for the CreationTime tie-break so two batches with identical timestamps produce a deterministic winner, matching the ordering rules migration 61's self-heal now uses inside the migration stream.
Four failure modes ride the same state machine and were addressable
together on the cultivator/planter side:
1. OnBatchConfirmed doc/code split. The interface said errors are
logged and do not roll back the confirmation, but the code
returned the error, skipping MarkBatchConfirmed. That left the
batch stuck in Broadcast forever while the publish and
augmenter side-effects had already fired, so a restart re-runs
everything non-idempotently. Now log-and-continue, which
matches the interface and relies on migration 62's event_key
dedup for augmenter re-runs.
2. Cancel/dead-cultivator deadlock. cancelMintingBatch could find
a cultivator in the planter's map, push into its CancelReqChan
(buffered), then wait forever on the reply channel because the
cultivator's main goroutine had already stopped reading
CancelReqChan (blocked in SignalCompletion, or past the
post-broadcast select). Add a Done() channel on Cultivator,
closed once assetCultivator returns, and select on it in
cancelMintingBatch so a late cancel surfaces an actionable
"already completed" error instead of hanging until Quit. Also
buffer completionSignals so SignalCompletion never blocks the
goroutine while the gardener is inside a stateReq closure.
3. Startup race vs the singleton pre-broadcast index. Start()
launches cultivators for resumed Frozen batches, then starts
the gardener. A seedling arriving before the cultivator has
moved its batch past Frozen would trip the partial unique
index added by migration 61 with a raw SQL error. Guard the
"no pending batch" case in addSeedling by refusing the
seedling if any cultivator still holds its batch in
{Pending, Frozen}, with a clear user-facing error.
4. Two-truth split in the Frozen and Committed branches.
RootAssetCommitment/GenesisPacket.Pkt/ChainFees were set on the
live b.cfg.Batch before their DB writes, so a concurrent
Copy() (e.g. from FinalizeBatch) could observe the in-memory
batch ahead of what is on disk -- the exact hazard commit
caaa621 was supposed to close. Stage the mutations on a
staging copy (for the augmenter and derived scripts), pass
locals into the store calls, then apply the mutations to the
live batch only once the DB write returns. The Copy() panic
on RootAssetCommitment.Copy is no longer reachable.
Small footguns exposed by the confirmation-path work above:
- NoOpAugmenter.ValidateSeedling now rejects any seedling that
requests SupplyCommitments. Without a real augmenter wired
there is no substance to emit the pre-commit anchor output or
the post-confirmation mint event, and silently accepting the
seedling let the batch proceed with supply-commit disabled
even though the caller had asked for it.
- PreCommitBindData grows a NewPreCommitBindData constructor
that enforces `InternalKey.PubKey != nil` at the boundary.
The zero value would nil-deref in the store's upsert path;
the augmenter and mocks now route through the constructor.
- GenesisAugmenter.OnBatchConfirmed logs the error from
HasDelegationKey before dropping the asset from mint-event
emission. Matches main's log-then-drop behavior; the previous
silent drop hid legitimate store errors as if the asset just
lacked delegation.
- mintpublish.NewPublisher panics if batchSize == 0. The
PublishMintBatch loop advances by batchSize, so a zero would
have spun forever; a wiring-time invariant deserves an
upfront panic.
- SupplyPreCommitStore.FetchDelegationKey's fallback pick is now
documented as "lowest precommits.id first"; the query gained
the matching ORDER BY in the previous commit.
Cancel() writes to the buffered respCh (size 1) and then returns, which triggers the goroutine's `defer close(b.done)`. Both cases become ready simultaneously in the planter's select, and Go picks randomly -- so a successful cancel could still surface the "already completed" error path even though the batch was cancelled and persisted correctly. The impact was mild (state on disk was correct, retries would refuse the already-cancelled batch) but the error surface misled callers. Nest a non-blocking respCh receive inside the Done() case so a queued reply is always preferred over the goroutine's exit signal.
The repair tool's SliceStable claimed it inherited a deterministic tie-break from FetchNonFinalBatches' input order, but the underlying query had no ORDER BY. Two pre-broadcast batches with identical CreationTime therefore had an arbitrary winner -- opposite of the reason SliceStable was chosen in the first place. Give the query the same explicit tie-break migration 61 uses -- creation_time_unix DESC, batch_id DESC -- so the repair tool lands on the same "most recent" row the migration would preserve, and update the misleading comment. assertGenesisPsbtFinalized in planter_test.go picked "the last non-cancelled batch" via fn.Last, relying on the pre-existing undefined order to place the newest at the tail. Under the new newest-first ordering it becomes fn.First. No behavioral change in the test's intent, just alignment with the guaranteed order.
The prior log-and-continue behavior claimed confirmation completion without the cause of completion being present: OnBatchConfirmed is the site where supply-commit mint events are durably recorded, and suppressing its error advanced the batch past the retry point while still owing the event. The event_key dedup index (migration 62) exists specifically to make the augmenter retry idempotent, but the mint lifecycle -- the only agent positioned to fire that retry -- had just been severed from the retry by the log-and-continue. Return the error, so the batch stays in BatchStateBroadcast and the confirmation branch re-runs on restart. Universe publish above is already idempotent; the event_key dedup covers the augmenter side; MarkBatchConfirmed is what advances state on disk, so retry is safe. Pin the invariant with a new test that wires an injectable augmenter into the planter harness, forces OnBatchConfirmed to fail, asserts the batch is not advanced past Broadcast, then flips the augmenter to succeed, restarts the planter, and asserts the batch advances to Finalized cleanly on the retry.
Two golangci-lint findings against `--new-from-rev=main` on the
hardening branch:
* planter.go:2914 exhaustive: the pre-broadcast-batch guard's
switch only cares about Pending and Frozen. Add an empty
default arm so the exhaustive linter treats the omission as
intentional.
* cultivator.go:889 lll: the FundedPsbt struct literal ran to
87 chars on the ChangeOutputIndex line. Hoist the value into
a local so the field assignment fits within 80 chars.
The parameter was previously only used in a log message; the loop terminated on the fixpoint check `nextState == currentState` and ignored the caller's declared target entirely. That's a name-vs- behavior split: callers reading the signature reasonably expect targetState to be a contract, but the function delivered a fixpoint-until-you-stop with no assertion that the fixpoint reached matched the target. Keep the fixpoint check as the primary exit -- it's the correct model for this state machine, since the terminal states self-transition (Broadcast waits, Finalized is idle) -- and add a post-loop assertion that the fixpoint we reached matches the declared target. A mismatch now surfaces as an error instead of a silent success, catching either a caller passing the wrong target or a state-machine change under our feet. Rewrite the doc comment to describe what the function actually does and what targetState now means.
BatchStateConfirmed on main names two different essences with the
same word: an *input state* for the Confirmed branch of the state
machine, and a *mid-branch durable checkpoint* written by
MarkBatchConfirmed. The Confirmed branch published universe proofs,
emitted the supply-commit event, called MarkBatchConfirmed (which
advanced disk state to Confirmed), and then registered the anchor
tx with the re-org watcher via WatchProofs. A crash between
MarkBatchConfirmed and WatchProofs left disk at Confirmed with no
re-org callback registered by us; on restart the fast-forward
skipped the branch entirely, and the re-org watcher's own Start-
time recovery re-registered the anchor tx with its
DefaultUpdateCallback -- silently dropping the universe re-publish
that updateMintingProofs performs on any subsequent re-org.
Dissolve the identity conflation by making MarkBatchConfirmed the
LAST persistence write in the branch. WatchProofs now runs first;
disk-Confirmed genuinely means "every step the Confirmed branch
owes is done." A crash before MarkBatchConfirmed keeps the batch
at Broadcast so the whole branch re-runs on restart -- universe
publish is idempotent, augmenter side is deduped by event_key
(migration 62), storeMintingProof rides upserts, and WatchProofs
against a fresh watcher instance is a first-time registration.
Also rewrite the surrounding comments to describe the new
ordering, note the semantic justification for the restart fast-
forward, and drop the stale TODO on the Finalized case ("confirmed
should just be the final state?") -- that concern is fully
resolved by the new ordering.
Add a ShouldFail atomic.Bool hook to MockProofWatcher and a
testWatchProofsFailureAbortsConfirmation test that forces
WatchProofs to fail, asserts the batch stays at Broadcast (under
the old ordering this assertion would fail because
MarkBatchConfirmed would have run before the WatchProofs failure),
restarts with the mock succeeding, and asserts the batch advances
to Finalized. Pins the ordering invariant against regression.
jtobin
force-pushed
the
tapgarden-hardening
branch
from
f30771f to
03e23f0
Compare
Member
Author
|
To be obviated by a series of smaller PR's, starting with the linked one. |
github-project-automation
Bot
moved this from 🆕 New
to ✅ Done
in Taproot-Assets Project Board
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
2 participants
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.
Ok, hang on, hear me out..
I can hear you now. 47 commits? +6500/-4000?! April 1st was like two months ago, what gives?
This is the result of a general tapgarden refactor I embarked on after making #2141. That was sort of the straw that broke the camel's back; by the time I'd made that PR, I had literally lost count of the number of problems I'd encountered and fixed in tapgarden. So, I decided to investigate more deeply, by way of a holistic analysis, why the package has historically been so buggy.
The result is probably best described by summarizing what problems, previously possible, are now made structurally impossible, and by what broad change each was accomplished. So, a summary, ctsy Opus:
Persistence atomicity. Previously it was possible for an in-memory batch state to advance while the corresponding database write failed, leaving memory and disk disagreeing about what state the batch was in; now, because every state mutation flows through a store method that touches memory only after the database commit succeeds, the two views cannot drift.
Funding overload. Previously it was possible for a fund-batch call to silently create a new batch when the caller meant to attach funding to an existing one (the bug behind [bug]: Mint batch can get stuck in FROZEN state after funding failure #2136); now, because "create" and "attach" are separate functions with non-overlapping signatures, the wrong intent will not type-check.
Singleton invariant. Previously it was possible for two unbroadcast minting batches to coexist on disk and block all subsequent minting; now, because the schema itself forbids more than one row in the pre-broadcast subset, that state cannot exist.
Determinism. Previously it was possible for the planter to pick a different "anchor seedling" on different runs of the same input because the choice depended on Go map iteration order; now, because the lookup scans deterministically and refuses to return when the singleton invariant fails, the choice is fixed.
Purity in output-key derivation. Previously it was possible for MintingOutputKey to return a cached value that didn't reflect the sibling argument the caller actually passed; now, because the function holds no state, its output is determined entirely by its inputs.
Restart-idempotence of side effects. Previously it was possible for a crash between persisting a batch as broadcast and importing its taproot output to leave the on-chain output unknown to the wallet forever; now, because import precedes the state-transition write, on-disk state can never outpace the side effect it commits.
Event deduplication. Previously it was possible for a restart-triggered re-fire of a mint event to log the same logical event twice and double-count the supply-commit update stream; now, because each event is keyed by a content hash with a uniqueness constraint, the second insert is silently absorbed by the schema.
No orphaned state-machine transitions. Previously it was possible for a deduplicated event to leave an empty supply-commit transition wedged in UpdatesPending; now, because the insert transaction rolls back whenever it detects a no-op dedupe, no empty transition can ever land.
Snapshot isolation. Previously it was possible for a subscriber holding a "snapshot" of a batch to share substructure with the live caretaker and observe mid-flight mutations through it; now, because copies are genuinely deep, snapshots are real isolation barriers.
Request/response binding. Previously it was possible for a cancel response to land on a different cancel request than the one that produced it, because the matching depended on a serialization discipline rather than the protocol itself; now, because each request carries its own reply channel, the binding is intrinsic.
Shutdown safety. Previously it was possible for a caretaker reaching completion in the same instant the planter was stopping to deadlock the entire shutdown path; now, because the completion send is abandonable on Quit, no notification can outlive its receiver.
Of the net ~2500 LoC added, roughly 900 lines are in added tests (mostly state machine property tests, IIRC), 800 are around reorganization (stuff that moves various 'complecting' machinery into other/new packages), and the rest is mostly in interface scaffolding + singleton-invariant/dedup/repair-flag machinery (the repair flag being a safety mechanism for old databases that disobeyed the implicit singleton invariant to upgrade).
So yes, It's huge, but: it passes our tests, plus new ones, and IMO enough LLM-enhanced eyes on it should be able to vet it appropriately.