Page MenuHomePhabricator

Remove code which warns about invalid longer chain and enters safe mode
AbandonedPublic

Authored by schancel on Aug 7 2017, 13:12.

Details

Reviewers
deadalnix
CCulianu
sickpig
freetrader
Group Reviewers
Restricted Project
Summary

Remove code which warns about invalid longer chain and enters safe mode

This warning was introduced in b8585384 , but no clear
reason was given why it should be necessary to care much
about invalid chains, let alone degrade our own service
based on their presence.

One possible purpose is to detect hard forks, but going
into safe mode based on a hardcoded limit like an estimated
6 blocks more work seems arbitrary and poorly designed.

Also, this warning code is not covered by tests, and is
causing actual issues for users (as reported in Issue 59
on ABC github). It seems leaving it in could serve as a
possible DoS attack vector more than anything useful.

Test Plan

make check
../qa/pull-tester/rpc-tests.py -extended

Diff Detail

Repository
rABC Bitcoin ABC
Branch
no_enter_safemode_on_invalid_chain
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 686
Build 686: arc lint + arc unit

Event Timeline

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptAug 7 2017, 13:12

After careful reading of the code I concur 100% with you. There's no reason to issue a warning and make RPC go into "safe" mode and fail just because a large *invalid* fork is detected.

This was clearly put in by core as a hard-fork detection mechanism but right now, in the field our clients are detecting a hard fork (because, after all, there is one!) and going into an undesired state as a result.

Approving because I consider leaving it in as a bug.

This revision is now accepted and ready to land.Aug 7 2017, 13:41
deadalnix requested changes to this revision.Aug 7 2017, 16:14
This revision now requires changes to proceed.Aug 7 2017, 16:14

No we are not removing this warning. It is useful to know one is on a minority fork. If the problem is getting into safe mode or whatever based on that, then fix that.

Is there a way to disable this warning for chain pre checkpoint/assumevalid ? Then that'd be the best option.

Ok, but warning gets really spammy with each new block -- maybe if you do keep the warning, limit it to a maximum number of times it can be issued -- or issue it only once per each unique 'invalid long chain found'.

Also reword it to indicate we are aware of a hard fork, and that it's normal, or somesuch.. not this panicky "everything is broken" message we had before. :)

@deadalnix -- you want the warning to be suppressed if the forked chain is pre-checkpoint height, correct? Is that what you're saying?

schancel abandoned this revision.
schancel added a reviewer: freetrader.