Merge bitcoin/bitcoin#24486: wallet: refactor: dedup sqlite blob binding

8ea6167099ddfe9a42464c3c8ae2acd895329d4f wallet: refactor: dedup sqlite blob binding (Sebastian Falbesoner)

Pull request description:

  This refactoring PR deduplicates repeated SQLite blob binding to a statement with a newly introduced helper function `BindBlobToStatement`, abstracting away the calls to `sqlite3_bind_blob(...)`.
  This should be more readable and less error-prone, e.g. in case that the error handling has to be adapted. As a slight drawback, the function where the binding happens is not printed anymore (`__func__`), i.e. one could argue this is not strictly a refactoring, but IMHO the advantages of deduplication outweigh this; binding errors are purely internal logic errors (wrong use of the sqlite API) rather than something that is dependend on external data like DB content.

ACKs for top commit:
  laanwj:
    Code review ACK 8ea6167099ddfe9a42464c3c8ae2acd895329d4f
  achow101:
    ACK 8ea6167099ddfe9a42464c3c8ae2acd895329d4f
  klementtan:
    ACK 8ea6167099ddfe9a42464c3c8ae2acd895329d4f

Tree-SHA512: 1de0d214f836bc405a01e98a3a2d71f2deaddc7d23c31cad80219d1614bec92619c06d9a4a091dd563d3e95ffb879649c29745d8f89669b2a5330552c212af3f

1 files changed, 26 insertions, 41 deletions

diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp
index 2b2181e70b..55949e6e7a 100644
--- a/src/wallet/sqlite.cpp
+++ b/src/wallet/sqlite.cpp
@@ -37,6 +37,22 @@ static void ErrorLogCallback(void* arg, int code, const char* msg)
     LogPrintf("SQLite Error. Code: %d. Message: %s\n", code, msg);
 }
 
+static bool BindBlobToStatement(sqlite3_stmt* stmt,
+                                int index,
+                                Span<const std::byte> blob,
+                                const std::string& description)
+{
+    int res = sqlite3_bind_blob(stmt, index, blob.data(), blob.size(), SQLITE_STATIC);
+    if (res != SQLITE_OK) {
+        LogPrintf("Unable to bind %s to statement: %s\n", description, sqlite3_errstr(res));
+        sqlite3_clear_bindings(stmt);
+        sqlite3_reset(stmt);
+        return false;
+    }
+
+    return true;
+}
+
 static std::optional<int> ReadPragmaInteger(sqlite3* db, const std::string& key, const std::string& description, bilingual_str& error)
 {
     std::string stmt_text = strprintf("PRAGMA %s", key);
@@ -377,14 +393,8 @@ bool SQLiteBatch::ReadKey(CDataStream&& key, CDataStream& value)
     assert(m_read_stmt);
 
     // Bind: leftmost parameter in statement is index 1
-    int res = sqlite3_bind_blob(m_read_stmt, 1, key.data(), key.size(), SQLITE_STATIC);
-    if (res != SQLITE_OK) {
-        LogPrintf("%s: Unable to bind statement: %s\n", __func__, sqlite3_errstr(res));
-        sqlite3_clear_bindings(m_read_stmt);
-        sqlite3_reset(m_read_stmt);
-        return false;
-    }
-    res = sqlite3_step(m_read_stmt);
+    if (!BindBlobToStatement(m_read_stmt, 1, key, "key")) return false;
+    int res = sqlite3_step(m_read_stmt);
     if (res != SQLITE_ROW) {
         if (res != SQLITE_DONE) {
             // SQLITE_DONE means "not found", don't log an error in that case.
@@ -418,23 +428,11 @@ bool SQLiteBatch::WriteKey(CDataStream&& key, CDataStream&& value, bool overwrit
 
     // Bind: leftmost parameter in statement is index 1
     // Insert index 1 is key, 2 is value
-    int res = sqlite3_bind_blob(stmt, 1, key.data(), key.size(), SQLITE_STATIC);
-    if (res != SQLITE_OK) {
-        LogPrintf("%s: Unable to bind key to statement: %s\n", __func__, sqlite3_errstr(res));
-        sqlite3_clear_bindings(stmt);
-        sqlite3_reset(stmt);
-        return false;
-    }
-    res = sqlite3_bind_blob(stmt, 2, value.data(), value.size(), SQLITE_STATIC);
-    if (res != SQLITE_OK) {
-        LogPrintf("%s: Unable to bind value to statement: %s\n", __func__, sqlite3_errstr(res));
-        sqlite3_clear_bindings(stmt);
-        sqlite3_reset(stmt);
-        return false;
-    }
+    if (!BindBlobToStatement(stmt, 1, key, "key")) return false;
+    if (!BindBlobToStatement(stmt, 2, value, "value")) return false;
 
     // Execute
-    res = sqlite3_step(stmt);
+    int res = sqlite3_step(stmt);
     sqlite3_clear_bindings(stmt);
     sqlite3_reset(stmt);
     if (res != SQLITE_DONE) {
@@ -449,16 +447,10 @@ bool SQLiteBatch::EraseKey(CDataStream&& key)
     assert(m_delete_stmt);
 
     // Bind: leftmost parameter in statement is index 1
-    int res = sqlite3_bind_blob(m_delete_stmt, 1, key.data(), key.size(), SQLITE_STATIC);
-    if (res != SQLITE_OK) {
-        LogPrintf("%s: Unable to bind statement: %s\n", __func__, sqlite3_errstr(res));
-        sqlite3_clear_bindings(m_delete_stmt);
-        sqlite3_reset(m_delete_stmt);
-        return false;
-    }
+    if (!BindBlobToStatement(m_delete_stmt, 1, key, "key")) return false;
 
     // Execute
-    res = sqlite3_step(m_delete_stmt);
+    int res = sqlite3_step(m_delete_stmt);
     sqlite3_clear_bindings(m_delete_stmt);
     sqlite3_reset(m_delete_stmt);
     if (res != SQLITE_DONE) {
@@ -473,18 +465,11 @@ bool SQLiteBatch::HasKey(CDataStream&& key)
     assert(m_read_stmt);
 
     // Bind: leftmost parameter in statement is index 1
-    bool ret = false;
-    int res = sqlite3_bind_blob(m_read_stmt, 1, key.data(), key.size(), SQLITE_STATIC);
-    if (res == SQLITE_OK) {
-        res = sqlite3_step(m_read_stmt);
-        if (res == SQLITE_ROW) {
-            ret = true;
-        }
-    }
-
+    if (!BindBlobToStatement(m_read_stmt, 1, key, "key")) return false;
+    int res = sqlite3_step(m_read_stmt);
     sqlite3_clear_bindings(m_read_stmt);
     sqlite3_reset(m_read_stmt);
-    return ret;
+    return res == SQLITE_ROW;
 }
 
 bool SQLiteBatch::StartCursor()