Repository: bitcoin/bitcoin
File: src/node/mempool_persist.cpp (C++)

Code before:
        if (!pool.m_opts.persist_v1_dat) {
            FastRandomContext{}.fillrand(xor_key);
            file << xor_key;
        }
        file.SetXor(xor_key);

        uint64_t mempool_transactions_to_write(vinfo.size());
        file << mempool_transactions_to_write;
        LogInfo("Writing %u mempool transactions to file...\n", mempool_transactions_to_write);
        for (const auto& i : vinfo) {
            file << TX_WITH_WITNESS(*(i.tx));
            file << int64_t{count_seconds(i.m_time)};
            file << int64_t{i.nFeeDelta};
            mapDeltas.erase(i.tx->GetHash());
        }

        file << mapDeltas;

        LogInfo("Writing %d unbroadcast transactions to file.\n", unbroadcast_txids.size());
        file << unbroadcast_txids;

        if (!skip_file_commit && !file.Commit())
            throw std::runtime_error("Commit failed");
        file.fclose();
        if (!RenameOver(dump_path + ".new", dump_path)) {
            throw std::runtime_error("Rename failed");
        }
        auto last = SteadyClock::now();

        LogInfo("Dumped mempool: %.3fs to copy, %.3fs to dump, %d bytes dumped to file\n",
                  Ticks<SecondsDouble>(mid - start),
                  Ticks<SecondsDouble>(last - mid),
                  fs::file_size(dump_path));
    } catch (const std::exception& e) {
        LogInfo("Failed to dump mempool: %s. Continuing anyway.\n", e.what());
        return false;
    }
    return true;
}

} // namespace node


Review comment (bug):
This `throw` would trigger the `Assume()` in `~AutoFile()` to fail, no?

Instead of needing to comb through each line in this change and in future modifications after this PR, I think calling a lambda from the destructor on failure is marginally safer, as suggested in https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2288411339 (but with more correct `errno` handling + maybe requiring the lambda be `noexcept`).

Requiring a lambda for the destructor should at the very least be listed as an alternative in the PR description with your rationale against it.

Code after:

        std::vector<std::byte> xor_key(8);
        if (!pool.m_opts.persist_v1_dat) {
            FastRandomContext{}.fillrand(xor_key);
            file << xor_key;
        }
        file.SetXor(xor_key);

        uint64_t mempool_transactions_to_write(vinfo.size());
        file << mempool_transactions_to_write;
        LogInfo("Writing %u mempool transactions to file...\n", mempool_transactions_to_write);
        for (const auto& i : vinfo) {
            file << TX_WITH_WITNESS(*(i.tx));
            file << int64_t{count_seconds(i.m_time)};
            file << int64_t{i.nFeeDelta};
            mapDeltas.erase(i.tx->GetHash());
        }

        file << mapDeltas;

        LogInfo("Writing %d unbroadcast transactions to file.\n", unbroadcast_txids.size());
        file << unbroadcast_txids;

        if (!skip_file_commit && !file.Commit()) {
            (void)file.fclose();
            throw std::runtime_error("Commit failed");
        }
        if (file.fclose() != 0) {
            throw std::runtime_error(
                strprintf("Error closing %s: %s", fs::PathToString(file_fspath), SysErrorString(errno)));
        }
        if (!RenameOver(dump_path + ".new", dump_path)) {
            throw std::runtime_error("Rename failed");
        }
        auto last = SteadyClock::now();

        LogInfo("Dumped mempool: %.3fs to copy, %.3fs to dump, %d bytes dumped to file\n",
                  Ticks<SecondsDouble>(mid - start),
                  Ticks<SecondsDouble>(last - mid),
                  fs::file_size(dump_path));
    } catch (const std::exception& e) {
        LogInfo("Failed to dump mempool: %s. Continuing anyway.\n", e.what());
        (void)file.fclose();
        return false;
    }
    return true;
}

} // namespace node