qa: Avoid race in p2p_invalid_block by waiting for the block request #14700
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
utACK fa21568 |
|
I'm curious, have you been able to reproduce the issue locally? I've ran the test 1,000+ times and couldn't. |
zsoli18
approved these changes
Nov 10, 2018
|
It seems to me that it would make more sense to separate p2p-layer behaviors that we want to test from consensus-layer behaviors that we want to test -- otherwise there's a risk that changes to one might break our tests for another. I was surprised, when reviewing this just now, that the behavior of Separately, we should write a test that covers what we want the p2p behavior to be when various kinds of valid and invalid block headers are received. It'd be great if changing p2p behavior with respect to block download meant only changing p2p-related tests, and not worrying about the downstream effects of changes like that on our consensus tests. Anyway, this PR is currently a strict improvement as the existing code is buggy, so utACK if you'd like to leave this as-is and leave discussion of the broader point for a future PR. |
|
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. |
maflcko
pushed a commit
that referenced
this pull request
Nov 13, 2018
…block request fa21568 qa: Avoid race in p2p_invalid_block by waiting for the block request (MarcoFalke) 6c787d3 tests: Make feature_block pass on centos (MarcoFalke) Pull request description: This hopefully fixes #14661, which I believe is caused by a race in `send_blocks_and_test`. By setting `request_block=False` we only effectively check `node.getbestblockhash() != blocks[-1].hash` before returning and checking the debug.log. By setting `request_block=True` (the default) we make sure that we send the block, then sync with a ping before asserting on the debug.log. Even if this patch doesn't fix the issue, it is good cleanup: There is no reason to not wait for the blocks to be requested, since in all these cases the header gives no indication that the block is consensus invalid. So this patch makes the test also a bit stricter and more useful. Unrelated to this, I also include a fix that makes the tests pass on latest CentOS. Tree-SHA512: c7abee3b7dc790a8af6c289159a7751bd962f6fa16c1537e7e21a0a0ef05b9596d1f4eb75319614603c05cb803e021314fa3596508ba443edd03046b25527e0f
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Nov 30, 2018
Github-Pull: bitcoin#14700 Rebased-From: fa21568
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 13, 2020
…block request Summary: fa21568208 qa: Avoid race in p2p_invalid_block by waiting for the block request (MarcoFalke) 6c787d340c tests: Make feature_block pass on centos (MarcoFalke) Pull request description: This hopefully fixes #14661, which I believe is caused by a race in `send_blocks_and_test`. By setting `request_block=False` we only effectively check `node.getbestblockhash() != blocks[-1].hash` before returning and checking the debug.log. By setting `request_block=True` (the default) we make sure that we send the block, then sync with a ping before asserting on the debug.log. Even if this patch doesn't fix the issue, it is good cleanup: There is no reason to not wait for the blocks to be requested, since in all these cases the header gives no indication that the block is consensus invalid. So this patch makes the test also a bit stricter and more useful. Unrelated to this, I also include a fix that makes the tests pass on latest CentOS. Tree-SHA512: c7abee3b7dc790a8af6c289159a7751bd962f6fa16c1537e7e21a0a0ef05b9596d1f4eb75319614603c05cb803e021314fa3596508ba443edd03046b25527e0f Backport of Core [[bitcoin/bitcoin#14700 | PR14700]] Test Plan: ./test_runner.py feature_block p2p_invalid_block Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6525
knst
pushed a commit
to knst/dash
that referenced
this pull request
Aug 10, 2021
…or the block request fa21568 qa: Avoid race in p2p_invalid_block by waiting for the block request (MarcoFalke) 6c787d3 tests: Make feature_block pass on centos (MarcoFalke) Pull request description: This hopefully fixes bitcoin#14661, which I believe is caused by a race in `send_blocks_and_test`. By setting `request_block=False` we only effectively check `node.getbestblockhash() != blocks[-1].hash` before returning and checking the debug.log. By setting `request_block=True` (the default) we make sure that we send the block, then sync with a ping before asserting on the debug.log. Even if this patch doesn't fix the issue, it is good cleanup: There is no reason to not wait for the blocks to be requested, since in all these cases the header gives no indication that the block is consensus invalid. So this patch makes the test also a bit stricter and more useful. Unrelated to this, I also include a fix that makes the tests pass on latest CentOS. Tree-SHA512: c7abee3b7dc790a8af6c289159a7751bd962f6fa16c1537e7e21a0a0ef05b9596d1f4eb75319614603c05cb803e021314fa3596508ba443edd03046b25527e0f
knst
pushed a commit
to knst/dash
that referenced
this pull request
Aug 10, 2021
…or the block request fa21568 qa: Avoid race in p2p_invalid_block by waiting for the block request (MarcoFalke) 6c787d3 tests: Make feature_block pass on centos (MarcoFalke) Pull request description: This hopefully fixes bitcoin#14661, which I believe is caused by a race in `send_blocks_and_test`. By setting `request_block=False` we only effectively check `node.getbestblockhash() != blocks[-1].hash` before returning and checking the debug.log. By setting `request_block=True` (the default) we make sure that we send the block, then sync with a ping before asserting on the debug.log. Even if this patch doesn't fix the issue, it is good cleanup: There is no reason to not wait for the blocks to be requested, since in all these cases the header gives no indication that the block is consensus invalid. So this patch makes the test also a bit stricter and more useful. Unrelated to this, I also include a fix that makes the tests pass on latest CentOS. Tree-SHA512: c7abee3b7dc790a8af6c289159a7751bd962f6fa16c1537e7e21a0a0ef05b9596d1f4eb75319614603c05cb803e021314fa3596508ba443edd03046b25527e0f
knst
pushed a commit
to knst/dash
that referenced
this pull request
Aug 16, 2021
…aiting for the block request fa21568 qa: Avoid race in p2p_invalid_block by waiting for the block request (MarcoFalke) 6c787d3 tests: Make feature_block pass on centos (MarcoFalke) Pull request description: This hopefully fixes bitcoin#14661, which I believe is caused by a race in `send_blocks_and_test`. By setting `request_block=False` we only effectively check `node.getbestblockhash() != blocks[-1].hash` before returning and checking the debug.log. By setting `request_block=True` (the default) we make sure that we send the block, then sync with a ping before asserting on the debug.log. Even if this patch doesn't fix the issue, it is good cleanup: There is no reason to not wait for the blocks to be requested, since in all these cases the header gives no indication that the block is consensus invalid. So this patch makes the test also a bit stricter and more useful. Unrelated to this, I also include a fix that makes the tests pass on latest CentOS. Tree-SHA512: c7abee3b7dc790a8af6c289159a7751bd962f6fa16c1537e7e21a0a0ef05b9596d1f4eb75319614603c05cb803e021314fa3596508ba443edd03046b25527e0f
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Aug 23, 2021
…aiting for the block request fa21568 qa: Avoid race in p2p_invalid_block by waiting for the block request (MarcoFalke) 6c787d3 tests: Make feature_block pass on centos (MarcoFalke) Pull request description: This hopefully fixes bitcoin#14661, which I believe is caused by a race in `send_blocks_and_test`. By setting `request_block=False` we only effectively check `node.getbestblockhash() != blocks[-1].hash` before returning and checking the debug.log. By setting `request_block=True` (the default) we make sure that we send the block, then sync with a ping before asserting on the debug.log. Even if this patch doesn't fix the issue, it is good cleanup: There is no reason to not wait for the blocks to be requested, since in all these cases the header gives no indication that the block is consensus invalid. So this patch makes the test also a bit stricter and more useful. Unrelated to this, I also include a fix that makes the tests pass on latest CentOS. Tree-SHA512: c7abee3b7dc790a8af6c289159a7751bd962f6fa16c1537e7e21a0a0ef05b9596d1f4eb75319614603c05cb803e021314fa3596508ba443edd03046b25527e0f
UdjinM6
added a commit
to dashpay/dash
that referenced
this pull request
Aug 29, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This hopefully fixes #14661, which I believe is caused by a race in
send_blocks_and_test. By settingrequest_block=Falsewe only effectively checknode.getbestblockhash() != blocks[-1].hashbefore returning and checking the debug.log. By settingrequest_block=True(the default) we make sure that we send the block, then sync with a ping before asserting on the debug.log.Even if this patch doesn't fix the issue, it is good cleanup: There is no reason to not wait for the blocks to be requested, since in all these cases the header gives no indication that the block is consensus invalid. So this patch makes the test also a bit stricter and more useful.
Unrelated to this, I also include a fix that makes the tests pass on latest CentOS.