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

theStack commented at 12:17 PM on April 25, 2020: contributor

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.

fanquake added the label Tests on Apr 25, 2020

DrahtBot commented at 5:39 PM on April 25, 2020: contributor

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.

DrahtBot cross-referenced this on Apr 25, 2020 from issue test: Add test for wtxid transaction relay by fjahr

DrahtBot cross-referenced this on Apr 25, 2020 from issue P2P: Mempool tracks locally submitted transactions to improve wallet privacy by amitiuttarwar

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

Concept ACK

DrahtBot cross-referenced this on Apr 29, 2020 from issue [doc / test / mempool] unbroadcast follow-ups by amitiuttarwar

DrahtBot added the label Needs rebase on Apr 29, 2020

theStack force-pushed on Apr 30, 2020

theStack commented at 1:33 PM on April 30, 2020: contributor

Rebased.

DrahtBot removed the label Needs rebase on Apr 30, 2020

DrahtBot cross-referenced this on May 1, 2020 from issue test: Check header hash in wait_for_getheaders by Nishikoh

brakmic commented at 10:50 AM on May 1, 2020: contributor

Code review ACK.

MarcoFalke commented at 2:21 PM on May 1, 2020: member

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

theStack force-pushed on May 1, 2020

theStack commented at 8:01 PM on May 1, 2020: contributor

test/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.

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.

glozow commented at 6:14 PM on May 6, 2020: member

code review ACK, and a small question

test: replace inv type magic numbers by constants eeaaa58d2c

test: add inventory type constant MSG_CMPCT_BLOCK b35e1d2471

test: explicit imports from test_framework.messages in p2p_invalid_messages.py 4a614ff88a

theStack force-pushed on May 7, 2020

theStack commented at 12:38 PM on May 7, 2020: contributor

Rebased on master and added changes suggested by gzhao408 (explicitely importing from test_framework.messages in p2p_invalid_messages.py).

DrahtBot cross-referenced this on May 8, 2020 from issue Refactor and slightly stricter p2p message processing by jonasschnelli

glozow commented at 7:02 PM on May 9, 2020: member

ACK 4a614ff

DrahtBot cross-referenced this on May 20, 2020 from issue Implement ADDRv2 support (part of BIP155) by vasild

MarcoFalke merged this on May 20, 2020

MarcoFalke closed this on May 20, 2020

sidhujag referenced this in commit cba2726f67 on May 20, 2020

theStack deleted the branch on Dec 1, 2020

Fabcien referenced this in commit db264f0835 on Feb 3, 2021

Fabcien referenced this in commit 6791b7f377 on Feb 3, 2021

deadalnix referenced this in commit 3bf72f599d on Feb 3, 2021

bitcoin locked this on Feb 15, 2022