fees: Remove CLIENT_VERSION serialization #29702
Conversation
The field is unused and there is no need to tie it to CLIENT_VERSION and increase it, if the format does not change.
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 279900+1.
Also, remove not needed and possibly redundant function name and class names from the log string.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
| @@ -957,12 +956,16 @@ void CBlockPolicyEstimator::FlushFeeEstimates() | |||
| } | |||
| } | |||
|
|
|||
| // The current format written, and the version required to read. Must be | |||
| // increased to at least 279900+1 on the next breaking change. | |||
| constexpr int CURRENT_FILE_VERSION{149900}; | |||
There was a problem hiding this comment.
static ? Move it to the top of the file?
Could consider defining three values:
static constexpr int FEE_FILE_MIN_READ_VERSION = 149900;
static constexpr int FEE_FILE_MAX_READ_VERSION = 279900;
static constexpr int FEE_FILE_WRITE_VERSION = 149900;
| fileout << 149900; // version required to read: 0.14.99 or later | ||
| fileout << CLIENT_VERSION; // version that wrote the file | ||
| fileout << CURRENT_FILE_VERSION; | ||
| fileout << 279900; // version that wrote the file. Used to be CLIENT_VERSION. Currently unused dummy field. |
There was a problem hiding this comment.
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.
Coupling the fees serialization with CLIENT_VERSION is problematic, because:
CLIENT_VERSIONmay change, even though the serialization format does not change. This is harmless, but still confusing.CLIENT_VERSIONis 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.CLIENT_VERSIONwhen 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.