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.

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

ERROR: CTxMemPool::ReadFeeEstimates() on komodod start up #523

Open
dimxy opened this issue Dec 27, 2021 · 8 comments
Open

ERROR: CTxMemPool::ReadFeeEstimates() on komodod start up #523

dimxy opened this issue Dec 27, 2021 · 8 comments

Comments

@dimxy
Copy link
Collaborator

dimxy commented Dec 27, 2021

On start komodod prints a error ERROR: CTxMemPool::ReadFeeEstimates() up-version (109900) fee estimate file
this is from this code

        filein >> nVersionRequired >> nVersionThatWrote;
        if (nVersionRequired > CLIENT_VERSION)
            return error("CTxMemPool::ReadFeeEstimates(): up-version (%d) fee estimate file", nVersionRequired);

nVersionRequired is set to 109900 in WriteFeeEstimates and recently CLIENT_VERSION changed from 3000100 to 70100 so the condition nVersionRequired > CLIENT_VERSION now is true.
Suggestion: change nVersionRequired from 109900 to 70100

@jmjatlanta
Copy link

jmjatlanta commented Dec 27, 2021

The logic within ReadFeeEstimates seems to be there to protect the code from reading "older" versions. And to the code, a lower number is an older version.

Changing nVersionRequired in ReadFeeEstimates disables the "version protection" feature of the function, so I imagine that is not what you were suggesting.

Changing nVersionRequired in WriteFeeEstimates would fix this issue for new clients, but would not solve the problem for clients that had the previous version. The error would still appear. In addition, the "version protection" would not work as intended for versions between 70100 and 109900. I do not know the history of those version numbers, so am unsure if that is a real problem or not (I am guessing not).

Other possible options:

  1. Fix CLIENT_VERSION to something above 109900 (logical)
  2. Add logic that specifically looks for 109900 and "does the right thing" (yuck)
  3. change nVersionRequired as you suggested and deal with the error message, making sure each run of komodod writes the file so that the error only appears once, only after the "upgrade". (the lazy but tolerable fix)

Update: I have verified that the fee estimate file is written on komodod shutdown, so with option 3, the error will only appear once if komodod shuts down properly.

Sample test: https://github.com/KomodoPlatform/komodo/compare/dev...jmjatlanta:jmj_issue_523?expand=1

@dimxy
Copy link
Collaborator Author

dimxy commented Dec 27, 2021

I agree that much better is to provide compatibility with the files written with the 109900 value
maybe we could make a special case for it:
from now on write with nVersionRequired == 70100
but allow also to read old files with 109900:

        filein >> nVersionRequired >> nVersionThatWrote;
        if (nVersionRequired > CLIENT_VERSION && nVersionRequired != 109900 /*allow old files*/)
            return error("CTxMemPool::ReadFeeEstimates(): up-version (%d) fee estimate file", nVersionRequired);

assuming the format has never changed in our codebase. So we establish protection for future versions

@DeckerSU
Copy link

DeckerSU commented Dec 28, 2021

My offer is to change version required to read in WriteFeeEstimates to 70100, like:

bool
CTxMemPool::WriteFeeEstimates(CAutoFile& fileout) const
{
    try {
        LOCK(cs);
        fileout << 70100; // version required to read: 0.7.1.0 or later

And users with existing fee_estimates.dat who will have the:

ERROR: CTxMemPool::ReadFeeEstimates(): up-version (109900) fee estimate file

should just delete the file and let the app re-create it.

@dimxy
Copy link
Collaborator Author

dimxy commented Dec 28, 2021

Even no need to delete the file. The error would show once on the first startup and the file will be overwritten on next write.
But maybe we need to provide that the error won't show itself and the file is read okay even at the first run ?

@DeckerSU
Copy link

Maybe. But i just don't like "special cases". What if we will add 109900 as an exception, but in future the format of this file will be changed and this exception will cause unforeseen consequences? I always vote for clean logic without "special cases" and "workaround of workarounds". Value of 109900 obviously was a mistake / oversight during version changing, so, i vote for totally get get rid of it, instead of turn it into exception.

@jmjatlanta
Copy link

Even no need to delete the file. The error would show once on the first startup and the file will be overwritten on next write. But maybe we need to provide that the error won't show itself and the file is read okay even at the first run ?

What about changing ERROR: to Warning: and perhaps changing the verbiage to denote that processing will continue (as does a nearby message)?

@dimxy
Copy link
Collaborator Author

dimxy commented Dec 30, 2021

is it okay to fail reading this file once? If yes we may just set nVersionRequired to 70100 on writing and instruct users to ignore the error one time at startup

DeckerSU added a commit to DeckerSU/KomodoOcean that referenced this issue Jan 27, 2022
KomodoPlatform/komodo#523

After these changes version required to read in fee_estimates.dat
will be set to 0x111d4 (70100) by default, instead of wrong (legacy
codebase) 109900 value. So, the error ERROR: CTxMemPool::ReadFeeEstimates()
on wallet startup, mentioned above wouldn't appear.

Delete of existing fee_estimates.dat before upgrade to the new version
of wallet with with commit included - also could be recommended.
@dimxy
Copy link
Collaborator Author

dimxy commented Jan 11, 2023

Maybe let's add a small PR with a fix like this DeckerSU/KomodoOcean@eccb8f7 to close this issue too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants