Finalize SQL Implementation#1291
Conversation
Coverage Report for CI Build 29839445611Warning No base build found for commit Coverage: 57.464%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
16e8413 to
b23830b
Compare
fbc57ec to
da96ff8
Compare
b23830b to
ecb2ba3
Compare
da96ff8 to
13f78bd
Compare
ecb2ba3 to
7341df7
Compare
Export the wallet birthday safety margin as BirthdaySafetyMargin so the runtime store construction path can back a requested birthday off by the same duration the legacy Create path uses, instead of hardcoding the value in two places.
Define backend-specific runtime database settings, validation, and safe defaults before wiring them into wallet construction.
Add backend-neutral runtime store handles and open paths for KVDB, SQLite, and PostgreSQL without changing wallet construction yet.
Construct unpublished wallets from the selected runtime store while retaining backend-specific legacy managers only for KVDB.
Add per-name construction guards so waiters cannot observe or rebuild a wallet before its owner publishes a complete instance.
Add SQL runtime wallet creation and secret seeding helpers, and reject watch-only master xpub creates that cannot yield accounts.
Route signer, recovery, rescan, transaction-read, and account paths through Store. The shared stable account identity changes must move together because recovery snapshots, target resolution, and scan persistence use the same key.
Switch Manager and Config to runtime stores, migrate construction tests and harnesses, and remove the legacy db_ops implementation after its production callers have moved behind Store.
Set the wallet DBConfig with an explicit kvdb backend and the harness walletdb path so the manager integration test drives the store-backed construction path.
Make terminal teardown single-owner and ordered: clear secret material, evict the manager cache entry, close stores, record the close result, then publish Stopped to all waiters.
Serialize same-name Load calls with the manager construction guard, and hold that guard through teardown so fresh stores cannot open before the prior instance closes.
Guard forward-only synced-tip updates. Preserve the last known-good tip when Store reads fail. Expose TxStatus through wallet for migrated runtime callers.
Regenerate watch-output scope queries and add the create-complete wallet column. Existing SQL wallets migrate as complete while new creates can persist an incomplete state explicitly.
Key targeted rescans by durable account IDs. Cover SQL and kvdb behavior for imported and derived accounts.
Persist incomplete state before account initialization and mark it complete only after initialization succeeds. Complete wallets reject over-create before mutation, while incomplete SQL and KVDB wallets replay missing accounts after strict identity checks.
Add a Manager-scoped registry that normalizes SQL store identities and hands create, load, and recovery paths reference-counted leases. Serialize pool opens and final closes so concurrent wallets share one store without closing peers or opening duplicate connection pools.
Bind incomplete creation to a durable normalized account manifest. Reject mismatched retries and incomplete loads. Complete only the original initialization plan.
Coordinate DB-wide SQL rollbacks across wallets sharing one runtime store. Quiesce syncers before reset, preserve genesis on full resets, and serialize global rewinds against ordinary SQL writes. Reject invalid rescan anchors and immediately restart syncers after a coordinated reset without lifecycle churn.
950e3de to
99030fe
Compare
|
No issues found by |
|
|
||
| r.mu.Unlock() | ||
|
|
||
| <-done |
There was a problem hiding this comment.
The opening wait here blocks on <-done with no ctx.Done() branch, while the entry.closing wait right below does select on the caller's context — a caller whose context is canceled during a slow or stuck pool open stays blocked until the open finishes; make the opening wait context-aware like the closing one. k3
| // metadata and not an ordinary block or an uninitialized block reference. | ||
| func ValidateGenesisBlock(genesis Block) error { | ||
| if genesis.Height != 0 { | ||
| return fmt.Errorf("%w: genesis height must be zero", ErrInvalidParam) |
There was a problem hiding this comment.
This line is 85 columns (limit is 80, counting a tab as 8), and the overflow repeats across the new code: block_common.go:34 (82), runtime_store_registry.go:143 (85)/:166 (84), reset_coordinator.go:127 (83), syncer.go:277/:486 (83), manager.go:376 (83) and ~15 more at 81-82; rewrap them. k3
|
No issues found by |
|
|
||
| c.mu.Unlock() | ||
|
|
||
| err := req.apply(full, height, genesis) |
There was a problem hiding this comment.
Participants remain syncStateSynced throughout quiescence and this DB reset, so CreateTransaction and FundPsbt still pass validateSynced and can select stale UTXOs because reads are not write-gated; mark every participant unsynced before cancellation and keep it unsynced through restart. (gpt-5.6-sol)
|
|
||
| return lease, nil | ||
|
|
||
| default: |
There was a problem hiding this comment.
After a failed open, opening is false while openErr and old reservations remain, so a fresh acquire enters this branch and clears that generation before its waiters finish, letting them return an uncounted or nil replacement lease; wait for failed reservations to drain or tag reservations by generation. (gpt-5.6-sol)
| ) | ||
| } | ||
|
|
||
| digest := sha256.Sum256([]byte(runtimeCfg.Postgres.DSN)) |
There was a problem hiding this comment.
Hashing the raw DSN gives equivalent URL, keyword, or reordered-option spellings separate pools and reset coordinators for the same PostgreSQL database, allowing one alias to write during another alias’s coordinated rollback; derive identity from a canonicalized parsed configuration. (gpt-5.6-sol)
| c.mu.Lock() | ||
|
|
||
| for syncer := range c.syncers { | ||
| syncer.resetAfterChainReset() |
There was a problem hiding this comment.
ResetChain rewrites every affected wallet’s persisted birthday, but this callback only resets syncer state, leaving Wallet.birthdayBlock stale so Controller.Info can report a block the reset deleted indefinitely; refresh each participant’s wallet-level birthday cache before restart. (gpt-5.6-sol)
| ) | ||
| } | ||
|
|
||
| err = verifyCreateManifest(w.createManifest, manifest) |
There was a problem hiding this comment.
The manifest excludes the private passphrase and only spendable SQL account seeding authenticates it, so watch-only shell and KVDB seed/xpub retries can complete with a passphrase that cannot unlock the persisted secrets; authenticate the retry passphrase before marking creation complete. (gpt-5.6-sol)
| errNilTxDetailMsgTx = errors.New("tx detail MsgTx is nil") | ||
| ) | ||
|
|
||
| // TxStatus is the wallet-relative validity state of a transaction, exposed on |
There was a problem hiding this comment.
This unrelated public TxStatus API addition was previously isolated but is now folded with two sync-tip fixes into the 838-line wallet: harden runtime sync state commit; restore it as its own atomic commit for reviewability and safe reverts. (gpt-5.6-sol)
| func (s *Store) execWrite(ctx context.Context, | ||
| fn func(*sqlc.Queries) error) error { | ||
|
|
||
| s.writeGate.RLock() |
There was a problem hiding this comment.
This RLock is acquired before ctx is observed, so a canceled write blocked behind a detached or stuck chain reset cannot return until that reset finishes; use a context-aware gate here and in the SQLite mirror. (gpt-5.6-sol)
3 participants
Finalizes the store-backed (SQL) wallet implementation. Adds the kvdb legacy
lifecycle/store adapters, the runtime
DBConfig(backend selection) and syncstate, and routes the legacy manager, signer, and syncer paths through the
db.Storeinstead of a rawwalletdbhandle.buildWalletthen constructs andruns the wallet from the selected store — kvdb via the legacy adapters, or a SQL
store — with recovery and full unit coverage.
Config.DBis retyped from a rawwalletdb.DBhandle toDBConfig. Becausethat retype fuses every
cfg.DBconsumer with the mutually-dependentconstruction pipeline, the
wallet: construct store-backed runtime walletcommit is intentionally atomic (no smaller subset compiles); the surrounding
commits stay small and each builds and test-compiles independently.
Stacked on
impl-secret-store.