test: Check msg type in msg capture is followed by zeros #25117

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2205-test-zero-🚛 changing 1 files +1 −5

MarcoFalke commented at 2:16 PM on May 12, 2022: member

Checking that they are not printable is an odd (and wrong) way to check that all chars are zero.
MarcoFalke added the label Tests on May 12, 2022
MarcoFalke commented at 2:16 PM on May 12, 2022: member

See https://github.com/bitcoin/bitcoin/issues/21079

in test/functional/p2p_message_capture.py:50 in fa395dffe3 outdated

  48 | -            remainder =  raw_msgtype.split(b'\x00', 1)[1]
  49 | +            msgtype, remainder = raw_msgtype.split(b'\x00', 1)
  50 |              assert(len(msgtype) > 0)
  51 |              assert(msgtype in MESSAGEMAP)
  52 | -            assert(len(remainder) == 0 or not remainder.decode().isprintable())
  53 | +            assert all(r == 0 for r in remainder)

laanwj commented at 2:44 PM on May 12, 2022:

Huh, yes, this is weird code. Agree the new formulation is better.

theStack commented at 2:48 PM on May 12, 2022: member
Concept ACK

Note that the .split() assignment fails if the raw_msgtype doesn't contain any zeroes:
```
>>> msgtype, remainder = bytes.fromhex("aabbcc").split(b'\x00', 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: not enough values to unpack (expected 2, got 1)
```
This problem is not introduced by this PR though; in the master branch it would occur as well:
```
>>> msgtype = bytes.fromhex("aabbcc").split(b'\x00', 1)[0]
>>> remainder = bytes.fromhex("aabbcc").split(b'\x00', 1)[1]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: list index out of range
```
I'm confused why this hasn't caused any problems. Whenever a msg-capture file is parsed that contains a message type that consumes the full 12 bytes (i.e. without zero), this should throw. This applies e.g. for ~sendaddrv2~, getcfheaders, or getcfcheckpt.
laanwj commented at 2:51 PM on May 12, 2022: member

Note that the .split() assignment fails if the raw_msgtype doesn't contain any zeroes:

Good point! May be better to use bytes.partition instead.
test: Check msg type in msg capture is followed by zeros faa5a7a573
MarcoFalke force-pushed on May 12, 2022
MarcoFalke commented at 3:08 PM on May 12, 2022: member

Thx, used rstrip + map lookup to check for trailing zeros
theStack approved
theStack commented at 3:27 PM on May 12, 2022: member

Oh nice, using rstrip is even simpler!

Code-review ACK faa5a7a5735b054b5fd2dca6ef6fca2fad6edbde
MarcoFalke merged this on May 13, 2022
MarcoFalke closed this on May 13, 2022
MarcoFalke deleted the branch on May 13, 2022
sidhujag referenced this in commit 237448ba7f on May 13, 2022
DrahtBot locked this on May 13, 2023

Contributors

Labels