Skip to content

wallet: Store transactions in a separate sqlite table - #33034

Draft
achow101 wants to merge 27 commits into
bitcoin:masterfrom
achow101:wallet-sql-txs-table
Draft

wallet: Store transactions in a separate sqlite table#33034
achow101 wants to merge 27 commits into
bitcoin:masterfrom
achow101:wallet-sql-txs-table

Conversation

@achow101

@achow101 achow101 commented Jul 21, 2025

Copy link
Copy Markdown
Member

The wallet uses SQLite as a key-value store even though SQLite is a powerful relational database engine. This causes us numerous headaches due to the need to serialize multiple fields together, and since record read from the database during loading can come in any order.

Notably, for transactions, if we were able to load them in the transaction order assigned by the wallet, PRs like #27865 would be a bit simpler and easier to reason about.

This PR makes it possible for us to do that by storing transactions in a separate table. This table has a column for each field in CWalletTx, which lets us use a SQL statement like SELECT * FROM transactions ORDER BY pos which guarantees us the order in which the transactions are loaded into the wallet.

The eventual goal is to use SQLite as a relational database with tables that store our keys and other metadata that can reference each other so that the wallet doesn't just use the database as a storage mechanism. But that is still a work in progress.

This PR makes use of the last client opened features introduced in #32895 to determine when the old record style needs to be upgraded to the new transactions table. This should give us sufficient upgrade-downgrade-upgrade handling to not lose any funds.

Depends on #32763, #32895, #33033

@DrahtBot

DrahtBot commented Jul 21, 2025

Copy link
Copy Markdown
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/33034.

Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers
Concept ACK w0xlt, rkrux

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #35786 (wallet: drop spent parents redundant cache invalidation and notification by furszy)
  • #35760 (wallet: make corrupted transaction records fail wallet loading instead of forcing a rescan by achow101)
  • #35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
  • #35501 (wallet: store all witness variants of a transaction by achow101)
  • #35445 (wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated by achow101)
  • #35302 (Silent Payments: Sending (take 2) by Eunovo)
  • #34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
  • #34502 (wallet: remove most asserts of WALLET_FLAG_DESCRIPTORS flag by rkrux)
  • #33033 (wallet, sqlite: Encapsulate SQLite statements in a RAII class by achow101)
  • #32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
  • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

  • InMempool -> InMempool [the comment says “written as InActive(abandoned=false)”; “InActive” is misspelled and should be “Inactive”]

2026-07-14 00:25:55

@DrahtBot

Copy link
Copy Markdown
Contributor

🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/46429203587
LLM reason (✨ experimental): The CI failure is caused by Python lint errors due to unresolved 'self' references in test files.

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

@achow101
achow101 force-pushed the wallet-sql-txs-table branch from b36fd77 to 3d61435 Compare July 22, 2025 06:16
@achow101
achow101 force-pushed the wallet-sql-txs-table branch from 3d61435 to 80e1937 Compare July 22, 2025 17:22
@achow101
achow101 force-pushed the wallet-sql-txs-table branch from 80e1937 to 18035d1 Compare July 23, 2025 00:05
@w0xlt

w0xlt commented Jul 23, 2025

Copy link
Copy Markdown
Contributor

Concept ACK

achow101 added 26 commits July 13, 2026 17:19
…ecord

LastClientFeatures are feature flags indicating what automatic upgrade
features supported by the last client that opened the wallet.

The LAST_OPENED_FEATURES record stores these flags and must be
set to match the exact features supported by the client that opens a
wallet.
Set these flags to avoid the automatic upgrade after migrating.
This class will be used to encapsulate a sqlite3_stmt
WriteKey and ExecStatement use the same code for the actual execution of
the statement. This is refactored into a separate function, also called
ExecStatement, and the original ExecStatement renamed to
ExecEraseStatement as it is only used by the erase functions.
Instead of constructing all SQLiteStatements when SQLiteBatch is
constructed, construct them once when they are needed before each read,
write, or erase operation. Once constructed, the statement will persist
for the lifetime of the SQLiteBatch to be reused across multiple
statements.
We will need to bind data types other than blob
To handle columns containing NULL values, Column needs to return some
value representing NULL, so make it a std::optional.
Not all blob data types fit in ColumnBlob, so we need additional
template type requirements to match those.
When loading a wallet, always load from the transactions table, except
when loading a legacy wallet for migration.

The original 'tx' records are only used to upgrade to the transactions
table if an upgraded is necessary.
@DrahtBot

DrahtBot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants