Bip339: small corrections and update spec to match implementation #961

The first "must" should remain "must" -- I don't see how software can advertise itself as having implemented this BIP yet not comply with that requirement. Am I missing something?

The sentence you added here looks good.

Is there a reason to require all >= 70016 peers to send a WTXIDRELAY msg even if they do not need or want it?

Of course not. Implementing this BIP is not a requirement for anyone!

That's why it's a may rather than a must. The peer may opt in to wtxid relay by sending the wtxidrelay message.

@sdaftuar @jnewbery I think perhaps you're interpreting the language differently. Here is my guess:

Suhas means "To advertize itself as having implemented this BIP, a peer must send wtxidrelay", and that a "may" here would mean that someone can implement BIP339 without sending it.

John is interpreting the "must" as unconditional, and wants to emphasize that it is allowed for someone to not opt into BIP339.

@jnewbery I think you're confusing compatibility with implementation.

Whether software that doesn't implement this BIP remains compatible with software that does is a separate issue from defining what implementation of this BIP means.

I think the most useful way to define what it means to implement this BIP is to require that implementing software actually exchange wtxids when announcing and fetching transactions. The alternative is absurd: why bother specifying an implementation when all software on the network was already compliant before we did anything?

I think the concern would be if it weren't clear that software could choose to not implement the BIP and still remain compatible. But that's why there's a backwards compatibility section... If the text there is not enough, I'd prefer we add additional text to make it clear that implementing this BIP is not inherently incompatible with txid-based relay.

I don't think I'm confused. As @sipa points out, we're simply interpreting the language differently.

The sentence "The WTXIDRELAY message must be sent in response to a VERSION message from a peer whose protocol version is >= 70016" implies strongly to me that if a peer sends us a protocol version >= 70016, then we must opt in to wtxid relay. That's fine for now, when 70016 is the highest p2p version (and you can therefore opt out by using version 70015), but if there's a later version then it's restrictive - an implementation may want to opt in to whatever feature is introduced by 70017, but not opt in to wtxid relay.

Perhaps we can reword it in a way that avoids any ambiguity:

If a node wishes to opt in to wtxid relay with a peer whose version is >= 70016, it MUST send a WTXIDRELAY message after receiving the VERSION message and before sending the VERACK message. A WTXIDRELAY message received after a VERACK message MUST be ignored or treated as invalid.

@jnewbery Thinking more about it, I think I agree with Suhas that his interpretation makes more sense. Every BIP supports not being implemented by some software, as after all, everything is opt-in. If a BIP poses issues with other software that doesn't implement it, that's something to go in a compatibility section.

So I don't agree that the BIP (as is) should be interpreted as version 70017, in any future version, implying that WTXIDRELAY must be sent. It is only true for software that chooses to implement BIP339, and anything is free not to do that.

If we want to make that more clear, the sentence could be prefixed with "To implement this BIP, ..." - making it clear that that is the choice. Just saying "wishes to opt in to" may still be ambiguous, because then the question may arise: what does it mean to implement the BIP, but not opt in?

ok, reverted to MUST

Why the relaxation to SHOULD here? Bitcoin Core's implementation doesn't do this, and I thought that several reviewers (including yourself) were in favor of stricter requirements where possible for stuff like this.

In particular, after negotiating wtxid-relay with a peer, Bitcoin Core will not process non-MSG_WTX announcements from that peer at all.

Also, why delete the requirement to request announced transactions from that peer using MSG_WTX getdata messages? I think if we were to clarify this to match the current spec, we'd just make it clear that this applies to recently announced transactions, with no promises on older ones (see my next review comment).

Why the relaxation to SHOULD here?

Sorry, you're right. This should stay as MUST instead of SHOULD.

Also, why delete the requirement to request announced transactions from that peer using MSG_WTX getdata messages?

I think with your improved sentence below, this isn't needed here (and actually makes things less clear). I also think it's better to document the announce behaviour and the request behaviour in different bullet points.

I'm not sure these sentences are the best way to describe the behavior. What I would have said, if someone thought the doc needed more clarity here, would be something like:

After receiving an INV message containing a MSG_WTX object, a node SHOULD use a MSG_WTX GETDATA message to request any announced transactions. 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).

To me, that implies that if a node makes a non-MSG_WTX GETDATA request, then they may not end up receiving the transaction they're looking for even if it was recently announced; but a peer that implements this BIP shouldn't disconnect or stop transaction relay with a peer just because they received such a GETDATA request.

With your language of using MAY instead of SHOULD, I read that as meaning that software implementing this BIP effectively MUST be able to produce a transaction regardless of whether it's requested by MSG_WTX or MSG_TX -- which seems overly strict on how the implementations should be written.

Agree. I'll take your sentence.

To be honest I don't think of 19569 as part of the implementation; orphan fetching from wtxid-relay peers is not mandated by the spec, it's purely an optimization that takes advantage of allowances in the BIP. The BIP is drafted to allow for design decisions like that, and I don't think that it would make sense to add PRs here every time we do something that operates under one of those allowances.

For example, if we changed the behavior of Bitcoin Core to get rid of mapRelay and only return transactions from our mempool, and we didn't want to look up transactions in our mempool twice (once by txid, once by wtxid), then a peer that sent us a MSG_TX getdata for something that is a wtxid would then no longer be able to fetch transactions from us successfully. While the BIP allows Bitcoin Core to operate in this way (at least, in my proposed language! maybe others disagree), I think a change like that wouldn't constitute a part of the implementation of this BIP that would make sense to include here.

I don't feel strongly enough to object to including 19569 if most reviewers (particularly @sipa) think it belongs here, this is just my gut reaction based on how I understand the design docs.

Yeah, I was on the fence about this. I wanted to include 19569 to demonstrate that requesting transactions by txid is legal even after negotiating wtxid relay. But as you say, it's not strictly part of the implementation of the spec, but rather an optimization that is permitted under the allowances of the spec. I'll remove it.

s/inv-type/inv type/ ("inv type" is also used in line 45 immediately above)

Good idea. Standardized to 'inv type'

s/, and/ and/

done

perhaps "A new wtxidrelay net message type is added"

I'm going to leave this. My main motivation here was to update the BIP to be in line with the implementation for the wtxidrelay handshake and transaction fetching. It feels like scope creep to touch other parts of the document.

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?

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).

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".

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).

ok