wallet: batch per-block writes in a single SQLite transaction #35237

pull IsmaelBuilds wants to merge 1 commits into bitcoin:master from IsmaelBuilds:wallet-batch-blockconnected-33618 changing 2 files +65 −23

IsmaelBuilds commented at 10:47 AM on May 7, 2026: none
When a connected block updates the wallet (a transaction relevant to the wallet is observed, or the periodic 144-block sync triggers), each individual write currently runs in its own implicit SQLite autocommit transaction. That forces an fsync per WriteKey, which is the proximate cause of the v29 -> v30 regression reported in #33618: in generatetoaddress-style workloads where every coinbase pays the wallet, each block goes from "no on-disk write" (pre-7bacabb204) to "two or three fsynced writes" (post-7bacabb204), making the RPC ~1.5-2x slower.

Reverting 7bacabb204 is not an option: it fixes a correctness bug (#31824 — best-block locator was lagging across unclean shutdown during a reorg, leaving wallet transactions stuck as abandoned).

This change keeps the correctness fix and addresses the perf regression by wrapping the per-block writes in a single explicit SQLite transaction opened at the top of CWallet::blockConnected and committed once at the end. All writes performed for the block (the new/updated wallet transactions, the best-block locator, conflict descendants, and address book updates for newly-seen receiving addresses) share the same WalletBatch, so SQLite issues one fsync at COMMIT time instead of one per individual key write.

To make this safe, the same WalletBatch* is propagated through the relevant call paths:
- SyncTransaction
- AddToWalletIfInvolvingMe
- AddToWallet
- MarkConflicted (-> RecursiveUpdateTxState's batch overload)
- WriteBestBlock
- SetAddressBook (rerouted to SetAddressBookWithDB when a batch is active, since reusing a fresh WalletBatch on the same connection would block on m_write_semaphore that the active txn already holds)
All new parameters default to nullptr, preserving call-site behavior for every other caller (rescan, mempool path, RPC, tests, etc.). Only the blockConnected path is batched.

If TxnBegin() fails for any reason, the code transparently falls back to the previous unbatched behavior — correctness is unchanged. If TxnCommit() fails, the in-memory tip is preserved (it was already updated) and the on-disk best-block locator may briefly lag; the periodic 144-block sync and chainStateFlushed callback will reconcile on the next opportunity, mirroring how the wallet already tolerated crash-driven divergence prior to 7bacabb204.

Fixes #33618.

wallet: batch per-block writes in a single SQLite transaction

When a connected block updates the wallet (a transaction relevant to the
wallet is observed, or the periodic 144-block sync triggers), each
individual write currently runs in its own implicit SQLite autocommit
transaction. That forces an fsync per WriteKey, which is the proximate
cause of the v29 -> v30 regression reported in #33618: in
`generatetoaddress`-style workloads where every coinbase pays the wallet,
each block goes from "no on-disk write" (pre-7bacabb204) to "two or three
fsynced writes" (post-7bacabb204), making the RPC ~1.5-2x slower.

Reverting 7bacabb204 is not an option: it fixes a correctness bug
(#31824 — best-block locator was lagging across unclean shutdown during
a reorg, leaving wallet transactions stuck as abandoned).

This change keeps the correctness fix and addresses the perf regression
by wrapping the per-block writes in a single explicit SQLite transaction
opened at the top of `CWallet::blockConnected` and committed once at the
end. All writes performed for the block (the new/updated wallet
transactions, the best-block locator, conflict descendants, and address
book updates for newly-seen receiving addresses) share the same
`WalletBatch`, so SQLite issues one fsync at COMMIT time instead of one
per individual key write.

To make this safe, the same `WalletBatch*` is propagated through the
relevant call paths:

  - SyncTransaction
  - AddToWalletIfInvolvingMe
  - AddToWallet
  - MarkConflicted (-> RecursiveUpdateTxState's batch overload)
  - WriteBestBlock
  - SetAddressBook (rerouted to SetAddressBookWithDB when a batch is
    active, since reusing a fresh WalletBatch on the same connection
    would block on m_write_semaphore that the active txn already holds)

All new parameters default to nullptr, preserving call-site behavior for
every other caller (rescan, mempool path, RPC, tests, etc.). Only the
blockConnected path is batched.

If TxnBegin() fails for any reason, the code transparently falls back to
the previous unbatched behavior — correctness is unchanged. If
TxnCommit() fails, the in-memory tip is preserved (it was already
updated) and the on-disk best-block locator may briefly lag; the
periodic 144-block sync and chainStateFlushed callback will reconcile on
the next opportunity, mirroring how the wallet already tolerated
crash-driven divergence prior to 7bacabb204.

Fixes #33618.

45cbd7bfc6

DrahtBot added the label Wallet on May 7, 2026
DrahtBot commented at 10:47 AM on May 7, 2026: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35237.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.
achow101 commented at 11:40 AM on May 7, 2026: member

I'm going to close this PR as the referenced issue does not describe anything that we want to change.
achow101 closed this on May 7, 2026

Contributors

Labels

Linked (view graph)

#31824 wallet: wrong balance and crash after reorg and unclean shutdown #33618 `generatetoaddress` 2-3x slower on v30 compared to v29

wallet: batch per-block writes in a single SQLite transaction #35237

Code Coverage & Benchmarks

Reviews