test: fix intermittent error in getblockfrompeer.py #27784

1. 
   mzumsande commented at 6:32 pm on May 30, 2023: contributor

   This adds an additional sync_blocks call, fixing an intermittent error caused by blocks arriving out of order due to how compact block relay may revert to headers processing when the tip hasn’t caught up, and resulting in slightly different pruning behavior. Making sure that all blocks from the previous tests are synced before generating more blocks makes this impossible.

   See #27749 (comment) and #27749 (comment) for a more detailed analysis.

   #27770 is a more long-term approach to avoid having to deal with magic pruneheight numbers in the first place, but that PR introduces a new RPC and needs more discussion.

   Fixes #27749.

2. test: add block sync to getblockfrompeer.py

   This fixes an intermittent error, caused by blocks arriving
   out of order due to how compact block relay may revert to headers
   processing when the tip hasn't caught up, and resulting in slightly
   different pruning behavior.
   
   Making sure that all blocks from the previous tests are synced before
   generating more blocks makes this impossible.
   See Issue #27749 for more details.

   9fe9074266
3. 
   DrahtBot commented at 6:32 pm on May 30, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	theStack, MarcoFalke

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Tests on May 30, 2023
5. DrahtBot added the label CI failed on May 30, 2023
6. theStack approved
7. 
   theStack commented at 7:57 pm on May 30, 2023: contributor

   ACK 9fe9074266c6d7ddb40384d81741df1fc94980ff

   Unfortunately there seems to be no reliable way to reproduce the issue on master (which could then be also ran on this PR to ensure that it is indeed fixed), but the explanation in #27749 (comment) makes sense to me.

8. 
   Sjors commented at 8:08 pm on May 30, 2023: member

   9fe9074266c6d7ddb40384d81741df1fc94980ff looks harmless, but I haven’t delved into the underlying issue. Can the original be reproduced by running the test in a loop??

   CI failure seems spurious.

9. 
   mzumsande commented at 8:31 pm on May 30, 2023: contributor

   Can the original be reproduced by running the test in a loop??

   I haven’t been able to reproduce it, I think it requires a constellation with a very special timing which should make it very rare even in the CI. One thing I did is check in the log is that node2 doesn’t use HeadersDirectFetchBlocks for any blocks > 205, and uses compact block reconstruction instead. This is different in the log of the failed run.

10. 
    theStack commented at 8:37 pm on May 30, 2023: contributor

    9fe9074 looks harmless, but I haven’t delved into the underlying issue. Can the original be reproduced by running the test in a loop??

    If you want to give that approach a try, the following shell script (which I have been running for days already, without any issue) should do exactly that:

    0#!/usr/bin/env bash
    1set -e
    2while true; do ./test/functional/rpc_getblockfrompeer.py; done
    

11. 
    maflcko commented at 6:00 am on May 31, 2023: member

    lgtm ACK 9fe9074266c6d7ddb40384d81741df1fc94980ff
12. fanquake merged this on May 31, 2023
13. fanquake closed this on May 31, 2023

    ---

14. mzumsande deleted the branch on May 31, 2023
15. sidhujag referenced this in commit 7704bfe509 on May 31, 2023
16. bitcoin locked this on May 30, 2024

Contributors
mzumsande DrahtBot theStack Sjors maflcko

Labels
Tests CI failed