Bip339: small corrections and update spec to match implementation

[jnewbery]

Updates text to match implementation, specifically that it's always permitted to request a transaction using the txid. Also includes some minor tidy-ups.

[jnewbery]

cc @sdaftuar @sipa

[sdaftuar]

The first commit seems unnecessary to me; I just took a quick look through the BIP repo and several other BIPs that introduce new p2p messages also use lower case (sendheaders BIP 130, feefilter BIP 133, compact blocks BIP 152) -- which is also what we send on the wire. It doesn't really matter but if it were up to me I'd just stick with lower case for consistency.

EDIT: Hah, you were making it consistent with the other p2p messages in this doc, not with other BIPs! Oops.

[jnewbery]

Thanks for the review @sdaftuar . I've addressed all of your review comments.

[jnewbery]

I just took a quick look through the BIP repo and several other BIPs that introduce new p2p messages also use lower case (sendheaders BIP 130, feefilter BIP 133, compact blocks BIP 152) -- which is also what we send on the wire

I mostly wanted these to be internally consistent within the document, so that it's clear that they're referring to the same concept. Most of the message types in this doc were uppercase which is why I standardized to that, but you're right that lowercase is better. I've updated the first commit to do that.

[jonatack]

ACK, some nits if you agree and retouch.

[jonatack]

ACK, looks like a net improvement (pun intended)

[sdaftuar]

On further thought -- would this last bullet be clearer if we wrote it like this:

After a node has received a wtxidrelay message from a peer, a node SHOULD use a MSG_WTX getdata message to request any announced transactions from that peer. A node MAY still request transactions from that peer using MSG_TX getdata messages, such as for transactions not recently announced by that peer (like the parents of recently announced transactions).

The way it's written here, it seems like if any peer sends us a MSG_WTX inv, that we should (a) respond with a MSG_WTX getdata (even if the peer hasn't negotiated wtxidrelay with us!) and (b) it's okay if we respond with a MSG_TX getdata (which is basically wrong, we're just trying to carve out reasons you might send one over the wire, we don't expect it to occur for a hash that appears as a MSG_WTX inv).

And should we add a new bullet saying that we (may? must?) ignore wtxidrelay messages unless the peer's version is >= 70016?

[ariard]

I think you can keep the SHOULD as a MUST as you underscore well enough a INV(MSG_WTX) announcement must be paired with a GETDATA(MSG_WTX). But also fine with your current alternative, which sounds clearer IMO.

Yes, good to check peer's version and must ignore them, some bitcoin libraries are currently forgetting to export p2p versions to let upstream software announce p2p extensions they want to speak (like rust-bitcoin).

[jnewbery]

My understanding of this BIP (and I think the most reasonable reading of it) is that sending a peer a wtxidrelay message to a peer means "please send inventory to me using MSG_WTX objects in the INV message". I don't think it should also mean "you must request transactions using MSG_WTX objects in the GETDATA message". That would implicitly mean that wtxidrelay must be enabled in both directions (because for you to request MSG_WTX objects, I must be announcing MSG_WTX objects). If that's the intention, then I think the BIP needs to explicitly say that, and also specify what happens if one side sends a wtxidrelay but the other side doesn't.

I think the protocol is simpler if it's enabled separately in both directions and wtxidrelay simply means "send me MSG_WTX INVs".

[sdaftuar]

My understanding of this BIP (and I think the most reasonable reading of it) is that sending a peer a wtxidrelay message to a peer means "please send inventory to me using MSG_WTX objects in the INV message".

This is a misunderstanding of the BIP and the implementation. Note that the original BIP text was (essentially) correct here -- the intention is that wtxidrelay is either on in both directions on a link, or off in both directions. This is also how the implementation works:

• for peers with a version >= 70016, we always send a wtxidrelay message, but we only enable it on a link if we also receive it
• peers that haven't sent us a wtxidrelay message (or whose version is too low) will have any MSG_WTX inv's ignored (not a requirement of the BIP, but certainly permitted under the BIP)
• if a peer has sent us a wtxidrelay message, they MUST announce transactions to us using MSG_WTX invs, so it would not make any sense for us to respond with MSG_TX getdata messages. There's no reason to think that requesting a hash and calling it a txid instead of a wtxid would allow our peer to successfully look it up.

The reason I'm ok with changing the MUST to a SHOULD here, regarding the getdata messages, is because sometimes you could imagine that a peer will announce a transaction via MSG_WTX, we'll hold off on fetching it for a while, and then orphan fetching will kick in and cause us to request that parent via txid -- which might be the same as the MSG_WTX wtxid (if it's a non-segwit transaction). So in that case, the traffic on the link might look like MSG_WTX inv followed by MSG_TX getdata. But that is a rare case, and we call it out explicitly in the new language, so the understanding should be that it's generally nonsensical to respond to a MSG_WTX inv with a MSG_TX getdata.

If you have more questions about the language, it's probably easiest to take it offline -- I'm happy to update the text with language that clarifies any confusion (and that seems easier than trying to hash it out here).

[jnewbery]

ok

[sdaftuar]

FYI - I picked this up in #966.