wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101 · Pull Request #19077 · bitcoin/bitcoin

achow101 · 2020-05-27T02:42:07Z

This PR adds a new class SQLiteDatabase which is a subclass of WalletDatabase. This provides access to a SQLite database that is used to store the wallet records. To keep compatibility with BDB and to complexity of the change down, we don't make use of many SQLite's features. We use it strictly as a key-value store. We create a table main which has two columns, key and value both with the type blob.

For new descriptor wallets, we will create a SQLiteDatabase instead of a BerkeleyDatabase. There is no requirement that all SQLite wallets are descriptor wallets, nor is there a requirement that all descriptor wallets be SQLite wallets. This allows for existing descriptor wallets to work as well as keeping open the option to migrate existing wallets to SQLite.

We keep the name wallet.dat for SQLite wallets. We are able to determine which database type to use by searching for specific magic bytes in the wallet.dat file. SQLite begins it's files with a null terminated string SQLite format 3. BDB has 0x00053162 at byte 12 (note that the byte order of this integer depends on the system endianness). So when we see that there is a wallet.dat file that we want to open, we check for the magic bytes to determine which database system to use.

I decided to keep the wallet.dat naming to keep things like backup script to continue to function as they won't need to be modified to look for a different file name. It also simplifies a couple of things in the implementation and the tests as wallet.dat is something that is specifically being looked for. If we don't want this behavior, then I do have another branch which creates wallet.sqlite files instead, but I find that this direction is easier.

achow101 · 2020-05-27T03:19:02Z

I've dropped the amalgamation file

jonasschnelli · 2020-05-27T15:54:20Z

Pretty amazing work! Thanks for doing this.
For testing purposes, would it make sense to add logdb (#5686, simple implementation) in order to test and benchmark?

Concept ACK on a BDB replacement for descriptor wallets.
Still unsure wether sqlite or an internal format should be chosen. Maybe a comparison(-matrix) of internal vs. sqlite could be done?

As for concrete implementation steps, maybe it would make sense to PR the DB flexibility first, then additional storage engines later.

achow101 · 2020-05-27T16:25:46Z

For testing purposes, would it make sense to add logdb (#5686, simple implementation) in order to test and benchmark?

I don't think it really makes sense to add a database system that we aren't going to use.

Still unsure wether sqlite or an internal format should be chosen. Maybe a comparison(-matrix) of internal vs. sqlite could be done?

I think there's two primary reasons to choose sqlite over an internal format.

Review and implementation are much simpler The library already exists so implementation just means correctly using the API. Reviewers won't have to review a file format implementation and convince themselves that that format won't corrupt and is robust.
Better guarantees of consistency and non-corruption. SQLite is very well tested and very widely used. I think they are able better guarantee that data will get written, won't get lost, and won't get corrupted, than we would be able to with an internal format.

As for concrete implementation steps, maybe it would make sense to PR the DB flexibility first, then additional storage engines later.

#18971 does the DB class stuff that gives us this flexibility. This PR is adding in the storage engine and the logic for CWallet to choose which storage to use.

practicalswift · 2020-05-27T19:21:45Z

Concept ACK

Nice work! Very readable code!

DrahtBot · 2020-05-27T19:50:17Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Unify wallet directory lock error message #20094 (wallet: Unify wallet directory lock error message by hebasto)
Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks #19502 (Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr)
wallet: let Listwalletdir do not iterate through our blocksdata. #19419 (wallet: let Listwalletdir do not iterate through our blocksdata. by Saibato)
refactor: remove ::vpwallets and related global variables #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
Fix crashes and infinite loop in ListWalletDir() #18095 (Fix crashes and infinite loop in ListWalletDir() by uhliksk)
net: Add NAT-PMP port forwarding support #18077 (net: Add NAT-PMP port forwarding support by hebasto)

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.

laanwj · 2020-05-28T10:41:25Z

CONCEPT ACK
🎉 🥳 🎉
Very happy to move on from BerkeleyDB and I've always liked sqlite as a versatile but still minimalistic replacement.

Rewrite uses the VACUUM command which does exactly what we want. A specific advertised use case is to compact a database and ensure that any deleted data is actually deleted.

MakeWalletDatabase no longer has a default DatabaseFormat. Instead callers, like CWallet::Create, need to specify the database type to create if the file does not exist. If it exists and NONE is given, then CreateWalletDatabase will try to autodetect the type.

Descriptor wallets don't support dumpwallet, so make the tests that do dumpwallet legacy wallet only.

achow101 · 2020-10-14T15:34:37Z

Rebased. There was a silent merge conflict with #20130.

@hebasto @S3RK @promag @Sjors @ryanofsky @meshcollider @fjahr Could you all re-review/re-ACK this so we can merge it before the feature freeze tomorrow?

hebasto

re-ACK c4a29d0, only rebased since my previous review, verified with git range-diff master d18892dcc c4a29d0a9.

ryanofsky

Code review ACK c4a29d0. I am honestly confused about reasons for locking into wallet.dat again when it's so easy now to use a clean format. I assume I'm just very dense, or there's some unstated reason, because the only thing that's been brought up are unrealistic compatibility scenarios (all require actively creating a wallet with non-default descriptor+sqlite option, then trying to using the descriptor+sqlite wallets with old software or scripts and ignoring the results) that we didn't pay attention to with previous PRs like #11687, which did not require any active interfaction.

Changes since previous review were: using pkg-config, naming/whitespace/documentation/quoting/const/log/assert changes, fixing leak if database constructor throws, enabling unconditional fullsync

ryanofsky · 2020-10-14T16:15:16Z

doc/files.md

-`./`         | `wallet.dat`      | Personal wallet (BDB) with keys and transactions
+`./`         | `wallet.dat`      | Personal wallet with keys and transactions. May be either a Berkeley DB or SQLite database file.
 `./`         | `.walletlock`     | Wallet lock file
+`./`         | `wallet.dat-journal` | SQLite Rollback Journal file for `wallet.dat`. Usually created at start and deleted on shutdown. A user *must keep it as safe* as the `wallet.dat` file.


In commit "Include sqlite3 in documentation" (6c6639a)

If need to update, there are two corrections that could be made:

Line 69 "Wallets are Berkeley DB (BDB) databases" is no longer true

Line 76 "Wallet lock file" should say "BDB wallet lock file"

Addressed in #20152.

Sjors · 2020-10-14T17:54:47Z

re-utACK c4a29d0

promag · 2020-10-14T20:30:55Z

Tested ACK c4a29d0.

I am honestly confused about reasons for locking into wallet.dat again when it's so easy now to use a clean format.

@ryanofsky clean format as in different file extension? I think it can be discussed and changed even after feature freeze, just like @achow says in OP.

jonatack

ACK c4a29d0, debug builds and test runs after rebase to latest master @ c2c4dba, some manual testing creating, using, unloading and reloading a few different new sqlite descriptor wallets over several node restarts/shutdowns.

Nice work and reviewing getting this in shape. Looking forward to testing this further.

2020-10-14T21:54:12Z Using SQLite Version 3.33.0

fjahr · 2020-10-14T23:40:33Z

reACK c4a29d0

S3RK · 2020-10-15T02:11:30Z

Re-review ACK c4a29d0
And thanks for fixing the silent merge conflict

fanquake · 2020-10-15T03:40:56Z

@meshcollider has declared that he'll be sorely disappointed if he doesn't get to pull the merge trigger on this PR.

meshcollider · 2020-10-15T07:11:52Z

re-utACK c4a29d0

Let's go 🚀

maflcko · 2020-11-05T17:21:05Z

src/wallet/sqlite.cpp

+            status = DatabaseStatus::FAILED_VERIFY;
+            return nullptr;
+        }
+        return db;


Does this return path need to set status = DatabaseStatus::SUCCESS;?

I am asking because gcc seems to generate binaries that will inject false positives into valgrind. I haven't found a bug in the code.

Though, the bdb version also seems to set ::SUCCESS, so it might be appropriate here as well?

I suppose it should, but istm it shouldn't matter. I'm pretty sure we never check status if there isn't a failure.

Jup, we don't check status if there is no failure. It is just a style question.

Also happening with clang, so I'll try to fix this

fanquake added the Wallet label May 27, 2020

achow101 force-pushed the sqlite-wallet branch 2 times, most recently from 92552af to e09e7d7 Compare May 27, 2020 03:18

achow101 force-pushed the sqlite-wallet branch 3 times, most recently from 92d36c5 to 20dcff6 Compare May 27, 2020 05:11

achow101 force-pushed the sqlite-wallet branch from 20dcff6 to 4680cf8 Compare May 27, 2020 16:17

achow101 force-pushed the sqlite-wallet branch 2 times, most recently from 930518e to 0ba7d4e Compare May 27, 2020 18:41

DrahtBot mentioned this pull request May 27, 2020

ci: tsan with -stdlib=libc++-10 #19041

Merged

achow101 force-pushed the sqlite-wallet branch 2 times, most recently from 25d9dfa to 76c7093 Compare May 27, 2020 21:04

achow101 force-pushed the sqlite-wallet branch from 76c7093 to e580a2f Compare May 28, 2020 20:55

achow101 and others added 13 commits October 14, 2020 11:28

Implement SQLiteDatabase::TxnBegin, TxnCommit, and TxnAbort

010e365

Implement SQLiteDatabase::Rewrite

b4df8fd

Rewrite uses the VACUUM command which does exactly what we want. A specific advertised use case is to compact a database and ensure that any deleted data is actually deleted.

Implement SQLiteDatabase::Verify

727e6b2

Implement SQLiteDatabase::MakeBatch

6045f77

Determine wallet file type based on file magic

ac38a87

walletutil: Wallets can also be sqlite

9b78f3c

Use network magic as sqlite wallet application ID

9d3d2d2

Set and check the sqlite user version

6173269

wallet: Enforce sqlite serialized threading mode

f023b7c

Include sqlite3 in documentation

6c6639a

Run dumpwallet for legacy wallets only in wallet_backup.py

310b0fd

Descriptor wallets don't support dumpwallet, so make the tests that do dumpwallet legacy wallet only.

Update wallet_multiwallet.py for descriptor and sqlite wallets

c4a29d0

hebasto approved these changes Oct 14, 2020

View reviewed changes

ryanofsky approved these changes Oct 14, 2020

View reviewed changes

jonatack reviewed Oct 14, 2020

View reviewed changes

meshcollider mentioned this pull request Oct 15, 2020

Sqlite wallet storage #18916

Closed

hebasto mentioned this pull request Oct 15, 2020

doc: Update wallet files in files.md #20152

Merged

achow101 mentioned this pull request Oct 15, 2020

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

This was referenced Oct 20, 2020

build: Make sqlite support optional (compile-time) #20156

Merged

wallet, refactor: Include headers instead of function declarations #20206

Closed

maflcko reviewed Nov 5, 2020

View reviewed changes

baby636 approved these changes Sep 21, 2021

View reviewed changes

Conversation

achow101 commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented May 27, 2020

Uh oh!

jonasschnelli commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented May 27, 2020

Uh oh!

practicalswift commented May 27, 2020

Uh oh!

DrahtBot commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Oct 14, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

Sjors commented Oct 14, 2020

Uh oh!

promag commented Oct 14, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr commented Oct 14, 2020

Uh oh!

S3RK commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 15, 2020

Uh oh!

meshcollider commented Oct 15, 2020

Uh oh!

maflcko Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

19 participants

achow101 commented May 27, 2020 •

edited

Loading

jonasschnelli commented May 27, 2020 •

edited

Loading

DrahtBot commented May 27, 2020 •

edited

Loading

laanwj commented May 28, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading

S3RK commented Oct 15, 2020 •

edited

Loading