Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adapt hard-coded version number #969

Merged
merged 1 commit into from
Apr 15, 2019
Merged

Adapt hard-coded version number #969

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/policy/fees.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ bool CBlockPolicyEstimator::Write(CAutoFile& fileout) const
{
try {
LOCK(cs_feeEstimator);
fileout << 149900; // version required to read: 0.14.99 or later
fileout << 10000; // version required to read: 0.1.0 or later
Copy link
Member

@thothd thothd Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we even need to write the version number here? for which purpose is it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a legacy from Bitcoin Core, because they apparently changed the format with version 0.14.99, so they wrote the version to detect that. We, of course, only have one version now, so this could be done cleaner, but I'm not sure we want to clean this up now. There are other places in the code which could be cleaned up regarding the version handling as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok do we already have an issue for it, for cleaning the codebase from hardcoded version numbers or so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think we have an issue yet. We removed most hard-coded version numbers by now, I believe. But there is logic left which does not make too much sense without them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an issue now: #980

fileout << CLIENT_VERSION; // version that wrote the file
fileout << nBestSeenHeight;
if (BlockSpan() > HistoricalBlockSpan()/2) {
Expand Down Expand Up @@ -935,9 +935,9 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein)
unsigned int nFileBestSeenHeight;
filein >> nFileBestSeenHeight;

if (nVersionRequired < 149900) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this check make sense for us in the first place?

We only have one format, the one we'll start with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#969 (comment)

if (nVersionRequired < 10000) {
LogPrintf("%s: incompatible old fee estimation data (non-fatal). Version: %d\n", __func__, nVersionRequired);
} else { // New format introduced in 149900
} else { // New format introduced in 10000
unsigned int nFileHistoricalFirst, nFileHistoricalBest;
filein >> nFileHistoricalFirst >> nFileHistoricalBest;
if (nFileHistoricalFirst > nFileHistoricalBest || nFileHistoricalBest > nFileBestSeenHeight) {
Expand Down