wallet: serialize Manager wallet assembly

[yyforyongyu]

Change Description

Implements roadmap Task 453.

Serialize every Manager Create and cold Load operation through one
Manager-wide lock. The lock spans durable creation, or the Load cache
decision, through backend assembly and cache installation. Concurrent
same-name Loads therefore return one exact Wallet pointer, and Create cannot
be overtaken between durable commit and runtime installation.

The cache remains map[string]*Wallet and contains only fully assembled
Wallets. Different wallet names intentionally assemble sequentially; there
are no per-name entries, channels, transient nil values, or result latches.

Steps to Test

1. Run: make unit pkg=wallet
2. Run the concurrent Manager Load integration test for each backend:
   • make itest chain=btcd db=kvdb icase="manager load concurrent"
   • make itest chain=btcd db=sqlite icase="manager load concurrent"
   • make itest chain=btcd db=postgres icase="manager load concurrent"
3. Run: make lint-check

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.

📝 Please see our Contribution Guidelines for further guidance.

[Lrifton92]

The race is real and the fix closes it. Worth saying that testManagerLoadConcurrent is genuinely discriminating rather than decorative: if two callers both assembled, each would return the runtime it built, so require.Same against first would fail — the assertion actually proves single assembly rather than just a consistent final state. Three things on the approach.

The lock is global, the documented race was per-name

The TODO this removes reads "concurrent Load calls for the same name are not serialized". The fix serializes every name: Load now holds the exclusive Manager lock across m.backend.load(...) (wallet/manager.go:381) and Create holds it across m.backend.create(...) (line 213). Both of those open stores, so wallet assembly across the whole Manager is now strictly one at a time, and a slow or hanging store open for one wallet blocks Load, Create and String for every other.

What makes this worth raising rather than shrugging at is where it lands. NewManager's own comment says "the legacy kvdb backend remains single-wallet until it is removed; SQL stores distinguish wallets by ID" — so the coarse lock is free on kvdb, which only ever holds one wallet, and costs exactly on the SQL backends that multi-wallet support exists for. A per-name guard (an in-flight map keyed by cfg.Name, or singleflight) would close the same race without the global chokepoint.

If the global lock is a deliberate simplification for now, that is a defensible call — but the new comment on wallets says "The Manager lock serializes Create and Load through assembly and cache installation" without noting it serializes across all names, and that is the part a future reader needs.

Create and Load now have different lock discipline

Load takes m.Lock(); defer m.Unlock(). Create takes m.Lock() at line 213 and unlocks manually on two paths — line 219 on the backend error, line 226 after installation. Both are correct today. But Create keeps going after the unlock (the ModeShell import), so it cannot simply defer, and that asymmetry is the fragile kind: any early return added between 213 and 226 later leaks the lock and wedges the Manager permanently, with no test that would catch it. Extracting the locked span into a small helper that can defer would make it structurally safe instead of safe-by-inspection.

Does Create need to consult the cache before installing?

Create never reads m.wallets[cfg.Name] before m.wallets[cfg.Name] = w. Under the new lock two concurrent Create calls for one name no longer interleave, but the second still overwrites the first's entry, orphaning a runtime the first caller is holding and may already have started — and the same applies to a Create following a Load of that name.

I could not settle from here whether the backends make that unreachable by failing the second create on a uniqueness constraint; if they do, this is a non-issue and worth a line in the comment saying so. If they do not, then "installation" clobbering a live entry seems in scope for a PR about making installation safe.