#35492 wallet: fail dump on incomplete writes
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35492 wallet: fail dump on incomplete writes | Author silent 100d | 2 | Strong | 54 + 19 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Make bitcoin-wallet dump check for write and close failures instead of only checking if the output file can be opened. The dump output is switched to AutoFile, with write errors caught, file commits and closes verified, and the parent directory synced. If writing fails, any partial dump file is removed and the tool returns an error.
Problem: bitcoin-wallet dump could silently produce an incomplete dump file if the filesystem ran out of space or an I/O error occurred during dumping. Because it only verified that the output file could initially be opened, it would still exit with success and print the private key warning despite leaving behind a corrupted or truncated dump.
Evidence of importance:
- PR description: '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.'
- Diff in src/wallet/dump.cpp confirms std::ofstream writes were unchecked and DumpWallet only returned false if opening failed or database cursor operations failed.
Reviewability
Stale · Author silent 100d
No author activity for 100 days; a review may wait a long time for a response.
Waiting on (from the thread read):
- nothing: no pending requests or unanswered items
Agreement
Strong
substantive support, no open objection (pablomartin4btc, pablomartin4btc)
Thread notes: Added maflcko's signed ACK comment c:6344206840 and updated their stance to support. All claims remain resolved and nothing is currently pending.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| maflcko (member) | suggestion, style | simplify control flow using a try-catch pattern with throws for errors | fixed · fix pushed · author simplified the control flow | answered | yes | 2026-06-09: '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`?' Status from: 2026-06-09: OSINTv96: 'Thanks, agreed. I simplified the control flow here so the success/failure paths are easier to read.' |
| sedited (member) | suggestion, approach | use AutoFile instead of ofstream, and call FileCommit and DirectoryCommit on success | fixed · fix pushed · author updated the implementation to use AutoFile with Commit and DirectoryCommit | answered | yes | 2026-07-24: 'I think this patch would be better if it used AutoFile instead of ofstream, and called FileCommit and DirectoryCommit on success.' Status from: 2026-07-24: OSINTv96: 'the patch now uses `AutoFile` with checked `Commit()` and `fclose()`, followed by `DirectoryCommit()` on the parent dire' |
Support:
- pablomartin4btc (member) (Approach ACK, 2026-06-09) [read]: fixes file write/close failure handling and adds test coverage for a file write failure scenario using RLIMIT_FSIZE
- pablomartin4btc (member) (ACK, 2026-09-09) [read]: switching to AutoFile, FileCommit, and DirectoryCommit guarantees durability on disk and across crashes
- maflcko (member) (ACK, 2026-10-22): review with --function-context --ignore-all-space ACK 385a001f66ad23b3963d5c0acf960b94359c4e21 [not substantive]
Participants:
- maflcko (member, support, 3 statements 2026-06-09 to 2026-10-22): ACKed using diff viewing options; previously suggested simplifying control flow and commented on RLIMIT_FSIZE test technique
- pablomartin4btc (member, support, 4 statements 2026-06-09 to 2026-09-09): Approach ACK and ACK; supported durability improvements using AutoFile, FileCommit, and DirectoryCommit
- sedited (member, neutral, 1 statements 2026-07-24 to 2026-07-24): suggested using AutoFile, FileCommit, and DirectoryCommit
Files
21 lines under test/bench/ci.
- src/wallet/dump.cpp +54/-41
- test/functional/tool_wallet.py +19/-2
Card
The short text the ranking pass compares across PRs.
Makes bitcoin-wallet dump detect write and close failures when exporting wallet data. Previously, the tool only verified that the dump file could be opened, meaning incomplete writes from disk-full or I/O errors were ignored and the command exited with success while leaving a truncated dump file. The change switches to AutoFile, verifies write operations and file commits, deletes partial dump files on failure, and adds a functional test enforcing failure when file size limits are exceeded.
Data
thread read through 2026-10-22 (10 statements), head 385a001f66
labels: Wallet
Categories
Wallet
P2 · bug fix (ranking pass: Moved down slightly: silent truncated dumps matter, but less than the load/migration corruption fixes above it.)
Prevents silent backup corruption where `bitcoin-wallet dump` reported success despite incomplete or failed writes. Previously, `DumpWallet()` only checked if the output file could be opened, meaning subsequent I/O or disk space failures still exited with success and left behind a truncated dump file.
Membership: Matches the 'Wallet' label and modifies `src/wallet/dump.cpp`.
Factors: security/stability 1, bug 2, performance 0, user value 1, leverage 0