test: check for specific bip157 disconnect reasons, add test coverage #28227

1. 
   theStack commented at 5:55 PM on August 6, 2023: contributor

   This PR checks for specific disconnect reasons using assert_debug_log in the functional test p2p_blockfilters.py. With that we ensure that the disconnect happens for the expected reason and also makes it easier to navigate between implementation and test code, i.e. both the questions "do we have test coverage for this disconnect cause?" (from an implementation reader's perspective) and "where is the code handling this disconnect cause?" (from a test reader's perspective) can be answered simply by grep-ping the corresponding debug message.

   Also, based on that, missing coverage for the (start height > stop height) disconnect case is added: https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/src/net_processing.cpp#L3050-L3056

2. 
   DrahtBot commented at 5:55 PM on August 6, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	furszy, MarcoFalke

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

3. DrahtBot added the label Tests on Aug 6, 2023
4. in test/functional/p2p_blockfilters.py:216 in a8b4ab6949 outdated

    210 | @@ -211,37 +211,55 @@ def run_test(self):
    211 |          ]
    212 |          for request in requests:
    213 |              peer_1 = self.nodes[1].add_p2p_connection(P2PInterface())
    214 | -            peer_1.send_message(request)
    215 | +            with self.nodes[1].assert_debug_log(expected_msgs=["requested unsupported block filter type"]):
    216 | +                peer_1.send_message(request)
    217 |              peer_1.wait_for_disconnect()
   

   ---

   ---

   

   maflcko commented at 6:37 AM on August 7, 2023:

                   peer_1.wait_for_disconnect()
   

   Generally I am not a fan of using assert_debug_log to sync. Especially when it is trivial to avoid by adding 4 spaces

   ---

   

   theStack commented at 10:21 AM on August 7, 2023:

   Thanks, fixed.

5. in test/functional/p2p_blockfilters.py:263 in a8b4ab6949 outdated

    275 | +        for request, expected_log_msg in requests:
    276 |              peer_0 = self.nodes[0].add_p2p_connection(P2PInterface())
    277 | -            peer_0.send_message(request)
    278 | +            with self.nodes[0].assert_debug_log(expected_msgs=[expected_log_msg]):
    279 | +                peer_0.send_message(request)
    280 |              peer_0.wait_for_disconnect()
   

   ---

   ---

   

   maflcko commented at 6:38 AM on August 7, 2023:

   same

6. maflcko approved
7. test: check for specific disconnect reasons in p2p_blockfilters.py

   This ensures that the disconnect happens for the expected reason and
   also makes it easier to navigate between implementation and test code,
   i.e. both the questions "do we have test coverage for this disconnect?"
   (from an implementation reader's perspective) and "where is the code
   handling this disconnect?" (from a test reader's perspective) can be
   answered simply by grep-ping the corresponding debug message.
   
   Can be easiest reviewed with `-w` (to ignore whitespace changes).

   63e90e1d3f
8. test: add bip157 coverage for (start height > stop height) disconnect 2ab7952bda
9. theStack force-pushed on Aug 7, 2023
10. DrahtBot added the label CI failed on Sep 4, 2023
11. DrahtBot removed the label CI failed on Sep 5, 2023
12. achow101 requested review from furszy on Sep 20, 2023
13. achow101 requested review from josibake on Sep 20, 2023
14. furszy approved
15. 
    furszy commented at 7:47 PM on September 27, 2023: member

    Looks good, code ACK 2ab7952b

16. 
    maflcko commented at 8:30 AM on September 28, 2023: member

    lgtm ACK 2ab7952bda8d15e91b03f8307839030cbb55614e

17. fanquake merged this on Oct 2, 2023
18. fanquake closed this on Oct 2, 2023

    ---

19. theStack deleted the branch on Oct 2, 2023
20. Frank-GER referenced this in commit d07073d5fc on Oct 5, 2023
21. bitcoin locked this on Oct 1, 2024

Contributors

theStackDrahtBotmaflckofurszy

Review Requested
josibake