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.
| Type | Reviewers |
|---|---|
| Stale ACK | w0xlt |
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]
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.