BIP339: Add wtxidrelay message and WTx inv type #446
Conversation
src/network/constants.rs
Outdated
| @@ -42,7 +42,7 @@ use std::{fmt, io, ops}; | |||
| use consensus::encode::{self, Encodable, Decodable}; | |||
|
|
|||
| /// Version of the protocol as appearing in network message headers | |||
| pub const PROTOCOL_VERSION: u32 = 70001; | |||
| pub const PROTOCOL_VERSION: u32 = 70016; | |||
There was a problem hiding this comment.
If you don't signal wtxid-relay version, your peer won't start to talk wtxid-relay with you. But if we upgrade PROTOCOL_VERSION, downstream software need to implement wtxid-relay to avoid interferences while talking with Core. Once more we should copy undocumented const from upstream (src/version.h) and implementation should set PROTOCOL_VERSION to what features set they support.
There was a problem hiding this comment.
Can we add some docs here about this number? I thought it was a result of ORing flags, but I see that also in bitcoin core it's hard coded like that.
There was a problem hiding this comment.
Good point about forcing downstream to implement it, I'll change it back to 70001.
What sort of docs were you thinking? Telling people to change it to reflect the features they support or how it's derived?
There was a problem hiding this comment.
But if we upgrade PROTOCOL_VERSION, downstream software need to implement wtxid-relay to avoid interferences while talking with Core
I don't think this is true? If you don't send the "wtxidrelay" message the only change is that you'll receive a new unknown message type, which is fine. IIUC, the only reason the protocol version was bumped here was that the message happens between version and verack and some folks may fail the connection for such messages.
There was a problem hiding this comment.
I think the real confusion here is that the new PROTOCOL_VERSION has nothing to do with wtxidrelay - it really means "I support receiving support messages between version and verack". It just so happens that one such message is wtxidrelay.
There was a problem hiding this comment.
It is a bit unclear what version this library should set as it really depends on the functionality of the software using it. Would you be ok leaving it as it is and adding a comment similar to the one I've added?
There was a problem hiding this comment.
Right, I think maybe having a PROTOCOL_VERSION constant in the crate is a bit of a misnomer - originally PROTOCOL_VERSION implied messages and features supported, but these days it really only indicates breaking changes to the protocol (ie in this case sending messages between version and verack), whereas new features are introduced by simply sending new message types which old nodes will ignore. Its probably best to just add a list of protocol versions with detailed descriptions of what they imply, though I don't think we need to go back to 0, just add the few most recent ones.
There was a problem hiding this comment.
the downside I just realized for this constant here, is that you get alert messages from peers (because it's smaller than 70012)
There was a problem hiding this comment.
Couldn't we (in a separate PR) remove the constant, use a hard-coded one for getheaders and getblocks and take the version for the version message as an argument from the user?
|
Code Review ACK 8c066e8. Thanks @jrawsthorne for clarifying PROTOCOL_VERSION usage. |
|
Rebased |
|
Could you perhaps squash? |
|
Squashed @stevenroose |
BIP339 support was just merged into core: bitcoin/bitcoin#18044
Not sure about changing the protocol version because I don't think every protocol is supported in rust-bitcoin yet
Not sure about the name for the new inv type, we already have WitnessTransaction