Tests: Fix deserialization of reject messages #7912

pull sdaftuar wants to merge 1 commits into bitcoin:master from sdaftuar:fix-mininode-reject changing 1 files +5 −2

sdaftuar commented at 8:36 PM on April 19, 2016: member

Assume that reject messages for blocks or transactions due to reason REJECT_MALFORMED will not include the hash of the block or tx being rejected.

I found BIP 61 to be slightly ambiguous on how to deserialize in this case, but it seems logical to assume that the peer would have bailed out on parsing the message and thus wouldn't know what hash to include. This is also consistent with the behavior of bitcoin core. @MarcoFalke Should these strings "block" and "tx" have the b prepended to them as well? I wasn't sure if leaving them out of #7853 was intentional or an oversight.
laanwj added the label Tests on Apr 20, 2016

Tests: Fix deserialization of reject messages

Assume that reject messages for blocks or transactions due to reason
REJECT_MALFORMED will not include the hash of the block or tx being rejected.

807fa47a1e

in qa/rpc-tests/test_framework/mininode.py:None in 5f3ea9bbb0 outdated
```
1004 |          r = ser_string(self.message)
1005 |          r += struct.pack("<B", self.code)
1006 |          r += ser_string(self.reason)
1007 | -        if (self.message == "block" or self.message == "tx"):
1008 | +        if (self.code != self.REJECT_MALFORMED and
1009 | +                (self.message == "block" or self.message == "tx")):
```
MarcoFalke commented at 10:35 AM on April 20, 2016:

Jup, please the b"" here.

Currently they are included in #7814 but it also makes sense to add them here.

laanwj commented at 11:18 AM on April 20, 2016:

right: rationale is that message ids are byte sequences directly from the packet, not strings for user consumption, so they should be bytes not str
MarcoFalke commented at 11:50 AM on April 20, 2016:

If you feel strongly about adding them in the other places as well:

msg_reject: self.reason = b""

and (optionally)

def ser_uint256(u): def ser_string_vector(l): CAlert().serialize(self):
laanwj commented at 12:07 PM on April 20, 2016:

Well it is important to avoid unexpected results. b'123'=='123' evaluates to False in Python3.

MarcoFalke commented at 12:13 PM on April 20, 2016:

I have all of this included in my py3 branch, so it is not mandatory to fix all issues right now. (Otherwise I'd be creating pulls every other day to keep the whole py2 codebase "updated") E.g. we got new test in fundrawtransaction.py which are using the old except JSONRPCException,e: syntax but it is not worth to create a pull for those right now, imo.

sdaftuar commented at 1:23 PM on April 20, 2016:

Thanks, will just add the b to the lines that I'm changing here.
sdaftuar force-pushed on Apr 20, 2016
sdaftuar commented at 1:30 PM on April 20, 2016: member

Updated with bytes prefix.
laanwj commented at 2:10 PM on April 20, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/7912/commits/807fa47a1e5c9f072d7dbf549bf17f66c47dbf46
MarcoFalke commented at 3:16 PM on April 20, 2016: member

utACK 807fa47
MarcoFalke merged this on Apr 21, 2016
MarcoFalke closed this on Apr 21, 2016
MarcoFalke referenced this in commit 59ad56851a on Apr 21, 2016
MarcoFalke referenced this in commit 7a8348994b on Jun 9, 2016
nomnombtc referenced this in commit 1c293fd2ef on Nov 12, 2016
nomnombtc referenced this in commit 026454e7ad on Nov 12, 2016
nomnombtc referenced this in commit f270b1e3ea on Nov 13, 2016
sickpig referenced this in commit 5613dea189 on Nov 14, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7853 [qa] py2: Unfiddle strings into bytes explicitly