qa: Avoid race in p2p_invalid_block by waiting for the block request #14700

1. 
   MarcoFalke commented at 8:53 PM on November 9, 2018: member

   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.

2. tests: Make feature_block pass on centos 6c787d340c
3. qa: Avoid race in p2p_invalid_block by waiting for the block request fa21568208
4. MarcoFalke added the label Tests on Nov 9, 2018
5. MarcoFalke added this to the milestone 0.17.1 on Nov 9, 2018
6. MarcoFalke added the label Needs backport on Nov 9, 2018
7. 
   lucash-dev commented at 12:12 AM on November 10, 2018: contributor

   utACK fa21568

8. 
   lucash-dev commented at 12:16 AM on November 10, 2018: contributor

   I'm curious, have you been able to reproduce the issue locally? I've ran the test 1,000+ times and couldn't.

9. zsoli18 approved
10. 
    sdaftuar commented at 6:49 PM on November 12, 2018: member

    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 send_blocks_and_test when request_block=False is to not send the block at all, rather than force delivery of the block whether or not the peer requested it. Anyway I think we should just get rid of the request_block parameter altogether and test what happens if the block were delivered outright (which is the case that I think we care about in all the places we currently pass in False); ideally we'd do that both via p2p and via RPC's submitblock.

    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.

11. 
    DrahtBot commented at 3:22 PM on November 13, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #14696 (qa: Add explicit references to related CVE's in p2p_invalid_block test. by lucash-dev)

    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.

12. MarcoFalke merged this on Nov 13, 2018
13. MarcoFalke closed this on Nov 13, 2018

    ---

14. MarcoFalke referenced this in commit 5d605b2745 on Nov 13, 2018
15. MarcoFalke deleted the branch on Nov 13, 2018
16. fanquake referenced this in commit f9db08e8ca on Nov 30, 2018
17. 
    fanquake commented at 2:45 PM on November 30, 2018: member

    fa21568 has been backported in #14835. 6c787d3 shouldn't need a backport to 0.17.

18. fanquake removed the label Needs backport on Nov 30, 2018
19. luke-jr referenced this in commit d1242767d5 on Dec 21, 2018
20. deadalnix referenced this in commit aea5b41c25 on Jun 13, 2020
21. knst referenced this in commit d2cf77542d on Aug 10, 2021
22. knst referenced this in commit f0b99c3a87 on Aug 10, 2021
23. knst referenced this in commit abd05eb8d8 on Aug 16, 2021
24. PastaPastaPasta referenced this in commit 5bda9afcf6 on Aug 23, 2021
25. UdjinM6 referenced this in commit f38aa44bfd on Aug 29, 2021
26. DrahtBot locked this on Sep 8, 2021

Milestone
0.17.1