test: Test that low difficulty chain fork is rejected

MarcoFalke commented at 10:04 pm on August 5, 2019: member

To prevent OOM, Bitcoin Core will reject chain forks at low difficulty by default. This is the only use-case of checkpoints, so add a test for it to make sure the feature works as expected. If it didn’t work, checkpoints would have no use-case and we might as well remove them

MarcoFalke force-pushed on Aug 5, 2019

fanquake added the label Tests on Aug 5, 2019

DrahtBot commented at 2:05 am on August 6, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#8994 (Testchains: Introduce custom chain whose constructor… by jtimon)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Coverage

Coverage	Change (pull 16551, 308ed29008fff26b27ca181642ca06f705fde6e6)	Reference (master, e5fdda68c6d2313edb74443f0d1e6d2ce2d97f5e)
Lines	+0.0119 %	89.1963 %
Functions	+0.0940 %	82.1787 %
Branches	+0.0129 %	46.6003 %

Updated at: 2019-08-07T12:48:33.276456.

practicalswift commented at 12:06 pm on August 6, 2019: contributor

Concept ACK

laanwj commented at 1:32 pm on August 14, 2019: member

Concept ACK

jamesob commented at 3:42 pm on August 14, 2019: member

Concept ACK, will review in next few days.

DrahtBot added the label Needs rebase on Aug 14, 2019

MarcoFalke force-pushed on Aug 14, 2019

DrahtBot removed the label Needs rebase on Aug 14, 2019

Sjors commented at 1:56 pm on August 16, 2019: member

Concept ACK, but I find the test hard to read atm. I’m confused about what’s actually in blockheader_testnet3.hex. Are those the headers without the block hash of the real testnet3 chain? Because I don’t see the checkpoint hash in there. Also, rather than having two lines with fork: and a parser to deal with that, maybe just hard code those in the test itself? Alternatively, create a seperate file for the fork? At what height does this fork start? Genesis or same height as checkpoint?

test: Pass down correct chain name in tests fa31dc1bf4

MarcoFalke force-pushed on Aug 16, 2019

MarcoFalke commented at 2:38 pm on August 16, 2019: member

Added an inline comment to hopefully explain it better, @Sjors

Are those the headers without the block hash of the real testnet3 chain? Because I don’t see the checkpoint hash in there.

Serialized headers include only the hash of the previous block, so if the last header is the checkpoint, the block hash of the checkpoint will never appear in the data file.

in test/functional/p2p_dos_header_tree.py:64 in fa0c247ed8 outdated

59+        self.log.info("Feed all fork headers (fails)")
60+        with self.nodes[0].assert_debug_log(['bad-fork-prior-to-checkpoint (code 67)']):
61+            self.nodes[0].p2p.send_message(msg_headers(self.headers_fork))
62+            self.nodes[0].p2p.wait_for_disconnect()
63+
64+        self.log.info("Feed all fork headers (succeeds)")

Sjors commented at 3:27 pm on August 16, 2019:

Nit: “Disable checkpoints and feed low work fork headers again (succeeds)”

in test/functional/p2p_dos_header_tree.py:59 in fa0c247ed8 outdated

54+            'hash': '000000002a936ca763904c3c35fce2f3556c559c0214345d31b1bcebf76acb70',
55+            'branchlen': 546,
56+            'status': 'headers-only',
57+        } in self.nodes[0].getchaintips()
58+
59+        self.log.info("Feed all fork headers (fails)")

Sjors commented at 3:29 pm on August 16, 2019:

Nit: “Feed headers of fork with less PoW (fails due to checkpoint)”

in test/functional/p2p_dos_header_tree.py:48 in fa0c247ed8 outdated

43+        self.headers_fork = [l for l in h_lines if l.startswith(FORK_PREFIX)]
44+
45+        self.headers = [FromHex(CBlockHeader(), h) for h in self.headers]
46+        self.headers_fork = [FromHex(CBlockHeader(), h) for h in self.headers_fork]
47+
48+        self.log.info("Feed all non-fork headers")

Sjors commented at 3:31 pm on August 16, 2019:

Nit: “Feed non-fork headers, up to and including the first checkpoint (succeeds)”

Sjors approved

Sjors commented at 3:43 pm on August 16, 2019: member

ACK fa0c247, but added some suggestions for clearer wording.

To make the behavior more clear, it helps to include non DoS examples. Can you generate a testnet fork with more proof of work? It should be shorter than the real chain so it doesn’t reach checkpoint height.

With and without checkpoints enabled I would expect this fork to be accepted if it’s the first thing we see.

If we see it after we’ve already seen headers up to the checkpoint, then with checkpoints enabled I expect this to be rejected. With checkpoints disabled, I expect a reorg.

test: Test that low difficulty chain fork is rejected 333317ce6b

MarcoFalke force-pushed on Aug 16, 2019

MarcoFalke commented at 5:06 pm on August 16, 2019: member

With and without checkpoints enabled I would expect this fork to be accepted if it’s the first thing we see.

Added test case (and node 1). Also, fixed up your comment suggestions.

Sjors commented at 5:28 pm on August 16, 2019: member

Thanks for adding the node 1 example. Code review ACK 333317c

MarcoFalke referenced this in commit ca97d292ce on Sep 12, 2019

MarcoFalke merged this on Sep 12, 2019

MarcoFalke closed this on Sep 12, 2019

MarcoFalke deleted the branch on Sep 12, 2019

sidhujag referenced this in commit 1fa7bebf7a on Sep 12, 2019

deadalnix referenced this in commit 4475ac8660 on Jun 7, 2020

deadalnix referenced this in commit 3411c5a89e on Jun 7, 2020

DrahtBot locked this on Dec 16, 2021

test: Test that low difficulty chain fork is rejected #16551

Conflicts

Coverage