SQLite statements are C objects which can be long lived, so having them be initialized in a constructor, and cleanup everything in a destructor, makes the sqlite code cleaner and easier to read. This also lets us have the various functions needed to interact with sqlite statements be member functions.
This will be particularly useful in future work which makes use of more complicated SQL statements.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33033.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
🚧 At least one of the CI tasks failed.
Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/46428794695
LLM reason (✨ experimental): The CI failure is caused by a static assertion failure in the sqlite.cpp file during compilation.
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.
143+ static_cast<size_t>(sqlite3_column_bytes(m_stmt, col))
144+ );
145+ } else {
146+ // GCC <= 12 and Clang <= 16 don't like static_assert(false) in an else, so we use the following to achieve the same effect
147+ // From: https://devblogs.microsoft.com/oldnewthing/20200311-00/?p=103553
148+ static_assert(!sizeof(T*));
ALWAYS_FALSE for this, so that the workaround docs for the C++23 defect report are in one place only.
35@@ -36,7 +36,10 @@ class transaction_identifier
36 }
37
38 public:
39+ using value_type = uint256::value_type;
40+
41 transaction_identifier() : m_wrapped{} {}
42+ transaction_identifier(std::span<const value_type> sp) : m_wrapped(sp) {}
unsigned char directly, like std::span<const unsigned char > sp) ?
uint256’s value_type, and we wouldn’t need to make any changes here in that case.
I don’t understand this change. The commit message just says “util: Add value_type and span constructor to (W)Txid”, but it doesn’t say why or where this is needed. Compiling without this (locally) seems to pass.
Also, conceptually, it seems to go in the opposite direction that we want to go in. This makes implicit construction from any byte span to transaction_identifier possible again. The constructor transaction_identifier(const uint256& wrapped) is private and will still catch those cases, but if this implementation detail is changed in the future, I presume direct and implicit construction from uint256 will be possible again.
I think it would be good to explain this change a bit better for reviewers.
In SQLiteStatement::Column<T>(), one of the ways we construct the appropriate data from a blob is to do reinterpret_cast<T::value_type>. Txid met all of the requirements for using that constructor, except that it didn’t have a value_type, hence this commit adding it.
But it looks like I didn’t include that commit in this PR. Rather, it’s https://github.com/bitcoin/bitcoin/pull/33034/commits/afe99b3b7df6c658fb4068c6418dfd7ebf151460 in #33034.
I see. Though, it may still be good to make this one explicit to avoid silent uint256 conversions in the future?
Also, Txid::data is std::byte*, so value_type should probably be std::byte as well.
I haven’t tried, but I presume both modifications should compile with afe99b3b7df6c658fb4068c6418dfd7ebf151460
70- sqlite3_reset(stmt);
71- return false;
72+ T(data, size);
73+};
74+
75+/** RAII class that encapsualtes a sqlite3_stmt */
encapsualtes -> encapsulates [spelling error in comment “encapsualtes” makes the word misspelled]
In d13ffed94ad5f5d488f8d72e4d5d7f1c3fb27c2b “sqlite: Have SQLiteCursor store SQLiteStatement”
It doesn’t seem to be used anymore here; builds and tests fine without it.
0--- a/src/wallet/sqlite.h
1+++ b/src/wallet/sqlite.h
2@@ -12,7 +12,6 @@
3
4struct bilingual_str;
5
6-struct sqlite3_stmt;
7struct sqlite3;
8
9namespace wallet {
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
These 3 commits from #33034 looks pretty generic and related enough. Can they be included in this PR?
74+
75+/** RAII class that encapsulates a sqlite3_stmt */
76+class SQLiteStatement
77+{
78+private:
79+ sqlite3* m_db{nullptr};
m_db can never be nullptr since it is initialized by the constructor to a presumably non-null value. Consider making this a reference sqlite3& m_db;. That would signal to the readers that it can never be null and will also make it impossible to have weird states of this class that somehow end up having a null member.
83+ explicit SQLiteStatement(sqlite3* db, const std::string& stmt_text)
84+ : m_db(db)
85+ {
86+ int res = sqlite3_prepare_v2(m_db, stmt_text.c_str(), -1, &m_stmt, nullptr);
87+ if (res != SQLITE_OK) {
88+ throw std::runtime_error(strprintf(
Just an observation:
Some of the callers of sqlite3_prepare_v2() call sqlite3_finalize() on failure. The doc says that if prepare fails then *ppStmt is set to NULL and Invoking sqlite3_finalize() on a NULL pointer is a harmless no-op.
So it is ok to omit the finalize call.
84+ : m_db(db)
85+ {
86+ int res = sqlite3_prepare_v2(m_db, stmt_text.c_str(), -1, &m_stmt, nullptr);
87+ if (res != SQLITE_OK) {
88+ throw std::runtime_error(strprintf(
89+ "SQLiteDatabase: Failed to prepare SQL statement: %s\n", sqlite3_errstr(res)));
SQLiteStatement, is this intentionally SQLiteDatabase?
stmt_text).
94-}
95+ ~SQLiteStatement()
96+ {
97+ Reset();
98+ sqlite3_finalize(m_stmt);
99+ }
Reset() is not needed here?
127+
128+ template<typename T>
129+ T Column(int col)
130+ {
131+ if constexpr (std::integral<T> && sizeof(T) <= 4) {
132+ return sqlite3_column_int(m_stmt, col);
From https://sqlite.org/c3ref/column_blob.html:
if the column index is out of range, the result is undefined
Maybe at the beginning of the function, add:
0Assert(col < sqlite3_column_count());
129+ T Column(int col)
130+ {
131+ if constexpr (std::integral<T> && sizeof(T) <= 4) {
132+ return sqlite3_column_int(m_stmt, col);
133+ } else if constexpr (std::integral<T> && sizeof(T) <= 8) {
134+ return static_cast<int64_t>(sqlite3_column_int64(m_stmt, col));
T is unsigned? Maybe add https://en.cppreference.com/w/cpp/types/is_signed.html here: std::integral<T> && std::is_signed<T>? (and leave it to drop to “else assert always false” for unsigned Ts)
157- sqlite3_finalize(pragma_read_stmt);
158- error = Untranslated(strprintf("SQLiteDatabase: Failed to prepare the statement to fetch %s: %s", description, sqlite3_errstr(ret)));
159- return std::nullopt;
160- }
161- ret = sqlite3_step(pragma_read_stmt);
162+ SQLiteStatement pragma_read_stmt(db, stmt_text);
ReadPragmaInteger() would have returned nullopt. Now it would throw. Its only caller is SQLiteDatabase::Verify() which would have returned false and now would let the exception propagate. The only caller of SQLiteDatabase::Verify() is MakeSQLiteDatabase(). 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?
std::nullopt.
163+ int ret = pragma_read_stmt.Step();
164 if (ret != SQLITE_ROW) {
165- sqlite3_finalize(pragma_read_stmt);
166 error = Untranslated(strprintf("SQLiteDatabase: Failed to fetch %s: %s", description, sqlite3_errstr(ret)));
167 return std::nullopt;
168 }
Now ReadPragmaInteger() would return nullopt on some errors and throw on others. What about making it to only throw? Then its return type would change from std::optional<int> to int and its callers would not need to check if (!read_result.has_value()) ....
I guess also SQLiteStatement::Step() can throw if the return value of sqlite3_step() is not SQLITE_ROW or SQLITE_DONE.
134+ return static_cast<int64_t>(sqlite3_column_int64(m_stmt, col));
135+ } else if constexpr (std::is_same_v<T, std::string>) {
136+ const char* text = (const char*)sqlite3_column_text(m_stmt, col);
137+ size_t size = sqlite3_column_bytes(m_stmt, col);
138+ std::string str_text(text, size);
139+ return str_text;
sqlite3_column_text() can return nullptr, better check that. The previous caller did check:
0- const char* msg = (const char*)sqlite3_column_text(stmt, 0);
1- if (!msg) {
2- error = ...
Done, returns an empty string now.
But the only time sqlite3_column_text should return NULL is if the column contains a NULL value. Empty strings are not stored as NULL.
595- int res = sqlite3_finalize(m_cursor_stmt);
596- if (res != SQLITE_OK) {
597- LogPrintf("%s: cursor closed but could not finalize cursor statement: %s\n",
598- __func__, sqlite3_errstr(res));
599+ m_cursor_stmt = std::make_unique<SQLiteStatement>(db, stmt_text);
600+}
nit, here and in the other constructor:
0SQLiteCursor::SQLiteCursor(sqlite3* db, const std::string& stmt_text)
1 : m_cursor_stmt(std::make_unique<SQLiteStatement>(db, stmt_text)) {}
458- stmt_description, sqlite3_errstr(res));
459- }
460- *stmt_prepared = nullptr;
461+ // Reset the unique_ptr to destroy the contained SQLiteStatement
462+ stmt_prepared->reset();
463 }
The second element in the pair (stmt_description) is not used. This snippet can be reduced to:
0 for (auto stmt : {&m_read_stmt, &m_insert_stmt, &m_overwrite_stmt, &m_delete_stmt, &m_delete_prefix_stmt}) {
1 stmt->reset();
2 }
Or simple stupid:
0 m_read_stmt.reset();
1 m_insert_stmt.reset();
2 m_overwrite_stmt.reset();
3 m_delete_stmt.reset();
4 m_delete_prefix_stmt.reset();
93- return true;
94-}
95+ ~SQLiteStatement()
96+ {
97+ Reset();
98+ sqlite3_finalize(m_stmt);
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.
From: https://sqlite.org/c3ref/finalize.html
If the most recent evaluation of statement S failed, then sqlite3_finalize(S) returns the appropriate error code or extended error code
I am not sure what is the best way to handle this. Ignoring the return value does not look good.
Maybe assert that sqlite3_finalize() returns SQLITE_OK? And also somehow make sure that if “the most recent evaluation of statement S failed”, then this is handled already, before the destructor is called. That would mean if sqlite3_step(), called from Step() fails (result is not SQLITE_ROW or SQLITE_DONE) then to call sqlite3_finalize() from inside Step()?
Maybe (not tested):
0 int Step()
1 {
2 const int ret = sqlite3_step(m_stmt);
3 if (ret == SQLITE_ROW || ret == SQLITE_DONE) {
4 return ret;
5 }
6 sqlite3_finalize(m_stmt);
7 m_stmt = nullptr;
8 // then the destructor can assert(sqlite3_finalize(m_stmt) == SQLITE_OK);
9 }
Related to #33033 (review)
If the most recent evaluation of statement S failed, then sqlite3_finalize(S) returns the appropriate error code or extended error code
I am not sure what is the best way to handle this. Ignoring the return value does not look good.
This text reads to me as sqlite3_finalize propagating any previous errors, e.g. from sqlite3_step. Since any errors from that should already be handled, I think it’s ok to ignore the result of finalize. I think the previous code is actually incorrect since the error result is not actually a result of finalize failing.
Yeah, I was thinking the same. It is just super odd to ignore database errors (in general). I guess the sqlite API is odd here.
Edit: if you retouch, maybe change int Step() to [[nodiscard]] int Step() to hint the callers to not ignore the return value.
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.
