test: Test that low difficulty chain fork is rejected #16551
Conversation
fa898ef to
fae28b4
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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
Updated at: 2019-08-07T12:48:33.276456. |
|
Concept ACK |
1 similar comment
|
Concept ACK |
|
Concept ACK, will review in next few days. |
fae28b4 to
fa982dd
Compare
|
Concept ACK, but I find the test hard to read atm. I'm confused about what's actually in |
fa982dd to
fa0c247
Compare
|
Added an inline comment to hopefully explain it better, @Sjors
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. |
There was a problem hiding this comment.
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.
fa0c247 to
333317c
Compare
Added test case (and node 1). Also, fixed up your comment suggestions. |
|
Thanks for adding the node 1 example. Code review ACK 333317c |
333317c test: Test that low difficulty chain fork is rejected (MarcoFalke) fa31dc1 test: Pass down correct chain name in tests (MarcoFalke) Pull request description: 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 ACKs for top commit: Sjors: Thanks for adding the node 1 example. Code review ACK 333317c Tree-SHA512: 90dffa540d0904f3cffb61d2382b1a26f84fe9560b7013e4461546383add31a8757b350616a6d43217c59ef7b8b2a1b62bb3bab582c679cbb2c660a782ce7be1
…ected 333317c test: Test that low difficulty chain fork is rejected (MarcoFalke) fa31dc1 test: Pass down correct chain name in tests (MarcoFalke) Pull request description: 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 ACKs for top commit: Sjors: Thanks for adding the node 1 example. Code review ACK 333317c Tree-SHA512: 90dffa540d0904f3cffb61d2382b1a26f84fe9560b7013e4461546383add31a8757b350616a6d43217c59ef7b8b2a1b62bb3bab582c679cbb2c660a782ce7be1
Summary: bitcoin/bitcoin@fa31dc1 --- Partial backport of Core [[bitcoin/bitcoin#16551 | PR16551]] Test Plan: ./test/functional/test_runner.py Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6430
Summary: bitcoin/bitcoin@333317c --- Concludes backport of Core [[bitcoin/bitcoin#16551 | PR16551]] Test Plan: ./test/functional/test_runner.py p2p_dos_header_tree.py Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6431
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