Repairing the large-work fork warning system

jl2012 commented at 7:02 pm on December 29, 2016: contributor

The large-work fork warning system is broken under header first validation. This should fix it by storing all valid headers, even if the block contents or the previous blocks are invalid.

It also stores invalid-version-but-otherwise-valid headers. This allows a potential planned hardfork to be followed by the client (a simplified version of the hardfork bit and hardfork warning system I proposed: https://github.com/bitcoin/bips/pull/317 and https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2016-December/013332.html )

jl2012 force-pushed on Dec 29, 2016

fanquake added the label Validation on Dec 30, 2016

MarcoFalke added this to the milestone 0.14.0 on Jan 2, 2017

petertodd commented at 7:02 pm on January 5, 2017: contributor

Concept ACK on warning re: invalid version headers.

in qa/rpc-tests/forkwarning.py: in d4f94717cf outdated

198+            assert_equal(self.nodes[0].getinfo()['blocks'], 105)
199+            warning = self.nodes[0].getinfo()['errors']
200+            if (j < 7):
201+                assert_equal(warning, BASIC_WARNING)
202+            else:
203+                # Warning starts when block 103 is found since it doesn't knwo the block is invalid

paveljanik commented at 11:55 am on January 8, 2017:

knwo -> know

in qa/rpc-tests/forkwarning.py: in d4f94717cf outdated

208+        for b in new_blocks:
209+            self.submit_block(b)
210+            assert_equal(self.nodes[0].getinfo()['blocks'], 105)
211+            assert_equal(warning, FORK_WARNING)
212+
213+        # Build to 185a to stop the warning

paveljanik commented at 11:56 am on January 8, 2017:

185a -> 185 Edit: Or you are using the a character to distinguish between these two forks?

jl2012 commented at 6:50 pm on March 8, 2017:

yes. Maybe I could make the comments clearer

laanwj commented at 11:01 am on January 12, 2017: member

Concept ACK, but due to the non-localized changes required to core code my opinion is that it is too risky to include in 0.14, given that it just fixes the fork check. I’d prefer to merge this after the 0.14 split-off.

MarcoFalke removed this from the milestone 0.14.0 on Jan 12, 2017

MarcoFalke added this to the milestone Future on Jan 12, 2017

luke-jr commented at 4:48 pm on January 12, 2017: member

To actually fix the warning system, we need to avoid banning peers that give us invalid blocks. Unfortunately, we currently also rely on that banning to ensure we have peers on the same network as us.

jl2012 commented at 6:59 pm on January 12, 2017: contributor

@luke-jr we actually don’t ban all types of invalid blocks. For example, violation of all softforks after P2SH are not banned. Also, with this PR it won’t ban a node for sending header of a child of invalid block. As long as the PoW and timestamp are valid, the header chain will be followed.

jl2012 force-pushed on Mar 8, 2017

Store invalid nVersion headers and valid header of invalid blocks 5cd334727d

Update fork warning messages ff1a46d86e

[qa] test fork warning system 6bcd6a3633

jl2012 force-pushed on Mar 9, 2017

rebroad commented at 10:23 am on March 13, 2017: contributor

@luke-jr there’s a middle ground perhaps. set a maximum number of peers on different chains.

sdaftuar commented at 8:52 pm on March 6, 2018: member

@sipa @jl2012 and I discussed this a bit in person. I think we should revisit this PR after we have better DoS protections in place against storing arbitrarily long headers chains. (I plan to propose one potential solution to this soon.)

fanquake commented at 12:59 pm on April 26, 2018: member

Closing for now. This can be revisited in future as suggested by @sdaftuar.

fanquake closed this on Apr 26, 2018

MarcoFalke removed this from the milestone Future on Apr 26, 2018

MarcoFalke locked this on Sep 8, 2021

Repairing the large-work fork warning system #9443