wallet: SQL-only signer account-secret fallback#1254
Open
yyforyongyu wants to merge 785 commits into
Open
Conversation
Rename the pg package's private helpers, adapters, and row conversion types so they rely on the package namespace instead of repeating pg in every identifier. This keeps the backend-specific package easier to scan now that the split already provides the backend context.
Rename the sqlite backend's exported store API to the package-local Store, Config, and NewStore names now that the backend lives in its own package. The sqlite config type and tests also move into the sqlite package so callers import the backend package for its concrete constructor surface. This removes package stutter at sqlite call sites while keeping the shared db package focused on backend-neutral helpers and errors.
Rename the pg backend's exported store API to the package-local Store, Config, and NewStore names now that the backend lives in its own package. The postgres config type and tests also move into the pg package so callers import the backend package for its concrete constructor surface. This removes package stutter at pg call sites while keeping the shared db package focused on backend-neutral helpers and errors.
Allow wrapcheck in the split pg and sqlite backend packages while the shared helper layer still sits in wallet/internal/db. This keeps lint signal elsewhere without forcing wrapper noise into the mechanical split series.
Require callers to pass explicit, non-zero page limits and remove shared default and max-limit policy from the page package. This keeps page focused on request/result semantics while stores own query sizing and validation. Use pointer-style After and Next tokens so manual pagination can forward the next page token directly without HasAfter or HasNext plumbing. Keep result assembly in page, update wallet and address stores to fetch limit+1 rows explicitly, and refresh tests and docs to match the simpler API.
Hide the page limit behind Request construction so callers cannot set it to zero with struct literals while still allowing After to remain a plain continuation token. This keeps page-by-page usage direct while pushing limit validation closer to request creation instead of relying solely on store entry points. Use page.NewRequest in pagination tests and route store code through Request.Limit so SQL builders and result assembly continue to use the same positive limit contract. Keep the store-side zero-limit checks as defensive validation for zero-valued requests.
bitcoind v30 enables v2 transport by default, while the shared btcd\nminer in the integration harness still runs in legacy transport mode.\nThis can leave the bitcoind backend stuck at height 0 during startup.\n\nPin bitcoind to v1 in the harness so the backend connects\ndeterministically during CI and local bitcoind itests.
Keep non-SQL domain errors unchanged in shared normalization while still preserving existing SQL wrappers and transport classification.
Add shared runtime stats types and expose them through the main store interface so callers can read retry, health, ambiguous commit, and SQL error counters. Non-SQL stores return an empty runtime snapshot.
Add sqlite-local runtime adapters and store state so later call-site changes can share read retry, SQL error classification, unhealthy-store tracking, and write execution semantics.
Switch sqlite mutating store operations to the package-local runtime write helper so transaction execution, SQL error classification, and ambiguous commit handling stay centralized.
Switch sqlite read-only store operations to the package-local runtime read helper so retry policy and SQL error classification stay centralized without direct query plumbing in each method.
yyforyongyu
force-pushed
the
impl-signer-account-secret
branch
from
59c3541 to
a1832b2
Compare
yyforyongyu
commented
Jun 4, 2026
yyforyongyu
left a comment
yyforyongyu
left a comment
Collaborator
Author
There was a problem hiding this comment.
LGTM (deepseek-v4-pro)
yyforyongyu
commented
Jun 4, 2026
yyforyongyu
left a comment
yyforyongyu
left a comment
Collaborator
Author
There was a problem hiding this comment.
LGTM (deepseek-v4-pro)
Collaborator
Author
|
LGTM 🔑 |
Collaborator
Author
|
LGTM 🐿️ ( |
yyforyongyu
force-pushed
the
impl-signer-account-secret
branch
from
a1832b2 to
9206c0c
Compare
Collaborator
Author
|
LGTM 🔑 |
yyforyongyu
commented
Jun 8, 2026
| // one account. The returned EncryptedPrivateKey is still ciphertext; | ||
| // callers must decrypt it through the wallet key vault before deriving | ||
| // private child keys. | ||
| GetAccountSecret(ctx context.Context, query GetAccountSecretQuery) ( |
Collaborator
Author
There was a problem hiding this comment.
i am questioning why do we need this method - when the wallet is unlocked, we will cache the secrets in the cache? thus that whenever we need to create secrets, we will use the cache to do so?
in addition what is the relationship among account secret, wallet secret, keyvault and cache?
Collaborator
Author
|
On the account-secret / cache / keyvault question: I think the durable encrypted account secret is still needed for SQL-only accounts, but the long-term domain boundary should be keyvault rather than signer or general wallet code fetching Current state in this branch:
Ideal shape:
So I agree this should not become the permanent public AccountStore-facing signing API. For this PR, either we keep |
Introduce GetAccountSecret on db.AccountStore so the wallet signer can read the encrypted account-level signing material that SQL backends persist for SQL-only accounts. The kvdb backend returns db.ErrAccountSecretUnavailable because its signing path still resolves through the legacy waddrmgr address manager. Add the matching pg/sqlite sqlc queries, account_secrets itest coverage for both SQL backends, and a GetAccountSecret method on the shared walletmock.Store so wallet-side tests can stub the new contract.
Add GetAccountSecret to runtimeCache so wallet-side consumers can read SQL-backed account signing material through a single boundary. The storeRuntimeCache implementation is a pass-through today, marked with the same TODO(yy) as the other cache reads so the cache layer can grow typed error wrapping later without churning callers.
When waddrmgr reports an account or scope miss, the legacy DeriveFromKeyPathCache fallback also fails for accounts that only live in the SQL store. Fall back to the encrypted account-level private key fetched from the store cache, decrypt it through keyVault, derive the requested branch and index, and zero intermediate key material before returning. Add SQLite-backed signing coverage for a SQL-only derived account in signer_test so the fallback exercises a real SQL store end-to-end and the existing kvdb-backed coverage keeps regressing the legacy path.
yyforyongyu
force-pushed
the
impl-signer-account-secret
branch
from
9206c0c to
b0f6271
Compare
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
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.
Supersedes #1249, which auto-closed when its base branch impl-wallet-metadata-store was deleted; rebased onto sql-wallet. Adds the account-secret lookup contract, exposes store account secrets through the cache, and derives SQL-only account private keys via the keyVault in the signer.