Skip to content

Commit

Permalink
Merge bitcoin#29067: test: Treat msg_version.relay as unsigned, Remov…
Browse files Browse the repository at this point in the history
…e `struct` packing in messages.py

55556a6 test: Remove struct import from messages.py (MarcoFalke)
fa3fa86 scripted-diff: test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fafc0d6 test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fa3886b test: Treat msg_version.relay as unsigned (MarcoFalke)

Pull request description:

  `struct` has many issues in messages.py:

  * For unpacking, it requires to specify the length a second time, even when it is already clear from the `f.read(num_bytes)` context.
  * For unpacking, it is designed to support a long format string and returning a tuple of many values. However, except for 3 instances in `messages.py`, usually only a single value is unpacked and all those cases require an `[0]` access.
  * For packing and unpacking of a single value, the format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.

  I presume the above issues lead to accidentally treat `msg_version.relay` as a "signed bool", which is fine, but confusing.

  Fix all issues by using the built-in `int` helpers `to_bytes` and `from_bytes` via a scripted diff.

  Review notes:

  * `struct.unpack` throws an error if the number of bytes passed is incorrect. `int.from_bytes` doesn't know about "missing" bytes and treats an empty byte array as `int(0)`. "Extraneous" bytes should never happen, because all `read` calls are limited in this file. If it is important to keep this error behavior, a helper `int_from_stream(stream, num_bytes, bytes, byteorder, *, **kwargs)` can be added, which checks the number of bytes read from the stream.
  * For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messages are not identical.

ACKs for top commit:
  stickies-v:
    ACK 55556a6
  theStack:
    re-ACK 55556a6

Tree-SHA512: 1cef8cdfd763fb424ed4b8be850a834b83fd0ef47fbea626a29784eb4f4832d44e42c4fe05b298b6070a933ef278b0222289a9955a97c86707e091e20bbb247a
  • Loading branch information
glozow committed Jan 30, 2024
2 parents 411ba32 + 55556a6 commit 78c06a3
Showing 1 changed file with 107 additions and 111 deletions.
Loading

0 comments on commit 78c06a3

Please sign in to comment.