wip: setup ll linter#1244
Draft
GustavoStingelin wants to merge 691 commits into
Draft
Conversation
Tidy the root and each real nested module while continuing across modules and failing at the end if any tidy run errors. This prevents silent submodule failures and avoids collapsing nested modules under a single parent path.
`make tidy-module` runs `go mod tidy` in the repository root and in each
nested Go module, including `./wtxmgr`. Since `wtxmgr` has its own
`go.mod`, it must be tidy-able as an independent module.
Before this change, `wtxmgr/log.go` imported
`github.com/btcsuite/btcwallet/build`, which lives in the root module
rather than the `wtxmgr` module. That creates a cross-module dependency:
when `go mod tidy` runs inside `./wtxmgr`, it tries to resolve
`github.com/btcsuite/btcwallet/build` as if it were available from the
published parent module, and the tidy step fails.
The logger initialization did not actually require the root `build`
package here. The previous code called `build.NewSubLogger("TMGR", nil)`,
and with a nil sublogger factory that helper falls back to
`btclog.Disabled`. Initializing `wtxmgr` directly with `btclog.Disabled`
therefore preserves the same default behavior: logging remains disabled
until a parent package installs a logger explicitly via `wtxmgr.UseLogger`.
This keeps `wtxmgr` self-contained for module tidy/build checks while
preserving the package's runtime logging behavior.
SQLite does not have a dedicated native timestamp type, so consistent timestamp behavior depends on how the driver serializes Go `time.Time` values. The SQL wallet schema already mixes: - application-supplied `time.Time` values (for example birthday timestamps), - and SQLite-generated timestamp values such as `current_timestamp` for metadata fields like `created_at` and `updated_at`. Set `_time_format=sqlite` on the sqlite DSN so the driver encodes and decodes `time.Time` using SQLite's expected timestamp layout. This keeps round-tripping predictable and makes app-written timestamps consistent with SQLite-generated timestamp values across the schema.
Postgres 18 can start listening on 5432 before it is ready to accept SQL queries, which makes the shared postgres itest container race with CREATE DATABASE during CI. Wait for a successful SQL round trip instead of a listening port so the postgres integration tests do not fail with connection resets or unexpected EOFs during setup.
Working on SQL migrations and queries currently requires separate sql-lint, sql-format, and sqlc invocations, and the linting steps can rewrite SQL after code generation. Add make sql to run linting, formatting, and generation in one place so local development normalizes the SQL first and only regenerates the Go files once from the final contents.
When a parent test creates NewTestStore(t) and its subtests call t.Parallel(), the parent finishes and releases its parallel slot while subtests are still running. Another parent test fills that slot and opens its own store, but the original subtests still hold their store open. This leads to more open store connections than the parallel limit allows, exhausting PostgreSQL connections. Fix by creating NewTestStore(t) inside each parallel subtest so the store lifecycle is tied to the subtest's parallel slot. Table-driven error tests use plain struct params with WalletID injected after wallet creation. Setup funcs in TestGetAddress and TestListAddresses accept store and walletID parameters so each subtest operates on its own isolated database.
bitcoind can race the rpctest miner listener during harness startup and stick at height 0 until a later reconnect, which shows up as a flaky bitcoind v30 itest failure in CI. Poll the miner P2P address before starting external backends so the first outbound peer attempt has a listener to connect to.
The test's stop() method asserted that the queriedPeer channel was empty after shutdown. However, the neutrino work manager can non-deterministically dispatch the same query to multiple peers via retries or worker redistribution, producing extra OnGetData callbacks. This caused sporadic "did not consume all queriedPeer signals" failures. Replace the strict empty-check with a drain loop. Tests that need to verify specific query counts already do so explicitly via assertPeerQueried.
Keep the parallel coverage itests from exhausting Postgres connections without relying on a tighter transient overlap estimate.
Document btcwallet's build, test, and style conventions so coding agents can work consistently in this repository.
Ignore the repo-local .worktrees directory so auxiliary worktree checkouts do not pollute git status.
Clarify the published-status wording, explain the dedicated blockless ListTxns path, and make DeleteTx state the exact unmined statuses it accepts. These comments make the tx store contract easier to read before the method implementations land.
Clarify the transaction-state terminology in the handwritten SQL and migration comments so unmined, invalid, and rollback paths are described consistently. Keep the matching sqlc output in the same commit so the generated query docs stay aligned with the handwritten SQL.
Make CreateTx insert-only, replace UpdateTxLabel with a patch-style UpdateTx API, and switch CreateTx credits to a map keyed by output index. This narrows block/state ownership to UpdateTx, gives duplicate tx inserts a dedicated public error, and simplifies the caller-side credit contract.
Add the wallet-owned invalid-parent lookup used by CreateTx to reject child spends of outputs whose parent transaction is already invalid. Keep the handwritten SQL and regenerated sqlc bindings together so the query contract stays reviewable in one place.
Replace the narrow confirmation helper query with a generic transaction-state update query that can patch block assignment and status together. Keep the handwritten SQL and regenerated sqlc bindings together so the new UpdateTx query contract is reviewed in one commit.
Add the dedicated no-block transaction query used by ListTxns when callers request wallet history without a confirming block. Keeping the handwritten SQL and regenerated sqlc bindings together makes the new read path reviewable without mixing in the Go listing logic.
Fix the sqlite AcquireUtxoLease query so the now_utc parameter is expanded inside the ON CONFLICT update predicate. Without this change sqlc leaves a raw sqlc.arg expression in the generated statement and sqlite rejects the lease write with a syntax error. Keeping the query fix in its own SQL commit makes the following LeaseOutput implementation commit a pure backend-wiring and test change.
Account reads now return balance fields directly, so callers that need an account balance can use GetAccount or the list-account APIs without a separate AccountManager method. Detailed UTXO accounting is covered by the UTXO-facing APIs, making the public Wallet.Balance wrapper redundant on this staging branch.
Wallet.GetAccount now returns *db.AccountInfo and Wallet.ListAccounts plus its ByScope and ByName variants return *AccountsResult with the field shape changed: Accounts is now []db.AccountInfo. Internally the methods still use waddrmgr.AccountProperties + fetchAccountBalances; the new propertiesToAccountInfo helper bridges that legacy shape into the canonical db.AccountInfo at the public boundary, dropping the redundant AccountResult wrapper. The deprecated Wallet.Accounts API uses the same AccountsResult shape; its body is updated to build []db.AccountInfo directly. Subsequent commits route the new methods through the wallet's store layer; with the public surface already shaped around db.AccountInfo the routing commits do not need to introduce a reverse conversion helper.
The imported-account guard added to ListAccountsByName pushed the method over the cyclop limit and made the error-handling stanza harder to read. Move the per-scope name lookup and result assembly into a small helper so the public method stays focused on iterating active scopes. This is a behavior-preserving cleanup before the account-store routing commits replace the legacy lookup path.
The existing UnspentOutputs filters out outputs currently held by an active output lease. Callers reporting a wallet balance to external consumers need a walk that includes leased UTXOs so the returned total reflects the wallet's full UTXO value independent of leasing state. Add a public variant that calls fetchCredits with includeLocked=true and surface it on the wtxmgr.TxStore interface so adapters using the interface (e.g. the kvdb account store) can switch to it for balance reporting while leaving the spendable-only path on the existing UnspentOutputs helper.
Hook the cache + keyVault into createTestWalletWithMocks ahead of the Store-routing commits that depend on the keyVault for derived-account crypto. No production behavior change; the keyVault wiring is test-only at this commit.
Add Name *string to db.BalanceParams so backends can disambiguate
accounts that share AccountNumber == 0 under the imported-account
masking contract. Validate gains two new sentinels:
- ErrBalanceParamsNameWithoutScope — Name requires Scope (account
names are scope-unique).
- ErrBalanceParamsAccountAndName — Account and Name are mutually
exclusive.
This commit is a pure contract addition; no backend or caller uses
Name yet. The follow-up commit wires the SQL backend to honor the
new field; the kvdb backend and wallet layer pick it up when their
respective routing/implementation commits land.
Motivation:
- Imported accounts have AccountNumber == 0 in db.AccountInfo per
the masking contract.
- On SQL backends accounts.account_number is NULL for imported
rows; filtering Balance by Account=0 silently matches the
default derived account, not the imported one.
- On kvdb backends the wallet layer previously needed a
resolveAccountNumber workaround to translate the masked 0
into the real waddrmgr-internal number — leaking the waddrmgr
counter back into the wallet through BalanceParams.Account.
A first-class Name filter resolves both backend issues at the
contract layer, keeping the masking contract intact at the public
boundary.
The pg and sqlite Balance queries gain an optional account_name
filter alongside the existing account_number filter, matching the
new BalanceParams.Name field on db.BalanceParams.
- wallet/internal/sql/pg/queries/utxos.sql: account_name filter
added with sqlc.narg('account_name')::TEXT cast.
- wallet/internal/sql/sqlite/queries/utxos.sql: account_name
filter added with cast(sqlc.narg('account_name') AS TEXT).
sqlc regenerates BalanceParams in both backends to include an
AccountName sql.NullString field. The pg + sqlite adapters thread
params.Name through via a small local nullableStringNS helper.
Behavior:
- params.Name == nil → existing behavior (no name filter).
- params.Name != nil → SQL filter narrows to the named account.
A missing name returns zero rows naturally (the JOIN narrows
to nothing), matching the contract's "missing name → zero
balance" rule.
This fixes the SQL silent-zero bug for imported accounts at the
store layer: before this change, querying Balance for an imported
account passed Account=0 (the masked value) which matched the
default derived account (account_number=0), not the imported row
(account_number IS NULL). Imported-account balance reads silently
returned 0. With Name, callers (including the still-to-route
Wallet.Balance) get the correct sum.
The kvdb backend picks up params.Name when its Balance impl lands
on the downstream stack; this commit is SQL-only and leaves kvdb's
Balance untouched.
Read the encrypted master HD private key from waddrmgr's main bucket via the new EncryptedMasterHDPriv export. Watch-only wallets are surfaced as db.ErrSecretNotFound. Introduce a narrow kvdbAddrStore interface and an addrManager type-assert helper so kvdb stays decoupled from waddrmgr.AddrStore's full surface.
Translate the GetAccount contract onto waddrmgr's bucket layout: look up the scoped manager, resolve the account number by name or direct value, and materialize a db.AccountInfo via AccountProperties + IsWatchOnlyAccount. Origin maps from the account-level watch-only flag; IsWatchOnly combines wallet- and account-level state. Add loadAccountInfo, resolveAccountNumber, and translateAccountErr helpers shared with later list/rename methods.
Fetch the account snapshot via w.cache.GetAccount and the balance via w.store.Balance. The wallet manager no longer opens a walletdb.View for GetAccount.
Walk every active scoped key manager (or just the filter scope) via ForEachAccount and materialize each row into db.AccountInfo. The waddrmgr-reserved ImportedAddrAccount pseudo-account is skipped. Optional name filter narrows to a single account name.
Wire ListAccounts through cache.ListAccounts + a new assembleAccountsResult helper that pairs cache snapshots with the balance fields the SQL backend now populates on each AccountInfo. ListAccountsByScope/ByName retain their legacy waddrmgr + fetchAccountBalances paths for the moment; subsequent commits route them through the same helper one method at a time. TestListAccounts is updated to mock the cache.ListAccounts call instead of the waddrmgr + UnspentOutputs fan-out.
ListAccountsByScope reuses the assembleAccountsResult helper introduced for ListAccounts, threading the cache snapshot path through the existing scope-validation guard. ListAccountsByName remains on the legacy waddrmgr + fetchAccountBalances path and is switched separately in the following commit. TestListAccountsByScope is updated to mock the cache.ListAccounts call; a new TestListAccountsByScopeUnknownScope pins the waddrmgr.ErrScopeNotFound surface that the validation guard preserves.
ListAccountsByName joins ListAccounts and ListAccountsByScope on the assembleAccountsResult helper. The legacy waddrmgr + fetchAccountBalances path has no remaining callers and is removed along with its supporting types: accountFilter, filterOption, withScope, and scopedBalances. The unit tests that exercised those helpers directly (TestFetchAccountBalances and TestListAccountsWithBalances) go with them; the kvdb store now owns the equivalent coverage for the UTXO-aggregation logic. TestListAccountsByName is updated to mock the cache.ListAccounts call.
Look up the account number (by name or directly), call waddrmgr.AccountStore.RenameAccount, and translate the not-found error to db.ErrAccountNotFound.
Validate the new account name locally, then call w.store.RenameAccount. Preserve waddrmgr.ManagerError semantics so callers using waddrmgr.IsError(err, ...) keep working when the kvdb adapter wraps the underlying manager error via fmt.Errorf. The wallet manager no longer owns a walletdb.Update tx for renames. TestRenameAccount lands in this commit alongside the routing change, mocking w.store.RenameAccount + the not-found / invalid-name surfaces.
Implement Store.Balance by walking the wallet's unspent outputs and applying the BalanceParams filters. The walk uses UnspentOutputsIncludingLocked so the kvdb total includes leased outputs (matching the SQL backend semantics). Per-UTXO classification is split out as classifyBalanceUTXO + balanceFilterAllows for cyclop; confirmation gating lives in confsAllowed + calcConfs.
Run the shared CreateDerivedAccountWithOps workflow on top of waddrmgr's bucket layout. The createDerivedAccountOps adapter caches the FetchScopedKeyManager result, allocates the next account number via AllocateDerivedAccountNumber, and persists the wallet-derived material via PutDerivedAccountWithKeys inside the same walletdb tx, so a deriveFn or persistence failure rolls the counter back. `accountstore_timestamps.go` gains the `putAccountCreatedAt` helper for the side-bucket creation timestamp the kvdb path persists.
Build the AccountDerivationFunc closure via buildAccountDeriveFn and call w.store.CreateDerivedAccount. The store invokes the deriveFn after allocating the account number; the wallet no longer needs a walletdb.Update transaction for NewAccount. Preserve waddrmgr.ManagerError so callers using waddrmgr.IsError(err, ...) keep working when kvdb wraps the underlying manager error. The test fixture introduces stubAccountDeriveFn + helpers to drive the deriveFn build path through the mock keyVault + GetEncryptedHDSeed. TestNewAccount covers both the success and ErrDuplicateAccount surfaces.
Route kvdb account imports through waddrmgr's existing NewAccountWatchingOnly path inside one walletdb.Update transaction. This keeps legacy kvdb behavior intact: imported account-level xpubs use the normal per-scope account counter rather than a new fixed imported slot. Validate the imported xpub up front, reject private account material that waddrmgr cannot store, support dry-run rollback after deriving sample external/internal addresses, and avoid registering missing scopes during dry runs. This extends the shared imported-account store contract with DryRun and AddrSchema. PostgreSQL and SQLite are updated in the same commit so all store implementations honor the same dry-run rollback and missing-scope schema override semantics.
Route the spendable-key import path through w.store.CreateImportedAccount. Wallet.ImportAccount now returns *db.AccountInfo; the AccountManager interface entry follows. The dry-run and per-account address-schema (non-default BIP49+) paths stay on the legacy waddrmgr walk via importAccountLegacy: the store-backed creator does not yet expose a rollback simulation hook, and db.CreateImportedAccountParams does not yet carry a per-account AddrSchema. The legacy walk uses propertiesToAccountInfo at the boundary so the public API still returns *db.AccountInfo. TestImportAccount mocks w.store.CreateImportedAccount on the non-dry-run + default-schema happy path.
kvdb wallets can delete the master HD private key with waddrmgr.Manager.NeuterRootKey after the default scopes are created. The legacy NewAccount path still works in that state because it derives new accounts from the scoped coin-type private key, not the root key. Defer db.ErrSecretNotFound from the wallet derive callback and let the kvdb adapter fall back to ScopedKeyManager.DeriveAccountKeys once the account number has been allocated. Keep SQL backends on the existing root-key callback path.
Coverage Report for CI Build 26420334908Warning No base build found for commit Coverage: 53.664%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
yyforyongyu
force-pushed
the
sql-wallet
branch
2 times, most recently
from
8139f20 to
90eaacc
Compare
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
3 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.
WIP