#33033 wallet, sqlite: Encapsulate SQLite statements in a RAII class
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #33033 wallet, sqlite: Encapsulate SQLite statements in a RAII class | Needs rebase | 0 (+3) | Blocked | 230 | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
This pull request introduces `SQLiteStatement`, an RAII wrapper around `sqlite3_stmt` pointers in the wallet's SQLite backend. It replaces manual preparation and finalization of SQLite statements across `SQLiteBatch` and `SQLiteCursor` with member functions for binding, stepping, resetting, and retrieving column data.
Problem: SQLite prepared statements are C pointers requiring manual allocation and finalization, which is error-prone and clutters wallet database code, especially as upcoming work introduces more complex SQL statements.
Reviewability
Ready · Needs rebase
Conflicts with master; review the logic now, the rebase is mechanical.
Waiting on (from the thread read):
- author: address polespinasa's objection against copying SQLiteStatement and question on ExecEraseStatement, and davidgumberg's nodiscard suggestion 2026-09-24
- author: author to address polespinasa's objection about copying SQLiteStatement double finalizing, question on ExecEraseStatement, and nodiscard suggestion 2026-09-24
- reviewer: polespinasa to review author's explanation regarding sqlite3_int64 static_cast 2026-10-16
Agreement
Blocked
objection that needs a change, nobody has replied (polespinasa)
Thread notes: Author replied to polespinasa's suggestion on static_cast (rc:4092420947), moving it to answered. Eunovo's claim rc:3645464121 is set to open because the author only deferred addressing it. / Updated polespinasa's suggestion on the static_cast to answered following achow101's response. Reclassified Eunovo's deferred pzTail suggestion as open, and added Eunovo's question on Bind throwing exceptions and w0xlt's question on span types.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| w0xlt (contributor) | question, interface | why not use unsigned char directly for the span parameter | answered · author explained uint256 value_type could theoretically change | answered | yes | 2025-07-23: 'Is there any specific reason not to use `unsigned char` directly, like `std::span<const unsigned char > sp)` ?' Status from: 2025-07-23: achow101: 'In theory we could change `uint256`'s `value_type`, and we wouldn't need to make any changes here in that case.' |
| maflcko (member) | objection, interface | makes implicit construction from byte spans to transaction_identifier possible again, bypassing type safety protections | fixed · fix pushed · author moved the commit to #33034 | changed | yes | 2025-07-24: 'This makes implicit construction from any byte span to `transaction_identifier` possible again.' Status from: 2025-07-24: achow101: 'I've dropped this commit from this PR and moved it to #33034.' |
| vasild (member) | objection, correctness | ReadPragmaInteger throwing would propagate and set database status to FAILED_LOAD instead of FAILED_VERIFY on verification failures | fixed · fix pushed · changed to catch exception and return std::nullopt | changed | yes | 2025-11-12: 'Before, on `Verify()` failure (return `false`) `MakeSQLiteDatabase()` would have set `status = DatabaseStatus::FAILED_VERIFY` and now (exception) it would set `status = DatabaseStatus::FAILED_LOAD`. Is this ok?' Status from: 2025-11-17: achow101: 'I've changed this to catch the exception and return `std::nullopt`.' |
| vasild (member) | objection, safety | sqlite3_column_text can return nullptr, potentially causing undefined behavior when constructing a string without a null check | fixed · fix pushed · handled nullptr and returns empty string | changed | yes | 2025-11-12: '`sqlite3_column_text()` can return `nullptr`, better check that. The previous caller did check' Status from: 2025-11-17: achow101: 'Done, returns an empty string now.' |
| vasild (member) | objection, correctness | ignoring sqlite3_finalize return values in the destructor prevents signaling errors to callers | accepted · author explained sqlite3_finalize only repeats prior errors, accepted in vasild's ACK | answered | yes | 2025-11-12: 'Some of the callers of `sqlite3_finalize()` used to check its return value. Here in the destructor it is too late to signal an error to the caller... Ignoring the return value does not look good.' Status from: 2025-11-20: vasild: 'ACK 0af7c2a11819a21e11abf34b4229c5c21150d827' |
| davidgumberg (member) | suggestion, maintenance | initialize SQLiteStatement objects on first use rather than upfront in batch initialization | fixed · fix pushed · added commit to construct statement on first use | answered | yes | 2025-12-24: 'I think it would be cheaper and less complicated to initialize statement objects just before they are used in the various `Read*()` `Erase*()` , and it takes better advantage of the fact that statemen' Status from: 2026-01-02: achow101: 'However, we can construct the `SQLiteStatement` on first use instead of always, so I've added a commit to do that.' |
| Eunovo (member) | question, correctness | whether Bind should throw an exception instead of returning false since failure to bind cannot be recovered from | answered · author explained not all callers have exception handling | answered | yes | 2026-07-24: 'I wonder if `Bind` should throw an `Exception` instead? It doesn't look like we can recover from a failure to `sqlite3_bind_blob`.' Status from: 2026-07-24: achow101: 'Not all callers of this will have exception handling, but they all should have handling if it fails.' |
| Eunovo (member) | objection, correctness | uncompiled trailing SQL is silently ignored when multiple statements are passed to sqlite3_prepare_v2 without checking pzTail | open · author deferred addressing this ("If I need to retouch"), leaving it open | answered | yes | 2026-07-24: 'provide the `pzTail` argument and throw an error if `pzTail` is not Null after `sqlite3_prepare_v2` (this indicates that there is uncompiled SQL code left)' |
| polespinasa (member) | objection, safety | copying SQLiteStatement would cause double finalization of m_stmt and likely crash | open | changed | no | 2026-09-24: 'A copy would double finalize `m_stmt` (I think that would crash).' |
| polespinasa (member) | suggestion, maintenance | remove redundant static_cast and evaluate static_assert condition in Column template | answered · author explained sqlite3_int64 is a typedef and cast ensures conversion to used type | answered | yes | 2026-09-24: 'I think the static_cast is a no-op. sqlite3_column_int64 already returns an int_64.' Status from: 2026-10-16: achow101: 'It's a `sqlite3_int64` which is a typedef of `long long int`. The static cast is to put it into a type that we actually use.' |
| polespinasa (member) | question, maintenance | ExecEraseStatement is mentioned in the commit message as renaming ExecStatement, but is never defined in .cpp | open | answered | no | 2026-09-24: 'You mention in the commit message that ExecStatement is renamed to `ExecEraseStatement`, but this last is never defined in .cpp.' |
| davidgumberg (member) | suggestion, interface | mark the function as [[nodiscard]] | open | answered | no | 2026-09-24: 'This should be marked `nodiscard`' |
Support:
- w0xlt (contributor) (ACK, 2025-07-23): Looks good to me [not substantive]
- rkrux (member) (Concept ACK, 2025-10-31): Started reviewing; I'm not opposed to this introduction at the moment and I will get more clarity soon in the review. Initial Concept ACK 1829b3512d7bf430ec44daa801eb8fadf46cd30b [not substantive]
- vasild (member) (Approach ACK, 2025-11-12): Approach ACK 1829b3512d7bf430ec44daa801eb8fadf46cd30b [not substantive]
- vasild (member) (ACK, 2025-11-20): ACK 0af7c2a11819a21e11abf34b4229c5c21150d827 [not substantive]
- Eunovo (member) (2026-07-24): Looks good to me. Left some comments. [not substantive]
- pablomartin4btc (member) (ACK, 2026-07-27) [read: walletdb.cpp]: ACK 1682818a7b19 Left some inline nits. Two broader observations I think worth noting: [not substantive]
- polespinasa (member) (Concept ACK, 2026-09-24): cACK [not substantive]
Participants:
- maflcko (member, objection, 4 statements 2025-07-22 to 2025-07-24): objected to Txid span constructor re-enabling implicit conversions; fixed when author moved the commit
- w0xlt (contributor, support, 2 statements 2025-07-23 to 2025-07-23): ACKed after an inline question on span byte types
- rkrux (member, support, 2 statements 2025-10-31 to 2025-10-31): Concept ACKed and suggested cherry-picking related statement commits
- vasild (member, support, 17 statements 2025-11-12 to 2025-11-20): Approach ACKed and approved with ACK after reviewing error handling, return types, and statement finalization
- davidgumberg (member, objection, 3 statements 2025-12-24 to 2026-09-24): suggested constructing statements on first use rather than upfront in batch initialization; suggested nodiscard
- Eunovo (member, objection, 4 statements 2026-07-24 to 2026-07-27): suggested verifying pzTail after sqlite3_prepare_v2 to avoid discarding trailing SQL
- sedited (member, neutral, 1 statements 2026-07-24 to 2026-07-24): pinged reviewers for follow-up review
- pablomartin4btc (member, support, 8 statements 2026-07-27 to 2026-07-27): ACKed with observations on cursor bind failure throws and pzTail handling
- polespinasa (member, objection, 4 statements 2026-09-24 to 2026-09-24): Concept ACKed; objected that copying SQLiteStatement would double finalize and crash, and made inline suggestions
Dependencies
Enables:
- #33034 wallet: Store transactions in a separate sqlite table
Files
0 lines under test/bench/ci.
- src/wallet/sqlite.cpp +206/-197
- src/wallet/sqlite.h +24/-15
Card
The short text the ranking pass compares across PRs.
This PR wraps SQLite statement pointers in an RAII class `SQLiteStatement` to manage allocation, binding, column extraction, and finalization automatically within the wallet. It eliminates boilerplate and manual cleanup across `SQLiteBatch` and `SQLiteCursor`, laying necessary groundwork for more complex queries in upcoming wallet schema improvements such as #33034. The change has strong reviewer support and is ready for re-ACKs following recent updates addressing minor feedback.
Data
thread read through 2026-10-16 (77 statements), head 4ba56542a8
labels: Needs rebase
Categories
Wallet
P2 · cleanup (assessed alone as P3; Raised from P3: it is the prerequisite for #33034, which in turn unblocks #27865; a base cannot sit below what it unblocks.)
Encapsulates manual `sqlite3_stmt` management into an RAII class to reduce boilerplate and prevent resource leaks. While it prepares the wallet SQLite backend for upcoming work like #33034 ('wallet: Store transactions in a separate sqlite table'), it is primarily an internal refactoring with no direct user-facing bug fix or feature.
Membership: Modifies SQLite database backend files in src/wallet/sqlite.cpp and src/wallet/sqlite.h.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1