Send tx relay flag with version message #2763

bitcoind parses but does not send tx relay flag in the version message, see #2534

I believe the correct value to send is true, since bitcoind does not expect filter sets and wants to receive inv right after start.

ACK code change. Administrivia:

1. git commit message should include text such as "fixes #2534" or similar. Your pull request text included information that should be archived permanently inside the commit message. In general, commit messages should be more informative than this.

2. We are currently deployed with a buggy BIP 37 interpretation. We should consider whether it warrants bumping the protocol version, along with this patch. Otherwise, you have two different behaviors in the field, with the same protocol version.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c87f462b74fe94ddf7ac1018c399bb9772776e16 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

It's not really a protocol change, is it? It's just a change in the reference client implementation.

It is a bug fix but might answer the question if protocol > reference implementation :)

All fields in the version message except for the first ones are optional - clients need to be prepared for both missing fields or more fields than they know, as this is exchanged before the protocol version is negotiated. Because of that, it cannot be a protocol change, and it's not a bug fix either.

I'm in favor of this however, it makes behaviour more consistent across clients.

I doubt series of fields can be optional since they are not identified by the serialization. How to parse then if we add more?

LGTM

It's in the code - you don't have to add the last field, but if you do, you have to add all the preceding fields too.

Sipa is right, we should merge this, although it's a fairly minor thing.

This is how the reference client parses it: https://github.com/bitcoin/bitcoin/blob/v0.8.2/src/main.cpp#L3201

Essentially by checking whether we're at the end of the message already. And yes it must be optional, as you're parsing/sending this before you know which version the peer supports.

Since the version number is preceding this in the message, optionality is not mandatory but chosen. I think it would be more future proof and flexible to decide on the structure by version read and not by checking the length. I believe the current way of parsing is ugly, but agree is sustainable.

The version isn't acked by the peer yet, so you don't know whether they'll support it. But indeed that's just a problem in one direction, not both. And yes, it's ugly. For more complex extensions later a separate message may be more appropriate...

ACK

This has two ACKs so should be mergeable now.

IRC user reports this change causes a remote node (ver 32200) over Tor to drop them (socket error 104, connection reset by peer). Reverting this change fixes the issue.

Ver 32200 has been/will be hard forked off, so I'm not sure why we should care?

Yes, that is true, but because of a reason completely unrelated to the P2P protocol, so we should at least find out why this results in not accepting these version messages - who knows which other versions are involved?

It is wise to investigate, it however would mean that the flag would not be optional but must be absent to work.

Or there is just some broken alternative implementation sending that version string which is intolerant of the flag.

That sounds like a more reasonable explanation.