#35763 util: write JSON atomically in WriteJson
https://github.com/bitcoin/bitcoin/pull/35763 · · +73/-24 in 7 files, 1 commits · labels: Utils/log/libs
Goal
- Prevent corrupted or truncated node configuration and banlist files if the process is interrupted
- Consolidate safe atomic file replacement into shared settings write utilities
This PR moves atomic file replacement logic into common::WriteSettings by writing to a .tmp file and invoking RenameOver(). As a result, both settings.json and banlist.json are written atomically, preventing truncated files on write interruption, and ArgsManager no longer requires custom temp-path handling.
Problem: Previously, CBanDB::Write wrote banlist.json directly in place, risking an empty or corrupt banlist if the process was killed during serialization. Additionally, atomic write guarantees existed only in ArgsManager callers rather than in the shared file-writing utility.
Category: Utilities (logging, arguments, libraries) (#12 of 66)
P3 · crash safety
- P3 because it improves crash safety and persistence robustness for node configuration and banlist files
- Prevents file corruption or truncation if the node terminates during serialization
P3 because it improves crash safety and persistence robustness for the node's configuration and banlist. Making WriteSettings write to a temporary file before renaming prevents truncated files if the node crashes during writing, addressing a real file integrity hazard with clean internal consolidation.
Membership: Modifies shared settings file writing and path management in src/common/settings.* and src/common/args.*.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0
Reviewability: Ready
- Ready for review, with all reviewer questions and suggestions addressed in the latest update
All reviewer questions regarding error cleanup, test assertions, and API naming have been addressed in the latest push.
Author status: active; addressed review feedback with push on 2026-09-10
Resolved concerns:
- winterrdog and Herb-ops noted test verification deficiencies and suggested making atomic rename the default rather than exposing an unsafe direct-write API.
- winterrdog raised potential temporary file accumulation; author showed that the fixed .tmp path overwrites cleanly rather than proliferating, adding a unit test to verify.
- winterrdog raised an API asymmetry between ReadSettings and WriteJson; author agreed to revert the helper name to WriteSettings to keep symmetry with ReadSettings.
Agreement: Strong
- Broad agreement on making atomic replacement the default in shared write utilities
- Supported making atomic writes standard and verified test coverage (Herb-ops, winterrdog)
- Concerns regarding naming symmetry and temporary file accumulation were resolved (winterrdog)
Strong: winterrdog and Herb-ops support atomic writes in WriteSettings; naming concerns resolved
Reviewers agreed with encapsulating the atomic rename pattern inside WriteSettings so callers cannot bypass it. The author adopted suggested changes and resolved all open questions.
- Herb-ops ACKed 07ec5af with comments on test coverage and error log wording.
- winterrdog suggested Approach B (always write atomically inside helper) and gave approach ACK.
- winterrdog noted naming mismatch (WriteJson vs ReadSettings); author adopted Option A to retain WriteSettings.
Review verdicts (DrahtBot): 0 (+1)
- Stale ACK: Herb-ops
- Approach ACK: winterrdog
Files
46 lines under test/bench/ci.
- src/test/banman_tests.cpp +44/-0
- src/common/args.cpp +6/-10
- src/common/settings.cpp +10/-4
- src/addrdb.cpp +6/-7
- src/common/settings.h +5/-1
- src/common/args.h +1/-1
- src/test/argsman_tests.cpp +1/-1
Card
This PR moves atomic file creation into common::WriteSettings so that files like settings.json and banlist.json are written to a .tmp path and atomically renamed into place. This prevents partially written or corrupted JSON files if bitcoind crashes or is interrupted during a write. Reviewers supported making the safe atomic path the default, and recent pushes addressed temporary file cleanup concerns and API naming symmetry. The PR is clean, well-tested, and ready for final review.