Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3457681627384baf41c7b1d809d87a52a2c94e07 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.

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:
'Warning: Large-work fork detected, forking after block $HASH' for %s in -alertnotify
"CheckForkWarningConditions: Warning: Large valid fork found\n forking the chain at height %d (%s)\n lasting to height %d (%s).\n" in debug.log for large valid-work fork
"CheckForkWarningConditions: Warning: Found invalid chain at least ~6 blocks longer than our best chain.\n" for the previous warning condition.
and translated versions of either
"Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade." for large, valid fork
or
"Warning: The network does not appear to fully agree! Some miners appear to be experiencing issues." for the previous warning condition.
in getinfo/status bar.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7c4ad5d8dffb116cbb8c2e429f19452cbb056d38 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.

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?

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.
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.

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-0.8.3.6 is comprised of a merge of the following two branches
https://github.com/litecoin-project/litecoin/commits/exp-mark11
litecoin specific commits on top of bitcoin-0.8.3
https://github.com/litecoin-project/litecoin/commits/exp-btc09backports
Select bitcoin-0.9 cherry-picks applied on top of bitcoin-0.8.3

litecoin-project@286c31b
litecoin-project@76cf1ab
litecoin-project@e751fad
Add these back to litecoin-0.8.3.6 and -reindex to see noisy warnings.

https://github.com/litecoin-project/litecoin/tree/exp-forkalert
FYI: I tested it yesterday after re-applying the three patches above (an older version of this PR that applied cleanly to bitcoin-0.8.3, prior to the recent rebase). A fresh Litecoin blockchain sync and a subsequent -reindex both were uneventful.

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)
A month ago I had a corrupted database that exhibited a great many errors in the log but did not crash. sipa's post immediately before it said, "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?"

Was his question addressed?

ACK. I re-indexed an old chain that has lots of forks and got no scary warnings.

I updated BIP50 with the fact that this is done. Great work Matt!