#35492 wallet: fail dump on incomplete writes
https://github.com/bitcoin/bitcoin/pull/35492 · · +73/-43 in 2 files, 2 commits · labels: Wallet
Goal
- Prevent wallet dumps from reporting success if writing fails or disk space runs out
- Delete partial dump files on failure so users do not mistake them for valid backups
Updates `DumpWallet()` in `src/wallet/dump.cpp` to use `AutoFile`, check write operations for failure, flush changes to disk via `Commit()`, and synchronize the parent directory before reporting success. If an error occurs during writing or closing, the partial dump file is removed and an error is returned. A functional test using POSIX `RLIMIT_FSIZE` verifies failure handling.
Problem: Previously, `bitcoin-wallet dump` only verified that the output file could initially be opened. Any subsequent write or close failure still resulted in a success exit code, leaving behind an incomplete dump file that users could mistake for a valid backup.
Category: Wallet (#14 of 84)
P2 · bug fix
- P2 because silent dump failures could mislead users into trusting truncated backups
- Ensuring dump integrity prevents accidental key loss during wallet recovery
Fixes a silent failure condition during wallet dumps where writing errors or disk-full conditions still reported success and left truncated files. Wallet backup and recovery correctness is critical to prevent accidental key loss, making this a durable reliability fix for wallet tooling.
Membership: Modifies src/wallet/dump.cpp which implements wallet backup and export functionality.
Factors: security/stability 1, bug 2, performance 0, user value 2, leverage 1
Reviewability: Ready
- Ready for review: clean CI, no conflicts, and all reviewer feedback has been addressed
The code has clean CI, no merge conflicts, addressed all previous feedback, and has an ACK from pablomartin4btc.
Author status: active; addressed all reviewer requests in the latest push
Resolved concerns:
- maflcko suggested replacing nested conditional error checks with a local try-catch pattern, which the author implemented.
- sedited suggested using `AutoFile` instead of `std::ofstream` and adding `FileCommit`/`DirectoryCommit`, which the author incorporated.
- pablomartin4btc noted that AutoFile error messages discard `errno`, spun this out into follow-up PR #36210, and gave a full ACK to this PR's approach.
Agreement: Strong
- Strong support because durability guarantees prevent crash-related corruption (pablomartin4btc)
- Earlier suggestions on commit handling and error paths were fully incorporated (sedited, maflcko)
Strong: substantive ACK with durability rationale from pablomartin4btc; prior comments from maflcko and sedited addressed
pablomartin4btc tested and approved the PR, noting that AutoFile::Commit and DirectoryCommit provide necessary durability guarantees across crashes. Earlier reviewers saw their review comments incorporated.
- pablomartin4btc ACK 385a001: 'Switching to AutoFile is a meaningfully stronger guarantee...'
- sedited suggested AutoFile and DirectoryCommit; author implemented both
- maflcko commented positively on the RLIMIT_FSIZE test technique
Review verdicts (DrahtBot): 1
- ACK: pablomartin4btc
Dependencies
Enables:
- #36210 (follow-up to surface OS error detail on AutoFile failures)
Files
21 lines under test/bench/ci.
- src/wallet/dump.cpp +54/-41
- test/functional/tool_wallet.py +19/-2
Card
This PR modifies bitcoin-wallet dump to detect write failures, commit data to disk with fsync and directory sync, and remove incomplete dump files on error. Previously, write errors after opening the destination file were ignored, allowing truncated dumps to exit with success and potentially causing users to trust incomplete key backups. Reviewers praised the RLIMIT_FSIZE test coverage and the addition of durable filesystem syncs. The PR has a detailed ACK from pablomartin4btc and is ready for merge consideration.