Fix feeestimate #4598

-Fix bug where we were checking index value against the wrong vector size. This could cause the wrong value to be returned or to return an out of bounds value. -Move the clearing of the sorted sample vectors to before the debug logging. The debug logging will now give the right answer

ACK, good catches!

Untested ACK

unrelated, but I just got several segmentation faults. It turned out that the code below caused the problem. The scenario was delta=1 and history.size()=0. If I read the code correctly, this results in -1 index access in the last line. Maybe we can fix that in this pull request, so I dont have to submit another 'Fix feeestimate'. (EDIT: I guess for history to be empty, the file fee_estimates.dat has to exist, but with no entries.)

    BOOST_FOREACH(const CTxMemPoolEntry& entry, entries)
    {
        // How many blocks did it take for miners to include this transaction?
        int delta = nBlockHeight - entry.GetHeight();
        if (delta <= 0)
        {
            // Re-org made us lose height, this should only happen if we happen
            // to re-org on a difficulty transition point: very rare!
            continue;
        }
        if ((delta-1) >= (int)history.size())
            delta = history.size(); // Last bucket is catch-all
        entriesByConfirmations[delta-1].push_back(&entry); <=========== segfault
    }

I'd be happy to update the PR, but not sure what the right solution is. I could change the line that seg faulted to:

if (delta > 0)
    entriesByConfirmations.at(delta-1).push_back(&entry);

And I think that there isn't anywhere else that would throw an exception, but it seems like the code is not really designed for a history of 0 size.

Was this the result of reading a corrupted data file? It would make sense to sanity check the data file on read as well.

@cozz : the mystery is how history.size() could ever become zero. Looking at the code, I agree with @morcos -- your estimates.dat file must have been corrupted somehow.

There are already sanity checks in CBlockAverage::Read; adding a sanity check in CMinerPolicyEstimator::Read() like:

    filein >> numEntries;
    if (numEntries <= 0 || numEntries > ...uh, some large value...)
         throw runtime_error("Corrupt estimates file.");

... and maybe an assert(history.size() > 0) in the right spot(s) would be best fix.

The problem is gone after deleting fee_estimates.dat. I assume I was using an old incompatible fee_estimates.dat. So lets not bother much, your suggested sanity checks/asserts sound good to me.

I just added another commit to sanity check the reading of the file. I read fee_estimates.dat into a temporary history vector first, because otherwise the sanity checking could have failed in reading the individual CBlockAverage's and still left an inconsistent state, so I didn't want to replace the state we were initialized with unless we knew processing the whole file succeeded.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4598_e59441f0863d20c6f205c4854a37507267183863/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Thanks a lot for this, it seems to have solved quite some crashes that people were experiencing, especially on testnet.