ui: Warn the user on file corruption in mempool.dat #14932

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:Mf1812-testFileCorruption changing 5 files +45 −3

MarcoFalke commented at 2:52 AM on December 12, 2018: member

File corruption is a common cause of issue reports. Often the user is not aware of hardware defects, because corruption error messages are hidden in the debug log file.

Improve this by handing file corruption error messages directly to the user.
MarcoFalke added the label Utils/log/libs on Dec 12, 2018
MarcoFalke force-pushed on Dec 12, 2018
test: Add feature_file_corruption.py a7328ab43c
[ui] Add UIError and UIWarning 3d01bddd2a
ui: Warn the user on corrupt mempool.dat 9aaf300143
MarcoFalke force-pushed on Dec 12, 2018
jonasschnelli commented at 4:21 AM on December 12, 2018: contributor

Would adding levelDB/UTXO-set corruption to the test make sense?
gmaxwell commented at 4:52 AM on December 12, 2018: contributor

Do we really want to report on mempool.dat corruption? The right thing to do, I think, is to just ignore it. Usually the txn will be out of date anyways. This would also potentially reduce our ability to change the format in the future: it would be totally reasonable for a major version to just not support the prior's mempool.dat format but not nice to pop up pointless warnings.

Any of the blokchain stuff sure... but mempool and peers are pretty ephemeral.
jonasschnelli commented at 5:57 AM on December 12, 2018: contributor

Tend to agree with @gmaxwell, maybe turn this into a Blocks/UTXO-set corruption detection pull?
laanwj commented at 9:36 AM on December 12, 2018: member

it would be totally reasonable for a major version to just not support the prior's mempool.dat format but not nice to pop up pointless warnings.

Not sure how this is done right now, but could treat this case separately; have an header with version number and ignore newer versions. Warn only when the format claims to be the old version and mis-parses.

But from a user perspective I agree with you, that corruption in ephemeral files (mempool, peers, ...) should be logged but not pop up a worrying warning. This is what the log is for.
MarcoFalke commented at 5:19 PM on December 12, 2018: member

The first thing that is serialized is the version. If we change the format and don't bump the version, it is a bug in our code. Warning on that would at least point out the bug.
DrahtBot commented at 5:35 PM on December 12, 2018: member



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



Coverage

Coverage Change (pull 14932) Reference (master)

Lines -0.0344 % 87.2941 %

Functions -0.0302 % 84.3495 %

Branches -0.0221 % 51.3853 %

<sup>Updated at: 2018-12-12T17:35:06.827752.</sup>
gmaxwell commented at 5:38 PM on December 12, 2018: contributor

Still, as a matter of ui principle, we should probably not warn users on anything that isn't important to them and/or does not require action on their part. And I /think/ mempool doesn't fit this criteria (thought I could be convinced otherwise). -- thanks for the point about versions, though.

I can't tell you how many times I've given tech support to a user and encountered something like "I reindexed this piece of junk three times taking a whole day each time and was still getting my logs flooed with errors! (ERROR: AcceptToMemoryPool: inputs already spent). Next I deleted all .dat files, and I'm not seeing those errors (thank god!) but it seems to be downloading the blockchain again and I can no longer see any of my coins or addresses. How do I get it to show my balance again?"

Things like the mempool are a cache, if the cache is corrupted, we should probably flush it and move on without bothering the user. If we wanted to create some kind of support information screen with a "support code" that is just some hex representation of a bitfield that sets bits for things like "I dumped my mempool dat" but isn't at risk of freaking out a user, that would be fine...

I just really worry about sending users down a hypochondriac rathole where they ultimately waste time or even corrupt their data because of a harmless state they didn't need to do anything about. ... since I've seen it too many times.

Coverage	Change (pull 14932)	Reference (master)
Lines	-0.0344 %	87.2941 %
Functions	-0.0302 %	84.3495 %
Branches	-0.0221 %	51.3853 %

in test/functional/feature_file_corruption.py:28 in 9aaf300143

  23 | +        with open(mempool_dat_path, 'rb+') as mempool_dat:
  24 | +            mempool_dat.seek(9)
  25 | +            mempool_dat.write(b'ff')
  26 | +        with self.nodes[0].assert_debug_log(expected_msgs=['Failed to deserialize mempool data on disk']):
  27 | +            self.start_nodes()
  28 | +        self.stop_node(0, expected_stderr='Warning: Failed to deserialize mempool data on disk: CAutoFile::read: end of file: iostream error. Continuing anyway.')

ken2812221 commented at 4:49 AM on December 13, 2018:

The detail error message varies on different OS (Linux: iostream error, Windows: iostream stream error).

luke-jr commented at 5:44 AM on December 20, 2018: member

We'd need to be careful on a version bump, as number 2 is already used for Knots / #9422
MarcoFalke closed this on Dec 20, 2018
MarcoFalke deleted the branch on Dec 20, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

ui: Warn the user on file corruption in mempool.dat #14932

Coverage