util: atomically write banlist.json and rename WriteSettings to WriteJsonUnsafe #35763

Summary

This is a follow-up to #35384 (comment)

• CBanDB::Write now writes banlist.json.tmp and uses RenameOver, so a failed/interrupted write can’t leave a truncated banlist.json (same pattern as settings.json).
• Renamed common::WriteSettings → common::WriteJsonUnsafe and documented that callers must pair it with an atomic rename.
• Added a unit test that checks the write/read roundtrip and that the .tmp file is gone after success.

<!--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.

<!--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 ?