fees: Remove CLIENT_VERSION serialization #29702

1. 
   maflcko commented at 9:19 am on March 22, 2024: member

   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.

2. 
   DrahtBot commented at 9:19 am on March 22, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	hodlinator, TheCharlatan
   Concept ACK	ajtowns

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] 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.

3. in src/policy/fees.cpp:961 in fa8efa98f1 outdated

   955@@ -957,12 +956,16 @@ void CBlockPolicyEstimator::FlushFeeEstimates()
   956     }
   957 }
   958 
   959+// The current format written, and the version required to read. Must be
   960+// increased to at least 279900+1 on the next breaking change.
   961+constexpr int CURRENT_FILE_VERSION{149900};
   

   ---

   ---

   

   ajtowns commented at 9:18 am on April 9, 2024:

   static ? Move it to the top of the file?

   Could consider defining three values:

   0static constexpr int FEE_FILE_MIN_READ_VERSION = 149900;
   1static constexpr int FEE_FILE_MAX_READ_VERSION = 279900;
   2static constexpr int FEE_FILE_WRITE_VERSION = 149900; 
   

   ---

   

   maflcko commented at 2:49 pm on July 5, 2024:

   static ? Move it to the top of the file?

   Sure, done both.

   Could consider defining three values:

   I think one value is enough for now. One can always add more values, if they are used in the code in the future.

4. in src/policy/fees.cpp:968 in fa8efa98f1 outdated

   965     try {
   966         LOCK(m_cs_fee_estimator);
   967-        fileout << 149900; // version required to read: 0.14.99 or later
   968-        fileout << CLIENT_VERSION; // version that wrote the file
   969+        fileout << CURRENT_FILE_VERSION;
   970+        fileout << 279900; // version that wrote the file. Used to be CLIENT_VERSION. Currently unused dummy field.
   

   ---

   ---

   

   ajtowns commented at 9:20 am on April 9, 2024:

   Storing nVersionThatWrote seems somewhat useful for manually debugging a corrupt fee data file, and figuring out how the bug occurred. If we’re just storing a constant, then the value isn’t going to be useful, so it seems better to just write (int)0 instead – that’s the amount of information it will convey… In any event, “version that wrote the file” is not accurate anymore if we’re making it something different from CLIENT_VERSION.

   ---

   

   maflcko commented at 2:39 pm on July 5, 2024:

   Storing nVersionThatWrote seems somewhat useful for manually debugging a corrupt fee data file, and figuring out how the bug occurred.

   I’d say the write-timestamp of the file and the corresponding debug.log section is more helpful, because the debug log also contains the CLIENT_VERSION serialized (as string) (possibly with commit_id), as well as any other stuff and config that happened.

   Happy to keep the value and instead write the 4 byte truncation of the commit id instead, falling back to CLIENT_VERSION if it is missing. But not sure if it is worth it.

   If we’re just storing a constant, then the value isn’t going to be useful, so it seems better to just write (int)0 instead

   Sure, happy to use anything here, as it is unused in Bitcoin Core anyway.

   I am using 0 for now.

5. 
   ajtowns commented at 9:20 am on April 9, 2024: contributor

   Concept ACK
6. 
   TheCharlatan commented at 1:26 pm on June 25, 2024: contributor

   Concept ACK
7. maflcko force-pushed on Jul 5, 2024
8. 
   maflcko commented at 2:54 pm on July 5, 2024: member

   rebased + addressed review
9. DrahtBot added the label CI failed on Aug 20, 2024
10. DrahtBot removed the label CI failed on Aug 22, 2024
11. maflcko force-pushed on Aug 28, 2024
12. 
    maflcko commented at 4:09 am on August 29, 2024: member

    Trivial rebase to adjust dev comments to account for CLIENT_VERSION bump in master.
13. achow101 requested review from ajtowns on Oct 15, 2024
14. achow101 requested review from TheCharlatan on Oct 15, 2024
15. in src/policy/fees.cpp:36 in fa7c73c9d6 outdated

    30@@ -32,6 +31,10 @@
    31 #include <stdexcept>
    32 #include <utility>
    33 
    34+// The current format written, and the version required to read. Must be
    35+// increased to at least 289900+1 on the next breaking change.
    36+static constexpr int CURRENT_FILE_VERSION{149900};
    

    ---

    ---

    

    hodlinator commented at 10:03 pm on October 15, 2024:

    nit: Even though it is static and thus file-local, I would still prefer it had a prefix mentioning “fees”. Might be clearer in various development tools listing symbols even if it’s clear to the compiler.

    0static constexpr int FEES_FILE_VERSION{149900};
    

    ---

    

    maflcko commented at 10:38 am on October 16, 2024:

    Thanks, clarified the name, because it wasn’t in a namespace itself (only the global one).
16. in src/policy/fees.cpp:995 in fa7c73c9d6 outdated

    993-        filein >> nVersionRequired >> nVersionThatWrote;
    994-        if (nVersionRequired > CLIENT_VERSION) {
    995+        int nVersionRequired, dummy;
    996+        filein >> nVersionRequired >> dummy;
    997+        if (nVersionRequired > CURRENT_FILE_VERSION) {
    998             throw std::runtime_error(strprintf("up-version (%d) fee estimate file", nVersionRequired));
    

    ---

    ---

    

    hodlinator commented at 10:21 pm on October 15, 2024:

    nit: Could improve adjacent line:

    0            throw std::runtime_error(strprintf("File version (%d) indicates it was generated by a newer version of the software.", nVersionRequired));
    

    ---

    

    maflcko commented at 10:47 am on October 16, 2024:

    Either should be fine, because it just ends up in the debug log (not really for end-users). What about:

    0throw std::runtime_error(strprintf("File version (%d) too high to be read.", nVersionRequired));
    

    if up-version is too vague?

    ---

    

    hodlinator commented at 8:37 pm on October 22, 2024:

    Yes, in case it somehow gets corrupted or modified by an entirely different software, that makes sense.

    Appreciated if you re-touch.

17. 
    hodlinator commented at 8:06 am on October 16, 2024: contributor

    Concept ACK fa7c73c9d6db856e68309cfe00c5fd00d845a6d9

    nFileVersion aka nVersionThatWrote was presumably passed into TxConfirmStats::Read in order to make it possible to deserialize the data differently for files written by older versions of the software to maintain some kind of upgrade path. Since no such backwards compatibility logic was implemented yet, it can technically be removed as done now.

    One could conceptually treat the remaining version value (presently still called nVersionRequired) as the version to perform backwards compatible reads for instead of logging “Incompatible old fee estimation data”… and skipping the rest of the read. But I guess that could be up to future changers of the format to re-add+implement.

18. fees: Pin "version that wrote" to 0

    The field is unused and there is no need to tie it to CLIENT_VERSION and
    increase it, if the format does not change.

    fa5126adcb
19. fees: Pin required version to 149900

    There is no need to compare the field to CLIENT_VERSION. Either the
    format remains compatible and the value can be left unchanged, or it is
    incompatible and the value needs to be increased to at least 289900+1.

    ddddbac9c1
20. maflcko force-pushed on Oct 16, 2024
21. TheCharlatan approved
22. 
    TheCharlatan commented at 8:22 am on October 22, 2024: contributor

    ACK fa8bd0be8432fda3c7312050433a6deb6722a073
23. DrahtBot requested review from hodlinator on Oct 22, 2024
24. hodlinator approved
25. 
    hodlinator commented at 9:56 pm on October 22, 2024: contributor

    ACK fa8bd0be8432fda3c7312050433a6deb6722a073

    Have a slight preference for the commit removing CLIENT_VERSION and the commit introducing CURRENT_FEES_FILE_VERSION being one and the same in order to give a more complete picture of the change.

    Used hexdump to inspect old ~/.bitcoin/regtest/fee_estimates.dat and ~/.bitcoin/fee_estimates.dat files, confirming that the first two little-endian integers correspond to the minimum required version and CLIENT_VERSION (version that wrote).

    Modified the required version and got expected outputs:

    (Increase)

    02024-10-22T21:38:58Z [warning] Unable to read policy estimator data (non-fatal): up-version (150156) fee estimate file
    12024-10-22T21:38:58Z Failed to read fee estimates from /home/hodlinator/.bitcoin/fee_estimates.dat. Continue anyway.
    

    (Decrease)

    02024-10-22T21:45:44Z [warning] Incompatible old fee estimation data (non-fatal). Version: 149644
    

26. maflcko force-pushed on Oct 23, 2024
27. fees: Log non-fatal errors as [warning], instead of info-level

    Also, remove not needed and possibly redundant function name and class
    names from the log string. Also, minimally reword the log messages.
    Also, remove redundant trailing newlines from log messages, while
    touching.

    fa1c5cc9df
28. maflcko force-pushed on Oct 23, 2024
29. 
    hodlinator commented at 6:53 pm on October 23, 2024: contributor

    re-ACK fa1c5cc9df116411edca172f8b80fc225c776554

    Thanks for adjusting the version error for too high values!

    Makes sense to remove explicit newlines given #30929.

30. DrahtBot requested review from TheCharlatan on Oct 23, 2024
31. TheCharlatan approved
32. 
    TheCharlatan commented at 8:36 pm on October 23, 2024: contributor

    Re-ACK fa1c5cc9df116411edca172f8b80fc225c776554
33. fanquake merged this on Oct 24, 2024
34. fanquake closed this on Oct 24, 2024

    ---

35. maflcko deleted the branch on Oct 24, 2024

Contributors
maflcko DrahtBot ajtowns TheCharlatan hodlinator

Review Requested
ajtowns