Version 2 sendcmpct for integration of segwit with CB #423
Conversation
a018aab to
833a37b
Compare
| The changes to transaction and block relay in BIP 144 introduce separate MSG_FILTERED_ versions of messages in getdata, | ||
| allowing a receiver to choose individually where witness data is wanted. | ||
|
|
||
| This method is not useful for compact blocks because `cmpctblock` blocks can be sent unsollicitedly in high-bandwidth |
There was a problem hiding this comment.
s/unsollicitedly/unsolicitedly/
7d40066 to
b885153
Compare
|
ping @TheBlueMatt |
|
@sipa Maybe you should like to the pull request that enables this feature in the reference implementation section. |
|
@btcdrak Done. |
|
I'm not sure I understand the thinking behind how feature negotiation is intended to work. The original BIP made a reference to a version "handshake" in the future, which I figured meant that peers would be looking at each others sendcmpct version number to choose, say, the minimum common behavior. This BIP modification doesn't really talk about how the handshake works between version 1 and version 2 clients, and looking at the implementation in bitcoin/bitcoin#8393, it seems that extra state is used to make this determination (namely the nServices advertised by a node). My initial thought was that it might be better to not rely on state outside the sendcmpct message (but I'm not sure, maybe for segwit this is appropriate) -- but regardless I think it'd be helpful to at least spell out more exactly in the BIP what the intended protocol here is. |
| # Transactions inside cmpctblock messages (both those used as direct announcement and those in response to getdata) and in blocktxn should include witness data, using the same format as responses to getdata MSG_WITNESS_TX, specified in BIP144. | ||
| # Short transaction IDs sent to us in cmpctblock messages, and sent by us in getblocktxn messages, will be computed using the witness hash (the wtxid as defined in BIP 141). | ||
|
|
||
| For nodes that support segwit, it is RECOMMENDED to support both version 1 and 2 compact blocks. To do so, two sendcmpct messages can be sent at startup: the first one for version 1 and a second one for version 2. |
There was a problem hiding this comment.
I would argue MAY, not RECOMMENDED.
|
Modified the proposal to use the announce-multiple-sendcmpct-versions mechanism instead, and adapted the related core PR. |
|
@sdaftuar @TheBlueMatt Comments on the modification? |
| # Transactions inside cmpctblock messages (both those used as direct announcement and those in response to getdata) and in blocktxn should include witness data, using the same format as responses to getdata MSG_WITNESS_TX, specified in BIP144. | ||
| # Short transaction IDs sent to us in cmpctblock messages, and sent by us in getblocktxn messages, are computed using the same process as in version 1, but using the wtxid as defined in BIP 141 instead of the txid. | ||
|
|
||
| Nodes that support segwit MAY choose to support both version 1 and 2 compact blocks. To do so, two sendcmpct messages can be sent at startup: the first one for version 1 and a second one for version 2. |
There was a problem hiding this comment.
As I mentioned in bitcoin/bitcoin#8393 (comment), I think that if we require the version messages to be sent in a particular order for correct operation, then we should clarify the language here. Perhaps:
To do so, two sendcmpct messages can be sent at startup and MUST be in this order: the first one for version 1 and a second one for version 2.
However I think I'd prefer to drop the requirement that they be sent in a particular order -- is there a good reason to require this?
EDIT: just saw @sipa's response in bitcoin/bitcoin#8393. I'm fine with leaving this requirement in.
|
I have one more observation, aside from what I mentioned above regarding the strict ordering of version messages. BIP 152 isn't clear on how getdata messages for MSG_CMPCT_BLOCK should be handled if the peer requesting the compact block hasn't sent us a sendcmpct message. The BIP prohibits requesting a compact block before receiving a sendcmpct, but there's no similar prohibition in the other direction. However, if your peer hasn't sent you a sendcmpct, then you don't know what version to use for the response. The natural thing to do would be to fall through to the next requirement in the BIP, which is that if you don't respond with a compact block, then you must respond with a block. However post-segwit, there's no mechanism to figure out whether the block should be with or without witness. I propose that the BIP be updated to indicate that nodes either SHOULD or MUST not respond to a getdata(MSG_CMPCT_BLOCK) if the requestor never sent a sendcmpct message to begin with (as there's no way to know what version to use). I think then it should be pretty safe for the fallback-to-sending-a-block code to also use the information from the negotiated compact block version to determine whether to include witnesses. @TheBlueMatt @sipa Thoughts? |
|
@sdaftuar Addressed comments, I hope. |
|
Ping @sdaftuar @TheBlueMatt ? |
|
@sipa Looks good to me, thanks! |
|
Upon thinking more, I think my versioing proposal was very ill-conceived. "sendcmpct" has two meanings - both "send me this version of unsolicited compact blocks/send me this version of compact blocks when I request them" and "I can send you this version of compact blocks". It is hard to read into it exactly what version of compact block will be sent to you in response to your request for a compact block. We could tie the two together explicitly and make clear that you shouldnt request compact blocks until you have both send your own sendcmpct and received one, at which point you know that the version they will be sending you is the version you requested last which they also had in one of their sendcmpct messages. Otherwise we need a way to tell the two cmpctblock messages apart (could use a different message name, or a different encoding). |
|
@TheBlueMatt Yes, I was realizing that too. I don't think it's a problem at this point, but it complicates reasoning a bit. Right now, there are two ways through which the sending of compact blocks is requested: one through the bool in the My suggestion would be to keep things as-is now, but for a future extension, we introduce an explicit |
|
Its well-defined but not well-defined in the spec right now. At least we need to specify what, exactly, a sendcmpct is indicating to the other side, which isnt particularly well-defined, nor how you should select version for different directions, etc. I can take a crack at wording if you prefer. |
Please do! |
|
Proposed version negotiation semantics: TheBlueMatt@7059fd1 Note that I think these might actually need some slight tweaks to the implementation, but provide better features than I thought they would, sorry about that. |
|
That suggestion codifies that ping/pong serializes p2p messages with sendcmpct messages at startup, which I think is perfectly reasonable, but if we decide against it a cmpctverack or so can easily be added instead. |
|
Cherry-picked @TheBlueMatt's suggestions. |
|
I added a few more commits to master...TheBlueMatt:segwitcb. The first two clarify some things and add an example. The only change is that priority order is now highest-priority-first instead of highest-priority-last, which simplifies implementation somewhat. The last commit changes the protocol entirely, adding a cmpctack message. This has the advantage that you could implement receiving of some version of compactlocks without implementing sending that encoding, as well as simplifying the protocol slightly (instead of having to check if the current protocol version is higher-priority according to your probably-compile-time list of supported version you know which version you're using directly from the ack message) at the expensee of complicating the implementation somewhat (now you have to add support for another message type and special-case version 1). The last commit is definitely not worth it if we dont anticipate adding more than one or so more versions, but might be worth it if we anticipate compact blocks version 4, 5, 6 at some point. I'll bring this up in the IRC meeting later today. |
| # Nodes SHOULD check for a protocol version of >= 70014 before sending sendcmpct messages. | ||
| # Nodes MUST NOT send a request for a MSG_CMPCT_BLOCK object to a peer before having received a sendcmpct message from that peer. | ||
| # Nodes MUST NOT request a MSG_CMPCT_BLOCK object before having sent all sendcmpct messages to that peer which they intend to send, as the peer cannot know what version protocol to use in the response. |
There was a problem hiding this comment.
Should be combined with previous line.
There was a problem hiding this comment.
Because they're both setting the conditions for requesting a compact block?
| ==Protocol Versioning== | ||
| # The protocol version negotiation allows two nodes to agree on the version of compact blocks which they will exchange. As it is only in a single field, it does not allow a node to support a specific version in only one direction (sending or receiving). | ||
| # Upon connection establishment, a node SHOULD send a burst of sendcmpct messages containing every version of compact block encodings for which they are willing to support sending cmpctblock and blocktxn messages, and receiving getblocktxn messages. These messages SHOULD be ordered in the order of the priority which the node wishes to receive cmpctblock/blocktxn messages, with the highest-priority version sendcmpct message sent last. | ||
| # The encoding version used to send a cmpctblock or blocktxn message or to receive a getblocktxn message MUST be the second integer (version number) in the most recent sendcmpct message received for which a sendcmpct message with the same version number was sent. |
There was a problem hiding this comment.
This sounds confusing. @sipa says it is outdated as well.
There was a problem hiding this comment.
You want to be reviewing as of commit af6330a, I believe. Still, the version there has pretty much the same text. Any suggestions to clarify?
There was a problem hiding this comment.
I would try splitting it into two sentences... or maybe "The encoding version used for sending compact blocks is determined by the union of those supported by both nodes, specifically the most recent one received from the peer."
There was a problem hiding this comment.
Oops, I forgot that @TheBlueMatt had proposed changes which weren't included yet. Fixed now.
| # The protocol version negotiation allows two nodes to agree on the version of compact blocks which they will exchange. As it is only in a single field, it does not allow a node to support a specific version in only one direction (sending or receiving). | ||
| # Upon connection establishment, a node SHOULD send a burst of sendcmpct messages containing every version of compact block encodings for which they are willing to support sending cmpctblock and blocktxn messages, and receiving getblocktxn messages. These messages SHOULD be ordered in the order of the priority which the node wishes to receive cmpctblock/blocktxn messages, with the highest-priority version sendcmpct message sent last. | ||
| # The encoding version used to send a cmpctblock or blocktxn message or to receive a getblocktxn message MUST be the second integer (version number) in the most recent sendcmpct message received for which a sendcmpct message with the same version number was sent. | ||
| # Nodes MUST NOT send a sendcmpct message which contains a version number other than the version number which has been negotiated for receiving cmpctblock/blocktxn messages after sending a request for a MSG_CMPCT_BLOCK object, sending a cmpctblock, getblocktxn, blocktxn, or pong message. |
There was a problem hiding this comment.
This is also confusing. Maybe start the sentence with "After"?
There was a problem hiding this comment.
Are you suggestiong just flipping the sentence? I find that to be slightly more confusing.
There was a problem hiding this comment.
Yes, it is very unclear how to parse this current sentence.
| # Upon connection establishment, a node SHOULD send a burst of sendcmpct messages containing every version of compact block encodings for which they are willing to support sending cmpctblock and blocktxn messages, and receiving getblocktxn messages. These messages SHOULD be ordered in the order of the priority which the node wishes to receive cmpctblock/blocktxn messages, with the highest-priority version sendcmpct message sent last. | ||
| # The encoding version used to send a cmpctblock or blocktxn message or to receive a getblocktxn message MUST be the second integer (version number) in the most recent sendcmpct message received for which a sendcmpct message with the same version number was sent. | ||
| # Nodes MUST NOT send a sendcmpct message which contains a version number other than the version number which has been negotiated for receiving cmpctblock/blocktxn messages after sending a request for a MSG_CMPCT_BLOCK object, sending a cmpctblock, getblocktxn, blocktxn, or pong message. | ||
| # Nodes MUST NOT set the first integer (boolean) value in sendcmpct messages to 1 in any sendcmpct message prior to the last sendcmpct message which contains a novel second integer (version number). |
There was a problem hiding this comment.
This is also confusing. So all sendcmpct messages will have non-1 for the first integer, except for the final such message?
There was a problem hiding this comment.
This line is removed in the changes I proposed sipa include.
|
is this up to date with the code PR? |
|
My currently proposed version (which I believe is up to date with the code) is https://github.com/TheBlueMatt/bips/blob/af6330a573ad8c452ae2044c0f0e84c64d7d4124/bip-0152.mediawiki |
|
|
|
Included some commits from @TheBlueMatt's branch. |
|
Oh I'm commenting on an out-of-date version, nevermind. |
| @@ -136,7 +137,31 @@ A new inv type (MSG_CMPCT_BLOCK == 4) and several new protocol messages are adde | |||
| ## For each short transaction ID from the original cmpctblock, in order, find the corresponding transaction either from the blocktxn message or from other sources and place it in the first available position in the block. | |||
| # Once the block has been reconstructed, it shall be processed as normal, keeping in mind that short transaction IDs are expected to occasionally collide, and that nodes MUST NOT be penalized for such collisions, wherever they appear. | |||
|
|
|||
| ===Implementation Notes=== | |||
| ==Protocol Versioning== | |||
| # The protocol version negotiation allows two nodes to agree on the version of compact blocks which they will exchange. As it is only in a single field, it does not allow a node to support a specific version in only one direction (sending or receiving). | |||
There was a problem hiding this comment.
/version/versions? Each node can request different versions first that both can support.
There was a problem hiding this comment.
ACK, @sipa you want to fix the second version in the line to versions?
| ===Sample Version Implementation=== | ||
| # By way of example, an implementation of the above protocol might look like the following. | ||
| # Upon exchanging version/verack messages, a node immediately sends its list of sendcmpct announcements to the other side, with the version which it wants to receive sent first. | ||
| # Upon receiving the first sendcmpct announcement with a protocol version which is understood from the remote peer, a node will "lock in" the compact block encoding version which will be used to encode compact blocks to that peer. |
There was a problem hiding this comment.
Perhaps add:
The node then sets the current receive-protocol-version in use on the connection as that version, and uses it to decode new compact block messages.
And on the following line:
/Upon receiving a sendcmpct announcement/Upon receiving subsequent sendcmpct announcements/
Makes it much easier to parse the meaning.
There was a problem hiding this comment.
Seems reasonable to me, though maybe make it three lines then, as receiving and sending versions have logically distinct behavior.
|
@instagibbs Added a commit to incorporate your suggestions. |
| @@ -149,7 +149,8 @@ A new inv type (MSG_CMPCT_BLOCK == 4) and several new protocol messages are adde | |||
| # By way of example, an implementation of the above protocol might look like the following. | |||
| # Upon exchanging version/verack messages, a node immediately sends its list of sendcmpct announcements to the other side, with the version which it wants to receive sent first. | |||
| # Upon receiving the first sendcmpct announcement with a protocol version which is understood from the remote peer, a node will "lock in" the compact block encoding version which will be used to encode compact blocks to that peer. | |||
| # Upon receiving a sendcmpct announcement with a protocol version which is understood from the remote peer, a node will check if that protocol version is higher-receive-priority than the current receive-protocol-version in use on the connection, and switch to that version for decoding new compact block messages received. | |||
| # The node then sets the current receive-protocol-version in use on the connection as that version, and uses it to decode new compact block messages. | |||
| @@ -149,7 +149,8 @@ A new inv type (MSG_CMPCT_BLOCK == 4) and several new protocol messages are adde | |||
| # By way of example, an implementation of the above protocol might look like the following. | |||
| # Upon exchanging version/verack messages, a node immediately sends its list of sendcmpct announcements to the other side, with the version which it wants to receive sent first. | |||
| # Upon receiving the first sendcmpct announcement with a protocol version which is understood from the remote peer, a node will "lock in" the compact block encoding version which will be used to encode compact blocks to that peer. | |||
| # Upon receiving a sendcmpct announcement with a protocol version which is understood from the remote peer, a node will check if that protocol version is higher-receive-priority than the current receive-protocol-version in use on the connection, and switch to that version for decoding new compact block messages received. | |||
| # The node then sets the current receive-protocol-version in use on the connection as that version, and uses it to decode new compact block messages. | |||
| # Upon receiving subsequent sendcmpct announcements with a protocol version which is understood from the remote peer, a node will check if that protocol version is higher-receive-priority than the current receive-protocol-version in use on the connection, and switch to that version for decoding new compact block messages received. | |||
There was a problem hiding this comment.
s/which is understood from the remote peer,/which is understood from the remote peer (ie which has been announced),/
|
utACK 78cbaca |
No description provided.