wallet: make encryption state updates atomic #35752

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

Problem: Encrypting a wallet updates database records and live key state, but failures can leave the two out of sync. Wallet encryption can still return success after a master-key write fails, and a passphrase change can update only memory. Descriptor-key write failures can publish keys that were not persisted, while erase failures can commit both plaintext and encrypted records. A failed transaction commit instead aborts the node. These paths require a local database failure and are not remotely triggerable.

Fix: Make the covered encryption updates atomic across the database and memory. Check every required master-key write, encrypted descriptor-key write, plaintext descriptor-key erase, and transaction commit before publishing live state. Passphrase changes encrypt a copy of the master key and replace the live master key only after the database write succeeds. The covered failures leave database and memory unchanged so encryption and passphrase changes can be retried. Five fault-injection tests exercise these paths through the public wallet interface.

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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
#34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
#34400 (wallet: parallel fast rescan (approx 8x speed up with 8 threads) by Eunovo)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)

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

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

refactor/test: share SQLite execution blocker

Move the test-only SQLite statement blocker into the wallet test utilities so transaction failure tests can reuse one implementation.

Call the stateless base execution handler directly instead of storing a redundant handler instance.

a5cf8b08d3

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, leaving the same-process wallet unretryable.

Record both outcomes before making the transaction recoverable and publishing memory only after commit.

f15242d3c5

wallet: abort failed encryption transactions

Wallet encryption published master and descriptor key state before the database transaction completed.
A failed master key write was ignored, while a failed commit terminated after memory had changed.

Run the operation through `RunWithinTxn()` and publish the wallet master key only afterward.
Descriptor encryption now requires an active batch transaction because its staged in-memory update is owned by a commit listener.
Only a successful commit invokes that update, so abort and commit-failure paths leave descriptor memory unchanged and retryable.

8bd65dc8d1

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 making master-key encryption non-mutating and publishing the result only after persistence succeeds.

95df28ee22

wallet: reject failed passphrase changes

`ChangeWalletPassphrase()` updated the in-memory master key before writing it to the database.
If `WriteMasterKey()` failed, the new passphrase worked only in memory while the old passphrase remained on disk.

Encrypt into a local copy.
Update the in-memory master key only after the database write succeeds.

093deb7c86

test: characterize descriptor key write failure

Descriptor encryption currently reports success after an encrypted-key record write fails, publishing wallet and descriptor encryption state even though one encrypted record was not persisted.
The published state also prevents retry.

Record that behavior before propagating the write failure and staging descriptor memory.

7d1bd29c1c

wallet: abort failed descriptor key writes

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

Check each write before staging its encrypted value, so `RunWithinTxn()` aborts without publishing memory.
`WriteCryptedDescriptorKey()` returns before attempting the plaintext erase when the encrypted write fails, and the unchanged state permits retry.

d83522065a

test: characterize descriptor key erase failure

Descriptor encryption currently reports success after a plaintext-key erase fails.
The transaction commits both plaintext and encrypted records, publishes encrypted descriptor state, and leaves the operation unretryable.

Check both database record types and live descriptor state before propagating the erase failure.

c511f473dc

wallet: abort failed descriptor key erases

`WriteCryptedDescriptorKey()` ignored failures to erase the corresponding plaintext descriptor key.
Encryption could commit both record forms and publish encrypted descriptor state.

Return the erase result so `RunWithinTxn()` aborts.
The abort preserves the plaintext record, rolls back the encrypted record, and does not run the commit callback, leaving memory unchanged and retryable.

4b243f486e

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