lwwallet: optional SQLite backend for the wallet database - #1216
Open
guggero wants to merge 7 commits into
Open
Conversation
ForAll returns as soon as the caller's callback fails, which leaves the *sql.Rows open. The deferred cancel only releases the query's timeout context, so the result set keeps holding a connection from the pool for the rest of the process lifetime. With the wallet backend limited to a single connection, a handful of aborted bucket walks is enough to wedge every later query. The loop also treated a row error as a clean end of iteration, so a result set truncated by a scan or driver failure was reported to the caller as a fully enumerated bucket. Return rows.Err() so a partial walk surfaces as an error instead.
Every file in this package carries a `js && wasm` build tag, so it is
invisible to `golangci-lint`, which runs against the host platform. A
handful of findings accumulated behind that tag:
- `err == sql.ErrNoRows` comparisons that a wrapped driver error would
slip past (errorlint), replaced with `errors.Is`.
- `strings.Replace` with n = -1 instead of `strings.ReplaceAll`.
- `fmt.Sprintf` calls over a plain concatenation with no format verbs
and no arguments.
- `getPrefixedTableName`, dead since the table name moved into the
`db` struct.
- A `walletdb.ReadTx` type assertion on a `walletdb.ReadWriteTx`,
which already embeds that interface.
None of this changes behavior. Fixing it first keeps the following
commit, which drops the build tags and brings the package under the
linter, a mechanical one.
Nothing in this package is browser-specific: it is a `walletdb` implementation over `database/sql`, and the driver, DSN and pragmas are all supplied by the caller. The build tags record its first consumer — the browser wallet store, which needed a `walletdb` backend because BoltDB is unavailable under js/wasm — not an actual platform restriction. Dropping them lets native builds put btcwallet's wallet database on a SQL driver as well, which is what an embedded daemon on a mobile platform needs: one storage engine for all daemon state, in a directory the host application chooses, with no mmap'd BoltDB file to exclude from the platform's file-backup machinery. It also stops the package from being a blind spot for the tooling. Both `go build ./...` and `golangci-lint` only ever see the host platform, so until now neither covered this code. No behavior change for existing builds: no native code imports the package yet.
The sqlbase wiring for btcwallet's wallet store — connection-set initialization, table prefix, query timeout and the transaction-level lock — is not browser-specific, but it currently lives in the browser-only wallet database file. Move it into an untagged openSQLWalletDB helper so the platform files are left with only what is genuinely platform-specific: the driver name and the DSN. The next commit adds a second caller for native builds. No functional change: the browser path opens the same database with the same settings.
Config.DBBackend selects the engine btcwallet's wallet database runs on: "bolt" (the default, unchanged) keeps the classic wallet.db BoltDB file, "sqlite" keeps wallet.sqlite.db on modernc.org/sqlite through internal/sqlbase — the same walletdb implementation the browser build already uses. Both databases live under the existing Config.DBDir, so the choice does not move any state out of the directory the caller already controls. The motivation is running the daemon embedded in a mobile application. There the host application dictates which directory the platform includes in its cloud backup, and everything in that directory should be one storage engine a backup can capture consistently. BoltDB's mmap'd, free-list-bearing file is the odd one out among the daemon's databases: it has to be special-cased by whatever copies the directory, and it is the reason a daemon that otherwise speaks only SQL still links BoltDB. Pointing the wallet at SQLite too removes that exception. The two backends deliberately use different file names, and resolution fails when the other backend's file is present in DBDir. Switching the backend of an initialized wallet directory would otherwise be indistinguishable from a first start, and btcwallet would create a second, empty wallet beside the existing — possibly funded — one. The existence probe answers from the database file's presence for both backends, which is what btcwallet's own probe for a local database already does. Keeping it free of side effects matters more for a SQL backend than for BoltDB: opening the database to look inside would create the very database the caller is asking about, and hold its write lock until the handle is closed again. The create/open lifecycle test is parameterized over the backends the platform offers, so the SQLite store has to satisfy the same contract as BoltDB rather than only the parts a new test remembers to cover.
newRootCmd registers the wallet flags inline while every other group of daemon flags has its own registrar, and at 200 lines the function is at the limit funlen enforces — the next flag added anywhere in it fails lint. Move the wallet block behind registerWalletFlags, following registerArkServerFlags and its siblings. No flag, default or help text changes.
Expose lwwallet's wallet database backend selection as a daemon config option and CLI flag, so the network directory can be made to hold SQLite databases only. The default is unchanged. The value is rejected at config-validation time rather than passed through blindly: an unknown backend would otherwise only surface when the operator first tries to create or unlock the wallet, which for an auto-unlocking daemon means a failed startup with the cause several layers down. The option is deliberately not settable per wallet lifetime: lwwallet refuses to resolve a backend whose counterpart's database file is already present, so a change on an initialized network directory fails with the file it found rather than creating an empty second wallet.
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
1 participant
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.
Lets
lwwalletkeep btcwallet's wallet database in SQLite instead ofBoltDB, selectable via
wallet.dbbackend(bolt, the unchangeddefault, or
sqlite). Both databases live in the network directory asbefore.
Why
For a daemon embedded in a mobile application, the host application
decides which directory the platform includes in its cloud backup, and
everything in that directory should be one storage engine a backup can
capture consistently. BoltDB's mmap'd, free-list-bearing file is the odd
one out among the daemon's databases: it has to be special-cased by
whatever copies the directory, and it is the reason a daemon that
otherwise speaks only SQL still links BoltDB.
The
walletdb-over-database/sqlimplementation this uses is not new —internal/sqlbasehas been backing the browser wallet store all along.It was simply tagged
js && wasm.Commits
sqlbase: close the result set and check row errors in ForAll—standalone bug fix.
ForAllleaked the*sql.Rows, and its pooledconnection, whenever the caller's callback failed, and reported a
result set truncated by a row error as a fully enumerated bucket.
Affects the browser build today.
sqlbase: address linter findings in the package— no functionalchange. The build tags kept the package out of
golangci-lint, whichonly sees the host platform.
sqlbase: drop the js && wasm build tags— nothing in the package isbrowser-specific; the tags recorded its first consumer, not a platform
restriction. Also ends the package's blind spot for
go build ./...and the linter.
lwwallet: extract the SQL wallet database opener— no functionalchange; moves the platform-independent sqlbase wiring out of the
browser-only file.
lwwallet: add a SQLite wallet database backend— the feature.cmd/waved: group the wallet backend flags into a registrar— thewallet flags were the last group still inline in
newRootCmd, whichsat exactly at the
funlenlimit.waved: add wallet.dbbackend for the lwwallet backend— configoption, CLI flag, validation, sample config, CLI guide.
Notes for review
names on purpose, and resolution fails when the other backend's file
is present. Switching backends on an initialized directory would
otherwise look exactly like a first start, and btcwallet would create
a second, empty wallet beside the existing — possibly funded — one.
database file's presence on both backends, as btcwallet's own probe for
a local database does. Opening the database to look inside would create
the very database the caller is asking about and hold its write lock.
_txlock=immediate. The wallet reads before itwrites inside one transaction (deriving the next address reads the
last-used index, then bumps it), and a deferred transaction that
upgrades to a writer after another connection committed fails with
SQLITE_BUSY_SNAPSHOT, which bypasses the busy handler and so is notabsorbed by
busy_timeout.waved.db. That keeps the wallet's pool the sole writer of its file,which is what makes the hazard above a non-issue.
Testing
The create/open lifecycle test is parameterized over the backends the
platform offers, so the SQLite store has to satisfy the same contract as
BoltDB rather than only the parts a new test remembers to cover. New
tests cover backend-to-file resolution, the mix-up guard, and that a
SQLite wallet leaves no BoltDB file behind. Default behavior is
unchanged.