Skip to content

Conversation

@jnewbery
Copy link
Contributor

feature_block.py occasionally fails on Travis. I believe this is due to
a a race condition when reconnecting to bitcoind after a subtest that
expects disconnection. If the test runs ahead and sends the INV for the
subsequent test before we've received the initial sync getheaders, then
we may end up sending two headers messages - one as a response to the
initial sync getheaders and one in response to the INV getheaders. If
both of those headers fail validation with a DoS score of 50 or higher,
then we'll unexpectedly be disconnected.

There is only one validation failure that has a DoS score bewteen 50 and
100, which is high-hash. That's why the test is failing immediately
after the "Reject a block with invalid work" subtest.

Fix is to wait for the initial getheaders from the peer before we
start populating our blockstore. That way we won't have any invalid
headers to respond to it with.

feature_block.py occasionally fails on Travis. I believe this is due to
a a race condition when reconnecting to bitcoind after a subtest that
expects disconnection. If the test runs ahead and sends the INV for the
subsequent test before we've received the initial sync getheaders, then
we may end up sending two headers messages - one as a response to the
initial sync getheaders and one in response to the INV getheaders. If
both of those headers fail validation with a DoS score of 50 or higher,
then we'll unexpectedly be disconnected.

There is only one validation failure that has a DoS score bewteen 50 and
100, which is high-hash. That's why the test is failing immediately
after the "Reject a block with invalid work" subtest.

Fix is to wait for the initial getheaders from the peer before we
start populating our blockstore. That way we won't have any invalid
headers to respond to it with.
@jnewbery
Copy link
Contributor Author

This test bug would disappear with #11639, since that changes the DoS disconnect/ban behaviour. However, this change is probably still useful to ensure that there's no cross-contamination between subtests.

@jnewbery
Copy link
Contributor Author

Longer term, can we just have proper unit tests for validation please 😁

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c1d7420. I'm not very familiar with this test but the explanation and fix make sense. Is a similar fix needed for the new invalid_tx test in #13003 that is now getting disconnected?

@jnewbery
Copy link
Contributor Author

Is a similar fix needed for the new invalid_tx test in #13003 that is now getting disconnected?

No - this is specific to invalid block headers (and more specifically - only to block headers that fail PoW validation).

@fanquake fanquake added the Tests label Apr 20, 2018
@PierreRochard
Copy link
Contributor

utACK c1d7420

@maflcko maflcko merged commit c1d7420 into bitcoin:master Apr 22, 2018
maflcko pushed a commit that referenced this pull request Apr 22, 2018
c1d7420 [tests] Fix feature_block flakiness (John Newbery)

Pull request description:

  feature_block.py occasionally fails on Travis. I believe this is due to
  a a race condition when reconnecting to bitcoind after a subtest that
  expects disconnection. If the test runs ahead and sends the INV for the
  subsequent test before we've received the initial sync getheaders, then
  we may end up sending two headers messages - one as a response to the
  initial sync getheaders and one in response to the INV getheaders. If
  both of those headers fail validation with a DoS score of 50 or higher,
  then we'll unexpectedly be disconnected.

  There is only one validation failure that has a DoS score bewteen 50 and
  100, which is high-hash. That's why the test is failing immediately
  after the "Reject a block with invalid work" subtest.

  Fix is to wait for the initial getheaders from the peer before we
  start populating our blockstore. That way we won't have any invalid
  headers to respond to it with.

Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f
codablock pushed a commit to codablock/dash that referenced this pull request Jan 8, 2020
c1d7420 [tests] Fix feature_block flakiness (John Newbery)

Pull request description:

  feature_block.py occasionally fails on Travis. I believe this is due to
  a a race condition when reconnecting to bitcoind after a subtest that
  expects disconnection. If the test runs ahead and sends the INV for the
  subsequent test before we've received the initial sync getheaders, then
  we may end up sending two headers messages - one as a response to the
  initial sync getheaders and one in response to the INV getheaders. If
  both of those headers fail validation with a DoS score of 50 or higher,
  then we'll unexpectedly be disconnected.

  There is only one validation failure that has a DoS score bewteen 50 and
  100, which is high-hash. That's why the test is failing immediately
  after the "Reject a block with invalid work" subtest.

  Fix is to wait for the initial getheaders from the peer before we
  start populating our blockstore. That way we won't have any invalid
  headers to respond to it with.

Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f
UdjinM6 pushed a commit to dashpay/dash that referenced this pull request Jan 11, 2020
…nTestFramework (#3277)

* [tests] Change feature_block.py to use BitcoinTestFramework

* [tests] Fix flake8 warnings in feature_block.py

* [tests] Tidy up feature_block.py

- move all helper methods to the end
- remove block, create_tx and create_and_sign_tx shortcuts
- remove --runbarelyexpensive option, since it defaults to True and it's
unlikely that anyone ever runs the test with this option set to false.

* [tests] Add logging to feature_block.py

* [tests] Improve assert message when wait_until() fails

* Merge bitcoin#13048: [tests] Fix feature_block flakiness

c1d7420 [tests] Fix feature_block flakiness (John Newbery)

Pull request description:

  feature_block.py occasionally fails on Travis. I believe this is due to
  a a race condition when reconnecting to bitcoind after a subtest that
  expects disconnection. If the test runs ahead and sends the INV for the
  subsequent test before we've received the initial sync getheaders, then
  we may end up sending two headers messages - one as a response to the
  initial sync getheaders and one in response to the INV getheaders. If
  both of those headers fail validation with a DoS score of 50 or higher,
  then we'll unexpectedly be disconnected.

  There is only one validation failure that has a DoS score bewteen 50 and
  100, which is high-hash. That's why the test is failing immediately
  after the "Reject a block with invalid work" subtest.

  Fix is to wait for the initial getheaders from the peer before we
  start populating our blockstore. That way we won't have any invalid
  headers to respond to it with.

Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f

* Temporarely rename MAX_BLOCK_SIZE -> MAX_BLOCK_BASE_SIZE

We'll undo this after the next commit. This avoids merge many conflicts and
makes reviewing easier.

* Rename MAX_BLOCK_BASE_SIZE back to MAX_BLOCK_SIZE

* Use DoS score of 100 for bad-blk-sigops

This was accidently changed to 10 while backporting bitcoin#7287 and causes
test failures in p2p-fullblocktest.py

* Use allowOptimisticSend=true when sending reject messages

This fixes test failures in p2p-fullblocktest.py which expects reject
messages to be sent/received before connections get closed.

* Fix p2p-fullblocktest.py

- CBlock and friends are still in test_framework.mininode
- "-whitelist" causes connections to not be dropped, which in turn causes
  sync_blocks with reconnect=True to fail
- "bad-cb-amount" does not cause a ban in Dash, so reconnect must be False
- Dash already bans when a header is received which is a child of an invalid
  header, causing block requests to never happen

* Backport missing changes from bitcoin#13003

bitcoin#13003 was backported out of order which causes missed changes.

* Bump p2p-fullblocktest timeouts

* Increase RPC timeout in p2p-fullblocktest.py

Co-authored-by: John Newbery <[email protected]>
Co-authored-by: MarcoFalke <[email protected]>
TheArbitrator pushed a commit to TheArbitrator/dash that referenced this pull request Jun 4, 2021
c1d7420 [tests] Fix feature_block flakiness (John Newbery)

Pull request description:

  feature_block.py occasionally fails on Travis. I believe this is due to
  a a race condition when reconnecting to bitcoind after a subtest that
  expects disconnection. If the test runs ahead and sends the INV for the
  subsequent test before we've received the initial sync getheaders, then
  we may end up sending two headers messages - one as a response to the
  initial sync getheaders and one in response to the INV getheaders. If
  both of those headers fail validation with a DoS score of 50 or higher,
  then we'll unexpectedly be disconnected.

  There is only one validation failure that has a DoS score bewteen 50 and
  100, which is high-hash. That's why the test is failing immediately
  after the "Reject a block with invalid work" subtest.

  Fix is to wait for the initial getheaders from the peer before we
  start populating our blockstore. That way we won't have any invalid
  headers to respond to it with.

Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f
TheArbitrator pushed a commit to TheArbitrator/dash that referenced this pull request Jun 9, 2021
c1d7420 [tests] Fix feature_block flakiness (John Newbery)

Pull request description:

  feature_block.py occasionally fails on Travis. I believe this is due to
  a a race condition when reconnecting to bitcoind after a subtest that
  expects disconnection. If the test runs ahead and sends the INV for the
  subsequent test before we've received the initial sync getheaders, then
  we may end up sending two headers messages - one as a response to the
  initial sync getheaders and one in response to the INV getheaders. If
  both of those headers fail validation with a DoS score of 50 or higher,
  then we'll unexpectedly be disconnected.

  There is only one validation failure that has a DoS score bewteen 50 and
  100, which is high-hash. That's why the test is failing immediately
  after the "Reject a block with invalid work" subtest.

  Fix is to wait for the initial getheaders from the peer before we
  start populating our blockstore. That way we won't have any invalid
  headers to respond to it with.

Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f
TheArbitrator pushed a commit to TheArbitrator/dash that referenced this pull request Jun 21, 2021
c1d7420 [tests] Fix feature_block flakiness (John Newbery)

Pull request description:

  feature_block.py occasionally fails on Travis. I believe this is due to
  a a race condition when reconnecting to bitcoind after a subtest that
  expects disconnection. If the test runs ahead and sends the INV for the
  subsequent test before we've received the initial sync getheaders, then
  we may end up sending two headers messages - one as a response to the
  initial sync getheaders and one in response to the INV getheaders. If
  both of those headers fail validation with a DoS score of 50 or higher,
  then we'll unexpectedly be disconnected.

  There is only one validation failure that has a DoS score bewteen 50 and
  100, which is high-hash. That's why the test is failing immediately
  after the "Reject a block with invalid work" subtest.

  Fix is to wait for the initial getheaders from the peer before we
  start populating our blockstore. That way we won't have any invalid
  headers to respond to it with.

Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 30, 2021
…nTestFramework (dashpay#3277)

* [tests] Change feature_block.py to use BitcoinTestFramework

* [tests] Fix flake8 warnings in feature_block.py

* [tests] Tidy up feature_block.py

- move all helper methods to the end
- remove block, create_tx and create_and_sign_tx shortcuts
- remove --runbarelyexpensive option, since it defaults to True and it's
unlikely that anyone ever runs the test with this option set to false.

* [tests] Add logging to feature_block.py

* [tests] Improve assert message when wait_until() fails

* Merge bitcoin#13048: [tests] Fix feature_block flakiness

c1d7420 [tests] Fix feature_block flakiness (John Newbery)

Pull request description:

  feature_block.py occasionally fails on Travis. I believe this is due to
  a a race condition when reconnecting to bitcoind after a subtest that
  expects disconnection. If the test runs ahead and sends the INV for the
  subsequent test before we've received the initial sync getheaders, then
  we may end up sending two headers messages - one as a response to the
  initial sync getheaders and one in response to the INV getheaders. If
  both of those headers fail validation with a DoS score of 50 or higher,
  then we'll unexpectedly be disconnected.

  There is only one validation failure that has a DoS score bewteen 50 and
  100, which is high-hash. That's why the test is failing immediately
  after the "Reject a block with invalid work" subtest.

  Fix is to wait for the initial getheaders from the peer before we
  start populating our blockstore. That way we won't have any invalid
  headers to respond to it with.

Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f

* Temporarely rename MAX_BLOCK_SIZE -> MAX_BLOCK_BASE_SIZE

We'll undo this after the next commit. This avoids merge many conflicts and
makes reviewing easier.

* Rename MAX_BLOCK_BASE_SIZE back to MAX_BLOCK_SIZE

* Use DoS score of 100 for bad-blk-sigops

This was accidently changed to 10 while backporting bitcoin#7287 and causes
test failures in p2p-fullblocktest.py

* Use allowOptimisticSend=true when sending reject messages

This fixes test failures in p2p-fullblocktest.py which expects reject
messages to be sent/received before connections get closed.

* Fix p2p-fullblocktest.py

- CBlock and friends are still in test_framework.mininode
- "-whitelist" causes connections to not be dropped, which in turn causes
  sync_blocks with reconnect=True to fail
- "bad-cb-amount" does not cause a ban in Dash, so reconnect must be False
- Dash already bans when a header is received which is a child of an invalid
  header, causing block requests to never happen

* Backport missing changes from bitcoin#13003

bitcoin#13003 was backported out of order which causes missed changes.

* Bump p2p-fullblocktest timeouts

* Increase RPC timeout in p2p-fullblocktest.py

Co-authored-by: John Newbery <[email protected]>
Co-authored-by: MarcoFalke <[email protected]>
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants