wallet+db: add GetAccountSecret Store surface and keyvault routing#1272
Draft
yyforyongyu wants to merge 119 commits into
Draft
wallet+db: add GetAccountSecret Store surface and keyvault routing#1272yyforyongyu wants to merge 119 commits into
yyforyongyu wants to merge 119 commits into
Conversation
Coverage Report for CI Build 28320808284Warning No base build found for commit Coverage: 56.298%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
yyforyongyu
force-pushed
the
impl-runtime-store-5
branch
from
449c243 to
e919fcb
Compare
yyforyongyu
force-pushed
the
impl-secret-store
branch
from
7eee2d2 to
63a7a7d
Compare
Collaborator
Author
|
cc @GustavoStingelin for taking over, I think we can open a new PR instead, as this PR is only a draft. |
yyforyongyu
force-pushed
the
impl-runtime-store-5
branch
from
e919fcb to
e8d5e1a
Compare
yyforyongyu
force-pushed
the
impl-secret-store
branch
2 times, most recently
from
c70b257 to
f572dca
Compare
yyforyongyu
force-pushed
the
impl-runtime-store-5
branch
2 times, most recently
from
b7b60d7 to
a354b6c
Compare
yyforyongyu
force-pushed
the
impl-secret-store
branch
from
f572dca to
370e20c
Compare
yyforyongyu
force-pushed
the
impl-runtime-store-5
branch
from
a354b6c to
0908fc0
Compare
yyforyongyu
force-pushed
the
impl-secret-store
branch
from
370e20c to
315a429
Compare
yyforyongyu
force-pushed
the
impl-runtime-store-5
branch
from
0908fc0 to
91960b1
Compare
yyforyongyu
force-pushed
the
impl-secret-store
branch
from
315a429 to
7c30a37
Compare
yyforyongyu
force-pushed
the
impl-runtime-store-5
branch
from
91960b1 to
edba65b
Compare
yyforyongyu
force-pushed
the
impl-secret-store
branch
2 times, most recently
from
4b277a5 to
de9ddb6
Compare
yyforyongyu
force-pushed
the
impl-runtime-store-5
branch
8 times, most recently
from
a5924bd to
edea01b
Compare
Cover that a relevant transaction carrying both a wallet-owned and an unrelated third-party output credits only the owned output.
Add the helpers that convert recovery scan results into Store batch params: horizon merging, horizon flattening, per-output tx params, and the scan-result block shape.
Write recovery scan batches through db.Store so rescans persist tx notifications, horizons, and synced blocks through one store operation.
Route targeted recovery scan results through the Store batch API without advancing the wallet's synced blocks.
Read the synced tip through syncedTip so checkRollback, advanceChainSync, and scanWithRewind use the Store's tip rather than the legacy addrStore tip in store-backed mode.
scanHorizonParams emitted db.ScanHorizon with an empty AccountName, so ApplyScanBatch fell back to numeric account resolution. That is unsafe for imported and imported-xpub accounts, whose Store-facing numbers are masked to 0 and collide with the default derived account. Thread the *RecoveryState into storeScanBatchParams and stamp every emitted horizon with the durable account name resolved from the recovery state, keeping the numeric fallback only for unexpected missing names.
Add coverage proving a confirmed chain.RelevantTx notification builds an ApplyTxBatch whose transaction carries the confirming block while SyncedTo stays nil, so the standalone notification records the confirmed tx without advancing the wallet sync tip.
Add store-backed reads for recovery scan account horizons: ListAccounts for full scans and GetAccount for targeted scans, converting store account metadata into RecoveryState horizon shape. Every imported account is withheld from the horizons, since its number is masked to 0 and would collide with the default derived account in the by-number recovery lookahead. The wiring of these readers into the scan-state loader follows in a later commit.
Cover storeScanHorizons over both the full-scan ListAccounts path and the targeted GetAccount path, including that imported accounts are withheld from the emitted recovery horizons.
Add store-backed loading of active recovery scan addresses through paginated ListAddresses queries, extracting one address per stored script.
Add store-backed loading of UTXOs that recovery scans should watch through ListOutputsToWatch, converting each store UTXO row into the recovery scan credit shape.
Aggregate store-backed horizon, address, and watch-output reads into loadStoreScanData and route wallet and targeted scan-data loading through the store when store wiring is available.
The Store-backed targeted rescan resolved each AccountScope by number in storeScanHorizons. Both Store backends mask an imported account's number to 0, so an imported-xpub target was indistinguishable from the default derived account 0 and could seed the wrong horizon. Resolve the public targets into identity-aware scanTargets up front through the legacy manager, which still keys accounts by their non-masked number. The keyless imported-address bucket is skipped before any Store lookup; every other target carries its durable AccountName, and the Store horizon load prefers the name, mirroring the ScanHorizon contract and the SQL GetHorizonAccount behavior. A number lookup is never issued for an imported target. The legacy walletdb path is unchanged: it still resolves by number, which it does not mask.
Add real-kvdb-backed coverage for the targeted rescan identity fix: a syncer wired over a real, unlocked waddrmgr-backed Store. The keyless imported-address bucket produces no horizon and issues no number lookup (a real by-number lookup would surface ErrAccountNotFound). A setup with the default derived account 0 and an imported-xpub account masked to number 0 resolves the imported target by its durable name, classifies it as imported, and withholds it, so it is never mis-resolved as the default derived account.
The syncer runtime helpers were routed through the transitional Store but kept an "if s.store == nil" branch that fell back to the legacy walletdb DB* helpers, plus two positive "if s.store != nil" scan-data conditionals that did the same. Production newSyncer is always constructed with the wallet's Store, so nil Store is impossible on these migrated paths and the fallback is dead, contradictory design: a migrated runtime path must always use the Store. Remove the nil-store fallbacks from rewindToBlock, syncedBlockHashes, unminedTxns, updateSyncTip, putTxNotifications, putBlockNotifications, putSyncBatch, putTargetedBatch, syncedTip, loadTargetedScanData, and loadWalletScanData so each helper always uses the Store, and rewrite the "store when configured, falling back to the legacy walletdb path" comments to describe the Store-only behavior. The legacy DB* helpers in db_ops.go are left in place for now; they are removed once no non-deferred call site needs them.
newSyncer kept the Store optional behind a variadic syncerStoreConfig, but the migrated runtime paths now always read and write through the Store, so a nil Store is impossible. Drop the variadic and take store and walletID as mandatory positional parameters. Update every newSyncer call site mechanically to pass the Store: production wiring in manager.go, plus the benchmark, db_ops, and syncer test setups. The store-backed test bodies are rewritten in the following commits.
Rewrite the rollback, initialization, run-loop, run-step, wait, and init-chain-sync tests to drive the syncer through the Store instead of the legacy addrStore/walletdb setup: replace setupTestDB and mockAddrStore wiring with walletmock.Store expectations, and add the expectSyncedTip helper that stubs the synced tip via Store.GetWallet. Drop TestLoadScanDataLegacyFallback and TestRewindToBlockFallsBackToLegacy, which covered the now-removed nil-store fallbacks.
Rewrite the scan-strategy, scan-batch, scan-target, advance-chain-sync, fetch-and-filter, dispatch, scan-data loading, and handle-scan-req tests to source horizons, addresses, watched outputs, and the synced tip from the Store via walletmock expectations, replacing the legacy addrStore and walletdb-backed setup.
Rewrite the handle-chain-update, process-chain-update, and broadcast unmined-txn tests to read the synced tip and unmined transactions from the Store via walletmock expectations, and add the serializeTx helper that encodes a transaction for stubbing TxInfo.SerializedTx.
resolveMasterFingerprint read the master HD public key with a direct walletdb.View against the address manager during wallet load. The master public key is public wallet metadata, so route it through Store.GetWallet instead, removing the last direct walletdb transaction for public-metadata reads from wallet/manager.go. To serve the read, add MasterHDPubKey to the waddrmgr.AddrStore interface (already implemented on *waddrmgr.Manager) and have kvdb's GetWallet fill WalletInfo.MasterPubKey from it, tolerating ErrNoExist for shell, watch-only, and pre-master-key wallets. The MasterHDPubKey read is extracted into a small readMasterPubKey helper to keep GetWallet within the complexity limit. The DBCreateWallet/DBLoadWallet create/load path is left as-is: kvdb's Store.CreateWallet is unimplemented and the Store does not return the legacy waddrmgr.Manager/wtxmgr.Store the runtime needs, so that remains justified runtime-construction scope. Passphrase, private-key, and signer flows are unchanged (deferred to ADR 0010). TestManagerLoadSuccess now asserts the loaded wallet's cached master fingerprint is non-zero and matches the create-time value, locking in the Store-routed read.
Add LegacyManagerVault wrapping walletdb.DB and *waddrmgr.Manager, implementing keyvault.Vault by delegating to the legacy address manager. Unlock authenticates the private passphrase inside a walletdb view and ignores the auto-lock timeout, since the controller keeps owning that timer. Lock is void and swallows the idempotent ErrLocked, logging any other failure. IsLocked, Encrypt, and Decrypt forward to the manager, and RefreshPrivatePassphrase is a no-op because the manager rotates its crypto state in place. The shim lives beside the other kvdb legacy adapters and lets later commits route wallet auth and key-material encryption through the vault seam without changing behavior. (cherry picked from commit fec2904)
Add the GetAccountSecret query to the pg and sqlite account query sets and regenerate the sqlc bindings. The query joins accounts to key_scopes and left-joins account_secrets so callers can distinguish a watch-only account (no secret row) from an absent account. Generated code is kept in its own commit, separate from the hand-written Go that consumes it. (cherry picked from commit 99c5994)
Add the AccountSecret result type and the GetAccountSecretQuery selector, plus the GetAccountSecretQuery.Validate helper and the ErrAccountSecretUnavailable sentinel. AccountSecret carries only encrypted private-key material; decryption stays with the wallet key vault. These types back the per-backend GetAccountSecret implementations that follow. (cherry picked from commit dc242ba)
Add the PostgreSQL Store.GetAccountSecret method backed by the generated GetAccountSecret query. It validates the selector, maps the row to the backend-independent AccountSecret, and reports a typed ErrAccountNotFound when no account row matches. The method is not yet part of the AccountStore interface; that wiring lands once every backend implements it. (cherry picked from commit 69c0b75)
Add the SQLite Store.GetAccountSecret method backed by the generated GetAccountSecret query, mirroring the PostgreSQL implementation: validate the selector, map the row to AccountSecret, and surface ErrAccountNotFound for a missing account. Interface wiring lands once all backends implement it. (cherry picked from commit cf0865f)
Add GetAccountSecret to the AccountStore interface now that pg and sqlite implement it, and complete the remaining implementations: the kvdb backend reports ErrAccountSecretUnavailable (legacy secrets stay in waddrmgr), and the testify mock store gains the method. The runtime cache exposes a pass-through GetAccountSecret seam, and an itest covers derived, watch-only, and absent-account outcomes. (cherry picked from commit 21ce20d)
Build the keyVault from kvdb.NewLegacyManagerVault during Load and OpenWithRetry instead of the not-yet-implemented keyvault.NewDBVault stub. The kvdb-open path now flows wallet auth and key-material encryption through a real legacy-backed vault while behavior stays unchanged. The SQL-open path keeps the DBVault stub until its runtime crypto lands. (cherry picked from commit 2d073a5)
Route handleUnlockReq and handleLockReq through the key vault instead of the wallet-owned DBUnlock helper, and delete DBUnlock and its test. Unlock passes a negative timeout so the controller keeps owning its auto-lock timer; Lock is void, with the idempotent ErrLocked swallow now living in the vault. The vault interface has no passphrase-change method, so the passphrase rotation stays in the wallet: handleChangePassphraseReq persists the change through DBPutPassphrase and then asks the vault to refresh its runtime state on a private rotation. The controller tests assert against the vault and the restored DBPutPassphrase path. (cherry picked from commit da88730)
Add deriveStoredAccountChildKey, which decrypts an account's encrypted private key through the key vault and derives the branch/index leaf key while zeroing intermediate material, and isWaddrmgrAccountClassError for classifying waddrmgr scope/account misses. Also add the ErrWatchOnlyAccount and ErrAccountNotInStore sentinels. These primitives back the store-backed signer fallback wired in next. (cherry picked from commit be8bb18)
Add derivePathPrivKey and resolveDerivedPrivKeyFromStore, and thread a context through the signing paths (ECDH, SignDigest, ComputeRawSig, DerivePrivKey and the output/imported-key helpers). When the legacy waddrmgr lookup misses on a scope or account, resolve the account's encrypted private key from the Store and derive the leaf key through the key vault, with precise watch-only and not-in-store errors. Covers SQL-only accounts that have no mirrored waddrmgr row. (cherry picked from commit de9ddb6)
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.
Adds the account-secret surface to the runtime
db.Store: theGetAccountSecretquery, secret types, sqlite/pg implementations, and the store-contract method.
Also adds the legacy manager keyvault shim and routes wallet unlock/lock and
signer key derivation through the keyvault (store-backed derivation primitives,
with a legacy-miss fallback).
Split out of the secret Store PR for reviewability; content-preserving, no
commit changes. Stacked on
impl-runtime-store-5.