Merge bitcoin/bitcoin#29702: fees: Remove CLIENT_VERSION serialization

fa1c5cc fees: Log non-fatal errors as [warning], instead of info-level (MarcoFalke)
ddddbac fees: Pin required version to 149900 (MarcoFalke)
fa5126a fees: Pin "version that wrote" to 0 (MarcoFalke)

Pull request description:

  Coupling the fees serialization with CLIENT_VERSION is problematic, because:

  * `CLIENT_VERSION` may change, even though the serialization format does not change. This is harmless, but still confusing.
  * If a serialization format change was backported (unlikely), it may lead to incorrect results.
  * `CLIENT_VERSION` is changed at a different time during the release process than any serialization format change. This is harmless for releases of Bitcoin Core, but may be confusing when using the development branch.
  * It is harder to reason about a global `CLIENT_VERSION` when changing the format, than to reason about a versioning local to the module.

  Fix all issues by pinning the current version number in the module locally. In the future it can then be modified locally to the module, if needed.

ACKs for top commit:
  hodlinator:
    re-ACK fa1c5cc
  TheCharlatan:
    Re-ACK fa1c5cc

Tree-SHA512: 93870176ed50cc5a734576d66398a6036b31632228a9e05db1fa5452229e35ba4126f003e7db246aeb9891764ed47bde4470c674ec2bce7fd3ddd97e43944627

Author: fanquake

src/policy/fees.cpp

@@ -5,7 +5,6 @@
 
 #include <policy/fees.h>
 
-#include <clientversion.h>
 #include <common/system.h>
 #include <consensus/amount.h>
 #include <kernel/mempool_entry.h>
@@ -32,6 +31,10 @@
 #include <stdexcept>
 #include <utility>
 
+// The current format written, and the version required to read. Must be
+// increased to at least 289900+1 on the next breaking change.
+constexpr int CURRENT_FEES_FILE_VERSION{149900};
+
 static constexpr double INF_FEERATE = 1e99;
 
 std::string StringForFeeEstimateHorizon(FeeEstimateHorizon horizon)
@@ -168,7 +171,7 @@ class TxConfirmStats
      * Read saved state of estimation data from a file and replace all internal data structures and
      * variables with this state.
      */
-    void Read(AutoFile& filein, int nFileVersion, size_t numBuckets);
+    void Read(AutoFile& filein, size_t numBuckets);
 };
 
 
@@ -414,7 +417,7 @@ void TxConfirmStats::Write(AutoFile& fileout) const
     fileout << Using<VectorFormatter<VectorFormatter<EncodedDoubleFormatter>>>(failAvg);
 }
 
-void TxConfirmStats::Read(AutoFile& filein, int nFileVersion, size_t numBuckets)
+void TxConfirmStats::Read(AutoFile& filein, size_t numBuckets)
 {
     // Read data file and do some very basic sanity checking
     // buckets and bucketMap are not updated yet, so don't access them
@@ -961,8 +964,8 @@ bool CBlockPolicyEstimator::Write(AutoFile& fileout) const
 {
     try {
         LOCK(m_cs_fee_estimator);
-        fileout << 149900; // version required to read: 0.14.99 or later
-        fileout << CLIENT_VERSION; // version that wrote the file
+        fileout << CURRENT_FEES_FILE_VERSION;
+        fileout << int{0}; // Unused dummy field. Written files may contain any value in [0, 289900]
         fileout << nBestSeenHeight;
         if (BlockSpan() > HistoricalBlockSpan()/2) {
             fileout << firstRecordedHeight << nBestSeenHeight;
@@ -976,7 +979,7 @@ bool CBlockPolicyEstimator::Write(AutoFile& fileout) const
         longStats->Write(fileout);
     }
     catch (const std::exception&) {
-        LogPrintf("CBlockPolicyEstimator::Write(): unable to write policy estimator data (non-fatal)\n");
+        LogWarning("Unable to write policy estimator data (non-fatal)");
         return false;
     }
     return true;
@@ -986,20 +989,20 @@ bool CBlockPolicyEstimator::Read(AutoFile& filein)
 {
     try {
         LOCK(m_cs_fee_estimator);
-        int nVersionRequired, nVersionThatWrote;
-        filein >> nVersionRequired >> nVersionThatWrote;
-        if (nVersionRequired > CLIENT_VERSION) {
-            throw std::runtime_error(strprintf("up-version (%d) fee estimate file", nVersionRequired));
+        int nVersionRequired, dummy;
+        filein >> nVersionRequired >> dummy;
+        if (nVersionRequired > CURRENT_FEES_FILE_VERSION) {
+            throw std::runtime_error{strprintf("File version (%d) too high to be read.", nVersionRequired)};
         }
 
         // Read fee estimates file into temporary variables so existing data
         // structures aren't corrupted if there is an exception.
         unsigned int nFileBestSeenHeight;
         filein >> nFileBestSeenHeight;
 
-        if (nVersionRequired < 149900) {
-            LogPrintf("%s: incompatible old fee estimation data (non-fatal). Version: %d\n", __func__, nVersionRequired);
-        } else { // New format introduced in 149900
+        if (nVersionRequired < CURRENT_FEES_FILE_VERSION) {
+            LogWarning("Incompatible old fee estimation data (non-fatal). Version: %d", nVersionRequired);
+        } else { // nVersionRequired == CURRENT_FEES_FILE_VERSION
             unsigned int nFileHistoricalFirst, nFileHistoricalBest;
             filein >> nFileHistoricalFirst >> nFileHistoricalBest;
             if (nFileHistoricalFirst > nFileHistoricalBest || nFileHistoricalBest > nFileBestSeenHeight) {
@@ -1015,9 +1018,9 @@ bool CBlockPolicyEstimator::Read(AutoFile& filein)
             std::unique_ptr<TxConfirmStats> fileFeeStats(new TxConfirmStats(buckets, bucketMap, MED_BLOCK_PERIODS, MED_DECAY, MED_SCALE));
             std::unique_ptr<TxConfirmStats> fileShortStats(new TxConfirmStats(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE));
             std::unique_ptr<TxConfirmStats> fileLongStats(new TxConfirmStats(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE));
-            fileFeeStats->Read(filein, nVersionThatWrote, numBuckets);
-            fileShortStats->Read(filein, nVersionThatWrote, numBuckets);
-            fileLongStats->Read(filein, nVersionThatWrote, numBuckets);
+            fileFeeStats->Read(filein, numBuckets);
+            fileShortStats->Read(filein, numBuckets);
+            fileLongStats->Read(filein, numBuckets);
 
             // Fee estimates file parsed correctly
             // Copy buckets from file and refresh our bucketmap
@@ -1038,7 +1041,7 @@ bool CBlockPolicyEstimator::Read(AutoFile& filein)
         }
     }
     catch (const std::exception& e) {
-        LogPrintf("CBlockPolicyEstimator::Read(): unable to read policy estimator data (non-fatal): %s\n",e.what());
+        LogWarning("Unable to read policy estimator data (non-fatal): %s", e.what());
         return false;
     }
     return true;