wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077

In commit “Change WalletLocation::Exists to check for wallet file existence” (6e7ef52e359e55bc82a30207a701995f0ca255d7)

Minor: I can’t really figure out what this commit and also the previous commit “wallet: Don’t Verify if database location doesn’t exist” (b64e40310742e3975ae704a801ccafa73d8617bf) are doing. Changes seem harmless, but they are making code more complicated. It would be good if commit messages mentioned motivations in commits like these where the motivations aren’t obvious.

The motivation is so that we check the wallet file itself as part of the db type checking later on. IIRC there were some issues with where the wrong file type was being used that necessitated these changes.

I’ll try to expand the commit message

In commit “Add libsqlite3” (31243f5482bb1c8a71affbe7ced6653a09bd6829)

Would suggest splitting this commit and other build and depends and travis related commits into a separate build PR so it can get feedback from bitcoin build aficionados (and so this PR can more approachable for regular and wallet reviewers).

I think probably build reviewers will want a --with-sqlite configure option to allow sqlite to be disabled in the build even if it is present in the system. They might also want the sqlite location to be determined through pkgconfig instead of assumed to be in the system include directory.

re: #19077 (review)

In commit “Add libsqlite3” (10124c3d6c5176d5df94964e530a4b0c6edd8381)

Minor: Maybe someone more familiar with the build can weigh in, but it seems like it could be desirable to allow building bitcoin without sqlite even if sqlite is installed on the system. Also, I think other dependency checks have been switched to use pkg-config (#18307) instead ad-hoc methods like this.

In commit “Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch” (08847ca4de58c112da4b6455230f7b454afa543c)

Minor: Suggest sqlite3* m_db{nullptr}; or sqlite3* m_db = nullptr; here to be sure this is safe without looking and even if someone adds another constructor.

In commit “Introduce g_file_paths” (d038d04a4e3e0522a51cf0d39110749f8395f61a)

Minor: Pretty sure we can get rid of these globals with more sane loading code in the wallet, but in any case could consider switching RecursiveMutex to Mutex if possible and switching cs_sqlite to g_sqlite_mutex to follow newer conventions

In commit “Implement SQLiteDatabaseVersion” (b2f2e7171861fe2998f2c27af44b65b0e1f558aa)

Minor: SQLiteLibraryVersion might be a more descriptive name

In commit “Initialize and Shutdown sqlite3 globals” (5c78c9094a0158e4d466b696a3bbbcbd35b51b15)

Minor: Would add comment saying this is a no-op if it is already called. Another option would be to only call it when g_file_paths is empty.

In commit “Initialize and Shutdown sqlite3 globals” (5c78c9094a0158e4d466b696a3bbbcbd35b51b15)

Return value isn’t checked here. Would suggest at least logging an error so we know if something has gone wrong.

A couple of comments here:

• file at path is opened twice.
• nit, in DetermineStorageType you check IsSQLiteFile first, maybe change there for now as most is BDB.
• maybe follow up, this could use DetermineStorageType.

file at path is opened twice.

It is?

Done the other 2 suggestions.

In commit “Add/Remove m_file_path to/from g_file_paths in con/destructor” (316002da052cad68c54a48e918d1d5f2ca58c0d0)

Minor: Developer notes and current clang-format config don’t indent namespace contents

Also, I wonder if these variables are even necessary. If sqlite supports opening databases in an exclusive mode, there should be no need for our code to maintain this additional list of database files.

1. I suppose the locking is done in the calling code. Do I understand correctly that it’s managed by cs_wallet?

2. nit: I believe it’s better to have all members private. Why not make SQLiteBatch a friend class or just pass the statement handlers when we construct SQLiteBatch object.

I suppose the locking is done in the calling code. Do I understand correctly that it’s managed by cs_wallet?

For the most part, yes. Sometimes we do need to handle concurrency withing SQLiteDatabase but those should already be handled by m_refcount and sqlite itself.

nit: I believe it’s better to have all members private. Why not make SQLiteBatch a friend class or just pass the statement handlers when we construct SQLiteBatch object.

I suppose now we could pass them in. In a previous revision, I don’t think that was possible.

But it’s easier to just let them be public members. It would be 6 extra arguments to pass them in and we are already giving the SQLiteDatabase container.

For the most part, yes. Sometimes we do need to handle concurrency withing SQLiteDatabase but those should already be handled by m_refcount and sqlite itself.

Could you please elaborate on this part? It looks like there are indeed places when we access WalletDatabase instance from another thread without acquiring lock first. For example CWallet::chainStateFlushed. From my understanding of sqlite it’s safe to use by multiple threads, but they should be using different connections. Which is not the case in this example. And I’m not sure how m_refcount will help with that.

I guess in BDB this was handled by cs_db within BerkeleyDatabase::Open

According to https://sqlite.org/threadsafe.html, the default multithreading mode is serialized which means that a single database connection can be used from multiple threads safely. So no locking is needed with that.

I suppose we should enforce that when opening by setting SQLITE_OPEN_FULLMUTEX?

Thanks for clarification.

I suppose we should enforce that when opening by setting SQLITE_OPEN_FULLMUTEX?

I think that’s a great idea. I would say that SQLITE_CONFIG_SERIALIZED is even better. It explicitly say that it’s safe to use both connection and prepared statement objects.

In commit “Implement SQLiteDatabase::Open” (d1559561a68dd6604efd91baa5dbd28c995d0074)

re: #19077 (review)

It will be closed when the SQLiteDatabase is destructed.

It does looks like db pointer can be leaked here if ret is not OK.

re: #19077 (review)

In commit “Implement SQLiteDatabase::Rewrite” (0d75b4013400b5a6bfa1f5c75082a8ff0701bd84)

I think it’s fine

This whole loop doesn’t seem like a good idea. If it actually serves a purpose, there should be a comment about it with specifics. Otherwise I think Rewrite should directly vacuum, and this sleepy loop thing just looks like voodoo.

5ece9f577a6e6463b15b0d88f35a7f18808936f2

Why not keep it as NO_WALLET?

This test environment is a little weird.

The wallet is actually enabled and BDB is being used. However the BDB being used needs to be built with some special flags, so it can’t be built via depends. What this option does is disable BDB in depends so that BDB can be built separately with whatever it needs to work in this env. Because SQLite doesn’t need this special building, it can be built by depends. So we don’t want NO_WALLET because we actually still want the wallet here. We just want NO_BDB so we can deal with it later.

8c7832a45f9acb088a87689573c2be47870b5c8a

It feels strange to me that the log callback makes everything blow up in a special case. I think we use asserts for developer errors but I don’t think that’s the case here. Also doesn’t this mean that the throw std::runtime_error on L36 will never be used?

It feels strange to me that the log callback makes everything blow up in a special case. I think we use asserts for developer errors but I don’t think that’s the case here.

This would only be hit in a developer error or a sqlite bug.

Also doesn’t this mean that the throw std::runtime_error on L36 will never be used?

I don’t think so?

Hm, the docs aren’t very clear on this (or I can’t find the right place) so I can not say yet if there are cases where ret == SQLITE_OK but the log callback is still being called. That would be the only case when this seems valuable because otherwise the runtime_error would be hit in case there is a problem and the error message would be clearer I think? Yes, this could be a bug in SQLite but in general, we are trusting that the ret/res codes are not lying so I don’t know why we would do it differently here.

On the flip side, the runtime_error will currently only be hit if ret != SQLITE_OK but the ErrorLogCallback is not being called. Again, I couldn’t find details/guarantees on this in the docs but at least they say the callback will be used ‘whenever anomalies occur’, so my expectation is that it would always be called as whenever ret != SQLITE_OK is true. So I think the runtime_error may never be hit unless there is a bug in SQLite.

Overall it just feels strange to do anything but logging in a function that is called *LogCallback.

re: #19077 (review)

Agree with achow that this is an appropriate place to assert. We’re passing along a context value, and asserting we are passed back the same value. We’re not checking for a runtime error, just documenting an assumption about how the code should is supposed to work, and adding a sanity check to detect if the assumption is wrong.

Perhaps citing from the sqlite3 manual in the comment is more clear:

0// From sqlite3_config() documentation for the SQLITE_CONFIG_LOG option: 
1// "The void pointer that is the second argument to SQLITE_CONFIG_LOG is passed through as
2// the first parameter to the application-defined logger function whenever that function is 
3// invoked."
4// Assert that this is the case:

cf3a2373b0aa7a20ff15367730161ac18f3f302b

nit

0    const bool read_only = (!strchr(mode, '+') && !strchr(mode, 'w'));
1    const bool create = strchr(mode, 'c') != nullptr;
2    int flags;

e4d88e26a5da6f119b6a0124fb18ee9909e486a4

Maybe use a runtime_error here instead of an assert? Same in EraseKey.

01b3cb026806784ae20eb670bd9aa40c43a94e54

nit, use initializer {nullptr}?

94d5ee30677b2282e87f79a6f945bceff41faaad

This is the case when neither DB exists and a new one is created, right? Would be worth a comment here I think.

81d9ce910f61aed16c7298150775b973102db498

Maybe check and error if a different format was passed?

In commit “Add sqlite to travis and depends” (443d029fb859262c56b8be32205a2b4540dbb843)

Minor: Would be nice to preserve alphabetical order and avoid need to change unrelated lines

In commit “Add sqlite to travis and depends” (443d029fb859262c56b8be32205a2b4540dbb843)

Minor: Spacing on this line is inconsistent with the rest of the file

In commit “Add SQLiteDatabase and SQLiteBatch dummy classes” (f35f89f1ae53914f6183ec3d7da6a1d0c27bed16)

See main comment for reasoning, but this is awkward to change later so I think the commit introducing this should start with the appropriate final value (hopefully “wallet.sqlite” or similar)

In commit “Initialize and Shutdown sqlite3 globals” (13cca0d881acc81dbf4a6496bb02d4fefc905a77)

There’s still a race condition here if two databases are opened at the same time. sqlite3_config will only be called once for both databases, but only one of the databases will actually block waiting for the call to complete, so the other database open will most likely fail. Similar races can happen on close.

Instead of trying to be clever and lock-free, it would be better to be dumb and mutex-full.

0static Mutex g_sqlite_mutex;
1static int g_sqlite_count GUARDED_BY(g_sqlite_mutex) = 0;

0LOCK(g_sqlite_mutex);
1if (++g_sqlite_count == 1) {
2   sqlite_config...
3}

0LOCK(g_sqlite_mutex);
1if (--g_sqlite_count == 0) {
2   sqlite_shutdown...
3}

In commit “Implement SQLiteDatabase::Open” (ecc06767e1a91b11179ffb5b70808b69a9c6b552)

Note for future improvement: We should probaby stop this non-standard use of flags. “w” or “a” normally creates a new file not “c”. https://www.gnu.org/software/libc/manual/html_node/Opening-Streams.html. It might also be better to switch away from modes strings to readonly / require_new / require_existing boolean options.

re: #19077 (review)

I did an implementation based on this PR to move from modes string to bool flags; and removed read-only mode as well. I’ll open a PR once this one will be merged.

Since these changes would only simplify this PR, and the more complicated parts don’t involve sqlite, I think you could open a separate PR based on master just omitting the sqlite parts. Also, the change can go further and drop the batch create option, just using the DatabaseOptions::require_existing option instead (storing it if needed).

In commit “Implement SQLiteDatabase::Open” (ecc06767e1a91b11179ffb5b70808b69a9c6b552)

Could avoid the need for this preamble using CREATE TABLE IF NOT EXISTS syntax: https://www.sqlite.org/draft/lang_createtable.html

In commit “Implement SQLiteDatabase::Open” (ecc06767e1a91b11179ffb5b70808b69a9c6b552)

I think you need to do this check even in the (m_db == nullptr) case above in case the file or filesystem is read-only. According to https://www.oreilly.com/library/view/using-sqlite/9781449394592/re303.html SQLITE_OPEN_READWRITE will “Attempt to open the file read/write. If this is not possible, open the file read-only. Opening the file read-only will not result in an error.”

In commit “sqlitedb: Create and lock the wallet directory” (af798a8fd9b3b1a1e0a63a665f6d89c73e8614a3)

This doesn’t seem like the most ideal approach. We were forced to use .walletlock in #11904 because the old version of BDB we use doesn’t support set_lk_exclusive. But with sqlite it would seem simpler and safer to use its exclusive locking functionality: https://www.sqlite.org/pragma.html#pragma_locking_mode instead of rolling our own which is probably less portable and reliable, requires code duplication, and is nonstandard not working with other sqlite database tools.

In commit “sqlitedb: Create and lock the wallet directory” (4f3e5569c3c0eb83d50f7f88e9b673fa857fa08a)

re: #19077 (review)

Since only an EXCLUSIVE lock is acquired after the first write, I’m not sure that the locking pragma provides strong enough guarantees for us. Another bitcoind could conceivably open the same wallet while only the SHARED lock is held.

No bitcoind should open the wallet in shared mode. The simplest approach is for every bitcoind to open the wallet in exclusive mode and fail if exclusive lock cannot be acquired

I did write another suggestion about opening the database earlier to simplify code and avoid closing, unlocking, and reopening the database after verifying, but this is orthogonal to type of locking used.

From https://sqlite.org/pragma.html#pragma_locking_mode

The first time the database is read in EXCLUSIVE mode, a shared lock is obtained and held. The first time the database is written, an exclusive lock is obtained and held.

So opening in exclusive mode means that there is a potential case where bitcoind has not yet written anything and thus only has a shared lock. In that case, another bitcoind could open the same file (and not write anything) and also acquire a shared lock. See https://sqlite.org/lockingv3.html for the definitions of shared and exclusive locks in sqlite3.

In commit “sqlitedb: Create and lock the wallet directory” (a6415a4dc752db171bd842fb376c5c5a919c04e5)

re: #19077 (review)

From sqlite.org/pragma.html#pragma_locking_mode

Oh, you are absolutely right. Just setting the pragma doesn’t get the lock, you have to do an empty transaction to acquire it. This just means you need 3 statements to open the database: pragma locking_mode=exclusive; begin exclusive transaction; commit; instead of 1 statement.

Using the sqlite exclusive lock would be more reliable than .walletlock and simpler because it would be a part of database opening, not in some other code path separate from opening, and because it would no longer requiring extra code removing .walletlock at shutdown (this code already had a bug previously in this PR).

In case it helps, I found it pretty straightforward to test locking from the command line:

• Terminal 1

  0$ sqlite3 db.sqlite
  1sqlite> pragma locking_mode=exclusive;
  2exclusive
  3sqlite> begin exclusive transaction;
  4sqlite> commit;
  5sqlite> 
  

• Terminal 2

  0$ sqlite3 db.sqlite
  1sqlite> pragma locking_mode=exclusive;
  2exclusive
  3sqlite> begin exclusive transaction;
  4Error: database is locked
  5sqlite> 
  

This just means you need 3 statements to open the database:

Then there’s the potential for race conditions in 2 instances: before the pragma is executed, and after it is executed but before the transaction begins.

Edit: Oh I guess we just exit with the database is in use error when begin transaction fails.

re: #19077 (review)

Then there’s the potential for race conditions in 2 instances: before the pragma is executed, and after it is executed but before the transaction begins.

There’s no race condition here:

0db = sqlite_open(path); // Or error "sorry, couldn't open database"
1sqlite_exec(db, "pragma locking_mode=exclusive"); // Or error "sorry, couldn't set locking mode"
2sqlite_exec(db, "begin exclusive transaction"); // Or error "sorry, database is currently in use"
3sqlite_exec(db, "commit"); // Or error "sorry, unexpected failure"

There would be a race condition if the exclusive lock was released after the commit statement. But the point of the pragma is to extend the exclusive lock from point where it is first acquired until the point where the database is closed.

Just noticed @ryanofsky’s comment: #19077 (review)

I think it’s safer to explicitly check if the table is present. In general it’s nice to have a clearly separate code path for stuff we only do on wallet creation (and upgrade).

re: #19077 (review)

Just noticed @ryanofsky’s comment: #19077 (comment)

I think it’s safer to explicitly check if the table is present. In general it’s nice to have a clearly separate code path for stuff we only do on wallet creation (and upgrade).

Dropping ‘if not exists’ and adding an error sounds good (though it might be helpful to say what is unsafe or what dangerous cases you are thinking of). I only suggested using ‘if not exists’ SQL syntax to avoid reimplementing it in C++.

Alternatively, if create is meant as “create if needed”, then it’s seems safer to explicitly check if the table doesn’t exist yet. In that case the error “Failed to create new database” is incorrect.

It is use as a “create if needed.” All wallet DBs are created with the mode “cr+”. I don’t think we should be giving an error if the table doesn’t exist in that case either.

re: #19077 (review)

It is use as a “create if needed.” All wallet DBs are created with the mode “cr+”. I don’t think we should be giving an error if the table doesn’t exist in that case either.

It would require changes outside sqlite.cpp/sqlite.h but it should be possible to throw an error instead of defensively creating tables if the database already already exists and the tables are missing. I think it would be a little safer and probably preferable, but not critical.

0344cce: is PRAGMA fullfsync ephemeral? Documentation isn’t very clear about that. If so, it shouldn’t be under create. On macOS 10.15.7 with sqlite 3.28.0 when I call PRAGMA fullfsync; it returns 0.

Update: it’s fine, create is confusingly named, but this is code is run every time you load a wallet

In commit “Add sqlite to travis and depends” (8bfbad00d4bd94f2b9006e6375ea7dc11adee6fb)

s/NO_SQLITET/NO_SQLITE

In commit “Add SQLiteDatabase and SQLiteBatch dummy classes” (b6eeb2b5ee6964a9034aca6f4be2b72d532589ca)

I think the refcounting code in this PR is confusing and should be removed. Rewrite method can just vacuum without sleeping. Close method can unconditionally close m_db it’s not null. m_refcount doesn’t ever need be referenced, AddRef and RemoveRef don’t ever need to be called, and they be implemented in a transparent way to make it clear they are unused holdovers:

0void AddRef() override { assert(0); /* unused */ }
1void RemoveRef() override { assert(0); /* unused */ }

In commit “Add SQLiteDatabase and SQLiteBatch dummy classes” (f747380289b06ba1631e1cec30b96b5aecb61f36)

Comment “Indicate the a new database user has began using the database. Increments m_refcount” isn’t accurate

In commit “Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch” (46aa20b62e77b33e62e1e17ac6f6a6eb1a9efdce)

Can drop reference counting (see dummy classes comment)

In commit “Implement SQLiteBatch::Close” (fbf44184de758271e759978cd5037efcb4ba4eb1)

This should log an error if TxnAbort() fails. It might also be useful to log an error if TxnAbort succeeds, since this condition should never happen unless there is unpaired TxnBegin call or an unhandled exception.

In commit “Implement SQLiteBatch::Close” (fbf44184de758271e759978cd5037efcb4ba4eb1)

Can drop reference counting (see dummy classes comment)

In commit “Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch” (46aa20b62e77b33e62e1e17ac6f6a6eb1a9efdce)

I don’t think it makes sense to open the database in the batch constructor instead of in the database constructor. It makes the batch constructor assymetric (there’s no database close in the destructor), forces database to be needlessly opened, closed, and reopened when verification is enabled, delays error checking done on open (making debugging and diagnosis more difficult), and prevents taking full advantages of sqlite’s exclusive locking to prevent the database from being opened in different processes.

The purpose of opening the database within a batch is to only open it when the database is going to be used. It is then left open to avoid constantly closing and reopening it.

We also do it to set the mode on the database, but the usefulness of that is arguable but we can deal with that in a followup.

re: #19077 (review)

The purpose of opening the database within a batch is to only open it when the database is going to be used. It is then left open to avoid constantly closing and reopening it.

It sounds like my suggestion was not clear. My suggestion is to open the database when the database object is created, and close it when it is destroyed. This opens and closes the database fewer times than the current implementation (due to not having to open close and reopen for verification) and is simpler (also due to not having to open close and reopen for verification). Feel free to ignore the suggestion or to follow it up later, but I hope I am getting it across clearly.

In commit “Implement SQLiteDatabase::Open” (96d2a5f8b8ea7dd512deaac4d1d70702517af6c1)

Should drop this comment. Copying a wallet may be a problem if you do it in a crazy way, or it may be perfectly safe. BDB code started checking for copies only as a kludge (initial kludge protected against ambiguity in the bdb log format when multiple databases were opened in the same environment, and later the kludge was extended to work around inability to lock BDB databases with set_lk_exclusive.)

In commit “Implement SQLiteDatabase::Open” (96d2a5f8b8ea7dd512deaac4d1d70702517af6c1)

Should add NOT NULL constraint to key and value columns. Code already has to deal with empty string values, so it would be better not to throw in NULL values and errors as well where we don’t need them.

In commit “Add SQLiteDatabase and SQLiteBatch dummy classes” (b6eeb2b5ee6964a9034aca6f4be2b72d532589ca)

param is renamed to mode in later commit, could just call it mode here to avoid rename

In commit “Implement SQLiteDatabase::Verify” (fabd187ccb43d0cc266ae89cd2b2e3b3d098949a)

I don’t think you can close the db if opening failed. I think even on cases where we don’t act on errors, we should at least log them. It would be best if all the sqlite_* calls in this PR logged errors so we are never blindly debugging or guessing about errors like this.

From the docs

Whether or not an error occurs when it is opened, resources associated with the database connection handle should be released by passing it to sqlite3_close() when it is no longer required.

Logging is handled by global error log

re: #19077 (review)

Logging is handled by global error log

Thanks for explaining sqlite3_close. How do you know whether or not something is logged to the error log? Does every function call that doesn’t return SQLITE_OK log to the error log? If so, then I guess there’s no technical reason to handle errors twice even if code looks like it is silently discarding them. I would prefer the uniform approach of handling all errors, but this would give more of excuse not to.

Does every function call that doesn’t return SQLITE_OK log to the error log?

This is my current understanding of how it works based on my tests and the following line from documentation.

SQLite can be configured to invoke a callback function containing an error code and a terse error message whenever anomalies occur.

I can check particular scenarios if you’d like.

re: #19077 (review)

This is my current understanding of how it works based on my tests and the following line from documentation.

This looks like a misreading to me. The documentation could easily say every failing call is logged, but it doesn’t, and even goes out of it’s way to say it “strives to keep error logger traffic low.” But even all errors were guaranteed to be logged, would you expect someone else reading the code to know it? Or will the next person maintaining or debugging this code waste development time checking unchecked return values, instead of just being able to look at the code and see that every call is succeeding. That is why if I were writing this code, I would check for unexpected return values everywhere and print them. Fixing this is just a suggestion. It is fine to ignore the suggestion.

re: #19077 (review)

Indeed I assumed (maybe falsely) that the next maintainer is familiar with SQLite C api. My humble personal preference is not to complicate the code by duplicating the error messages, when the library can do it for us. I just tried to provide more context for the author and the reviewers, sorry if it wasn’t helpful.

This was helpful, but it’s not just assuming some basic level of familiarity with sqlite, it is assuming knowledge of undocumented behavior of the API. Debug logging and error handling are two overlapping things code can do, but neither is a subset of the other. We aren’t using bash/go/C style programming, and we have good non-verbose error handling options available, so I wouldn’t ignore error codes just because sqlite has a debug feature that prints some subset of errors with less context.

From https://sqlite.org/c3ref/open.html

Whether or not an error occurs when it is opened, resources associated with the database connection handle should be released by passing it to sqlite3_close() when it is no longer required.

Since it is ok for a database to not exist when we do Verify, I think it is correct to not log anything on the NOTFOUND error here.

In commit “Implement SQLiteDatabase::Verify” (fabd187ccb43d0cc266ae89cd2b2e3b3d098949a)

This looks like a bug. You can’t delete a prepared statement that wasn’t created, and this might lead to a bad pointer dereference since since stmt pointer is not initialized.

re: #19077 (review)

I’m not sure if we SHOULD finalize failed prepared statement, but it’s a no-op to finalize a NULL pointer.

The pointer looks uninitialized, not null. Even if this actually works, I wouldn’t code that looks so fragile and broken.

re: #19077 (review)

As an out parameter It’s set to NULL in case of an error. This is documented and to me the code in question looks like an intended use of sqlite api.

Sorry, where is this documented? Even assuming it is documented, would you expect anybody else reading the code to know this? Or will the next person maintaining or debugging this waste development time checking it, instead of just being able to look and see that the code won’t dereference an uninitialized pointer. If I were writing this, I would initialize the pointer to null. Fixing this is just a suggestion. It is fine to ignore the suggestion.

From https://sqlite.org/c3ref/prepare.html

*ppStmt is left pointing to a compiled prepared statement that can be executed using sqlite3_step(). If there is an error, *ppStmt is set to NULL.

So stmt should be NULL after prepare if it fails.

From https://sqlite.org/c3ref/finalize.html

Invoking sqlite3_finalize() on a NULL pointer is a harmless no-op.

The application must finalize every prepared statement in order to avoid resource leaks.

Finalizing a nullptr is safe. It also seems like it is recommended to finalize on failure, just in case.

Regardless, I will initialize stmt to nullptr.

In commit “Implement SQLiteDatabase::Verify” (fabd187ccb43d0cc266ae89cd2b2e3b3d098949a)

Would be good to disambiguate this error from error immediately above (step failed vs read failed)

In commit “Implement SQLiteDatabase::Verify” (fabd187ccb43d0cc266ae89cd2b2e3b3d098949a)

Would be good to prefix sqlite error lines with “Failed to verify” text if error string is currently empty here.

In commit “Implement SQLiteDatabase::Verify” (fabd187ccb43d0cc266ae89cd2b2e3b3d098949a)

It is odd to lock the wallet here because it means the wallet will be locked on opening when options.verify is true, but not locked until later when options.verify is false (later when the first batch is created).

The simplest thing approach would seem to just open and lock the database once when the database object is created instead of opening and locking multiple times in different circumstances.

In commit “Implement SQLiteDatabase::Flush, PeriodicFlush, and ReloadDbEnv as No-ops” (addc4630a2a6587b0e07b9c45644117542685fed)

All the comments in the commit description should be comments in the code, so they are not lost in git history and so code makes sense. Would also suggest implementing these methods inline in the header file to avoid giving a misleading impression from reading the class definition that they are implemented to do things.

In commit “sqlitedb: Create and lock the wallet directory” (4f3e5569c3c0eb83d50f7f88e9b673fa857fa08a)

This fails to unlock the wallet if it was opened with options.verify = true, but m_db is null because no batch was created. In practice, I’m guessing this never happens, but this is the type of complication which would go away if we used sqlite locking instead of implementing our own locking.

In commit “Implement SQLiteDatabase::RemoveRef and AddRef” (e63b9504257165780b2c56ec46bc842b8d680dbc)

Can drop reference counting (see dummy classes comment) and this whole commit