wallet: make encryption state updates atomic #35752

l0rinc commented at 2:02 AM on July 20, 2026: contributor

Problem: Wallet encryption and passphrase changes can leave database records and live key state out of sync when a database operation fails. Encryption can report success without persisting the master key, and a passphrase change can activate the new passphrase only in memory. Descriptor key write failures can publish keys that were not persisted, while erase failures can leave plaintext keys on disk. A failed encryption transaction commit aborts the node, and a re-encryption failure during a passphrase change can leave a previously locked wallet unlocked. Boolean results also force callers to duplicate error messages or report database and encryption failures as incorrect passphrases.

Fix: Publish wallet encryption state only after the database transaction commits, and publish newly inserted descriptor keys only after their database writes succeed. For passphrase changes, restore the original lock state after validating the old passphrase and replace the live master key only after persisting its re-encrypted value. These changes preserve the affected keys on failure and allow retries. Return structured errors from wallet unlock and passphrase changes so callers can share messages and report the specific failure. Fresh descriptor setup after the encryption transaction remains unchanged.

DrahtBot added the label Wallet on Jul 20, 2026

DrahtBot commented at 2:02 AM on July 20, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35752.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	w0xlt, achow101
Stale ACK	jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36031 (wallet: Remove mapMasterKeys and enforce that only one encryption key can exist by achow101)
#34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)

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.

in src/wallet/scriptpubkeyman.cpp:968 in b388cb6b7a outdated

 964 | @@ -965,7 +965,9 @@ bool DescriptorScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, Walle
 965 |              return false;
 966 |          }
 967 |          m_map_crypted_keys[pubkey.GetID()] = make_pair(pubkey, crypted_secret);
 968 | -        batch->WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret);
 969 | +        if (!batch->WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret)) {

vicjuma commented at 2:56 PM on July 21, 2026:

I did not observe any difference in the output before and after the change. It appears that the called function already handles this failure case, IMHO. Maybe I missed something

l0rinc commented at 8:49 PM on August 7, 2026:

Your test overrides WriteMasterKey(), so it never exercises the unchecked WriteCryptedDescriptorKey() failure fixed here.

in src/wallet/wallet.cpp:662 in b388cb6b7a outdated

 661 | +                bool written{WalletBatch(GetDatabase()).WriteMasterKey(master_key_id, new_master_key)};
 662 | +                if (written) master_key = std::move(new_master_key);
 663 |                  if (fWasLocked)
 664 |                      Lock();
 665 | -                return true;
 666 | +                return written;

vicjuma commented at 2:56 PM on July 21, 2026:

Intro

Works great. The error message is generalized, but I guess that is beyond this PR. Steps I used to reproduce the error. This mimics a database write failure

Testing

src/wallet/walletdb.cpp:151

// this function is used in both before and after the change. See the output below
bool WalletBatch::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey)
{
    // return WriteIC(std::make_pair(DBKeys::MASTER_KEY, nID), kMasterKey, true);
    return false;
}

Before this Change Fails silently with no error reporting and an unexpected log message

ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli help | grep encrypt
encryptwallet "passphrase"
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli createwallet "testencrypt"
{
  "name": "testencrypt"
}
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -rpcwallet=testencrypt encryptwallet "passphrase"
wallet encrypted; The keypool has been flushed and a new HD seed was generated. You need to make a new backup with the backupwallet RPC.
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$

Even with the WriteMasterKey always returning false.

After this Change Error is being reported successfully after the fail

ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoind
Bitcoin Core starting
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli createwallet "testencrypt-pr35752"
{
  "name": "testencrypt-pr35752"
}
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -rpcwallet=testencrypt-pr35752 encryptwallet "passphrase"
error code: -16
error message:
Error: Failed to encrypt the wallet.
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$

Conclusion

Reviewed wallet encryption write-failure handling; tested RPC failure paths; behavior matches expectations.

in src/wallet/wallet.cpp:865 in b388cb6b7a

 862 |              delete encrypted_batch;
 863 |              encrypted_batch = nullptr;
 864 |              return false;
 865 |          }
 866 | -        encrypted_batch->WriteMasterKey(nMasterKeyMaxID, master_key);
 867 | +        if (!encrypted_batch->WriteMasterKey(nMasterKeyMaxID + 1, master_key)) {

vicjuma commented at 2:56 PM on July 21, 2026:

Intro

Did the same to the src/wallet/walletdb.cpp:151 as above, but only after a successful wallet encryption.

Testing

For successful encryption

bool WalletBatch::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey)
{
    return WriteIC(std::make_pair(DBKeys::MASTER_KEY, nID), kMasterKey, true);
}

For testing silent failure

// this function is used in both before and after the change. See the output below
bool WalletBatch::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey)
{
    // return WriteIC(std::make_pair(DBKeys::MASTER_KEY, nID), kMasterKey, true);
    return false;
}

In this case, however, I am receiving an error, but it is somehow ambiguous Before the Change

ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoind
Bitcoin Core starting
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli loadwallet testpassphrase
{
  "name": "testpassphrase"
}
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -rpcwallet=testpassphrase encryptwallet "passphrase"
error code: -15
error message:
Error: running with an encrypted wallet, but encryptwallet was called.
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -rpcwallet=testpassphrase walletpassphrase "passphrase" 600
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -rpcwallet=testpassphrase walletpassphrasechange "passphrase" "newpassphrase"
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$

N/B: The last 2 commands were done with the write function that always returns false.

After the Change

ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli loadwallet testpassphrase-pr35752
{
  "name": "testpassphrase-pr35752"
}
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -rpcwallet=testpassphrase-pr35752 encryptwallet "passphrase"
error code: -15
error message:
Error: running with an encrypted wallet, but encryptwallet was called.
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -rpcwallet=testpassphrase-pr35752 walletpassphrase "passphrase" 600
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -rpcwallet=testpassphrase-pr35752 walletpassphrasechange "passphrase" "newpassphrase"
error code: -14
error message:
Error: The wallet passphrase entered was incorrect.
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$

The operation fails as expected. The reported error does not distinguish a database write failure from other failure modes (e.g. an incorrect passphrase), though this appears to be outside the scope of the changes in this PR.

l0rinc force-pushed on Jul 22, 2026

l0rinc marked this as a draft on Jul 23, 2026

l0rinc commented at 12:01 AM on July 23, 2026: contributor

Updated the failure handling and tests after review. Descriptor encryption now stages in-memory key updates until the database transaction commits, propagates encrypted-key write and plaintext-key erase failures, and returns false instead of aborting so encryption can be retried. The fault-injection tests exercise these failures through the public wallet interface. Turning to draft to get more conceptual review.

l0rinc renamed this:
~~wallet: handle encryption database write failures~~
RFC wallet: handle encryption database write failures
on Jul 23, 2026

l0rinc force-pushed on Jul 24, 2026

l0rinc marked this as ready for review on Jul 24, 2026

l0rinc renamed this:
~~RFC wallet: handle encryption database write failures~~
wallet: make encryption state updates atomic
on Jul 24, 2026

achow101 commented at 11:59 PM on August 7, 2026: member

Concept ACK

w0xlt commented at 5:56 AM on August 11, 2026: contributor

Concept ACK

I found the last commit particularly difficult to review because it changes WriteCryptedDescriptorKey() from ignoring the result of EraseIC(), as master does, to propagating it.

I'm not sure it's handling failure propagation safely when AddDescriptorKeyWithDB() adds a private key to an already encrypted wallet: the encrypted key may already have been committed, leaving a partial state while still reporting failure.

Something like the suggestion below may avoid this case, though there may be a better approach.

The failure case is very narrow, but it may still be worth addressing.

<details> <summary>suggestion</summary>

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index dea90f70eb..b509327f13 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -966,7 +966,7 @@ bool DescriptorScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, Walle
         if (!EncryptSecret(master_key, secret, pubkey.GetHash(), crypted_secret)) {
             return false;
         }
-        if (!batch->WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret)) {
+        if (!batch->WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret, /*erase_plaintext_key=*/true)) {
             return false;
         }
         crypted_keys[pubkey.GetID()] = make_pair(pubkey, std::move(crypted_secret));
@@ -1185,7 +1185,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const
         }
 
         m_map_crypted_keys[pubkey.GetID()] = make_pair(pubkey, crypted_secret);
-        return batch.WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret);
+        return batch.WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret, /*erase_plaintext_key=*/false);
     } else {
         m_map_keys[pubkey.GetID()] = key;
         return batch.WriteDescriptorKey(GetID(), pubkey, key.GetPrivKey());
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 9f9c4e634e..06f49fd5e1 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -156,9 +156,20 @@ struct EncryptionFailureSetup : WalletTestingSetup {
     {
         context.args = &m_args;
         context.chain = m_node.chain.get();
+        CreateWallet(WALLET_FLAG_DESCRIPTORS);
+    }
+
+    void CreateWallet(uint64_t create_flags)
+    {
         auto database{std::make_unique<FaultInjectingDatabase>()};
         fail_db = database.get();
-        wallet = TestCreateWallet(std::move(database), context, WALLET_FLAG_DESCRIPTORS);
+        wallet = TestCreateWallet(std::move(database), context, create_flags);
+    }
+
+    void RecreateWallet(uint64_t create_flags)
+    {
+        TestUnloadWallet(std::move(wallet));
+        CreateWallet(create_flags);
     }
 
     ~EncryptionFailureSetup() { TestUnloadWallet(std::move(wallet)); }
@@ -231,6 +242,27 @@ BOOST_FIXTURE_TEST_CASE(encrypt_wallet_descriptor_key_erase_failure, EncryptionF
     }
 }
 
+BOOST_FIXTURE_TEST_CASE(add_encrypted_descriptor_key_skips_plaintext_erase, EncryptionFailureSetup)
+{
+    BOOST_REQUIRE(wallet->EncryptWallet("passphrase"));
+    BOOST_REQUIRE(wallet->Unlock("passphrase"));
+
+    fail_db->FailNextErase(DBKeys::WALLETDESCRIPTORKEY);
+    AddKey(*wallet, GenerateRandomKey());
+}
+
+BOOST_FIXTURE_TEST_CASE(encrypt_wallet_fresh_descriptor_keys_skip_plaintext_erase, EncryptionFailureSetup)
+{
+    // Match the pre-encryption state of a non-blank wallet created with a passphrase: no plaintext keys or descriptors.
+    RecreateWallet(WALLET_FLAG_DESCRIPTORS | WALLET_FLAG_BLANK_WALLET);
+    wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
+
+    fail_db->FailNextErase(DBKeys::WALLETDESCRIPTORKEY);
+    BOOST_REQUIRE(wallet->EncryptWallet("passphrase"));
+    BOOST_CHECK(wallet->HasEncryptionKeys());
+    BOOST_CHECK(wallet->IsLocked());
+}
+
 BOOST_FIXTURE_TEST_CASE(update_non_range_descriptor, TestingSetup)
 {
     CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase());
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 0916dcaa47..cc8ce80399 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -222,12 +222,13 @@ bool WalletBatch::WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubk
     return WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)), std::make_pair(privkey, keypair_hash), false);
 }
 
-bool WalletBatch::WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const std::vector<unsigned char>& secret)
+bool WalletBatch::WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey,
+                                            const std::vector<unsigned char>& secret, bool erase_plaintext_key)
 {
     if (!WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORCKEY, std::make_pair(desc_id, pubkey)), secret, false)) {
         return false;
     }
-    return EraseIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)));
+    return !erase_plaintext_key || EraseIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)));
 }
 
 bool WalletBatch::WriteDescriptor(const uint256& desc_id, const WalletDescriptor& descriptor)
diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h
index 8397fff9cc..c75261557d 100644
--- a/src/wallet/walletdb.h
+++ b/src/wallet/walletdb.h
@@ -249,7 +249,9 @@ public:
     bool WriteOrderPosNext(int64_t nOrderPosNext);
 
     bool WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const CPrivKey& privkey);
-    bool WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const std::vector<unsigned char>& secret);
+    //! Erase the plaintext key only when converting an existing key, not when adding a new encrypted key.
+    bool WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey,
+                                   const std::vector<unsigned char>& secret, bool erase_plaintext_key);
     bool WriteDescriptor(const uint256& desc_id, const WalletDescriptor& descriptor);
     bool WriteDescriptorDerivedCache(const CExtPubKey& xpub, const uint256& desc_id, uint32_t key_exp_index, uint32_t der_index);
     bool WriteDescriptorParentCache(const CExtPubKey& xpub, const uint256& desc_id, uint32_t key_exp_index);

</details>

DrahtBot added the label Needs rebase on Aug 14, 2026

in src/wallet/test/wallet_tests.cpp:156 in f15242d3c5

 151 | +    AddKey(*wallet, GenerateRandomKey());
 152 | +
 153 | +    fail_db->FailNextWrite(DBKeys::MASTER_KEY); // The injected failure affects only the first attempt
 154 | +    for (bool success : {true, false}) { // TODO: The write failure is ignored, making the retry fail
 155 | +        BOOST_CHECK_EQUAL(wallet->EncryptWallet("passphrase"), success);
 156 | +        BOOST_CHECK_EQUAL(wallet->HasEncryptionKeys(), true); // TODO: The failed attempt publishes encryption state

achow101 commented at 8:28 PM on August 17, 2026:

In f15242d3c51841a8168d365db670e0797d2f64fc "test: characterize encryption transaction failures"

In addition to checking the wallet state, this should check if any of the records are present in the database.

l0rinc commented at 12:31 AM on August 18, 2026:

Done, thanks

in src/wallet/test/wallet_tests.cpp:178 in 95df28ee22

 173 | +{
 174 | +    AddKey(*wallet, GenerateRandomKey());
 175 | +    BOOST_REQUIRE(wallet->EncryptWallet("old_pass"));
 176 | +
 177 | +    fail_db->FailNextWrite(DBKeys::MASTER_KEY); // The injected failure affects only the first attempt
 178 | +    BOOST_CHECK( wallet->ChangeWalletPassphrase("old_pass", "new_pass")); // TODO: The failed write is reported as success

achow101 commented at 8:32 PM on August 17, 2026:

In 95df28ee22e758a993d328ebe83fdcc6f0e30a19 "test: characterize passphrase write failure"

This should check whether the db record changes.

l0rinc commented at 12:31 AM on August 18, 2026:

Done, thanks

l0rinc marked this as a draft on Aug 17, 2026

l0rinc force-pushed on Aug 17, 2026

l0rinc commented at 9:57 PM on August 17, 2026: contributor

Moved over to #35998, thanks @achow101.

l0rinc closed this on Aug 17, 2026

achow101 commented at 10:09 PM on August 17, 2026: member

Moved over to #35998, thanks @achow101.

bruh

I didn't open that pr in order to take this one over.

l0rinc reopened this on Aug 17, 2026

DrahtBot added the label CI failed on Aug 17, 2026

DrahtBot commented at 10:18 PM on August 17, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/32073588819/job/95521863865 LLM reason (✨ experimental): clang-tidy failed (warnings-as-errors) due to bugprone-unused-return-value in /src/wallet/test/wallet_tests.cpp (BOOST_CHECK_THROW/add_key() return value ignored).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label Needs rebase on Aug 17, 2026

in src/wallet/test/util.h:145 in 7f47a4d140

 140 | +    {
 141 | +    public:
 142 | +        explicit Batch(FaultInjectingDatabase& database) : SQLiteBatch(database), m_owner{database}
 143 | +        {
 144 | +            if (std::exchange(database.m_fail_commit, false)) {
 145 | +                SetExecHandler(std::make_unique<DbExecBlocker>("COMMIT TRANSACTION"));

achow101 commented at 11:18 PM on August 17, 2026:

In 7f47a4d140dc735b9ab04b3138c2c74a0cee25a2 "refactor/test: add wallet failure injection"

Why the exec blocker rather than overriding TxnCommit()?

l0rinc commented at 2:08 AM on August 18, 2026:

That's a lot simpler, thank you! FaultInjectingDatabase::Batch now overrides TxnCommit() directly and consumes the one-shot failure there.

in src/wallet/walletdb.cpp:235 in dfe96ed3c0 outdated

 228 | @@ -229,11 +229,12 @@ bool WalletBatch::WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubk
 229 |  
 230 |  bool WalletBatch::WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const std::vector<unsigned char>& secret)
 231 |  {
 232 | -    if (!WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORCKEY, std::make_pair(desc_id, pubkey)), secret, false)) {
 233 | -        return false;
 234 | -    }
 235 | -    EraseIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)));

achow101 commented at 11:27 PM on August 17, 2026:

In dfe96ed3c0c48ac886aa61ffee267fa611441118 "wallet: abort failed descriptor key erases"

I don't think splitting this up is the right way to go about fixing errors here. The reason that EraseIC is included in the Write is to avoid programmer errors where the plaintext is accidentally not erased when writing the encrypted key.

Instead of splitting, this could first do m_batch->HasKey(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)) and then only do the erase if the database had the record in the first place. Then the error from the erase can be propagated if it fails.

l0rinc commented at 2:07 AM on August 18, 2026:

Done. WriteCryptedDescriptorKey() now keeps both operations together - let me know if this is what you meant.

achow101 commented at 11:52 PM on August 17, 2026: member

I don't like the last commit, I don't think all of that refactoring and additional code in areas that shouldn't need to care about underlying encryption is worth the benefit that it gives.

Ultimately, what is the point of the last commit? It seems like it's trying to make the entirety of EncryptWallet atomic, so if anything fails in setting up the new descriptors, the wallet is reverted to be unencrypted and still usable to the user? I'm not sure that having all of this code to handle an error here is meaningfully useful, given that it is a corruption scenario; the normal paths become unnecessarily complicated.

In many corruption scenarios, I think it is ok/better for us to unload the wallet and tell the user corruption occurred and that they should restore a backup. We do not need to try so hard to recover from corruption scenarios; restoring previous state and staying operational is not a necessity.

The easiest way to ensure in-memory state matches disk is also to just unload and reload, rather than doing a bunch of caching/staging or whatever.

l0rinc force-pushed on Aug 18, 2026

DrahtBot removed the label CI failed on Aug 18, 2026

l0rinc force-pushed on Aug 18, 2026

DrahtBot added the label CI failed on Aug 18, 2026

DrahtBot commented at 2:56 AM on August 18, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/32092273017/job/95576690175 LLM reason (✨ experimental): CI failed because clang-tidy reported an error (bugprone-argument-comment) where the overwrite comment name didn’t match the fOverwrite parameter name in walletdb.cpp.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

l0rinc force-pushed on Aug 18, 2026

l0rinc commented at 4:13 AM on August 18, 2026: contributor

It took me a few rounds to get to a version that is simple enough, and the previous pushes contained needless refactors. Thanks for the reviews, undrafting, ready for review again!

l0rinc marked this as ready for review on Aug 18, 2026

DrahtBot removed the label CI failed on Aug 18, 2026

l0rinc force-pushed on Aug 18, 2026

l0rinc commented at 10:21 PM on August 18, 2026: contributor

AI review revealed that passphrase errors could be confused with master-key write failures, so I extended the error messages and tests to cover both cases. Also added database checks for master and descriptor key records that were missed agyer previous review.

in src/wallet/rpc/encrypt.cpp:162 in 8ff0c39fff

 158 | @@ -159,13 +159,13 @@ RPCMethod walletpassphrasechange()
 159 |      if (!pwallet->ChangeWalletPassphrase(strOldWalletPass, strNewWalletPass)) {
 160 |          // Check if the old passphrase had a null character (see #27067 for details)
 161 |          if (strOldWalletPass.find('\0') == std::string::npos) {
 162 | -            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect.");
 163 | +            throw JSONRPCError(RPC_WALLET_ERROR, "Error: The wallet passphrase entered was incorrect or the new master key could not be written to the wallet database.");

achow101 commented at 9:09 PM on August 19, 2026:

In 8ff0c39fff7d9a2add6c2c6c3154da8a52e51476 "wallet: reject failed passphrase changes"

I don't think we should be changing these errors. This is a new error that should get it's own message.

l0rinc commented at 1:10 AM on August 20, 2026:

That's what I did at first, but got scared of the huge diff and went this way instead. Your suggestion makes it a bit more complicated, but it makes sense, I've cherry-picked your related change and split it up to ease review - folding back some of the changes to previous commits to avoid churn.

in src/wallet/wallet.cpp:664 in 8ff0c39fff

 663 | +                    master_key = std::move(new_master_key);
 664 | +                } else {
 665 | +                    WalletLogPrintf("Writing the new master key to the wallet database failed\n");
 666 | +                }
 667 |                  if (fWasLocked)
 668 |                      Lock();

achow101 commented at 10:17 PM on August 19, 2026:

In 8ff0c39 "wallet: reject failed passphrase changes"

Behavior change, but this should really happen right after the Unlock at the top of this if. Otherwise, if we are unable to encrypt the master key with the new passphrase for whatever reason, the wallet will be locked if it was locked.

l0rinc commented at 2:35 AM on August 20, 2026:

Did it in a separate commit under your name and updated the PR description - thanks for the hint!

achow101 commented at 11:15 PM on August 19, 2026: member

https://github.com/achow101/bitcoin/commit/99963d18c99da443fcfcd768faaca848237e2c91 for my suggestions

l0rinc force-pushed on Aug 20, 2026

DrahtBot added the label CI failed on Aug 20, 2026

DrahtBot commented at 3:07 AM on August 20, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/32326323514/job/96298202338 LLM reason (✨ experimental): CI failed due to a lint violation: a fatal assert(false) was used in RPC code (src/wallet/rpc/encrypt.cpp), triggering the rpc_assert check.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Aug 20, 2026

achow101 commented at 7:58 PM on August 20, 2026: member

ACK 50f07b23282336629352772e72847364e61d0c94

jeanpablojp commented at 2:46 PM on August 23, 2026: contributor

tACK 50f07b23282336629352772e72847364e61d0c94

Built and ran the tests. Left two comments inline.

in src/wallet/test/wallet_tests.cpp:184 in 50f07b2328

 183 | +    BOOST_REQUIRE(wallet->EncryptWallet("old_pass"));
 184 | +    const auto master_key_record{fail_db->GetRecordValue(DBKeys::MASTER_KEY)};
 185 | +    BOOST_REQUIRE(master_key_record);
 186 | +
 187 | +    fail_db->FailNextWrite(DBKeys::MASTER_KEY); // The injected failure affects only the first attempt
 188 | +    BOOST_CHECK(!wallet->ChangeWalletPassphrase("old_pass", "new_pass"));

jeanpablojp commented at 2:46 PM on August 23, 2026:

No test covers the if (fWasLocked) Lock(); in ChangeWalletPassphrase. Dropped it and the unit suite still passed, wallet_encryption.py too. A BOOST_CHECK(wallet->IsLocked()); here catches it.

in src/wallet/walletdb.cpp:237 in 50f07b2328

 237 | +    const auto descriptor_key{std::make_pair(desc_id, pubkey)};
 238 | +    const auto plaintext_key{std::make_pair(DBKeys::WALLETDESCRIPTORKEY, descriptor_key)};
 239 | +    const auto encrypted_key{std::make_pair(DBKeys::WALLETDESCRIPTORCKEY, descriptor_key)};
 240 | +
 241 | +    if (!WriteIC(encrypted_key, secret, /*fOverwrite=*/false)) return false;
 242 | +    return !m_batch->Exists(plaintext_key) || EraseIC(plaintext_key);

jeanpablojp commented at 2:46 PM on August 23, 2026:

I think this is the same shape as the erase failure the commit already handles, the write lands and the next operation in the same batch doesn't. If Exists hits a read error it's indistinguishable from "not found", since HasKey is just res == SQLITE_ROW, and the erase gets skipped silently. Forcing that on regtest, encryptwallet succeeded and the next loadwallet failed with "Wallet corrupted". Without the guard the failure goes the other way, it fails loudly and doesn't leave a broken wallet behind. And the case it protects seems to take care of itself already, a DELETE that matches nothing returns SQLITE_DONE. I'd swap it for return EraseIC(plaintext_key);, then add_encrypted_descriptor_key_without_plaintext_record can drop the FailNextErase.

l0rinc commented at 6:28 PM on August 26, 2026:

Thanks for the review, will let the maintainers decide if we should do them here or in a follow-up.

w0xlt commented at 6:47 PM on August 25, 2026: contributor

ACK 50f07b23282336629352772e72847364e61d0c94

sedited commented at 12:01 PM on September 14, 2026: contributor

Ping for rebase.

DrahtBot added the label Needs rebase on Sep 14, 2026

l0rinc force-pushed on Sep 15, 2026

DrahtBot removed the label Needs rebase on Sep 15, 2026

l0rinc commented at 7:18 AM on September 15, 2026: contributor

Rebased to resolve the conflicts. Thanks @jeanpablojp for the lock-state checks and for catching the read-error ambiguity in Exists(), and @w0xlt for highlighting the encrypted-key insertion case.

Encrypted-key writes now always attempt the plaintext erase after a successful write and use a transaction when the caller has none, so erase or commit failures leave insertion retryable. I also expanded the insertion failure characterization, added checks for distinct passphrase and database errors, and clarified the test helpers, TODOs, and commit messages.

DrahtBot added the label Needs rebase on Sep 17, 2026

refactor/test: add wallet failure injection

Add a reusable SQLite-backed wallet database that can reject selected record writes, erases, or transaction commits and inspect stored records.

Co-authored-by: Ava Chow <github@achow101.com>

70381010c0

test: characterize encryption transaction failures

Wallet encryption currently reports success after a failed master-key write.
A failed transaction commit aborts after publishing master and descriptor encryption state, which prevents retry in the same process.

Record both outcomes before returning transaction failures as errors and publishing live state only after commit.

Co-authored-by: Ava Chow <github@achow101.com>

e6c374d302

wallet: abort failed encryption transactions

Wallet encryption publishes master and descriptor key state before the database transaction commits.
It also ignores a failed master-key write and aborts the process after a failed commit.

Use `RunWithinTxn()` for the master key and existing descriptor keys.
Stage descriptor keys in a commit listener and publish the master key after commit, so failed master-key writes and commits leave live state unchanged and encryption can be retried.

8b23fa4773

wallet: restore lock state before re-encryption

`ChangeWalletPassphrase()` temporarily unlocks a locked wallet after validating the old passphrase.
If `EncryptMasterKey()` then fails, the wallet returns without restoring its locked state.

Restore the original lock state immediately after validating the old passphrase, before re-encrypting or writing the master key.

2daa2f94f4

test: characterize passphrase write failure

Wallet passphrase changes currently report success after the master-key write fails.
The new passphrase works only in memory while the old passphrase remains on disk.

Record that behavior before encrypting a copy of the master key and publishing it only after the database write succeeds.

Co-authored-by: Ava Chow <github@achow101.com>

ee6d45a07e

wallet: reject failed passphrase changes

`ChangeWalletPassphrase()` updates the in-memory master key before writing it to the database.
If `WriteMasterKey()` fails, the new passphrase works only in memory while the old passphrase remains on disk.

Encrypt a copy of the master key, persist it, and replace the live value only after the write succeeds.
A failed write now leaves the old passphrase active and allows the change to be retried.

5bc0be16fe

test: characterize descriptor key failures

Descriptor encryption currently reports success after an encrypted-key record write or plaintext-key record erase fails.
Both failures publish wallet and descriptor encryption state and prevent retry.
An ignored erase also commits both records.

Record the existing write and erase failure behavior before propagating either error.

14ce3fdb49

wallet: abort failed descriptor key writes

`DescriptorScriptPubKeyMan::Encrypt()` ignores failed encrypted-key writes, allowing the transaction to publish keys whose records were not persisted.

Check each write before staging its encrypted value.
`RunWithinTxn()` can then abort without publishing memory, leaving encryption retryable.

d8a6f5e48d

test: cover encrypted descriptor key insertion

The descriptor-key write helper also inserts encrypted keys when no plaintext record exists.
Pin this path before propagating replacement erase failures, so insertion remains successful when there is no plaintext record to erase.

537e791915

wallet: abort failed descriptor key erases

`WriteCryptedDescriptorKey()` replaces a plaintext key during wallet encryption.
The same helper can insert an encrypted key when no plaintext record exists.

After writing the encrypted key, erase its plaintext record and propagate any erase failure.
SQLite accepts an erase when no record exists, so no existence check is needed that could mistake a read error for a missing key.
Start a transaction when the caller has not supplied one, keeping insertion writes and erases atomic as well.
Keeping both operations in the helper prevents replacement callers from omitting the erase while allowing new encrypted-key insertion.

Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
Co-authored-by: Ava Chow <github@achow101.com>

bd55810778

test: characterize descriptor insertion failure

Descriptor key insertion currently updates the live key map before writing the key record.
If the write, erase, or commit fails, a retry sees the live-only key and skips persistence while reporting success.

Record the plaintext and encrypted cases before making live-map publication conditional on successful database writes.

e3712b7d49

wallet: publish descriptor keys after writes

`AddDescriptorKeyWithDB()` publishes plaintext and encrypted keys before writing their database records.
A failed write leaves a live-only key, and the duplicate check prevents a retry from persisting it.

Write each key record first and update the corresponding live key map only after the write succeeds.
A failed insertion then leaves no key behind and can be retried.

f949b3ba2a

wallet: return passphrase errors with Expected

Change wallet unlock and passphrase methods from `bool` to `util::Expected<void, WalletError>` so they can return specific error codes and messages.
Add `WalletErrorCode::PassphraseIncorrect` for a passphrase that does not decrypt the wallet, since `UnlockNeeded` describes a locked wallet that callers can unlock and retry.
Update the wallet interfaces while callers continue to use the success state.

593554622e

l0rinc force-pushed on Sep 17, 2026

wallet: reuse unlock and passphrase change errors

RPC and GUI callers still report database and encryption failures as incorrect passphrases.
Use the structured wallet errors in RPC, GUI, and migration callers to report the specific failure and share passphrase error messages.
Map `WalletErrorCode::PassphraseIncorrect` to `RPC_WALLET_PASSPHRASE_INCORRECT` in `HandleWalletErrorCode()` so the passphrase RPCs reuse the shared error mapping.

6d2414542b

l0rinc force-pushed on Sep 17, 2026

DrahtBot added the label CI failed on Sep 17, 2026

l0rinc commented at 10:54 PM on September 17, 2026: contributor

Rebased after #34861 and changed wallet error handling to distinguish incorrect passphrases from locked-wallet errors, while keeping RPC responses unchanged.

w0xlt commented at 11:53 PM on September 17, 2026: contributor

reACK 6d2414542b86ff7d6eae0f4c873e47bc4e57a57c

DrahtBot requested review from jeanpablojp on Sep 17, 2026

DrahtBot requested review from achow101 on Sep 17, 2026

DrahtBot removed the label CI failed on Sep 17, 2026

DrahtBot removed the label Needs rebase on Sep 18, 2026

achow101 commented at 6:38 PM on September 22, 2026: member

ACK 6d2414542b86ff7d6eae0f4c873e47bc4e57a57c

achow101 merged this on Sep 22, 2026

achow101 closed this on Sep 22, 2026

l0rinc deleted the branch on Sep 22, 2026