-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077
Conversation
92552af
to
e09e7d7
Compare
I've dropped the amalgamation file |
92d36c5
to
20dcff6
Compare
Pretty amazing work! Thanks for doing this. Concept ACK on a BDB replacement for descriptor wallets. As for concrete implementation steps, maybe it would make sense to PR the DB flexibility first, then additional storage engines later. |
I don't think it really makes sense to add a database system that we aren't going to use.
I think there's two primary reasons to choose sqlite over an internal format.
#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. |
930518e
to
0ba7d4e
Compare
Concept ACK Nice work! Very readable code! |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
25d9dfa
to
76c7093
Compare
CONCEPT ACK |
135afa7 wallet: remove db mode string (Ivan Metlushko) Pull request description: This is a [follow-up](bitcoin#19077 (comment)) for bitcoin#19077 This PR simplifies DB interface by removing mode string from `WalletDatabase` and `WalletBatch`. The mode string was used to determine two flags for the instantiation of db connection: 1) read-only flag. Never used on connection level. And on batch level Is only used within `BerkeleyDatabase::Rewrite` where it's replaced with bool flag. 2) create flag. Is not required as we always check `require_existing` & `require_create` flags in `MakeDatabase()` before creating actual database instance. So we can safely default to always creating database if it doesn't exist yet. ACKs for top commit: achow101: ACK 135afa7 laanwj: Code review ACK 135afa7 Tree-SHA512: f49c07c7387c02e517a58199620a678a918f8dfc20d1347d29fd6adea0bc89698c26cb8eef42b0977961c11c207c4bbe109bc31059f47c126cc600b01fd987eb
…base and use it for new descriptor wallets c4a29d0 Update wallet_multiwallet.py for descriptor and sqlite wallets (Russell Yanofsky) 310b0fd Run dumpwallet for legacy wallets only in wallet_backup.py (Andrew Chow) 6c6639a Include sqlite3 in documentation (Andrew Chow) f023b7c wallet: Enforce sqlite serialized threading mode (Andrew Chow) 6173269 Set and check the sqlite user version (Andrew Chow) 9d3d2d2 Use network magic as sqlite wallet application ID (Andrew Chow) 9af5de3 Use SQLite for descriptor wallets (Andrew Chow) 9b78f3c walletutil: Wallets can also be sqlite (Andrew Chow) ac38a87 Determine wallet file type based on file magic (Andrew Chow) 6045f77 Implement SQLiteDatabase::MakeBatch (Andrew Chow) 727e6b2 Implement SQLiteDatabase::Verify (Andrew Chow) b4df8fd Implement SQLiteDatabase::Rewrite (Andrew Chow) 010e365 Implement SQLiteDatabase::TxnBegin, TxnCommit, and TxnAbort (Andrew Chow) ac5c161 Implement SQLiteDatabase::Backup (Andrew Chow) f6f9cd6 Implement SQLiteBatch::StartCursor, ReadAtCursor, and CloseCursor (Andrew Chow) bf90e03 Implement SQLiteBatch::ReadKey, WriteKey, EraseKey, and HasKey (Andrew Chow) 7aa4562 Add SetupSQLStatements (Andrew Chow) 6636a26 Implement SQLiteBatch::Close (Andrew Chow) 9382535 Implement SQLiteDatabase::Close (Andrew Chow) a0de833 Implement SQLiteDatabase::Open (Andrew Chow) 3bfa0fe Initialize and Shutdown sqlite3 globals (Andrew Chow) 5a488b3 Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch (Andrew Chow) ca8b7e0 Implement SQLiteDatabaseVersion (Andrew Chow) 7577b6e Add SQLiteDatabase and SQLiteBatch dummy classes (Andrew Chow) e87df82 Add sqlite to travis and depends (Andrew Chow) 54729f3 Add libsqlite3 (Andrew Chow) Pull request description: 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. ACKs for top commit: Sjors: re-utACK c4a29d0 promag: Tested ACK c4a29d0. fjahr: reACK c4a29d0 S3RK: Re-review ACK c4a29d0 meshcollider: re-utACK c4a29d0 hebasto: re-ACK c4a29d0, only rebased since my [previous](bitcoin#19077 (review)) review, verified with `git range-diff master d18892d c4a29d0`. 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 bitcoin#11687, which did not require any active interfaction. 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. Tree-SHA512: 19145732e5001484947352d3175a660b5102bc6e833f227a55bd41b9b2f4d92737bbed7cead64b75b509decf9e1408cd81c185ab1fb4b90561aee427c4f9751c
624bab0 test: add coverage for getwalletinfo format field (Jon Atack) 5e737a0 rpc, wallet: Expose database format in getwalletinfo (João Barbosa) Pull request description: Support for sqlite based wallets was added in #19077. This PR adds the `format` key in `getwalletinfo` response, that can be `bdb` or `sqlite`. ACKs for top commit: jonatack: Tested ACK 624bab0 laanwj: Code review ACK 624bab0. MarcoFalke: doesn't hurt ACK 624bab0 hebasto: ACK 624bab0, tested on Linux Mint 20 (x86_64). meshcollider: utACK 624bab0 Tree-SHA512: a81f8530f040f6381d33e073a65f281993eccfa717424ab6e651c1203cbaf27794dcb7175570459e7fdaa211565bc060d0a3ecbe70d2b6f9c49b8d5071e4441c
…info 624bab0 test: add coverage for getwalletinfo format field (Jon Atack) 5e737a0 rpc, wallet: Expose database format in getwalletinfo (João Barbosa) Pull request description: Support for sqlite based wallets was added in bitcoin#19077. This PR adds the `format` key in `getwalletinfo` response, that can be `bdb` or `sqlite`. ACKs for top commit: jonatack: Tested ACK 624bab0 laanwj: Code review ACK 624bab0. MarcoFalke: doesn't hurt ACK 624bab0 hebasto: ACK 624bab0, tested on Linux Mint 20 (x86_64). meshcollider: utACK 624bab0 Tree-SHA512: a81f8530f040f6381d33e073a65f281993eccfa717424ab6e651c1203cbaf27794dcb7175570459e7fdaa211565bc060d0a3ecbe70d2b6f9c49b8d5071e4441c
defe48a doc: Update wallet files in files.md (Hennadii Stepanov) Pull request description: This PR is a #19077 follow up, and it addresses the [comment](bitcoin/bitcoin#19077 (comment)): > 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" ACKs for top commit: RiccardoMasutti: ACK defe48a meshcollider: ACK defe48a Tree-SHA512: 39939f86a9c7842bf06913998305dcbd6209585f1da0fe9c274bac0572eb8464e59176884dd9e2b91312f34efad40cdeb4085ec72c2a2c1b33d16b6ab505140c
defe48a doc: Update wallet files in files.md (Hennadii Stepanov) Pull request description: This PR is a bitcoin#19077 follow up, and it addresses the [comment](bitcoin#19077 (comment)): > 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" ACKs for top commit: RiccardoMasutti: ACK defe48a meshcollider: ACK defe48a Tree-SHA512: 39939f86a9c7842bf06913998305dcbd6209585f1da0fe9c274bac0572eb8464e59176884dd9e2b91312f34efad40cdeb4085ec72c2a2c1b33d16b6ab505140c
status = DatabaseStatus::FAILED_VERIFY; | ||
return nullptr; | ||
} | ||
return db; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it should, but istm it shouldn't matter. I'm pretty sure we never check status
if there isn't a failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jup, we don't check status
if there is no failure. It is just a style question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also happening with clang, so I'll try to fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New function is not currently called but will be called in upcoming commits. It moves database path checking, and existence checking, and already-loaded checking, and verification into a single function so this logic does not need to be repeated all over higher level wallet code, and so higher level code does not need to change when SQLite support is added in bitcoin/bitcoin#19077. This also lets higher level wallet code make fewer assumptions about the contents of wallet directories. This commit just adds the new function and does not change behavior in any way.
Summary: New function is not currently called but will be called in upcoming commits. It moves database path checking, and existence checking, and already-loaded checking, and verification into a single function so this logic does not need to be repeated all over higher level wallet code, and so higher level code does not need to change when SQLite support is added in bitcoin/bitcoin#19077. This also lets higher level wallet code make fewer assumptions about the contents of wallet directories. This commit just adds the new function and does not change behavior in any way. This is a backport of [[bitcoin/bitcoin#19619 | core#19619]] [2/8] bitcoin/bitcoin@b5b4141 Depends on D10224 Test Plan: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10225
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [3/26] bitcoin/bitcoin@7577b6e Depends on D10472 Note: I had to comment out `m_mock` to avoid triggering compiler warnings about unused variables. Test Plan: ``` cmake .. -GNinja -DCMAKE_C_COMPILER=clang-12 -DCMAKE_CXX_COMPILER=clang++-12 -DENABLE_CLANG_TIDY=ON ninja ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10542
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [4/26] bitcoin/bitcoin@ca8b7e0 Depends on D10542 Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10543
…base/Batch Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [5/26] bitcoin/bitcoin@5a488b3 Depends on D10543 note: unused member `SQLiteBatch::m_database` commented out to avoid compiler warnings. Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10545
Summary: sqlite3 recommends that sqlite3_initialize be called when the application starts, and sqlite3_shutdown when it stops. Since we don't always use sqlite3, we initialize it when a SQLiteDatabse is constructed (calling sqlite3_initialize after initialized is a no-op). We call sqlite3_shutdown when we see that there are no databases opened. The number of open databases is tracked by an atomic g_dbs_open. This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [6/26] bitcoin/bitcoin@3bfa0fe Depends on D10545 Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10546
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [7/26] bitcoin/bitcoin@a0de833 Depends on D10546 Note: this uncomments the previously unused member vars `m_mock` (D10542) and `m_database` (D10545) Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10547
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [8/26] bitcoin/bitcoin@9382535 Depends on D10547 Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10551
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [9/26] bitcoin/bitcoin@6636a26 Depends on D10551 Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10552
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [10/26] bitcoin/bitcoin@7aa4562 Depends on D10552 Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10553
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [11/26] bitcoin/bitcoin@bf90e03 Depends on D10553 Test Plan: With clang-tiy: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10554
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [12/26] bitcoin/bitcoin@f6f9cd6 Depends on D10554 Test Plan: With Clang-tidy: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10555
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [13/26] bitcoin/bitcoin@ac5c161 Depends on D10555 Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D10561
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [14/26] bitcoin/bitcoin@010e365 Depends on D10561 Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D10562
Summary: 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. This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [15/26] bitcoin/bitcoin@010e365 Depends on D10562 Test Plan: With clang-tidy `ninja` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D10563
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [16/26] bitcoin/bitcoin@727e6b2 Note: the code in the original commit does not compile. I had to declare `int ret` in `SQLiteDatabase::Verify` Depends on D10563 Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D10564
Summary: This is a backport of [[bitcoin/bitcoin#19077 | core#19077]] [17/26] bitcoin/bitcoin@6045f77 Depends on D10564 Test Plan: With clang-tidy: `ninja` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D10565
Summary: New function is not currently called but will be called in upcoming commits. It moves database path checking, and existence checking, and already-loaded checking, and verification into a single function so this logic does not need to be repeated all over higher level wallet code, and so higher level code does not need to change when SQLite support is added in bitcoin/bitcoin#19077. This also lets higher level wallet code make fewer assumptions about the contents of wallet directories. This commit just adds the new function and does not change behavior in any way. This is a backport of [[bitcoin/bitcoin#19619 | core#19619]] [2/8] bitcoin/bitcoin@b5b4141 Depends on D10224 Test Plan: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10225
This PR adds a new class
SQLiteDatabase
which is a subclass ofWalletDatabase
. 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 tablemain
which has two columns,key
andvalue
both with the typeblob
.For new descriptor wallets, we will create a
SQLiteDatabase
instead of aBerkeleyDatabase
. 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 thewallet.dat
file. SQLite begins it's files with a null terminated stringSQLite format 3
. BDB has0x00053162
at byte 12 (note that the byte order of this integer depends on the system endianness). So when we see that there is awallet.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 aswallet.dat
is something that is specifically being looked for. If we don't want this behavior, then I do have another branch which createswallet.sqlite
files instead, but I find that this direction is easier.