wallet: publish descriptor top-ups after commit

[kiwidream]

Summary

Fixes #139.

Descriptor top-up now stages newly derived scripts and publishes wallet ownership/cache updates only after the backing database transaction commits. Standalone top-ups retain the descriptor lock through commit, then release descriptor/database state before acquiring the wallet lock for publication. Top-ups running inside caller-owned transactions register commit and abort listeners so successful transactions publish once while aborts restore descriptor, keypool, cache, and PQC state.

CWallet::m_cached_spks is now explicitly guarded by cs_wallet, and cache misses fall back to querying script managers during the narrow commit-to-publication handoff. Transaction listeners also abort on commit failure or scope exit and unwind in reverse order.

The mock wallet database now models active transactions and rollback, enabling regression coverage for commit failure, external transaction publication order, multi-top-up abort ordering, and concurrent cache readers.

Testing

• Built locally.
• Ran focused unit or functional tests for the changed area.
• Ran lint or formatting checks relevant to this change.
• Not run. Reason:

Commands and configurations:

• cmake --build build --target test_bitcoin -j4
• cmake --build build-debug --target test_bitcoin -j4
• build/bin/test_qbit --run_test='wallet_tests,walletdb_tests,wallet_p2mr_change_keypool_tests,wallet_p2mr_deferred_keypool_tests,wallet_p2mr_descriptor_tests' --log_level=message
• build-debug/bin/test_qbit --run_test='wallet_tests/DescriptorTopUp*' --log_level=message
• build-debug/bin/test_qbit --run_test='wallet_p2mr_change_keypool_tests,wallet_p2mr_deferred_keypool_tests' --log_level=message
• Docker lint image from ci/lint_imagefile: all checks passed.
• git diff --check

Target Branch

• This PR targets main or a maintainer-requested release branch such as 0.1.x.

Target: 1.x.x.

Risk / Review Notes

• Consensus, script, crypto, wallet, P2P, release, CI, or security-sensitive behavior changed.
• No consensus, script, crypto, wallet, P2P, release, CI, or security-sensitive behavior changed.

Notes:

• Wallet transaction publication and lock ordering changed; consensus, script validation, networking, and on-disk schema are unchanged.
• Review should focus on commit/abort callback lifetime, reverse-order rollback, and the wallet-to-descriptor lock order in cache-miss fallbacks.
• Deferred initial and low-watermark P2MR top-ups remain outside cs_wallet during derivation and persistence.

Docs / Process Impact

Choose exactly one:

• I updated public docs because this PR changes user-visible behavior, integration guidance, release/process guidance, or expected validation.
• No public docs update needed. Reason: this corrects internal wallet synchronization and transaction publication without changing user-facing APIs or configuration.

libbitcoinpqc Subtree Checklist (if src/libbitcoinpqc changed)

Not applicable; src/libbitcoinpqc is unchanged.

• Source commit is reachable from an immutable release tag in Qbit-Org/qbit-libbitcoinpqc.
• qbit imports the tagged upstream tree directly without pruning or a curated subtree branch.
• Subtree import/update was performed with contrib/devtools/update-libbitcoinpqc-subtree.sh.
• test/lint/libbitcoinpqc-subtree-check.sh passes locally.
• Any default tag change in contrib/devtools/update-libbitcoinpqc-subtree.sh is intentional and matches doc/subtrees/libbitcoinpqc.md.

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

---

Note

Medium Risk
Changes wallet lock ordering, in-memory vs durable publication, and transaction listener lifetimes; incorrect rollback or cache-miss fallback could miss or mis-attribute outputs, though consensus and on-disk schema are unchanged.

Overview
Descriptor keypool top-ups now stage new scripts and wallet-visible updates until the backing DB transaction succeeds, instead of updating TopUpCallback and notifications while writes may still roll back.

TopUpChange captures new scriptPubKeys plus a rollback closure; standalone top-ups commit under cs_desc_man, then call PublishTopUp outside that lock. Top-ups inside a caller-owned WalletBatch register commit/abort listeners so publication runs once on commit and descriptor/keypool/PQC state is restored on abort (including after commit failure).

m_cached_spks is explicitly GUARDED_BY(cs_wallet); TopUpCallback and cache helpers take cs_wallet. IsMine, GetScriptPubKeyMans, and GetSolvingProvider fall back to script managers on cache miss during the narrow post-commit handoff. SetCache narrows the descriptor lock scope before publishing.

WalletBatch destructor aborts active transactions; commit failure attempts abort; abort listeners run in reverse order. Migration/descriptor import paths abort when commit fails.

Mockable wallet DB implements real txn snapshots for tests; new wallet tests cover commit failure rollback, deferred publication in external txns, and concurrent cache readers. Functional test pins mock time for stable getblockchaininfo CLI comparison.

^{Reviewed by Cursor Bugbot for commit 07b614e. Bugbot is set up for automated code reviews on this repo. Configure here.}

[kiwidream]

PR Review

Findings

• [blocking] src/wallet/scriptpubkeyman.cpp:1563 — Transaction callbacks can outlive the descriptor manager. Both callbacks capture raw this, and the rollback closure does as well. During descriptor setup, SetupDescriptorScriptPubKeyMan() retains the new manager in a local unique_ptr; after the listener is registered, UnsetBlankWalletFlag(batch) can still throw. The manager is then destroyed before the outer WalletBatch destructor aborts, causing on_abort to lock freed cs_desc_man and invoke a rollback against freed storage. A persistence error during wallet creation can therefore become a use-after-free.

  • Potential resolution: Make the callbacks lifetime-aware using the existing SPKM lifetime token, or ensure the manager is owned for the entire batch lifetime. Add a failure-injection test that throws after listener registration but before ownership transfer, then aborts the batch under ASan.

• [high] src/wallet/wallet.cpp:1953 — The cache-miss fallback is not limited to committed handoff state. TopUpWithDBPreparedResult() inserts scripts into m_map_script_pub_keys before a caller-owned transaction commits, then releases cs_desc_man. The new IsMine, GetScriptPubKeyMans, and GetSolvingProvider fallbacks can therefore expose that staged state before persistence whenever the caller does not retain cs_wallet for the complete transaction. An abort restores the SPKM maps but cannot undo wallet transaction-processing or signing decisions made during that window. The fallback also permanently scans every SPKM for every unknown script, rather than only during the claimed narrow handoff.

  • Potential resolution: Gate fallback visibility on an explicit committed-but-not-yet-cached script set, or defer the relevant SPKM mutations until commit. Add a deterministic active-transaction test proving a new script is not wallet-visible before commit, remains invisible after abort, and is visible immediately after commit.

Consensus Impact

Consensus-adjacent

No consensus validation, on-chain serialization, or signature algorithm changes are present. However, src/wallet/wallet.cpp changes ownership and signing-provider selection, while src/wallet/scriptpubkeyman.cpp changes descriptor/PQC key visibility and transaction rollback. Issue #139 authorizes this wallet-publication and locking work, but not broader signing semantics. Wallet, P2MR signing, sanitizer, and concurrency coverage are therefore relevant; new consensus vectors are not required.

Review Scope

Reviewed issue #139 as the contract, both commits, all ten changed files, the complete diff against 1.x.x, SQLite and mock transaction behavior, descriptor setup/migration/external-signer callers, PR metadata, CI logs, and review state. No human review comments or unresolved review threads were present.

Deep Checks Performed

Traced standalone and caller-owned top-ups through commit, abort, scope-exit, multi-top-up reverse rollback, and object destruction. Checked restoration of descriptor ranges/cache, ECDSA and PQC key maps, signature counters, and deferred P2MR state. I also traced wallet-to-descriptor lock ordering and attempted to disprove the fallback concern through current callers; setup paths commonly retain cs_wallet, but TopUpWithDB has no such asserted precondition and the fallback itself has no durability gate.

Issue Fit

The patch correctly guards m_cached_spks, moves standalone publication after commit and outside descriptor/database locks, restores state after commit failure, and reverses abort-listener ordering. Deferred and low-watermark P2MR paths remain structurally intact.

The two findings leave callback lifetime and “ownership only after persistence” incomplete. The interface_bitcoin_cli.py mock-time change is unrelated scope drift, though isolated and test-only.

qbit-Specific Checks

Reviewed P2MR private-key persistence for plaintext and encrypted wallets, PQC signature-counter rollback, deferred keypool flags, cache/range restoration, provider fallback, and wallet/descriptor/database lock order. AuxPoW, ASERT, validation, P2P/PHOTON, release signing, and on-disk schema are unchanged.

Validation Reviewed

The PR reports release/debug builds, focused wallet and P2MR suites, Docker lint, and git diff --check. At head 07b614e8a5, all required checks are green. CI evidence includes:

• TSan and MSan: 181/181 unit suites passed, including wallet_tests and wallet_p2mr_descriptor_tests.
• ASan/LSan/UBSan, Windows, i686 debug, tidy, focused checks, and lint passed.
• Full MSan functional tests passed, including interface_bitcoin_cli.py and the wallet/P2MR functional suites.
• Local review: git diff --check passed.

Residual Risk

I did not perform a fresh local build. The concurrent-reader test has no scheduling barrier, so it does not deterministically prove the commit-to-publication handoff or the pre-commit invisibility invariant. No performance evidence covers the new all-manager scan on ordinary cache misses.