wallet: fail dump on incomplete writes #35492

pull OSINTv96 wants to merge 1 commits into bitcoin:master from OSINTv96:codex/wallet-dump-write-failure changing 2 files +65 −38

OSINTv96 commented at 6:32 AM on June 9, 2026: none


This makes bitcoin-wallet dump fail if writing the dump file does not complete successfully.

Previously, DumpWallet() only checked whether the dump file could be opened. If later writes or close failed, the tool could still return success and print the private-key warning, leaving a truncated dump file behind.

The change adds write/close checks while dumping wallet records. If writing fails, the partial dump file is removed and bitcoin-wallet returns an error.

The functional test now covers a forced incomplete dump on POSIX systems by limiting the dump process file size. It checks that:
- stdout stays empty;
- the command exits with failure;
- the write error is reported;
- the partial dump file is removed.
Tested with:
```
cmake -S /tmp/bitcoin-core-wallet-dump-pr -B /tmp/bitcoin-core-wallet-dump-pr-build -DBUILD_TESTS=OFF -DBUILD_BENCH=OFF -DBUILD_GUI=OFF -DENABLE_WALLET=ON -DBUILD_WALLET_TOOL=ON -DENABLE_IPC=OFF -DWITH_ZMQ=OFF -DCMAKE_BUILD_TYPE=RelWithDebInfo
cmake --build /tmp/bitcoin-core-wallet-dump-pr-build --target bitcoind bitcoin-wallet -j"$(sysctl -n hw.ncpu)"
python3 test/functional/tool_wallet.py --configfile=/tmp/bitcoin-core-wallet-dump-pr-build/test/config.ini
```
DrahtBot added the label Wallet on Jun 9, 2026
DrahtBot commented at 6:32 AM on June 9, 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/35492.



Reviews

See the guideline for information on the review process.

Type Reviewers

Approach ACK pablomartin4btc

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



Conflicts

No conflicts as of last run.

Type	Reviewers
Approach ACK	pablomartin4btc

in test/functional/tool_wallet.py:279 in 6787fdc4e0 outdated

 274 | +
 275 | +            def limit_dump_size():
 276 | +                import resource
 277 | +                import signal
 278 | +                signal.signal(signal.SIGXFSZ, signal.SIG_IGN)
 279 | +                resource.setrlimit(resource.RLIMIT_FSIZE, (512, 512))

maflcko commented at 7:01 AM on June 9, 2026:

TIL. Maybe we can use this in other tests 👀

in src/wallet/dump.cpp:122 in 6787fdc4e0
```
 118 | +        if (dump_file.fail()) set_write_error();
 119 |      } else {
 120 |          // Remove the dumpfile on failure
 121 |          dump_file.close();
 122 | +    }
 123 | +    if (!ret) {
```
maflcko commented at 7:13 AM on June 9, 2026:

nit: I find this a bit hard to read if (ret) ... else ... if !ret .... I wonder if the code was a bit cleaner if the pattern was a try-catch inside this function where each error was a throw?

OSINTv96 commented at 7:27 AM on June 9, 2026:

Thanks, agreed. I simplified the control flow here so the success/failure paths are easier to read.
OSINTv96 force-pushed on Jun 9, 2026
pablomartin4btc commented at 7:49 PM on June 9, 2026: member

Approach ACK.

This PR seems like an improvement, fixing file write/ close failure handling and adding coverage for a file write failure scenario that wasn't previously tested. I think RLIMIT_FSIZE is a nice way to exercise a real mid-write failure here. Alternatives like /dev/full, permission changes, or mocked writers would either test a less realistic path or require more plumbing.
wallet: fail dump on incomplete writes bb2a207d08
OSINTv96 force-pushed on Jun 10, 2026

Contributors

Labels

wallet: fail dump on incomplete writes #35492

Code Coverage & Benchmarks

Reviews

Conflicts