test: Unconditionally check for fRelay field in test framework #21357

pull jarolrod wants to merge 1 commits into bitcoin:master from jarolrod:test-framework-ver-msg-fix changing 1 files +5 −7

jarolrod commented at 6:52 AM on March 4, 2021: member

picking up #20411 (rebased onto master)

There is a discrepancy in the implementation of our p2p protocol between bitcoind and the testing framework. The fRelay field is an optional field at the end of a version message as of protocol version 70001. However, when deserializing a message in bitcoind, we don't check the version to see if it should have an fRelay field or not. Instead, we unconditionally attempt to deserialize into the field.

This commit brings the testing framework in line with the implementation in core.

This matters for a version message with the following fields:

Version = 60000 fRelay = 1

Bitcoind would deserialize this into a version message with Version=60000 and fRelay=1, whereas (before this commit) our testing framework would deserialize this into a version message with Version=60000 and fRelay=0.
fanquake added the label Tests on Mar 4, 2021
fanquake requested review from jnewbery on Mar 4, 2021
in test/functional/test_framework/messages.py:1054 in bfc4333a50 outdated
```
1056 | +        # But, unconditionally check it to match behaviour in bitcoind
1057 | +        try:
1058 | +            self.relay = struct.unpack("<b", f.read(1))[0]
1059 | +        except KeyboardInterrupt:
1060 | +            raise KeyboardInterrupt
1061 | +        except:
```
jnewbery commented at 9:27 AM on March 4, 2021:

I think better would be to just catch struct.error:

except struct.error: self.relay = 0

and allow all other exceptions to raise.
jarolrod commented at 5:57 PM on March 5, 2021:

addressed in 5c46992
jnewbery commented at 12:51 PM on March 10, 2021:

You can remove:

except KeyboardInterrupt: raise KeyboardInterrupt

since there's no blanket except catching, this is doing nothing (KeyboardInterrupt exceptions would be raised anyway)
jnewbery commented at 9:28 AM on March 4, 2021: member

Concept ACK. This seems reasonable. fRelay was added with p2p version 70001, but bitcoind has never checked that:

https://github.com/bitcoin/bitcoin/pull/1795/files#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R2972

I guess that other implementations adopted fRelay without bumping their version number.
jarolrod force-pushed on Mar 5, 2021
jarolrod commented at 5:57 PM on March 5, 2021: member
Updated from bfc4333 -> 5c46992, addressed @jnewbery suggestions

Changes:
- catch specific struct.error instead of blanket except

Unconditionally check for fRelay field in test framework

There is a discrepancy in the implementation of our p2p protocol between
bitcoind and the testing framework.  The fRelay field is an optional
field at the end of a version message as of protocol version 70001.
However, when deserializing a message in bitcoind, we don't check the
version to see if it should have an fRelay field or not.  Instead we
unconditionally attempt to deserialize into the field.

This commit brings the testing framework in line with the implementation
in core.

This matters for a version message with the following fields:

Version = 60000
fRelay = 1

Bitcoind would deserialize this into a version message with
Version=60000 and fRelay=1, whereas (before this commit) our testing
framework would deserialize this into a version message with
Version=60000 and fRelay=0.

39a9ec579f

jarolrod force-pushed on Mar 24, 2021
jarolrod commented at 12:05 AM on March 24, 2021: member

Updated from 5c46992 -> 39a9ec5, addressed @jnewbery comment

Changes: I misunderstood the initial suggestion. This removes the unnecessary handling of KeyboardInterrupt
jnewbery commented at 1:00 PM on March 24, 2021: member

utACK 39a9ec579f023ab262a1abd1f0c869be5b1f3f4d
in test/functional/test_framework/messages.py:1050 in 39a9ec579f
```
1052 | -            except:
1053 | -                self.relay = 0
1054 | -        else:
1055 | +        # Relay field is optional for version 70001 onwards
1056 | +        # But, unconditionally check it to match behaviour in bitcoind
1057 | +        try:
```
MarcoFalke commented at 5:56 PM on March 24, 2021:

The goal of the test framework is not to match behavior in bitcoind (we don't want to re-implement bitcoind in python), but to properly deserialize messages that have been serialized by bitcoind.

Thus, the try-catch can be removed completely.

jnewbery commented at 6:09 PM on March 24, 2021:

The deserialization code can be useful in other contexts, eg https://github.com/bitcoin/bitcoin/blob/f67b5dca576669bfed0cf4af9f2cc6273010a0d7/contrib/message-capture/message-capture-parser.py. Besides, there's no disadvantage to making this more robust.
MarcoFalke commented at 5:57 PM on March 24, 2021: member

seems fine, but I think we don't need any try-catch
MarcoFalke merged this on Mar 24, 2021
MarcoFalke closed this on Mar 24, 2021
jarolrod deleted the branch on Mar 24, 2021
sidhujag referenced this in commit 4fca6d84b4 on Mar 24, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#20411 test: Fix Version Message Deserialization Discrepancy