refactor: test: replace inv type magic numbers by constants #18764

1. 
   theStack commented at 12:17 pm on April 25, 2020: member

   Many functional tests still use magic numbers for inventory types, either passed to the CInv constructor or for comparing the type member of CInv. This PR replaces all of those by constants in the module test_framework.messages that have been introduced in commit c32cf9f62285b5cd18a5064aee91f0802f0f87a8: MSG_TX (1) or MSG_BLOCK (2).

   It also introduces a new constant MSG_CMPCT_BLOCK (naming as in src/protocol.h) and uses it to replace the remaining magic numbers.

   The occurences of the magic numbers were identified through greping for CInv( and type ==. The idea was first to create a scripted-diff, but since also adding missing imports is needed, this would be non-trivial. Besides, also some unneeded comments like # 2 == "Block" could be removed.

2. fanquake added the label Tests on Apr 25, 2020
3. 
   DrahtBot commented at 5:39 pm on April 25, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #19031 (Implement ADDRv2 support (part of BIP155) by vasild)
   • #18838 (test: Check header hash in wait_for_getheaders by Nishikoh)
   • #15197 (Refactor and slightly stricter p2p message processing by jonasschnelli)

   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.

4. 
   practicalswift commented at 4:50 am on April 26, 2020: contributor

   Concept ACK
5. DrahtBot added the label Needs rebase on Apr 29, 2020
6. theStack force-pushed on Apr 30, 2020
7. 
   theStack commented at 1:33 pm on April 30, 2020: member

   Rebased.
8. DrahtBot removed the label Needs rebase on Apr 30, 2020
9. 
   brakmic commented at 10:50 am on May 1, 2020: contributor

   Code review ACK.
10. 
    MarcoFalke commented at 2:21 pm on May 1, 2020: member

    0test/functional/wallet_resendwallettransactions.py:9:1: F401 'test_framework.messages.MSG_TX' imported but unused
    

11. theStack force-pushed on May 1, 2020
12. 
    theStack commented at 8:01 pm on May 1, 2020: member

    0test/functional/wallet_resendwallettransactions.py:9:1: F401 'test_framework.messages.MSG_TX' imported but unused
    

    Oops, that apparently happened in the course of rebasing, resolving a merge conflict. Fixed.

13. in test/functional/p2p_invalid_messages.py:209 in adb0cc4b8b outdated

    205@@ -206,10 +206,10 @@ def test_msgtype(self):
    206     def test_large_inv(self):
    207         conn = self.nodes[0].add_p2p_connection(P2PInterface())
    208         with self.nodes[0].assert_debug_log(['Misbehaving', 'peer=4 (0 -> 20): message inv size() = 50001']):
    209-            msg = messages.msg_inv([messages.CInv(1, 1)] * 50001)
    210+            msg = messages.msg_inv([messages.CInv(messages.MSG_TX, 1)] * 50001)
    

    ---

    ---

    

    glozow commented at 6:08 pm on May 6, 2020:

    Caught my eye because it has a messages. in front… Would it make sense to explicitly list imports at the top of p2p_invalid_messages.py like all the other files do? Just quickly looking for messages in the file, it doesn’t seem like too long of a list:

    from test_framework.messages import ser_string, msg_ping, msg_inv, msg_getdata, msg_headers, CInv, CBlockHeader, MSG_TX

    ---

    

    theStack commented at 12:23 pm on May 7, 2020:

    @gzhao408: Thanks for reviewing! The idea was to keep the changes as small as possible to make reviewing easier, i.e. that on every line touched, only the proclaimed magic number elemenations happen and nothing more. Looking at p2p_invalid_messages.py though, I agree that it could be fit in this PR, as there are not too many changes needed. Will add a commit doing that.
14. 
    glozow commented at 6:14 pm on May 6, 2020: member

    code review ACK, and a small question
15. test: replace inv type magic numbers by constants eeaaa58d2c
16. test: add inventory type constant MSG_CMPCT_BLOCK b35e1d2471
17. test: explicit imports from test_framework.messages in p2p_invalid_messages.py 4a614ff88a
18. theStack force-pushed on May 7, 2020
19. 
    theStack commented at 12:38 pm on May 7, 2020: member

    Rebased on master and added changes suggested by gzhao408 (explicitely importing from test_framework.messages in p2p_invalid_messages.py).
20. 
    glozow commented at 7:02 pm on May 9, 2020: member

    ACK 4a614ff
21. MarcoFalke merged this on May 20, 2020
22. MarcoFalke closed this on May 20, 2020

    ---

23. sidhujag referenced this in commit cba2726f67 on May 20, 2020
24. theStack deleted the branch on Dec 1, 2020
25. Fabcien referenced this in commit db264f0835 on Feb 3, 2021
26. Fabcien referenced this in commit 6791b7f377 on Feb 3, 2021
27. deadalnix referenced this in commit 3bf72f599d on Feb 3, 2021
28. DrahtBot locked this on Feb 15, 2022

Contributors
theStack DrahtBot practicalswift brakmic MarcoFalke glozow

Labels
Tests