Skip to content

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Oct 5, 2016

In protocol version 70001, an additional field fRelay was added to the version message (see https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages ).

Mininode does not currently include the fRelay field in its version messages, and so is technically sending malformed version messages. This PR fixes mininode's version messages.

@fanquake fanquake added the Tests label Oct 5, 2016
@maflcko
Copy link
Member

maflcko commented Oct 6, 2016

technically sending malformed version message

Are you sure that the field is required? The BIP implies that the field is optional:

If missing or true, no change in protocol behaviour occurs.

@jnewbery
Copy link
Contributor Author

jnewbery commented Oct 6, 2016

@MarcoFalke Yes - you're correct. I didn't read the BIP closely enough.

Still, I think mininode should specify the fRelay field. Even better: we should have a bloom filter test in qa which explicitly tests the field.

@laanwj
Copy link
Member

laanwj commented Oct 8, 2016

It makes sense for the test framework to support this.
utACK d7bf55c

@maflcko
Copy link
Member

maflcko commented Oct 8, 2016

Even better: we should have a bloom filter test in qa which explicitly tests the field.

Sure.

Please also make sure to deserialize nRelay and mention it in the repr.

@jonasschnelli
Copy link
Contributor

Agree with @MarcoFalke, you should extend the change to the deserialization method.

@jnewbery jnewbery changed the title [Testing] Include fRelay in mininode version messages [WIP] [Testing] Include fRelay in mininode version messages Oct 11, 2016
@maflcko
Copy link
Member

maflcko commented Nov 7, 2016

@jnewbery Are you still working on this?

@jnewbery jnewbery force-pushed the mininode-relay-field branch 3 times, most recently from ce364a1 to 6a4ed9d Compare November 9, 2016 02:33
@jnewbery jnewbery force-pushed the mininode-relay-field branch from 6a4ed9d to e5d682f Compare November 9, 2016 02:34
@jnewbery
Copy link
Contributor Author

jnewbery commented Nov 9, 2016

@MarcoFalke : deserialization and repr methods updated.

@maflcko maflcko changed the title [WIP] [Testing] Include fRelay in mininode version messages [Testing] Include fRelay in mininode version messages Nov 9, 2016
@maflcko
Copy link
Member

maflcko commented Nov 9, 2016

Thanks, utACK e5d682f

@laanwj laanwj merged commit e5d682f into bitcoin:master Nov 9, 2016
laanwj added a commit that referenced this pull request Nov 9, 2016
e5d682f Fix mininode version message format (jnewbery)
@jnewbery jnewbery deleted the mininode-relay-field branch November 10, 2016 11:15
codablock pushed a commit to codablock/dash that referenced this pull request Jan 15, 2018
…ages

e5d682f Fix mininode version message format (jnewbery)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
…ages

e5d682f Fix mininode version message format (jnewbery)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 24, 2019
…ages

e5d682f Fix mininode version message format (jnewbery)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants