Detect any sufficiently long fork and alert the user just like any other alert #2658
Conversation
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3457681627384baf41c7b1d809d87a52a2c94e07 for binaries and test log. |
|
I think it would be nice to not just use our default cryptic message, but provide some details (not only for this new condition). |
|
Overall looks good to me. Agree with Diapolo that the log message could be more helpful, like by including the fork block hash and stating why transactions may be incorrect. |
|
OK, rewrote the alert messages, now you get: |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7c4ad5d8dffb116cbb8c2e429f19452cbb056d38 for binaries and test log. |
|
Looks good to me. |
|
I think this code belongs in main rather than alert (rationale: it needs access to the current chainstate, which is maintained by main - alert shouldn't need a dependency on that). Code in general looks good, but needs a rebase. Also, I expect the most common case for a message like this to be a locally corrupted database. Maybe that is worth mentioning is the text? |
|
Interesting ... "CheckForkWarningConditions: Warning: Large valid fork found\n forking the chain at height" can happen with a daemon that does not fail if your database is corrupted in just the right way. Sorry I don't have a copy of the corrupted database to demonstrate this. I can imagine this will be a false alarm for someone in the future, and it would be ideal if the daemon had a way to differentiate between database corruption and a genuine problem on the network. |
|
Also note that this puts lots of needlessly scary-looking warnings in debug.log during a reindex. Perhaps it should be silent during a reindex if prior to the last checkpoint? |
|
@TheBlueMatt Rebase needed. |
|
@wtogami Hmm...I dont see any fork messages during -reindex, are you talking about a -reindex on a corrupted chainstate? |
Such a fork is defined as being at least 7 blocks long and having a tip which is within 72 blocks of our best block.
|
Rebased, moved code to main.cpp, because its not like that file isnt already too full. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f65e7092a200ee6e9453058c3dafbab62d9b4dcc for binaries and test log. |
| } | ||
|
|
||
| // We define a condition which we should warn the user about as a fork of at least 7 blocks | ||
| // who's tip is within 72 blocks (+/- 12 hours if no one mines it) of ours |
|
LGTM. Can we merge this now? |
|
Is the patch intended to warn about forks during the -reindex process where it is unnecessarily scary? Can it be somehow silent about forks until after the last checkpoint? |
|
Please do not further overload checkpoints. ... but yea, warning during reindex is obviously not good. :) |
|
Probably a condition of this should be "at least one block in the fork must have been rejected by us"? |
|
RE: warning during reindex: suppressing the alert if best-other-tip-time is more than a day (a week? or maybe if max(other-tip-time, best-tip-time) is...) in the past is probably the right thing to do: reasoning would be we don't care about old fork events that have long-since been resolved. But before implementing that, can we get a block index that demonstrates false positives during a reindex? (I've got a corrupt chain that I'll copy and then reindex if somebody else doesn't beat me to it). |
|
I removed these three patches from litecoin-0.8.x not because it was broken, but from concern that it would unnecessarily scare people with false positives that appeared to me during a normal reindex. Although I am not entirely certain it was the type of false positive you are asking for. I didn't investigate too hard as it seemed best to simply remove the patches for the upcoming release meant to minimize risk. https://github.com/litecoin-project/litecoin/commits/exp-mark11b litecoin-project@286c31b |
|
https://github.com/litecoin-project/litecoin/tree/exp-forkalert A month ago I saw loud and scary warnings during -reindex twice in a row. I don't know what is different now. Would a fresh sync with additional checkpoints result in a different local blockchain sans historic forks? Whatever the issue, I am unable to reproduce it with this freshly synced chain now. #2658 (comment) Was his question addressed? |
|
ACK. I re-indexed an old chain that has lots of forks and got no scary warnings. |
Detect any sufficiently long fork and alert the user just like any other alert
|
I updated BIP50 with the fact that this is done. Great work Matt! |
Tested with BitcoindComparisonTool (which generates a large fork) and -alertnotify script successfully notified me and I get the relevant warnings in getinfo
You'll get the usually cryptic error:
"errors" : "Warning: Displayed transactions may not be correct! You may need to upgrade, or other nodes may need to upgrade."
and alertnotify will be called with
Warning: Large-work fork detected. You may need to upgrade, or other nodes may need to upgrade.