wallet: fail dump on incomplete writes #35492

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 and AI policy for information on the review process.

Type	Reviewers
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.

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

sedited commented at 9:14 AM on July 24, 2026: contributor

I think this patch would be better if it used AutoFile instead of ofstream, and called FileCommit and DirectoryCommit on success.

wallet: commit dump output before success

Use AutoFile for dump writes, require Commit() and fclose() to succeed, and sync the parent directory before reporting a successful dump.

385a001f66

OSINTv96 commented at 10:20 AM on July 24, 2026: none

Thanks, that makes sense; the patch now uses AutoFile with checked Commit() and fclose(), followed by DirectoryCommit() on the parent directory.

sedited requested review from pablomartin4btc on Sep 8, 2026

in src/wallet/dump.cpp:63 in 385a001f66

  64 | +        throw_error(strprintf(_("Unable to write complete dump file %s."), fs::PathToString(path)));
  65 | +    };
  66 | +    auto write = [&](const std::string& line) {
  67 | +        try {
  68 | +            dump_file.write(MakeByteSpan(line));
  69 | +        } catch (const std::ios_base::failure&) {

pablomartin4btc commented at 4:06 AM on September 9, 2026:

Aside, not for this PR: AutoFile's I/O-failing throw sites (read/ignore/seek/write/write_buffer in streams.cpp) and BufferedFile::Fill() all throw fixed strings with no errno detail, so callers lose the actual OS error reason on failure. dump.cpp's own catch here also discards it in favor of a generic message. I'll follow up with a small PR that folds SysErrorString(errno) into those five throw sites and wires dump.cpp to surface the detail — a couple of existing catchers (e.g. rpc/blockchain.cpp's loadtxoutset) already forward e.what() to their callers, so they'd pick up the improvement for free.

pablomartin4btc commented at 4:07 AM on September 9, 2026: member

ACK 385a001f66ad23b3963d5c0acf960b94359c4e21

Switching to AutoFile is a meaningfully stronger guarantee than what the write/close-failbit checks alone gave us — std::ofstream::close() only flushes into the OS page cache, it never calls fsync . So even the version I'd already approach-ACK'd could report a "successful" dump that wasn't actually durable on disk if the machine crashed right after.

AutoFile::Commit() (→ FileCommit() — cross-platform fsync/FlushFileBuffers helper — actually forces the file's data to disk) plus DirectoryCommit() on the parent directory close that gap — the latter matters because a crash can lose the new file's directory entry even when the file's own data already hit disk (POSIX requires syncing the directory separately from the file).