test: Fix Version Message Deserialization Discrepancy

troygiorshev commented at 1:01 am on November 18, 2020: contributor

Inspired by theStack’s in Per-Peer Message Capture here. More context here and relevant BIPs are BIP37 and BIP60 (draft).

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 can 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 PR) our testing framework would deserialize this into a version message with Version=60000 and fRelay=0.

Concept ACK needed. Alternative: Change behavior in core? Got a few 😄

fanquake added the label Tests on Nov 18, 2020

sipa commented at 2:28 am on November 18, 2020: member

Concept ACK

in test/functional/test_framework/messages.py:1048 in 0a7eedc35a outdated

1057-            except:
1058-                self.nRelay = 0
1059-        else:
1060+        # Relay field is optional for version 70001 onwards
1061+        # But, unconditionally check it to match behaviour in bitcoind
1062+        try:

MarcoFalke commented at 6:18 am on November 18, 2020:

Looks like you still treat it as optional, no?

EDIT: Oh, nvm. You want it this way.

MarcoFalke commented at 7:01 am on November 23, 2020:

On a second thought, I don’t think this generally works the way you want it to work. (See my latest comment on your other pull request: #19509 (comment) )

So I’ve unresolved this conversation again.

MarcoFalke renamed this:
~~Fix Version Message Deserialization Discrepancy~~
test: Fix Version Message Deserialization Discrepancy
on Nov 18, 2020

MarcoFalke commented at 7:19 am on November 18, 2020: member

review ACK 0a7eedc35afd5c73d5ab7d2b5b305f8c795e5b42

theStack approved

theStack commented at 0:32 am on November 19, 2020: member

Code Review ACK 0a7eedc35afd5c73d5ab7d2b5b305f8c795e5b42 Thanks for fixing this!

in test/functional/test_framework/messages.py:1052 in 0a7eedc35a outdated

1059-        else:
1060+        # Relay field is optional for version 70001 onwards
1061+        # But, unconditionally check it to match behaviour in bitcoind
1062+        try:
1063+            self.nRelay = struct.unpack("<b", f.read(1))[0]
1064+        except:

laanwj commented at 10:54 am on November 22, 2020:

Please don’t use a blanket except here, but catch the specific exception struct.error. This could potentially mask another error, even “KeyboardInterrupt” (ctrl-C).

MarcoFalke commented at 7:01 am on November 23, 2020:

Would probably be best to remove the except wholesale. https://github.com/bitcoin/bitcoin/pull/20411/files#r528499393

jnewbery commented at 10:03 am on November 27, 2020:

This isn’t new. It’s simply moving the try-except out of an if block, but there’s no harm in fixing it up in this PR.

troygiorshev force-pushed on Nov 25, 2020

troygiorshev commented at 7:07 am on November 25, 2020: contributor

git range-diff master 0a7eedc 68500bb

Trivial rebase since 19509 needs a rebase and will be based off of this PR.

jnewbery commented at 10:03 am on November 27, 2020: member

ACK 68500bb48b63841591fae475e0b4760c021a6af6

This is actually more spec compliant. BIP 37, which introduced the fRelay field to the version message, does not specify a minimum p2p version: https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki.

I’m happy to reACK a version of this that narrows the exception catching to just struct.error exceptions.

[test] Move MIN_VERSION_SUPPORTED to p2p.py

The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MIN_VERSION_SUPPORTED to p2p.py.

Also rename to MIN_P2P_VERSION_SUPPORTED to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.

d50d7cdd52

[test] Move MY_VERSION to p2p.py

The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_VERSION to p2p.py.

Also rename to P2P_VERSION to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.

Also always set the nVersion field in CBlockLocator to 0 and ignore the
field in deserialized messages. The field is not currently used for
anything in Bitcoin Core.

e616c7cb26

[test] Move MY_SUBVERSION to p2p.py

The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_SUBVERSION to p2p.py.

Also rename to P2P_SUBVERSION.

e7ce7f1d13

[test] Move MY_RELAY to p2p.py

messages.py is for message and primitive data structures. Specifics
about the test framework's p2p implementation should be in p2p.py.

Also rename to P2P_VERSION_RELAY. Also rename msg_version.nRelay to
relay. In Bitcoin Core, this is referred to as fRelay, since it's a
bool, so this field has always been misnamed.

301ea8bc0d

[test] Add P2P_SERVICES to p2p.py

The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore specify the nServices value in the calling code,
not in the messages.py module.

e808800ca1

[test] Change P2P_SUBVERSION to be a string

This forces the user to convert to bytes to encode it in a version
message, which would result in an easy-to-detect type error if omitted.

Also add a check that new connections from the test framework to the
node have the correct user agent string. Again, this makes bugs easier
to detect if the user agent string ever changes.

c2f5a3b386

DrahtBot commented at 2:54 am on November 29, 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:

#20277 (p2p: Stop processing unrequested transactions during IBD and extend p2p_ibd_txrelay.py coverage by ariard)
#20171 (Add functional test test_txid_inv_delay by ariard)
#20079 (p2p: Treat handshake misbehavior like unknown message by MarcoFalke)
#19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)

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.

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.

95d8a3275f

troygiorshev force-pushed on Dec 2, 2020

troygiorshev commented at 2:54 pm on December 2, 2020: contributor

Fixed the error handling here, sorta. The error wouldn’t be in the struct unpacking as suggested, it would be in the file reading. (The try/catch is expecting the last byte to only optionally exist) However, it’s not clear to me that f.read() fails with the same error on all systems. It’s a ValueError in at least some systems, but I’m worried about breaking things, especially with this not being the main point of this PR.

I’m about to rebase this onto 20524 to prevent a merge conflict.

I realize now that this doesn’t completely fix the problems in 19509 that I was originally looking at, but this is still necessary. I’ll make the remaining changes in 19509, so after rebasing this will be ready for review.

troygiorshev force-pushed on Dec 2, 2020

troygiorshev commented at 2:57 pm on December 2, 2020: contributor

Rebased onto #20524.

This is ready for re-review.

DrahtBot added the label Needs rebase on Dec 12, 2020

DrahtBot commented at 1:02 pm on December 12, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

jnewbery commented at 9:32 am on January 25, 2021: member

@troygiorshev are you planning to rebase this, or should we mark it as up for grabs?

jnewbery commented at 10:11 am on February 20, 2021: member

Marking this as up for grabs

jnewbery closed this on Feb 20, 2021

jnewbery added the label Up for grabs on Feb 20, 2021

jnewbery commented at 10:13 am on February 20, 2021: member

The first 6 commits in this PR were merged in #20524, so it’s only the final Unconditionally check for fRelay field in test framework commit that would need to be included in a new PR.

fanquake removed the label Needs rebase on Mar 4, 2021

fanquake removed the label Up for grabs on Mar 4, 2021

MarcoFalke referenced this in commit ed49203daa on Mar 24, 2021

sidhujag referenced this in commit 4fca6d84b4 on Mar 24, 2021

DrahtBot locked this on Aug 16, 2022

test: Fix Version Message Deserialization Discrepancy #20411

Conflicts