Merge bitcoin/bitcoin#28207: mempool: Persist with XOR

fa6b053b5c964fb35935fa994cb782c0731a56f8 mempool: persist with XOR (MarcoFalke)

Pull request description:

  Currently the `mempool.dat` file stores data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan the file and move it into quarantine, or delete it, or corrupt it.

  While the local wallet is expected to re-submit any pending transactions, unrelated transactions may be missing from the mempool after a restart. This may cause fee estimates to be off, or may cause block relay to be slower.

  Fix this, similar to https://github.com/bitcoin/bitcoin/pull/6650, by rolling a random XOR pattern over the dat file when writing or reading it.

  Obviously this can only protect against programs that accidentally and unintentionally are trying to mess with the dat file. Any program that intentionally wants to mess with the dat file can still trivially do so.

ACKs for top commit:
  achow101:
    re-ACK fa6b053b5c964fb35935fa994cb782c0731a56f8
  glozow:
    reACK fa6b053b5c964fb35935fa994cb782c0731a56f8
  ismaelsadeeq:
    ACK fa6b053b5c964fb35935fa994cb782c0731a56f8

Tree-SHA512: ded2ce3d81bc944b828263534e3178a1e45a914fe8e024f4a14c6561a73e301820944ecc75dd704b3d4221a7a3a5c0597ccab79546250c1197609ee981fe324e

9 files changed, 49 insertions, 15 deletions

diff --git a/doc/release-notes-28207.md b/doc/release-notes-28207.md
new file mode 100644
index 0000000000..56b88da16a
--- /dev/null
+++ b/doc/release-notes-28207.md
@@ -0,0 +1,7 @@
+mempool.dat compatibility
+========================
+
+The `mempool.dat` file created by -persistmempool or the savemempool RPC will
+be written in a new format, which can not be read by previous software
+releases. To allow for a downgrade, a temporary setting `-persistmempoolv1` has
+been added to fall back to the legacy format.
diff --git a/src/init.cpp b/src/init.cpp
index 62e7e32272..52ebd4a626 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -458,6 +458,11 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-par=<n>", strprintf("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)",
         -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-persistmempool", strprintf("Whether to save the mempool on shutdown and load on restart (default: %u)", DEFAULT_PERSIST_MEMPOOL), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-persistmempoolv1",
+                   strprintf("Whether a mempool.dat file created by -persistmempool or the savemempool RPC will be written in the legacy format "
+                             "(version 1) or the current format (version 2). This temporary option will be removed in the future. (default: %u)",
+                             DEFAULT_PERSIST_V1_DAT),
+                   ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex. "
             "Warning: Reverting this setting requires re-downloading the entire blockchain. "
diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h
index 757be41b3c..d09fd2ba35 100644
--- a/src/kernel/mempool_options.h
+++ b/src/kernel/mempool_options.h
@@ -23,6 +23,8 @@ static constexpr unsigned int DEFAULT_BLOCKSONLY_MAX_MEMPOOL_SIZE_MB{5};
 static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY_HOURS{336};
 /** Default for -mempoolfullrbf, if the transaction replaceability signaling is ignored */
 static constexpr bool DEFAULT_MEMPOOL_FULL_RBF{false};
+/** Whether to fall back to legacy V1 serialization when writing mempool.dat */
+static constexpr bool DEFAULT_PERSIST_V1_DAT{false};
 /** Default for -acceptnonstdtxn */
 static constexpr bool DEFAULT_ACCEPT_NON_STD_TXN{false};
 
@@ -56,6 +58,7 @@ struct MemPoolOptions {
     bool permit_bare_multisig{DEFAULT_PERMIT_BAREMULTISIG};
     bool require_standard{true};
     bool full_rbf{DEFAULT_MEMPOOL_FULL_RBF};
+    bool persist_v1_dat{DEFAULT_PERSIST_V1_DAT};
     MemPoolLimits limits{};
 };
 } // namespace kernel
diff --git a/src/kernel/mempool_persist.cpp b/src/kernel/mempool_persist.cpp
index ff655c5ffa..4087308d1a 100644
--- a/src/kernel/mempool_persist.cpp
+++ b/src/kernel/mempool_persist.cpp
@@ -8,6 +8,7 @@
 #include <consensus/amount.h>
 #include <logging.h>
 #include <primitives/transaction.h>
+#include <random.h>
 #include <serialize.h>
 #include <streams.h>
 #include <sync.h>
@@ -34,14 +35,14 @@ using fsbridge::FopenFn;
 
 namespace kernel {
 
-static const uint64_t MEMPOOL_DUMP_VERSION = 1;
+static const uint64_t MEMPOOL_DUMP_VERSION_NO_XOR_KEY{1};
+static const uint64_t MEMPOOL_DUMP_VERSION{2};
 
 bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active_chainstate, ImportMempoolOptions&& opts)
 {
     if (load_path.empty()) return false;
 
-    FILE* filestr{opts.mockable_fopen_function(load_path, "rb")};
-    CAutoFile file{filestr, CLIENT_VERSION};
+    CAutoFile file{opts.mockable_fopen_function(load_path, "rb"), CLIENT_VERSION};
     if (file.IsNull()) {
         LogPrintf("Failed to open mempool file from disk. Continuing anyway.\n");
         return false;
@@ -57,9 +58,15 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
     try {
         uint64_t version;
         file >> version;
-        if (version != MEMPOOL_DUMP_VERSION) {
+        std::vector<std::byte> xor_key;
+        if (version == MEMPOOL_DUMP_VERSION_NO_XOR_KEY) {
+            // Leave XOR-key empty
+        } else if (version == MEMPOOL_DUMP_VERSION) {
+            file >> xor_key;
+        } else {
             return false;
         }
+        file.SetXor(xor_key);
         uint64_t num;
         file >> num;
         while (num) {
@@ -151,17 +158,22 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock
 
     auto mid = SteadyClock::now();
 
-    try {
-        FILE* filestr{mockable_fopen_function(dump_path + ".new", "wb")};
-        if (!filestr) {
-            return false;
-        }
-
-        CAutoFile file{filestr, CLIENT_VERSION};
+    CAutoFile file{mockable_fopen_function(dump_path + ".new", "wb"), CLIENT_VERSION};
+    if (file.IsNull()) {
+        return false;
+    }
 
-        uint64_t version = MEMPOOL_DUMP_VERSION;
+    try {
+        const uint64_t version{pool.m_persist_v1_dat ? MEMPOOL_DUMP_VERSION_NO_XOR_KEY : MEMPOOL_DUMP_VERSION};
         file << version;
 
+        std::vector<std::byte> xor_key(8);
+        if (!pool.m_persist_v1_dat) {
+            FastRandomContext{}.fillrand(xor_key);
+            file << xor_key;
+        }
+        file.SetXor(xor_key);
+
         file << (uint64_t)vinfo.size();
         for (const auto& i : vinfo) {
             file << *(i.tx);
diff --git a/src/node/mempool_args.cpp b/src/node/mempool_args.cpp
index f63d9875fc..ac26600919 100644
--- a/src/node/mempool_args.cpp
+++ b/src/node/mempool_args.cpp
@@ -93,6 +93,8 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainP
 
     mempool_opts.full_rbf = argsman.GetBoolArg("-mempoolfullrbf", mempool_opts.full_rbf);
 
+    mempool_opts.persist_v1_dat = argsman.GetBoolArg("-persistmempoolv1", mempool_opts.persist_v1_dat);
+
     ApplyArgsManOptions(argsman, mempool_opts.limits);
 
     return {};
diff --git a/src/streams.h b/src/streams.h
index ae1434cc1c..f5b441344f 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -476,7 +476,7 @@ class AutoFile
 {
 protected:
     std::FILE* m_file;
-    const std::vector<std::byte> m_xor;
+    std::vector<std::byte> m_xor;
 
 public:
     explicit AutoFile(std::FILE* file, std::vector<std::byte> data_xor={}) : m_file{file}, m_xor{std::move(data_xor)} {}
@@ -516,6 +516,9 @@ public:
      */
     bool IsNull() const { return m_file == nullptr; }
 
+    /** Continue with a different XOR key */
+    void SetXor(std::vector<std::byte> data_xor) { m_xor = data_xor; }
+
     /** Implementation detail, only used internally. */
     std::size_t detail_fread(Span<std::byte> dst);
 
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index efcb77f47c..e057d7ece1 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -412,6 +412,7 @@ CTxMemPool::CTxMemPool(const Options& opts)
       m_max_datacarrier_bytes{opts.max_datacarrier_bytes},
       m_require_standard{opts.require_standard},
       m_full_rbf{opts.full_rbf},
+      m_persist_v1_dat{opts.persist_v1_dat},
       m_limits{opts.limits}
 {
 }
diff --git a/src/txmempool.h b/src/txmempool.h
index 3b0b8cf519..122bac09b1 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -446,6 +446,7 @@ public:
     const std::optional<unsigned> m_max_datacarrier_bytes;
     const bool m_require_standard;
     const bool m_full_rbf;
+    const bool m_persist_v1_dat;
 
     const Limits m_limits;
 
diff --git a/test/functional/mempool_compatibility.py b/test/functional/mempool_compatibility.py
index fd3e219586..a126f164aa 100755
--- a/test/functional/mempool_compatibility.py
+++ b/test/functional/mempool_compatibility.py
@@ -28,7 +28,7 @@ class MempoolCompatibilityTest(BitcoinTestFramework):
 
     def setup_network(self):
         self.add_nodes(self.num_nodes, versions=[
-            200100,  # Last release with previous mempool format
+            200100,  # Last release without unbroadcast serialization and without XOR
             None,
         ])
         self.start_nodes()
@@ -59,7 +59,7 @@ class MempoolCompatibilityTest(BitcoinTestFramework):
         old_node_mempool.rename(new_node_mempool)
 
         self.log.info("Start new node and verify mempool contains the tx")
-        self.start_node(1)
+        self.start_node(1, extra_args=["-persistmempoolv1=1"])
         assert old_tx_hash in new_node.getrawmempool()
 
         self.log.info("Add unbroadcasted tx to mempool on new node and shutdown")