wallet: Allow users to create a wallet that encrypts all database records #28142

achow101 · 2023-07-24T21:15:15Z

A couple of users have requested that we support wallets that encrypt everything, even if the wallet is watch-only, in order to have better privacy if the wallet is stolen. This PR introduces an EncryptedDatabase backend for the wallet which encrypts/decrypts each key-value record individually before reading from or writing to the database.

EncryptedDatabase is only supported for SQLite databases and descriptor wallets. This was done in order to have an easier way to get downgrade protection that also does not involve writing an existing record in plaintext (e.g. minversion or flags). SQLite has a fixed field "application id" that we already use for cross-network protection. This is reused to detect if a sqlite database is an encrypted wallet, and thus it prevents older software from attempting to open such wallets.

In order to read records from the database, EncryptedDatabase will cache the decrypted key in memory so that it can lookup the encrypted key in the database. Values will always be decrypted when read.

The encryption scheme is the same one that we use for encrypting private keys. It's not that great, but I didn't feel like re-implementing it, and it seems good enough. The encryption key itself is encrypted with the passphrase and stored in a new record.

Wallets with encrypted databases cannot be loaded on start. They can only be loaded by explicit user action through loadwallet which now has a db_passphrase parameter to allow the decryption of these wallets. createwallet also has a db_passphrase to create these wallets. For now, there is no way to encrypt the database after it has been created, nor is there a way to change the passphrase.

DrahtBot · 2023-07-24T21:15:18Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr
Concept ACK	stevenroose

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29129 (wallet, rpc: add BIP44 account in createwallet by brunoerg)
#29119 (refactor: Use std::span over Span by maflcko)
#29112 (sqlite: Disallow writing from multiple SQLiteBatchs by achow101)
#29071 (refactor: Remove Span operator==, Use std::ranges::equal by maflcko)
#29054 (wallet: reenable sethdseed for descriptor wallets by achow101)
#26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#22341 (rpc: add path to gethdkey by Sjors)

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.

achow101 · 2023-07-24T21:22:57Z

cc @stevenroose

luke-jr · 2023-07-25T19:05:08Z

Concept NACK, I think. Wallet encryption is not going to help you if someone steals your wallet, only if someone gains very temporary access to your keyboard/mouse only. I don't think this full-encryption you're proposing has a real use case.

(What might make sense is to support fully encrypting backups.)

maflcko · 2023-07-25T14:00:09Z

src/wallet/sqlite.h

@@ -99,7 +101,7 @@ class SQLiteDatabase : public WalletDatabase
    SQLiteDatabase() = delete;

    /** Create DB handle to real database */
-    SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false);
+    SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false, std::array<const std::byte, 4> app_id_xor = {std::byte{0}, std::byte{0}, std::byte{0}, std::byte{0}});


Suggested change

SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false, std::array<const std::byte, 4> app_id_xor = {std::byte{0}, std::byte{0}, std::byte{0}, std::byte{0}});

SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false, std::array<const std::byte, 4> app_id_xor = {});

Nit: I think you can just let the compiler fill in the zeros for you by using {}.

maflcko · 2023-07-25T14:00:22Z

src/wallet/sqlite.h

@@ -146,7 +148,7 @@ class SQLiteDatabase : public WalletDatabase
    bool m_use_unsafe_sync;
 };

-std::unique_ptr<SQLiteDatabase> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+std::unique_ptr<SQLiteDatabase> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::array<const std::byte, 4> app_id_xor = {std::byte{0}, std::byte{0}, std::byte{0}, std::byte{0}});


stevenroose · 2023-08-10T18:03:39Z

Concept NACK, I think. Wallet encryption is not going to help you if someone steals your wallet, only if someone gains very temporary access to your keyboard/mouse only. I don't think this full-encryption you're proposing has a real use case.

(What might make sense is to support fully encrypting backups.)

I requested this because I have a real use case.. I run a full node that is on an unencrypted external drive. Bitcoin data is public data, it shouldn't be encrypted. I want to use the wallet for a watch-only wallet though. Unfortunately the wallet files live alongside the public blockchain data so I would have to store the wallet on an unencrypted drive (that I occasionally want to borrow out to friends for them to copy the blockchain in order not to have to download it). That would reveal my entire wallet to anyone that gets hold of this drive.

Same would hold for a laptop. The blockchain is public data, encrypting it entirely is just a pointless performance loss. So it should always be stored on an unencrypted disk for performance reasons. But then you can't use the wallet. Even an encrypted wallet (currently only encrypting the private keys) leaks your entire wallet's history.

stevenroose · 2023-08-10T18:04:01Z

Concept ACK :) (Concept Request, in fact)

furszy

I requested this because I have a real use case.. I run a full node that is on an unencrypted external drive. Bitcoin data is public data, it shouldn't be encrypted. I want to use the wallet for a watch-only wallet though. Unfortunately the wallet files live alongside the public blockchain data so I would have to store the wallet on an unencrypted drive (that I occasionally want to borrow out to friends for them to copy the blockchain in order not to have to download it). That would reveal my entire wallet to anyone that gets hold of this drive.

Why not just load the wallet from a different directory?
It is as simple as typing loadwallet <absolute_wallet_dir_path> true once.

Much safer to not share the wallet data at all than sharing it encrypted.

stevenroose · 2023-08-11T15:48:58Z

@furszy That would work. My experience though is that touching anything inside Core's directories can get me in trouble. I once renamed a wallet file and Qt broke. It would only work again if I went into some JSON file to rename the wallet there too.. I'll do that for now. But anyway, someone getting hold of your hard drive shouldn't learn your wallet metadata, I think that's a good principle.

furszy

@furszy That would work. My experience though is that touching anything inside Core's directories can get me in trouble. I once renamed a wallet file and Qt broke. It would only work again if I went into some JSON file to rename the wallet there too..

Well, that is not the case for loading the wallet from an external directory. No need to touch anything on your system. Just run the RPC command.

That being said, if you can trigger any crash by manually changing something on your system, feel more than welcome to create as many issues as you need until all crashes are resolved. We should be able to fix them all. Core shouldn't crash (unless you modify something when is running..).

maflcko · 2023-08-23T09:46:03Z

Maybe mark as draft while CI is red?

It's useful to be able to just read a record without the batch doing any sort of deserialization. The new overload of Read will just place the record's value into the provided DataStream.

EncryptedDatabase is a WalletDatabase that encrypts the records before writing them to an underlying WalletDatabase. This encryption occurs transparently to the higher level application logic so the wallet does not need to be concerned about whether the data it is writing is encrypted. In order to work with prefix matching and cursor iteration in an order that we are expecting, EncryptedDatabase maintains a map of the unencrypted record keys to the encrypted record keys. When given the plaintext record key to pull up, it can retrieve the encrypted record key and then retrieve the encrypted record from the underlying database.

EncrytpedDB wallets will use sqlite but with a different application id. This provides downgrade protection in addition to easy identification of encrypted dbs. The application id will be the network magic XOR'd with 0x36932d47 (randomly generated value).

Wallets with encrypted databases need the user to provide their database passphrase which cannot be done on start, so skip any such wallets on startup.

achow101 · 2024-01-06T00:03:50Z

Marking as up for grabs.

DrahtBot added the Wallet label Jul 24, 2023

achow101 force-pushed the encrypt-watch-only branch from eb35b83 to d97c315 Compare July 24, 2023 21:20

achow101 mentioned this pull request Jul 24, 2023

Open fully encrypted wallets bitcoin-core/gui#747

Closed

DrahtBot added the CI failed label Jul 24, 2023

This was referenced Jul 25, 2023

rpc: add path to gethdkey #22341

Closed

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

maflcko reviewed Jul 27, 2023

View reviewed changes

This was referenced Aug 2, 2023

Silent Payments: receiving #28202

Closed

Silent Payments: send and receive #27827

Closed

achow101 force-pushed the encrypt-watch-only branch from d97c315 to d581930 Compare August 4, 2023 23:08

furszy reviewed Aug 10, 2023

View reviewed changes

furszy reviewed Aug 11, 2023

View reviewed changes

achow101 force-pushed the encrypt-watch-only branch 2 times, most recently from 7e963f4 to 7870146 Compare August 23, 2023 16:27

DrahtBot removed the CI failed label Aug 24, 2023

DrahtBot added CI failed and removed CI failed labels Sep 6, 2023

DrahtBot mentioned this pull request Sep 11, 2023

wallet: Receive silent payment transactions #28453

Closed

DrahtBot mentioned this pull request Sep 12, 2023

kernel: Remove protocol.h/netaddress.h/compat.h from kernel headers #28423

Merged

DrahtBot added the Needs rebase label Sep 14, 2023

achow101 force-pushed the encrypt-watch-only branch from 7870146 to 9137722 Compare September 14, 2023 22:17

DrahtBot removed the Needs rebase label Sep 14, 2023

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

furszy mentioned this pull request Nov 1, 2023

wallet: Cleanup accidental encryption keys in watchonly wallets #28724

Merged

DrahtBot mentioned this pull request Dec 5, 2023

build: Enable -Wunreachable-code #28999

Merged

DrahtBot added the Needs rebase label Dec 11, 2023

achow101 added 2 commits December 11, 2023 15:32

crypter: Separate setting IV from setting key

486a203

crypter: Make sure IV is set too

c0e1a7e

achow101 force-pushed the encrypt-watch-only branch from 9137722 to f7fc592 Compare December 11, 2023 20:52

DrahtBot mentioned this pull request Dec 11, 2023

wallet: reenable sethdseed for descriptor wallets #29054

Closed

DrahtBot removed the Needs rebase label Dec 11, 2023

achow101 added 7 commits December 11, 2023 17:13

walletdb: Add WalletBatch::Read overload for DataStream

c76438d

It's useful to be able to just read a record without the batch doing any sort of deserialization. The new overload of Read will just place the record's value into the provided DataStream.

walletdb: Have MakeDatabase also create encrypted dbs

754c055

wallet, rpc: Be able to create and load wallets with encrypted dbs

c6aed1e

wallet: Skip loading on start of wallets with encrypted databases

8ac458a

Wallets with encrypted databases need the user to provide their database passphrase which cannot be done on start, so skip any such wallets on startup.

test: Add functional test for wallets with encrypted db

9b845cc

achow101 force-pushed the encrypt-watch-only branch from f7fc592 to 9b845cc Compare December 11, 2023 22:13

This was referenced Dec 13, 2023

refactor: Remove Span operator==, Use std::ranges::equal #29071

Merged

sqlite: Disallow writing from multiple SQLiteBatchs #29112

Merged

refactor: Use std::span over Span #29119

Closed

DrahtBot mentioned this pull request Dec 21, 2023

wallet, rpc: add BIP44 account in createwallet #29129

Closed

achow101 closed this Jan 6, 2024

achow101 added the Up for grabs label Jan 6, 2024

bitcoin locked and limited conversation to collaborators Jan 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: Allow users to create a wallet that encrypts all database records #28142

wallet: Allow users to create a wallet that encrypts all database records #28142

Uh oh!

achow101 commented Jul 24, 2023

Uh oh!

DrahtBot commented Jul 24, 2023 •

edited

Loading

Uh oh!

achow101 commented Jul 24, 2023

Uh oh!

luke-jr commented Jul 25, 2023

Uh oh!

maflcko Jul 25, 2023

Uh oh!

maflcko Jul 25, 2023

Uh oh!

stevenroose commented Aug 10, 2023

Uh oh!

stevenroose commented Aug 10, 2023

Uh oh!

furszy left a comment

Uh oh!

stevenroose commented Aug 11, 2023

Uh oh!

furszy left a comment •

edited

Loading

Uh oh!

maflcko commented Aug 23, 2023

Uh oh!

achow101 commented Jan 6, 2024

Uh oh!

Uh oh!

	SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false, std::array<const std::byte, 4> app_id_xor = {std::byte{0}, std::byte{0}, std::byte{0}, std::byte{0}});
	SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false, std::array<const std::byte, 4> app_id_xor = {});

wallet: Allow users to create a wallet that encrypts all database records #28142

wallet: Allow users to create a wallet that encrypts all database records #28142

Uh oh!

Conversation

achow101 commented Jul 24, 2023

Uh oh!

DrahtBot commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

achow101 commented Jul 24, 2023

Uh oh!

luke-jr commented Jul 25, 2023

Uh oh!

maflcko Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Aug 10, 2023

Uh oh!

stevenroose commented Aug 10, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Aug 11, 2023

Uh oh!

furszy 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!

maflcko commented Aug 23, 2023

Uh oh!

achow101 commented Jan 6, 2024

Uh oh!

Uh oh!

DrahtBot commented Jul 24, 2023 •

edited

Loading

furszy left a comment •

edited

Loading