util: write JSON atomically in WriteJson #35763

Summary

Follow-up to #35384 (comment)

• Renamed common::WriteSettings to common::WriteJson.
• WriteJson writes to path + ".tmp" and RenameOver()s into place, so a failed or interrupted write cannot leave a truncated destination. Both settings.json and banlist.json use this helper.
• Added a unit test for the write/read roundtrip, that the .tmp is gone after success, that rename failure is detected, and that a second failed write overwrites the same .tmp rather than creating another.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35763.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34520 (refactor: Add [[nodiscard]] to functions returning bool+mutable ref by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK

Thanks for picking these up.

cc: @sys-dev @ryanofsky

ACK 07ec5af152b5bcff1b74decb9c7b729dff6c2283, with two non-blocking observations:

The test is useful, but it also passes when CBanDB::Write is changed back to direct writing. Negative control proved this. It therefore does not verify the rename behavior named by the test.

The generalized WriteJsonUnsafe helper still reports “settings file” errors when writing banlist.json.tmp. “JSON file” or “file” would be accurate for both callers.

The test is useful, but it also passes when CBanDB::Write is changed back to direct writing. Negative control proved this. It therefore does not verify the rename behavior named by the test.

The generalized WriteJsonUnsafe helper still reports “settings file” errors when writing banlist.json.tmp. “JSON file” or “file” would be accurate for both callers.

these need attention. so, i think they are blocking

while going through this, another thought came to mind. right now WriteJsonUnsafe writes directly to whatever path it is given and relies on the caller to do the tmp-file + RenameOver() step. both current callers (args.cpp and addrdb.cpp) do this correctly, but that guarantee only lives in a comment. nothing stops a future caller from accidentally writing straight to a live file

i was wondering if it is worth making this a bit harder to misuse. two possible approaches came to mind

<details><summary><b>approach A: add <code>WriteJsonAtomic</code> alongside <code>WriteJsonUnsafe</code></b></summary>

keep the low-level helper as-is, but add a wrapper that handles the tmp-file + rename step:

// settings.h
bool WriteJsonUnsafe(const fs::path& path,
                     const std::map<std::string, SettingsValue>& values,
                     std::vector<std::string>& errors);

bool WriteJsonAtomic(const fs::path& dest_path,
                     const std::map<std::string, SettingsValue>& values,
                     std::vector<std::string>& errors);


// settings.cpp
bool WriteJsonAtomic(const fs::path& dest_path,
                     const std::map<std::string, SettingsValue>& values,
                     std::vector<std::string>& errors)
{
    const fs::path path_tmp{dest_path + ".tmp"};
    if (!WriteJsonUnsafe(path_tmp, values, errors)) {
        return false;
    }
    return RenameOver(path_tmp, dest_path);
}

callers become:

// either this
if (!WriteJsonUnsafe(path_tmp, values, errors)) return false;
if (!RenameOver(path_tmp, path)) return false;

// or this
if (!WriteJsonAtomic(path, values, errors)) return false;

this keeps the current API intact while giving callers a safe helper for the common case. existing callers with custom tmp-file handling can keep using WriteJsonUnsafe, while new callers can just use WriteJsonAtomic. the downside is that the footgun is still there. WriteJsonUnsafe is still publicly available, so a future caller can still accidentally bypass the atomic write path. it also means carrying two helpers that are closely related

</details>

<details><summary><b>approach B: make the helper always write atomically</b></summary>

this is quite similar to the idea @sys-dev suggested here.

instead of exposing both APIs, make the helper always write to a tmp file and rename it:

// settings.h
bool WriteJson(const fs::path& dest_path,
               const std::map<std::string, SettingsValue>& values,
               std::vector<std::string>& errors);

// settings.cpp
bool WriteJson(const fs::path& dest_path,
               const std::map<std::string, SettingsValue>& values,
               std::vector<std::string>& errors)
{
    SettingsValue out(SettingsValue::VOBJ);
    for (const auto& [key, value] : values) out.pushKVEnd(key, value);

    const fs::path path_tmp{dest_path + ".tmp"};

    std::ofstream file{path_tmp.std_path()};
    // ...error handling lives here...
    file << out.write(/*prettyIndent=*/4, /*indentLevel=*/1) << std::endl;

    return RenameOver(path_tmp, dest_path);
}

this makes the safe path the only path. every caller gets atomic writes by default, so it is no longer possible to accidentally write directly to a live file. the tradeoff is that every caller now gets .tmp + RenameOver() semantics, even if a future use case genuinely just wants "write these bytes to this exact path" (for example, a one-off dump or export). it also bakes the tmp-file naming scheme into the helper itself

</details>

my thinking: i lean toward approach B unless there is already a use case for exposing the raw write helper. it feels a bit simpler, and it removes the footgun instead of just documenting it. that said, i could easily be missing a future use case where writing directly to the given path is the better choice

any thoughts ?

@Herb-ops @winterrdog, can you review this again?

approach ACK

now, we can update the PR title & description to match what the current code actually does. they still mention WriteJsonUnsafe and callers doing the atomic rename, while the current implementation is self-contained and handles the "temp write + RenameOver()" itself