db: derive derived-account master fingerprint from wallet record#1241
Draft
yyforyongyu wants to merge 664 commits into
Draft
db: derive derived-account master fingerprint from wallet record#1241yyforyongyu wants to merge 664 commits into
yyforyongyu wants to merge 664 commits into
Conversation
This is a transitional abstraction while the wallet remains monolithic.
Introduce kvdb-backed Store implementation scaffolding.
The wallet now carries a monolithic db.Store for manager access and initialize the wallet's Store using the kvdb backend.
Use a single Store mock in tests to match the monolithic wallet store.
Route ReleaseOutput through the wallet Store abstraction.
Add unit tests for the kvdb-backed Store implementation.
Benchmarks should not lazily initialize the wallet Store.
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.
Add NamespaceKey, (*Manager).EncryptedMasterHDPriv, and (*Manager).MasterHDPubKey so external adapters (kvdb) can locate the address-manager bucket and read the master HD key material without depending on waddrmgr internals.
Add ScopedKeyManager.AllocateDerivedAccountNumber and PutDerivedAccountWithKeys so kvdb's CreateDerivedAccount can run the shared db.CreateDerivedAccountWithOps workflow on waddrmgr's bucket layout without going through the legacy mixed-concern NewAccount.
Add ScopedKeyManager.AllocateImportedAccountNumber and PutWatchOnlyAccountWithKeys so kvdb's CreateImportedAccount can persist watch-only rows via pure DB steps instead of the legacy NewAccountWatchingOnly wrapper.
Expand db.AccountInfo with PublicKey (plaintext per the sqldb stack), MasterKeyFingerprint, and AddrSchema fields. Delete the parallel AccountProperties type so AccountInfo becomes the single canonical account read model. Rename AccountPropsRowToProps to AccountPropsRowToInfo and have it return *AccountInfo populated with the new fields. The shared CreateImportedAccount helper and the AccountStore.CreateImportedAccount contract both now return *AccountInfo. The two SQL backends (pg, sqlite) and the kvdb placeholder pick up the new return type. The wallet mock store updates correspondingly. BuildAccountInfo signature is unchanged; the new fields default to zero values when not provided by the lean caller (used by the CreateDerivedAccount workflow that does not yet populate per-account key material — that lands with the deriveFn callback in a later commit).
Add BalanceParams.Scope so the wallet can constrain Balance reads to a single key scope. Legacy wallets allocate account numbers per scope, so the same account number (e.g. 0) can appear under BIP-0049 and BIP-0084 simultaneously; the existing BalanceParams.Account filter alone overcounts across scopes. Callers that pass Account should also pass Scope to make the filter uniquely scoped. The SQL Balance queries gain optional purpose/coin_type narg predicates on both pg and sqlite. The pg/sqlite adapters share a single db.ScopeFilter helper that unpacks the optional *db.KeyScope into the matching sql.NullInt64 args so the predicate shape stays identical across backends.
Add a wallet-private runtimeCache interface and pass-through storeRuntimeCache. The cache is wired alongside w.store in Manager.Load. Reads delegate to db.Store; real caching with bounded invalidation is future work.
Persist the wallet's plaintext master HD public key on `WalletInfo` instead of exposing a dedicated `WalletStore.GetMasterPubKey` read. `GetWallet` now returns the master pub key alongside the rest of the wallet metadata so a single read serves both the watch-only derivation path and any future `WalletInfo` cache. The doc comment on `GetEncryptedHDSeed` is also tightened to reflect that it returns the encrypted master HD private key (not a raw seed).
…ount Add AccountDerivationFunc and DerivedAccountData to the public AccountStore contract: callers now pass a wallet-supplied derivation callback alongside the existing CreateDerivedAccountParams. The pg, sqlite and kvdb implementations pick up the new signature; the shared CreateDerivedAccountWithOps workflow does not yet consume the callback (the next commit wires the inner Ops contract through to actually invoke and persist the derived material). This commit is a no-op at the storage level — every adapter still calls the existing workflow with the pre-callback signature, so account rows look identical to before. Splitting the outer-contract shape from the workflow lets reviewers track the public API surface in isolation from the internal Ops refactor that follows.
Wire the derivation callback all the way through. The shared CreateDerivedAccountWithOps workflow now invokes deriveFn after allocating the account number, validates the returned material against the wallet's watch-only mode, and forwards the validated DerivedAccountData to the inner ops adapter so each backend persists the same fields. CreateDerivedAccountOps.CreateDerivedAccount gains a *DerivedAccountData parameter; pg and sqlite adapters consume the supplied PublicKey + MasterKeyFingerprint on the accounts row and the EncryptedPrivateKey (when present) on the account_secrets row. Errors errNilAccountDerivationFunc / ErrNilDerivedAccountData / errMissingDerivedPublicKey / errMissingDerivedPrivateKey / errWatchOnlyDerivedPrivateKey distinguish the validation cases. New helpers validateDerivedAccountData, deriveAndValidate, and allocateAndPreviewAccountNumber keep CreateDerivedAccountWithOps under the cyclop budget. The outer AccountStore.CreateDerivedAccount signature was already in place from the preceding commit; pg and sqlite Store.CreateDerivedAccount now forward deriveFn to the workflow instead of dropping it on the floor. kvdb's stub still returns notImplemented — the real impl lands in the kvdb group.
Add newAccountDeriveFn factory + deriveBIP44AccountKey + masterKeyFingerprint helpers in wallet/account_derive.go. The factory returns a db.AccountDerivationFunc closure that derives the BIP44 account key from a captured master HD priv key, encrypts the account priv key via keyVault, and returns DerivedAccountData. Watch-only wallets reject new derivations.
Drop the unused `strings` import in `pg_sanitize_test.go` (only showed up under the `test_db_postgres` build tag) and tighten a doc-comment + wrap a long signature in `fixtures_pg_test.go`. Pure hygiene; behavior is unchanged. Lands here so the new account-store test paths added later in the stack don't need to carry these findings.
Widen the per-container pgMaxConns from 2x to 4x the steady-state pool size. The 2x multiplier proved too tight under heavy parallel test workloads on CI, producing spurious 'too many clients' failures during the parallel CreateWallet/CreateDB subtests in TestListAccounts and friends.
Add `wallet/internal/db/kvdb` to the existing wrapcheck exemption that covers the pg and sqlite backends. These split backend packages intentionally forward many calls into the shared db helpers and the unwrapped pass-through is the intended pattern.
…t reads Review feedback on PR btcsuite#1231 flagged that callers re-reading a derived or imported account through `GetAccount` / `ListAccounts` lose `AccountPubKey` and `MasterKeyFingerprint` compared to the previous waddrmgr-backed `AccountProperties` surface. `AccountRowToInfo` defaults both fields to `nil, 0` because the SQL account SELECTs project only counts, watch-only, and scope fields. Extend the 8 account-returning SELECTs (`GetAccountByScopeAndName`, `GetAccountByScopeAndNumber`, the `Wallet`-scoped variants, plus the 4 `ListAccounts*` queries) with `a.public_key` and `a.master_fingerprint`, mirroring what `GetAccountPropsById` already projects on the heavy path. `AccountInfoRow` widens with the matching `PublicKey []byte` and `MasterFingerprint sql.NullInt64` fields, `AccountRowToInfo` decodes the fingerprint via the same `Int64ToUint32` boundary the props path uses, and the pg + sqlite adapters thread the new sqlc fields into the row. The lightweight read path now matches the `waddrmgr.AccountProperties` contract a wallet-facing caller previously got, without falling back to the full props query. `TestGetAccountReturnsPublicKeyAndFingerprint` and `TestListAccountsReturnsPublicKey` exercise the round-trip across derived and imported accounts on both backends via the shared itest harness.
Widen the shared account-conversion plumbing in preparation for a dedicated balance query. `AccountInfoRow` gains `ConfirmedBalance` and `UnconfirmedBalance` int64 fields. `BuildAccountInfo` takes two matching `btcutil.Amount` parameters and forwards them onto `AccountInfo`. `AccountRowToInfo` reads the new row fields and converts to `btcutil.Amount` at the boundary. Behavior is unchanged: nothing populates the new fields yet. The pg and sqlite adapters still default the row's balance fields to zero because the account-returning SELECTs don't compute balance, so callers see the same zero balance they saw before. The dedicated balance query that fills these fields lands in the next commit; this commit is the type-only prep so the diff that follows is just SQL and adapter wiring rather than mixed-concern edits. `CreateDerivedAccount` continues to invoke `BuildAccountInfo` with `0, 0` since a freshly derived account has no UTXOs.
Add a dedicated single-account balance query on both pg and sqlite,
keyed by `(wallet_id, account_id)`. Returns one `(confirmed,
unconfirmed)` tuple summed from the UTXO set at read time using the
same confirmation predicate the existing `Balance` query in
`utxos.sql` uses: a tx is confirmed when `block_height IS NOT NULL`
and at or below the wallet's `synced_height`. Spent outputs
(`u.spent_by_tx_id IS NULL`) and the `tx_status IN (0, 1)` filter
match the rest of the balance code.
The account-returning `Get/List` SELECTs stay byte-identical with
master — balance is intentionally a separate concern that the Go
adapter dispatches when a caller actually needs it. The Go-side
wiring lands in the follow-up commit.
pg uses `::BIGINT` casts and `$N` positional args; sqlite uses
`cast(... AS INTEGER)` so the sqlc-generated row exposes `int64`
fields rather than `interface{}`.
Wire the pg and sqlite `accountGetQueries.byNumber` / `byName` paths to call the dedicated `AccountBalance` query alongside the account row fetch. Both queries run inside the same `execRead` closure so the row read and its balance lookup share a single read transaction and observe a consistent UTXO snapshot. Expose `SkipBalance bool` on `GetAccountQuery`. When true the adapter skips the balance dispatch entirely; the returned `AccountInfo` keeps its zero balance fields. Identity-only callers (autocomplete UIs, schema audits, internal lookups) avoid the UTXO scan by setting this. The byNumber/byName paths now bypass the generic `db.GetAccount` helper so they can capture `row.ID` (the internal `accounts.id`) that keys the balance query. A new `mapGetAccountErr` helper preserves the typed `ErrAccountNotFound` mapping that `db.GetAccount` provided, named identically on each backend. `TestGetAccountPopulatesBalance` covers populated confirmed + unconfirmed totals across both the by-name and by-number selectors, plus an empty-account control that asserts the balance defaults to zero. `TestGetAccountSkipBalanceZerosFields` asserts the opt-out zeros both fields on each selector even when UTXOs exist.
Add a dedicated batch balance query on both pg and sqlite that returns one `(account_id, confirmed, unconfirmed)` row per funded account in the supplied `account_ids` list, grouped by `account_id`. Scoping the aggregate to the caller's account list avoids summing UTXOs for accounts the caller has already filtered out (e.g. accounts in a different key scope, or unrelated accounts when reading by name). Accounts with no spendable outputs do not appear in the result so the Go caller can default the missing entries to zero. This query pairs with the single-account `AccountBalance` query: the list-style account read paths invoke `AccountBalances` once per `ListAccounts` call to avoid the N+1 pattern of repeating `AccountBalance` for every returned account. Confirmation predicate, status filter, and spent-output exclusion are identical so both queries return the same balance for an account. Same backend conventions as `AccountBalance`: pg uses `::BIGINT` casts and `$N` positional args; sqlite uses `cast(... AS INTEGER)` for sqlc-friendly int64 fields. The Go-side wiring lands in the follow-up commit.
Wire the pg and sqlite `accountListQueries.byScope` / `byName` / `all` paths to dispatch the dedicated `AccountBalancesByIDs` batch query alongside the row fetch. Both queries run inside the same `execRead` closure so the rows and their balances share a single read transaction and observe a consistent UTXO snapshot. Expose `SkipBalance bool` on `ListAccountsQuery`. When true the adapter skips the batch balance dispatch; every returned `AccountInfo` keeps its zero balance fields. Identity-only callers (autocomplete UIs, schema audits, internal lookups) avoid the UTXO scan by setting this. Each list method now calls `db.ProcessAccountRows` to convert the sqlc rows into `[]*db.AccountInfo`, populating each `AccountInfo`'s unexported `rowID` field with the SQL row ID via a per-dialect convert closure. The shared `db.AttachBalances` helper then issues `AccountBalancesByIDs(walletID, ids)` once with those row IDs, builds an `indexByID` map for O(1) slot lookup, and merges each balance into the matching `AccountInfo`. Accounts that the batch query didn't return (no spendable outputs) keep zero balance fields. `TestListAccountsPopulatesBalance` covers the scope and unfiltered selectors with mixed confirmed/unconfirmed UTXOs across three accounts (funded confirmed, funded unconfirmed, empty) and asserts each account's balance is in the result. `TestListAccountsSkipBalanceZerosFields` covers all three list selectors and asserts the opt-out path returns zero balance fields even when UTXOs exist.
Two sources of truth for the BIP32 master-key fingerprint on derived accounts is a drift hazard: `accounts.master_fingerprint` (per-row, populated at write time) and `HASH160(wallets.master_hd_pub_key)[0:4]` (wallet-level, the canonical input) cannot be guaranteed to agree. Same class of issue as the addresses.used cleanup. Make the wallet record the single source of truth for derived rows; keep per-account storage for imported rows where there is no wallet-level analog. Schema: - accounts.master_fingerprint stays nullable; a new CHECK constraint forces NULL for derived rows and NOT NULL for imported rows. - The full SSOT rationale lives in the column comment so future readers see it next to the column rather than via an ADR cross- reference. Queries: - Every read query that returns AccountInfo fields now also selects w.master_hd_pub_key alongside a.master_fingerprint (the wallets JOIN was already present). The shared row adapter uses the pubkey to derive the fingerprint for derived rows; imported rows continue to read the stored per-row value. Row adapter (wallet/internal/db/accounts_common.go): - AccountInfoRow + AccountPropsRow gain WalletMasterHDPubKey []byte. AccountRowToInfo / AccountPropsRowToInfo branch on origin: imported reads MasterFingerprint; derived calls a private masterKeyFingerprintFromExtKeyBytes helper on the wallet's master HD pubkey bytes. The helper matches wallet/account_derive.go:masterKeyFingerprint byte-for-byte so read-time and create-time values cannot drift. A missing or malformed master HD pubkey on a derived row surfaces a wrapped error rather than silently falling back to zero. Write paths: - pg/sqlite CreateDerivedAccount writes NULL master_fingerprint (formerly wrote derived.MasterKeyFingerprint). Schema CHECK enforces the invariant. - CreateImportedAccount unchanged; imported keeps the caller- supplied parent fingerprint. Test fixtures: - wallet/internal/db/itest/fixtures_common_test.go and wallet/internal/db/sqlite/txstore_listtxdetails_test.go previously set MasterPubKey to RandomBytes(32) or single placeholder bytes. After this commit any SQL read that materializes a derived account parses the column via hdkeychain.NewKeyFromString — random bytes would surface a spurious parse error. Both fixtures now use a fixed valid BIP32 test-vector xpub. sqlc-generated row types regenerated via 'make sqlc'.
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.
Summary
Status: draft, nice-to-have cleanup. Not load-bearing for the
account or address store routing — moves the derived-account master
fingerprint to a single source of truth (the wallet HD pubkey)
instead of storing it per-row.
Rationale
Two sources of truth for the BIP32 master-key fingerprint on derived
accounts is a drift hazard:
accounts.master_fingerprint— per-row, populated at write time.HASH160(wallets.master_hd_pub_key)[0:4]— wallet-level, thecanonical input.
In practice these always agree (the per-row value is computed from
the wallet pubkey at write time, and
wallets.master_hd_pub_keydoesn't mutate after wallet create), so the drift hazard is
theoretical rather than observed. This commit closes it anyway by
making the wallet record the single source of truth for derived
rows, while keeping per-account storage for imported rows where there
is no wallet-level analog.
Changes
Schema (pg + sqlite):
accounts.master_fingerprintstays nullable; new CHECKconstraint forces NULL for derived rows and NOT NULL for imported
rows.
readers see it next to the column.
Queries (pg + sqlite):
w.master_hd_pub_keyalongsidea.master_fingerprint(thewalletsJOIN was already present).for derived rows; imported rows continue to read the stored
per-row value.
Row adapter (
wallet/internal/db/accounts_common.go):AccountInfoRow+AccountPropsRowgainWalletMasterHDPubKey []byte.AccountRowToInfo/AccountPropsRowToInfobranch on origin:derived rows derive via the new
MasterKeyFingerprintFromExtKeyByteshelper; imported rows continue to read the stored per-row value
via
resolveMasterFingerprint.processAccountRowsfor batched conversion.Why this is draft
This is not a fix for any reported bug — no caller is currently
demonstrating a fingerprint drift. The PR is parked as a future
cleanup; reviewers can take it on its own time without it blocking
the account-stack routing PRs (#1240, #1236, #1239).
Where this came from
Originally lived on the address-branch (
impl-address-manager-store)where it had been carried along since the OLD account-stack history.
After review during the stack restructure, the conclusion was that
this commit doesn't belong on the address branch (no address-stack
commit references the symbols it adds). Splitting it out here keeps
the address-stack focused on address-store routing.