diff --git a/Makefile b/Makefile index 56b7c3ac45..a83202dc9b 100644 --- a/Makefile +++ b/Makefile @@ -162,7 +162,8 @@ itest-db-race: env CGO_ENABLED=1 GORACE="history_size=7 halt_on_errors=1" $(ITEST_DB_RACE) #? itest: Run integration tests -#? itest (vars): chain=btcd|bitcoind|neutrino backend=btcd|bitcoind|neutrino db=kvdb|sqlite|postgres +#? itest (vars): chain=btcd|bitcoind|neutrino backend=btcd|bitcoind|neutrino +#? itest (vars): db=sqlite|kvdb #? itest (vars): icase= (filter itest cases) #? itest (vars): timeout= verbose=1 nocache=1 #? itest (ex): make itest icase=manager @@ -175,7 +176,7 @@ itest: $(filter-out -test.run=%,$(TEST_FLAGS)) \ -args \ -chain="$(if $(backend),$(backend),$(if $(chain),$(chain),btcd))" \ - -db="$(if $(db),$(db),kvdb)" + -db="$(if $(db),$(db),sqlite)" # ========= # UTILITIES diff --git a/bwtest/harness.go b/bwtest/harness.go index 72bb2f92b9..d1bd877f85 100644 --- a/bwtest/harness.go +++ b/bwtest/harness.go @@ -2,6 +2,7 @@ package bwtest import ( "context" + "errors" "fmt" "path/filepath" "runtime/debug" @@ -262,6 +263,8 @@ func (h *HarnessTest) Stop() { func (h *HarnessTest) stopActiveWallets(ctx context.Context) error { h.Helper() + var stopErrs []error + for _, w := range h.ActiveWallets() { if w == nil { // Keep cleanup robust against partially initialized test state. A @@ -275,13 +278,20 @@ func (h *HarnessTest) stopActiveWallets(ctx context.Context) error { // NOTE: We intentionally don't call the deprecated WaitForShutdown/ // ShuttingDown methods here, as modern wallets might not have the // legacy fields initialized. + // + // Every wallet is stopped even after a failure: this is the + // sole teardown owner, so returning on the first error would + // leave every later-registered wallet running with its stores + // open, leaking them into the rest of the run. err := w.Stop(ctx) if err != nil { - return fmt.Errorf("stop wallet: %w", err) + stopErrs = append( + stopErrs, fmt.Errorf("stop wallet: %w", err), + ) } } - return nil + return errors.Join(stopErrs...) } // setUpChainClient creates and starts a chain client for the active harness diff --git a/bwtest/harness_wallet.go b/bwtest/harness_wallet.go index afa96d96a3..4a5701e240 100644 --- a/bwtest/harness_wallet.go +++ b/bwtest/harness_wallet.go @@ -1,7 +1,6 @@ package bwtest import ( - "context" "strings" "time" @@ -14,14 +13,17 @@ import ( ) const ( - // defaultPubPass is the standard public passphrase used by test wallets. + // defaultPubPass is the standard public passphrase used by test + // wallets. defaultPubPass = "public" - // defaultPrivPass is the standard private passphrase used by test wallets. + // defaultPrivPass is the standard private passphrase used by test + // wallets. defaultPrivPass = "private" - // defaultWalletRecoveryWindow keeps enough look-ahead addresses for test - // cases that derive multiple addresses while scanning historical blocks. + // defaultWalletRecoveryWindow keeps enough look-ahead addresses for + // test cases that derive multiple addresses while scanning historical + // blocks. defaultWalletRecoveryWindow = 20 // defaultWalletSyncRetryInterval controls how often wallet sync retries @@ -29,6 +31,34 @@ const ( defaultWalletSyncRetryInterval = 500 * time.Millisecond ) +// DBBackend resolves the harness-configured wallet database backend, rejecting +// any db value the harness cannot wire. kvdb runs directly off the subtest DB +// path; sqlite derives its path from that same kvdb path via the wallet's +// config defaults. postgres needs a DSN the harness does not provision, so it +// is rejected here with a clear message rather than failing deep inside +// Manager.Create. +// +// Manager-focused tests use this to build a wallet config directly while still +// honoring the -db flag. +func (h *HarnessTest) DBBackend() wallet.DBBackend { + h.Helper() + + backend := wallet.DBBackend(h.dbType) + switch backend { + // kvdb and sqlite are both wireable from the subtest DB path. + case wallet.DBBackendKVDB, wallet.DBBackendSQLite: + + case wallet.DBBackendPostgres: + h.Fatalf("db=%s is not supported by the itest harness: no "+ + "Postgres DSN is provisioned", h.dbType) + + default: + h.Fatalf("unknown db backend %q: use kvdb or sqlite", h.dbType) + } + + return backend +} + // CreateEmptyWallet creates, starts, and registers a new wallet instance. // // This is intended for non-manager integration tests that want a ready-to-use @@ -38,10 +68,18 @@ func (h *HarnessTest) CreateEmptyWallet() *wallet.Wallet { name := "itest-" + strings.ReplaceAll(h.Name(), "/", "_") + // Resolve the harness-configured backend. + backend := h.DBBackend() + cfg := wallet.Config{ // Use the subtest-scoped DB path and chain client prepared by // the harness. DB: wallet.DBConfig{ + // Honor the harness-configured backend so db=kvdb + // parity runs exercise kvdb rather than silently + // defaulting to SQLite. When sqlite is selected the + // SQLite path is derived from the kvdb path below. + Backend: backend, KVDB: wallet.KVDBConfig{ DBPath: h.WalletDBPath, }, @@ -53,7 +91,8 @@ func (h *HarnessTest) CreateEmptyWallet() *wallet.Wallet { RecoveryWindow: defaultWalletRecoveryWindow, WalletSyncRetryInterval: defaultWalletSyncRetryInterval, - // Use a unique wallet name per test to avoid collisions in logs. + // Use a unique wallet name per test to avoid collisions in + // logs. Name: name, PubPassphrase: []byte(defaultPubPass), } @@ -64,8 +103,9 @@ func (h *HarnessTest) CreateEmptyWallet() *wallet.Wallet { PubPassphrase: []byte(defaultPubPass), PrivatePassphrase: []byte(defaultPrivPass), - // Use an old birthday to ensure the wallet can discover historical - // blocks when used in tests that pre-mine chain state. + // Use an old birthday to ensure the wallet can discover + // historical blocks when used in tests that pre-mine chain + // state. Birthday: time.Now().Add(-1 * time.Hour), } @@ -76,13 +116,14 @@ func (h *HarnessTest) CreateEmptyWallet() *wallet.Wallet { err = w.Start(h.Context()) require.NoError(h, err, "failed to start wallet") - h.Cleanup(func() { - // We use a background context here because the test context might be - // canceled by the time cleanup runs. - _ = w.Stop(context.Background()) - }) - // Register the wallet so harness helpers can assert global invariants. + // + // Registration also hands teardown to the harness: the subtest cleanup + // stops every registered wallet via stopActiveWallets and asserts the + // error. We deliberately don't register a second Stop cleanup here. A + // duplicate cleanup would run first (LIFO) and discard the owning + // Stop's error, and the harness's later Stop would return nil + // because Stop is idempotent, masking a real teardown failure. h.RegisterWallet(w) return w diff --git a/docs/developer/adr/0008-integration-test-framework.md b/docs/developer/adr/0008-integration-test-framework.md index 515854d2e4..e7b63505aa 100644 --- a/docs/developer/adr/0008-integration-test-framework.md +++ b/docs/developer/adr/0008-integration-test-framework.md @@ -63,7 +63,10 @@ graph TD * For `bitcoind` tests: A separate `bitcoind` process connects (P2P) to the Miner to sync. * For `neutrino` tests: There is no separate "Chain Node" process; Neutrino connects directly to the Miner. * **`ChainBackend`**: The interface wrapper (`rpcclient` or `neutrino.ChainService`) used by the Wallet to communicate with the Chain Node. -* **`Database Backend`**: The storage layer (`kvdb`, `postgres`, `sqlite`). +* **`Database Backend`**: The storage layer. The wallet supports `sqlite` + (the default), `kvdb`, and `postgres`; the itest harness currently wires + only `sqlite` and `kvdb`, and rejects `postgres` because it provisions no + DSN. * **`HarnessTest`**: The specific test instance. It manages unique ports, temp directories, and process lifecycles. ### 2.3. Package Structure @@ -82,14 +85,17 @@ graph TD Configuration is handled via `go test` flags. ```bash -# Default (btcd + kvdb) +# Default (btcd + sqlite) make itest -# Explicit configuration -make itest chain=bitcoind db=postgres +# Explicit configuration. The wallet defaults to sqlite; pass db=kvdb to +# run the legacy walletdb parity suite. postgres is not supported by the +# itest harness (no Postgres DSN is provisioned), so db=postgres is +# rejected. +make itest chain=bitcoind db=kvdb # Run specific test case -make itest case=TestNewWallet +make itest icase=manager ``` * **Sequential Execution**: Tests run sequentially to avoid resource exhaustion and port conflicts, given the heavy overhead of spinning up multiple full nodes per test. @@ -113,7 +119,8 @@ make itest case=TestNewWallet ### Pros - **Consistency**: Clear separation between Network, Infrastructure, and Application. - **Isolation**: Per-test harnesses prevent state leaks. -- **Coverage**: capable of validating the entire support matrix. +- **Coverage**: capable of validating the chain-backend matrix; the database + matrix covers `sqlite` and `kvdb` until a PostgreSQL DSN is provisioned. ### Cons - **Resource Intensity**: Running a separate `bitcoind`/`btcd` process per test is CPU/RAM intensive. diff --git a/itest/main_test.go b/itest/main_test.go index 3ef90ad2bb..7d66e376d8 100644 --- a/itest/main_test.go +++ b/itest/main_test.go @@ -26,13 +26,15 @@ var ( // dbBackend defines the database backend to be used for the wallet // storage. - // Options: "kvdb" (default), "sqlite", "postgres". + // Options: "sqlite" (default), "kvdb". // - // This flag allows verifying that the wallet functions correctly across all - // supported database drivers. + // This flag allows verifying that the wallet functions correctly + // across the supported database drivers. It defaults to sqlite to + // match the wallet's default backend; pass -db=kvdb to run the legacy + // walletdb parity suite. dbBackend = flag.String( - "db", "kvdb", - "database backend to use (kvdb, sqlite, postgres)", + "db", "sqlite", + "database backend to use (sqlite, kvdb)", ) // shuffleSeedFlag is the source of randomness used to shuffle the test @@ -46,8 +48,8 @@ var ( // init configures rpctest to allocate unique listener ports. func init() { - // Use system-unique ports for rpctest harnesses so multiple local test runs - // don't collide. + // Use system-unique ports for rpctest harnesses so multiple local + // test runs don't collide. rpctest.ListenAddressGenerator = func() (string, string) { p2p := fmt.Sprintf(rpctest.ListenerFormat, port.NextAvailablePort()) rpc := fmt.Sprintf(rpctest.ListenerFormat, port.NextAvailablePort()) diff --git a/itest/manager_test.go b/itest/manager_test.go index 21d53c0b9c..2d8c9f13a6 100644 --- a/itest/manager_test.go +++ b/itest/manager_test.go @@ -3,7 +3,6 @@ package itest import ( - "context" "time" "github.com/btcsuite/btcwallet/bwtest" @@ -15,10 +14,14 @@ import ( func testCreateWallet(h *bwtest.HarnessTest) { h.Helper() - // Create a wallet using the Manager API. + // This is a manager-focused test, so drive the Manager API directly + // rather than the harness's CreateEmptyWallet convenience helper. Honor + // the harness-configured backend via h.DBBackend() so db=sqlite and + // db=kvdb genuinely exercise their respective backends instead of + // hard-coding one here. cfg := wallet.Config{ DB: wallet.DBConfig{ - Backend: wallet.DBBackendKVDB, + Backend: h.DBBackend(), KVDB: wallet.KVDBConfig{ DBPath: h.WalletDBPath, }, @@ -31,7 +34,6 @@ func testCreateWallet(h *bwtest.HarnessTest) { PubPassphrase: []byte("public"), } - manager := wallet.NewManager() params := wallet.CreateWalletParams{ Mode: wallet.ModeGenSeed, PubPassphrase: []byte("public"), @@ -39,20 +41,17 @@ func testCreateWallet(h *bwtest.HarnessTest) { Birthday: time.Now().Add(-1 * time.Hour), } + manager := wallet.NewManager() w, err := manager.Create(cfg, params) require.NoError(h, err, "failed to create wallet") err = w.Start(h.Context()) require.NoError(h, err, "failed to start wallet") - h.Cleanup(func() { - // We use a background context here because h.Context() might be - // cancelled already. - require.NoError( - h, w.Stop(context.Background()), "failed to stop wallet", - ) - }) // Register the wallet so harness helpers can assert global invariants. + // Registration also hands teardown to the harness, which stops every + // registered wallet during subtest cleanup, so we don't add a Stop + // cleanup here. h.RegisterWallet(w) // Mine a few blocks and require the wallet catches up. diff --git a/wallet/deprecated.go b/wallet/deprecated.go index 2a88c48953..22a39a0af1 100644 --- a/wallet/deprecated.go +++ b/wallet/deprecated.go @@ -7272,11 +7272,10 @@ func OpenWithRetry(db walletdb.DB, pubPass []byte, cbs *waddrmgr.OpenCallbacks, walletDeprecated: deprecated, // This deprecated constructor wires the kvdb store directly, so - // pin the kvdb backend explicitly. An unset Backend already - // resolves to kvdb in this PR, but the backend-dependent legacy - // paths (e.g. storeSyncedTo, import mirroring) rely on an - // explicit kvdb backend, so set it rather than lean on the - // default. + // pin the kvdb backend explicitly. An unset Backend now + // defaults to sqlite, and the backend-dependent legacy paths + // (e.g. storeSyncedTo, import mirroring) rely on an explicit + // kvdb backend, so set it rather than lean on the default. cfg: Config{ DB: DBConfig{Backend: DBBackendKVDB}, }, diff --git a/wallet/manager_test.go b/wallet/manager_test.go index 9e785ba1bf..88904bdd23 100644 --- a/wallet/manager_test.go +++ b/wallet/manager_test.go @@ -2181,10 +2181,14 @@ func TestManager_genRootKey(t *testing.T) { t.Parallel() m := NewManager() - cfg := Config{ChainParams: &chainParams} + cfg := Config{ + ChainParams: &chainParams, + DB: DBConfig{Backend: DBBackendKVDB}, + } - // With no configured kvdb path there is no legacy wallet to recover, so - // genRootKey takes the fresh-generation branch. + // With explicit kvdb and no configured path there is no legacy + // wallet to recover, so genRootKey takes the fresh-generation + // branch. key, err := m.genRootKey(cfg, CreateWalletParams{Mode: ModeGenSeed}) // Verify we got a valid private extended key. @@ -2200,7 +2204,10 @@ func TestManager_deriveRootKey(t *testing.T) { t.Parallel() m := NewManager() - cfg := Config{ChainParams: &chainParams} + cfg := Config{ + ChainParams: &chainParams, + DB: DBConfig{Backend: DBBackendKVDB}, + } // 1. ModeShell: Should return nil/nil (no root key for shell). t.Run("ModeShell", func(t *testing.T) { diff --git a/wallet/runtime_store_config.go b/wallet/runtime_store_config.go index 433effdb70..065ebd0a8f 100644 --- a/wallet/runtime_store_config.go +++ b/wallet/runtime_store_config.go @@ -54,9 +54,9 @@ type PostgresDBConfig struct { } // DBConfig selects and configures the wallet runtime store backend. A -// zero-valued Backend selects DBBackendKVDB, so wallets default to the -// legacy walletdb store. Set Backend to DBBackendSQLite or -// DBBackendPostgres explicitly to use the SQL runtime store. +// zero-valued Backend selects DBBackendSQLite, so new library-created +// wallets default to the SQLite runtime store. Existing kvdb-only wallets +// must set Backend to DBBackendKVDB explicitly to keep using walletdb. type DBConfig struct { // Backend identifies the selected runtime store backend. Backend DBBackend @@ -157,15 +157,15 @@ const defaultSQLiteDBName = "wallet.sqlite" // withDefaults returns c with the implicit runtime store defaults applied. // -// An unset Backend selects DBBackendKVDB so wallets default to the legacy -// walletdb store; callers must set Backend to DBBackendSQLite or -// DBBackendPostgres explicitly to use the SQL runtime store. When the resolved +// An unset Backend selects DBBackendSQLite so new library-created wallets +// default to the SQLite runtime store; callers opening an existing kvdb-only +// wallet must set Backend to DBBackendKVDB explicitly. When the resolved // backend is SQLite and no SQLite path was given, a deterministic default is // derived next to the legacy kvdb database (DB.KVDB.DBPath) so the runtime // store has a stable on-disk location. func (c DBConfig) withDefaults() DBConfig { if c.Backend == "" { - c.Backend = DBBackendKVDB + c.Backend = DBBackendSQLite } if c.Backend == DBBackendSQLite && c.SQLite.DBPath == "" && diff --git a/wallet/runtime_store_config_test.go b/wallet/runtime_store_config_test.go index 6093b192ad..2591c51c3d 100644 --- a/wallet/runtime_store_config_test.go +++ b/wallet/runtime_store_config_test.go @@ -19,19 +19,22 @@ func TestDBConfigValidate(t *testing.T) { wantErrMsg string }{ { - // An unset backend defaults to kvdb in this PR, so a - // config that sets only KVDB.DBPath validates as a kvdb - // wallet. - name: "unset defaults to kvdb with kvdb path", + // An unset backend defaults to SQLite, so a config that + // sets only KVDB.DBPath validates: the SQLite path is + // derived next to the kvdb database. + name: "unset defaults to sqlite with kvdb path", cfg: DBConfig{ KVDB: KVDBConfig{DBPath: "wallet.db"}, }, }, { - // A fully empty config defaults to KVDB, which needs no - // extra parameters, so it validates. - name: "empty config defaults to kvdb", - cfg: DBConfig{}, + // A fully empty config defaults to SQLite but has no + // kvdb path to derive the SQLite path from, so it is + // invalid. + name: "empty config is invalid", + cfg: DBConfig{}, + wantErr: ErrMissingParam, + wantErrMsg: errMsgSQLiteDBPath, }, { name: "kvdb explicit", @@ -162,8 +165,8 @@ func TestConfigValidateRuntimeStore(t *testing.T) { } // TestDBConfigWithDefaults verifies the implicit runtime store defaults: an -// unset backend selects KVDB, and an explicit SQLite backend with no SQLite -// path derives one next to the legacy kvdb database. +// unset backend selects SQLite and, when no SQLite path is given, a SQLite +// path is derived next to the legacy kvdb database. func TestDBConfigWithDefaults(t *testing.T) { t.Parallel() @@ -171,15 +174,18 @@ func TestDBConfigWithDefaults(t *testing.T) { kvdbPath := filepath.Join(kvdbDir, "wallet.db") - t.Run("unset backend defaults to kvdb", func(t *testing.T) { + t.Run("unset backend defaults to sqlite", func(t *testing.T) { t.Parallel() got := DBConfig{ KVDB: KVDBConfig{DBPath: kvdbPath}, }.withDefaults() - require.Equal(t, DBBackendKVDB, got.Backend) - require.Empty(t, got.SQLite.DBPath) + require.Equal(t, DBBackendSQLite, got.Backend) + require.Equal( + t, filepath.Join(kvdbDir, defaultSQLiteDBName), + got.SQLite.DBPath, + ) }) t.Run("explicit sqlite path is preserved", func(t *testing.T) { diff --git a/wallet/wallet_test.go b/wallet/wallet_test.go index 0cfa104874..cc6868b014 100644 --- a/wallet/wallet_test.go +++ b/wallet/wallet_test.go @@ -42,6 +42,10 @@ var ( } ) +// errMsgSQLiteDBPath is the missing-SQLite-path validation error substring +// asserted by the config validation tests. +const errMsgSQLiteDBPath = "DB.SQLite.DBPath" + // TestConfigValidate ensures that the Config.validate method correctly // identifies missing required parameters. func TestConfigValidate(t *testing.T) { @@ -76,9 +80,25 @@ func TestConfigValidate(t *testing.T) { }, expectedErr: "RecoveryWindow", }, + { + // With no backend set, the config defaults to SQLite + // and has no kvdb path to derive the SQLite path from, + // so validation fails on the missing SQLite path. + name: "missing default SQLite path", + config: Config{ + Chain: &bwmock.Chain{}, + ChainParams: &chainParams, + Name: "test-wallet", + RecoveryWindow: MinRecoveryWindow, + }, + expectedErr: errMsgSQLiteDBPath, + }, { name: "missing KVDB DBPath", config: Config{ + DB: DBConfig{ + Backend: DBBackendKVDB, + }, Chain: &bwmock.Chain{}, ChainParams: &chainParams, Name: "test-wallet",