[bitcoin-dev] BIP151 protocol incompatibility

[bitcoin-dev] BIP151 protocol incompatibility

[Eric Voskuil]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

The BIP151 proposal states:

> This proposal is backward compatible. Non-supporting peers will ignore
the encinit messages.

This statement is incorrect. Sending content that existing nodes do not
expect is clearly an incompatibility. An implementation that ignores
invalid content leaves itself wide open to DOS attacks. The version
handshake must be complete before the protocol level can be determined.
While it may be desirable for this change to precede the version
handshake it cannot be described as backward compatible.

e


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Pieter Wuille]

[-- Attachment #1: Type: text/plain, Size: 1158 bytes --]

On Feb 12, 2017 23:58, "Eric Voskuil via bitcoin-dev" <
bitcoin-dev@lists.linuxfoundation.org> wrote:

The BIP151 proposal states:

> This proposal is backward compatible. Non-supporting peers will ignore
the encinit messages.

This statement is incorrect. Sending content that existing nodes do not
expect is clearly an incompatibility. An implementation that ignores
invalid content leaves itself wide open to DOS attacks. The version
handshake must be complete before the protocol level can be determined.
While it may be desirable for this change to precede the version
handshake it cannot be described as backward compatible.


The worst possible effect of ignoring unknown messages is a waste of
downstream bandwidth. The same is already possible by being sent addr
messages.

Using the protocol level requires a strict linear progression of (allowed)
network protocol features, which I expect to become harder and harder to
maintain.

Using otherwise ignored messages for determining optional features is
elegant, simple and opens no new attack vectors. I think it's very much
preferable over continued increments of the protocol version.

-- 
Pieter

[-- Attachment #2: Type: text/html, Size: 1833 bytes --]

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Eric Voskuil]

[-- Attachment #1: Type: text/plain, Size: 2944 bytes --]

On 02/13/2017 12:47 AM, Pieter Wuille wrote:
> On Feb 12, 2017 23:58, "Eric Voskuil via bitcoin-dev"
> <bitcoin-dev@lists.linuxfoundation.org wrote:
> 
>     The BIP151 proposal states:
> 
>     > This proposal is backward compatible. Non-supporting peers will ignore
>     the encinit messages.
> 
>     This statement is incorrect. Sending content that existing nodes do not
>     expect is clearly an incompatibility. An implementation that ignores
>     invalid content leaves itself wide open to DOS attacks. The version
>     handshake must be complete before the protocol level can be determined.
>     While it may be desirable for this change to precede the version
>     handshake it cannot be described as backward compatible.
> 
> The worst possible effect of ignoring unknown messages is a waste of
> downstream bandwidth. The same is already possible by being sent addr
> messages.
> 
> Using the protocol level requires a strict linear progression of
> (allowed) network protocol features, which I expect to become harder and
> harder to maintain.
> 
> Using otherwise ignored messages for determining optional features is
> elegant, simple and opens no new attack vectors. I think it's very much
> preferable over continued increments of the protocol version.

As I said, it *may* be desirable, but it is *not* backward compatible,
and you do not actually dispute that above.

There are other control messages that qualify as "optional messages" but
these are only sent if the peer is at a version to expect them -
explicit in their BIPs. All adopted BIPs to date have followed this
pattern. This is not the same and it is not helpful to imply that it is
just following that pattern.

As for DOS, waste of bandwidth is not something to be ignored. If a peer
is flooding a node with addr message the node can manage it because it
understands the semantics of addr messages. If a node is required to
allow any message that it cannot understand it has no recourse. It
cannot determine whether it is under attack or if the behavior is
correct and for proper continued operation must be ignored.

This approach breaks any implementation that validates traffic, which is
clearly correct behavior given the existence of the version handshake.
Your comments make it clear that this is a *change* in network behavior
- essentially abandoning the version handshake. Whether is is harder to
maintain is irrelevant to the question of whether it is a break with
existing protocol.

If you intend for the network to abandon the version handshake and/or
promote changes that break it I propose that you write up this new
behavior as a BIP and solicit community feedback. There are a lot of
devices connected to the network and it would be irresponsible to break
something as fundamental as the P2P protocol handshake because you have
a feeling it's going to be hard to maintain.

e


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Jonas Schnelli]

[-- Attachment #1.1: Type: text/plain, Size: 1225 bytes --]


> All adopted BIPs to date have followed this
> pattern. This is not the same and it is not helpful to imply that it is
> just following that pattern.

Look at feefilter BIP 133
(https://github.com/bitcoin/bips/blob/master/bip-0133.mediawiki#backward-compatibility)
or sendheaders BIP130
(https://github.com/bitcoin/bips/blob/master/bip-0130.mediawiki#backward-compatibility)
Isn't it the same there?
Once BIP151 is implemented, it would make sense to bump the protocol
version, but this needs to be done once this has been
implemented/deployed. Or do I make a mistake somewhere?
>
> As for DOS, waste of bandwidth is not something to be ignored. If a peer
> is flooding a node with addr message the node can manage it because it
> understands the semantics of addr messages. If a node is required to
> allow any message that it cannot understand it has no recourse. It
> cannot determine whether it is under attack or if the behavior is
> correct and for proper continued operation must be ignored.
How do you threat any other not known message types? Any peer can send
you any type of message anytime. Why would your implementation how you
threat unknown messages be different for messages specified in BIP151?


</jonas>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Matt Corallo]

For the reasons Pieter listed, an explicit part of our version handshake and protocol negotiation is the exchange of otherwise-ignored messages to set up optional features.

Peers that do not support this ignore such messages, just as if they had indicated they wouldn't support it, see, eg BIP 152's handshake. Not sure why you consider this backwards incompatible, as I would say it's pretty clearly allowing old nodes to communicate just fine.

On February 13, 2017 10:36:21 AM GMT+01:00, Eric Voskuil via bitcoin-dev <bitcoin-dev@lists.linuxfoundation.org> wrote:
>On 02/13/2017 12:47 AM, Pieter Wuille wrote:
>> On Feb 12, 2017 23:58, "Eric Voskuil via bitcoin-dev"
>> <bitcoin-dev@lists.linuxfoundation.org wrote:
>> 
>>     The BIP151 proposal states:
>> 
>>     > This proposal is backward compatible. Non-supporting peers will
>ignore
>>     the encinit messages.
>> 
>>     This statement is incorrect. Sending content that existing nodes
>do not
>>     expect is clearly an incompatibility. An implementation that
>ignores
>>     invalid content leaves itself wide open to DOS attacks. The
>version
>>     handshake must be complete before the protocol level can be
>determined.
>>     While it may be desirable for this change to precede the version
>>     handshake it cannot be described as backward compatible.
>> 
>> The worst possible effect of ignoring unknown messages is a waste of
>> downstream bandwidth. The same is already possible by being sent addr
>> messages.
>> 
>> Using the protocol level requires a strict linear progression of
>> (allowed) network protocol features, which I expect to become harder
>and
>> harder to maintain.
>> 
>> Using otherwise ignored messages for determining optional features is
>> elegant, simple and opens no new attack vectors. I think it's very
>much
>> preferable over continued increments of the protocol version.
>
>As I said, it *may* be desirable, but it is *not* backward compatible,
>and you do not actually dispute that above.
>
>There are other control messages that qualify as "optional messages"
>but
>these are only sent if the peer is at a version to expect them -
>explicit in their BIPs. All adopted BIPs to date have followed this
>pattern. This is not the same and it is not helpful to imply that it is
>just following that pattern.
>
>As for DOS, waste of bandwidth is not something to be ignored. If a
>peer
>is flooding a node with addr message the node can manage it because it
>understands the semantics of addr messages. If a node is required to
>allow any message that it cannot understand it has no recourse. It
>cannot determine whether it is under attack or if the behavior is
>correct and for proper continued operation must be ignored.
>
>This approach breaks any implementation that validates traffic, which
>is
>clearly correct behavior given the existence of the version handshake.
>Your comments make it clear that this is a *change* in network behavior
>- essentially abandoning the version handshake. Whether is is harder to
>maintain is irrelevant to the question of whether it is a break with
>existing protocol.
>
>If you intend for the network to abandon the version handshake and/or
>promote changes that break it I propose that you write up this new
>behavior as a BIP and solicit community feedback. There are a lot of
>devices connected to the network and it would be irresponsible to break
>something as fundamental as the P2P protocol handshake because you have
>a feeling it's going to be hard to maintain.
>
>e

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Eric Voskuil]

[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]

On 02/13/2017 02:07 AM, Jonas Schnelli via bitcoin-dev wrote:
>> All adopted BIPs to date have followed this
>> pattern. This is not the same and it is not helpful to imply that it is
>> just following that pattern.
> 
> Look at feefilter BIP 133
> (https://github.com/bitcoin/bips/blob/master/bip-0133.mediawiki#backward-compatibility)
> or sendheaders BIP130
> (https://github.com/bitcoin/bips/blob/master/bip-0130.mediawiki#backward-compatibility)
> Isn't it the same there?

No. This is what I was referring to. These messages are enabled by
protocol version. If they are received by a node below the version at
which they are activated, they are unknown messages, implying an invalid
peer. The above messages cannot be sent until *after* the version is
negotiated. BIP151 violates this rule by allowing the new control
message to be sent *before* the version handshake.

> Once BIP151 is implemented, it would make sense to bump the protocol
> version, but this needs to be done once this has been
> implemented/deployed.

There are already nodes out there breaking connections based on the BIP.

> Or do I make a mistake somewhere?

Yes, the ordering of the messages. New messages can only be added after
the handshake negotiates the higher version. Otherwise the handshake is
both irrelevant (as Pieter is implying) and broken (for all existing
protocol versions).

>> As for DOS, waste of bandwidth is not something to be ignored. If a peer
>> is flooding a node with addr message the node can manage it because it
>> understands the semantics of addr messages. If a node is required to
>> allow any message that it cannot understand it has no recourse. It
>> cannot determine whether it is under attack or if the behavior is
>> correct and for proper continued operation must be ignored.

> How do you threat any other not known message types?

You may be more familiar with non-validating peers. If a message type is
not known it is an invalid message and the peer is immediately dropped.
We started seeing early drops in handshakes with bcoin nodes because of
this issue.

> Any peer can send you any type of message anytime.

Sure, a peer can do what it wants. It can send photos. But I'm not sure
what makes you think it would be correct to maintain the connection when
an *invalid* message is received.

> Why would your implementation how you threat unknown messages be
different for messages specified in BIP151?

Because it properly validates the protocol.

More than that it supports a configurable protocol range. So by setting
the min protocol (below which the node won't connect) and the max
protocol (at which it desires to connect) we can observe the behavior of
the network at any protocol levels (currently between 31402 and 70013).
This is very helpful for a development stack as it allows one to easily
test against each protocol level that one wishes to support.

e


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Eric Voskuil]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

On 02/13/2017 02:16 AM, Matt Corallo wrote:
> For the reasons Pieter listed, an explicit part of our version
handshake and protocol negotiation is the exchange of otherwise-ignored
messages to set up optional features.

Only if the peer is at the protocol level that allows the message:

compact blocks:

https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L217-L242

fee filter:

https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L211-L216

send headers:

https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L204-L210

filters:

https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L170-L196

> Peers that do not support this ignore such messages, just as if they
had indicated they wouldn't support it, see, eg BIP 152's handshake. Not
sure why you consider this backwards incompatible, as I would say it's
pretty clearly allowing old nodes to communicate just fine.

No, it is not the same as BIP152. Control messages apart from BIP151 are
not sent until *after* the version is negotiated.

I assume that BIP151 is different in this manner because it has a desire
to negotiate encryption before any other communications, including version.

e


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Matt Corallo]

I believe many, if not all, of those messages are sent irrespective of version number.

In any case, I fail to see how adding any additional messages which are ignored by old peers amounts to a lack of backward compatibility.

On February 13, 2017 11:54:23 AM GMT+01:00, Eric Voskuil <eric@voskuil.org> wrote:
>On 02/13/2017 02:16 AM, Matt Corallo wrote:
>> For the reasons Pieter listed, an explicit part of our version
>handshake and protocol negotiation is the exchange of otherwise-ignored
>messages to set up optional features.
>
>Only if the peer is at the protocol level that allows the message:
>
>compact blocks:
>
>https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L217-L242
>
>fee filter:
>
>https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L211-L216
>
>send headers:
>
>https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L204-L210
>
>filters:
>
>https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L170-L196
>
>> Peers that do not support this ignore such messages, just as if they
>had indicated they wouldn't support it, see, eg BIP 152's handshake.
>Not
>sure why you consider this backwards incompatible, as I would say it's
>pretty clearly allowing old nodes to communicate just fine.
>
>No, it is not the same as BIP152. Control messages apart from BIP151
>are
>not sent until *after* the version is negotiated.
>
>I assume that BIP151 is different in this manner because it has a
>desire
>to negotiate encryption before any other communications, including
>version.
>
>e

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Jonas Schnelli]

[-- Attachment #1.1: Type: text/plain, Size: 4579 bytes --]


>> Look at feefilter BIP 133
>> (https://github.com/bitcoin/bips/blob/master/bip-0133.mediawiki#backward-compatibility)
>> or sendheaders BIP130
>> (https://github.com/bitcoin/bips/blob/master/bip-0130.mediawiki#backward-compatibility)
>> Isn't it the same there?
> No. This is what I was referring to. These messages are enabled by
> protocol version. If they are received by a node below the version at
> which they are activated, they are unknown messages, implying an invalid
> peer. The above messages cannot be sent until *after* the version is
> negotiated. BIP151 violates this rule by allowing the new control
> message to be sent *before* the version handshake.
This indeed is not ideal for compatibility checks, but increases security.
I could not find a protocol specification that said communication must
be terminated when messages are transmitted before the version handshake
has been done. I mostly looked into Bitcoin-Cores implementation (which
means also into BitcoinXT/UT, where this is allowed).

Also. BIP151 clearly says that the requesting peer needs to initiate the
encryption (encinit).
In case of light clients not supporting BIP151 connecting to peers
supporting BIP151, there should never be transmission of new message
types specified in BIP151.
>
>> Once BIP151 is implemented, it would make sense to bump the protocol
>> version, but this needs to be done once this has been
>> implemented/deployed.
> There are already nodes out there breaking connections based on the BIP.
It could very likely be possible that the initial responding peer tries
to initiate a encryption session which would mean that BIP151 was not
implemented correctly.
Correct me if I'm wrong please.
>
>> Or do I make a mistake somewhere?
> Yes, the ordering of the messages. New messages can only be added after
> the handshake negotiates the higher version. Otherwise the handshake is
> both irrelevant (as Pieter is implying) and broken (for all existing
> protocol versions).
I could not find evidence of the protocol specification that would
forbid (=terminate connection) such messages and I think allowing
unknown-messages before the version handshake makes the protocol flexible.

Are there any reasons we should drop peers if they send us unknown, but
correctly formatted p2p packages (magic, checksum, etc.) before the
version handshake, ... but not drop them if we have received unknown
messages after the version handshake?

I can't see that a such spec. would reduce the DOS attack vector?

>
>>> As for DOS, waste of bandwidth is not something to be ignored. If a peer
>>> is flooding a node with addr message the node can manage it because it
>>> understands the semantics of addr messages. If a node is required to
>>> allow any message that it cannot understand it has no recourse. It
>>> cannot determine whether it is under attack or if the behavior is
>>> correct and for proper continued operation must be ignored.
>> How do you threat any other not known message types?
> You may be more familiar with non-validating peers. If a message type is
> not known it is an invalid message and the peer is immediately dropped.
> We started seeing early drops in handshakes with bcoin nodes because of
> this issue.
If this had happened, it's very likely because the responding peer tried
to initiate a encryption session which is against BIP151 specs.
>
>> Any peer can send you any type of message anytime.
> Sure, a peer can do what it wants. It can send photos. But I'm not sure
> what makes you think it would be correct to maintain the connection when
> an *invalid* message is received.
Check:
https://github.com/bitcoin/bitcoin/blob/a06ede9a138d0fb86b0de17c42b936d9fe6e2158/src/net_processing.cpp#L2595
I think it was a wise implementation decision to allow unknown (not
invalid) messages.
This had allowed us to deploy stuff like compact blocks, feefilter, etc.
without breaking backward compatibility.
IMO, without a such flexibility, the deployment complexity would be
irresponsible high without really solving the DOS problem.
>
>> Why would your implementation how you threat unknown messages be
> different for messages specified in BIP151?
>
> Because it properly validates the protocol.
For feefilter or compact block or sendheaders?
You can't link a (unimplemented) specification (improvement process) to
a protocol version before deployment. Or can you?
Once it has been widely deployed, we should set a protocol minversion
for BIP151, right.

</jonas>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Eric Voskuil]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On 02/13/2017 03:11 AM, Matt Corallo wrote:
> I believe many, if not all, of those messages are sent irrespective of version number.

In the interest of perfect clarity, see your code:

https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L1372-L1403

Inside of the VERACK handler (i.e. after the handshake) there is a peer
version test before sending SENDCMPCT (and SENDHEADERS).

I have no idea where the fee filter message is sent, if it is sent at
all. But I have *never* seen any control messages arrive before the
handshake is complete.

> In any case, I fail to see how adding any additional messages which
are ignored by old peers amounts to a lack of backward compatibility.

See preceding messages in this thread, I think it's pretty clearly
spelled out.

e

> On February 13, 2017 11:54:23 AM GMT+01:00, Eric Voskuil <eric@voskuil.org> wrote:
>> On 02/13/2017 02:16 AM, Matt Corallo wrote:
>>> For the reasons Pieter listed, an explicit part of our version
>> handshake and protocol negotiation is the exchange of otherwise-ignored
>> messages to set up optional features.
>>
>> Only if the peer is at the protocol level that allows the message:
>>
>> compact blocks:
>>
>> https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L217-L242
>>
>> fee filter:
>>
>> https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L211-L216
>>
>> send headers:
>>
>> https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L204-L210
>>
>> filters:
>>
>> https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L170-L196
>>
>>> Peers that do not support this ignore such messages, just as if they
>> had indicated they wouldn't support it, see, eg BIP 152's handshake.
>> Not
>> sure why you consider this backwards incompatible, as I would say it's
>> pretty clearly allowing old nodes to communicate just fine.
>>
>> No, it is not the same as BIP152. Control messages apart from BIP151
>> are
>> not sent until *after* the version is negotiated.
>>
>> I assume that BIP151 is different in this manner because it has a
>> desire
>> to negotiate encryption before any other communications, including
>> version.
>>
>> e


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Matt Corallo]

Sorry, I'm still missing it...
So your claim is that a) ignoring incoming messages of a type you do not recognize is bad, and thus b) we should be disconnecting/banning peers which send us messages we do not recognize (can you spell out why? Anyone is free to send your host address messages/transactions they are generating/etc/etc, we don't ban nodes for such messages, as that would be crazy - why should we ban a peer for sending us an extra 50 bytes which we ignore?), and thus c) this would be backwards incompatible with software which does not currently exist?

Usually "backwards incompatible" refers to breaking existing software, not breaking theoretical software. Note that, last I heard, BIP 151 is still a draft, if such software actually exists we can discuss changing it, but there are real wins in sending these messages before VERSION.

On February 13, 2017 12:17:11 PM GMT+01:00, Eric Voskuil <eric@voskuil.org> wrote:
>On 02/13/2017 03:11 AM, Matt Corallo wrote:
>> I believe many, if not all, of those messages are sent irrespective
>of version number.
>
>In the interest of perfect clarity, see your code:
>
>https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L1372-L1403
>
>Inside of the VERACK handler (i.e. after the handshake) there is a peer
>version test before sending SENDCMPCT (and SENDHEADERS).
>
>I have no idea where the fee filter message is sent, if it is sent at
>all. But I have *never* seen any control messages arrive before the
>handshake is complete.
>
>> In any case, I fail to see how adding any additional messages which
>are ignored by old peers amounts to a lack of backward compatibility.
>
>See preceding messages in this thread, I think it's pretty clearly
>spelled out.
>
>e
>
>> On February 13, 2017 11:54:23 AM GMT+01:00, Eric Voskuil
><eric@voskuil.org> wrote:
>>> On 02/13/2017 02:16 AM, Matt Corallo wrote:
>>>> For the reasons Pieter listed, an explicit part of our version
>>> handshake and protocol negotiation is the exchange of
>otherwise-ignored
>>> messages to set up optional features.
>>>
>>> Only if the peer is at the protocol level that allows the message:
>>>
>>> compact blocks:
>>>
>>>
>https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L217-L242
>>>
>>> fee filter:
>>>
>>>
>https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L211-L216
>>>
>>> send headers:
>>>
>>>
>https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L204-L210
>>>
>>> filters:
>>>
>>>
>https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L170-L196
>>>
>>>> Peers that do not support this ignore such messages, just as if
>they
>>> had indicated they wouldn't support it, see, eg BIP 152's handshake.
>>> Not
>>> sure why you consider this backwards incompatible, as I would say
>it's
>>> pretty clearly allowing old nodes to communicate just fine.
>>>
>>> No, it is not the same as BIP152. Control messages apart from BIP151
>>> are
>>> not sent until *after* the version is negotiated.
>>>
>>> I assume that BIP151 is different in this manner because it has a
>>> desire
>>> to negotiate encryption before any other communications, including
>>> version.
>>>
>>> e

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Eric Voskuil]

[-- Attachment #1: Type: text/plain, Size: 8857 bytes --]

On 02/13/2017 03:14 AM, Jonas Schnelli wrote:
>>> Look at feefilter BIP 133
>>> (https://github.com/bitcoin/bips/blob/master/bip-0133.mediawiki#backward-compatibility)
>>> or sendheaders BIP130
>>> (https://github.com/bitcoin/bips/blob/master/bip-0130.mediawiki#backward-compatibility)
>>> Isn't it the same there?
>> No. This is what I was referring to. These messages are enabled by
>> protocol version. If they are received by a node below the version at
>> which they are activated, they are unknown messages, implying an invalid
>> peer. The above messages cannot be sent until *after* the version is
>> negotiated. BIP151 violates this rule by allowing the new control
>> message to be sent *before* the version handshake.

> This indeed is not ideal for compatibility checks, but increases security.

The issue I raised is that it is not backward compatible. It sounds like
you agree but consider it a fair trade. My suggesting was that the BIP
be updated to reflect the lack of compatibility.

> I could not find a protocol specification that said communication must
> be terminated when messages are transmitted before the version handshake
> has been done.

It doesn't need to be specified, most of Bitcoin is unspecified. The
version handshake establishes the negotiated version. It is not possible
to determine if a message is of the negotiated version before the
version is negotiated. All messages apart from this one have followed
that rule.

> Also. BIP151 clearly says that the requesting peer needs to initiate the
> encryption (encinit).

An incoming connection will be dropped due to invalid protocol and
potentially banned depending on the implementation.

> In case of light clients not supporting BIP151 connecting to peers
> supporting BIP151, there should never be transmission of new message
> types specified in BIP151.

Not working with peers not supporting BIP151 is the compatibility issue.
But it sort of seems the intent in this case is to rely on that
incompatibility (expecting connections to nonsupporting peers to fail as
opposed to negotiating).

>>> Once BIP151 is implemented, it would make sense to bump the protocol
>>> version, but this needs to be done once this has been
>>> implemented/deployed.
>> There are already nodes out there breaking connections based on the BIP.

> It could very likely be possible that the initial responding peer tries
> to initiate a encryption session which would mean that BIP151 was not
> implemented correctly.
> Correct me if I'm wrong please.

I did consider the possibility, but there's this:

"Encryption initialization must happen before sending any other messages
to the responding peer (encinit message after a version message must be
ignored)."

https://github.com/bitcoin/bips/blob/master/bip-0151.mediawiki#specification

The BIP does not define "responding" and "requesting" peers, but:

"A peer that supports encryption must accept encryption requests from
all peers... The responding peer accepts the encryption request by
sending a encack message."

This implies the requesting peer is the peer that sends the message. You
seem to be saying that the requesting peer is the one that initiated
the connection and the responding peer is the connection receiver. If
this is the case it should be more clearly documented. But in the
case I experienced the "requester" of an encrypted session was also
the "receiver" of the connection.

>>> Or do I make a mistake somewhere?
>> Yes, the ordering of the messages. New messages can only be added after
>> the handshake negotiates the higher version. Otherwise the handshake is
>> both irrelevant (as Pieter is implying) and broken (for all existing
>> protocol versions).

> I could not find evidence of the protocol specification that would
> forbid (=terminate connection) such messages and I think allowing
> unknown-messages before the version handshake makes the protocol flexible.

Flexible is certainly one word for it. Another way to describe it is
dirty. Allowing invalid messages in a protocol encourages protocol
incompatibility. You end up with various implementations and eventually
have no way of knowing how they are impacted by changes. There could be
a range of peers inter-operating with the full network while running
their own sub-protocols. Given the network is public and strong
identification of peers is undesirable, the invalid messages would
reasonably just get sent to everyone. So over time, what is the
protocol? Due to certain "flexibility" it is already a hassle to
properly implement.

> Are there any reasons we should drop peers if they send us unknown, but
> correctly formatted p2p packages (magic, checksum, etc.) before the
> version handshake, ... but not drop them if we have received unknown
> messages after the version handshake?

There is no reason to treat invalid messages differently based on where
they occur in the communication. After the handshake the agreed version
is known to both peers. As a result there is never a reason for an
invalid message to be sent. Therefore it is always proper to drop a peer
that sends an invalid message.

> I can't see that a such spec. would reduce the DOS attack vector?

This was previously addressed (immediately below).

>>>> As for DOS, waste of bandwidth is not something to be ignored. If a peer
>>>> is flooding a node with addr message the node can manage it because it
>>>> understands the semantics of addr messages. If a node is required to
>>>> allow any message that it cannot understand it has no recourse. It
>>>> cannot determine whether it is under attack or if the behavior is
>>>> correct and for proper continued operation must be ignored.
>>> How do you threat any other not known message types?
>> You may be more familiar with non-validating peers. If a message type is
>> not known it is an invalid message and the peer is immediately dropped.
>> We started seeing early drops in handshakes with bcoin nodes because of
>> this issue.

> If this had happened, it's very likely because the responding peer tried
> to initiate a encryption session which is against BIP151 specs.

See above.

>>> Any peer can send you any type of message anytime.
>> Sure, a peer can do what it wants. It can send photos. But I'm not sure
>> what makes you think it would be correct to maintain the connection when
>> an *invalid* message is received.

> Check:
> https://github.com/bitcoin/bitcoin/blob/a06ede9a138d0fb86b0de17c42b936d9fe6e2158/src/net_processing.cpp#L2595
> I think it was a wise implementation decision to allow unknown (not
> invalid) messages.
> This had allowed us to deploy stuff like compact blocks, feefilter, etc.
> without breaking backward compatibility.
> IMO, without a such flexibility, the deployment complexity would be
> irresponsible high without really solving the DOS problem.

This is a misinterpretation. The failure to validate did not enable
anything except possibly some broken peers not getting dropped. None of
the protocol changes previously deployed require the older version peer
to allow invalid messages. While it may appear otherwise, due to a
particular implementation, it is never necessary to send a message to a
peer that the peer does not understand. The handshake gives each peer
the other peer's version. That obligates the newer peer to conform to
the older (or disconnect if the old is insufficient). That's the nature
of backward compatibility.

>>> Why would your implementation how you threat unknown messages be
>> different for messages specified in BIP151?
>>
>> Because it properly validates the protocol.

> For feefilter or compact block or sendheaders?

Yes, this is the purpose of version negotiation, which is why there are
version and verack messages. And this is also why, in the satoshi
client, two of the above messages are sent from the verack handler. The
feefilter message is sent dynamically but only if the peer's version
allows it.

> You can't link a (unimplemented) specification (improvement process) to
> a protocol version before deployment. Or can you?

I'm not sure I follow your question. The BIP should presumably declare a
version number if one is necessary.

> Once it has been widely deployed, we should set a protocol minversion
> for BIP151, right.

In general you should set a version before it's ever live on the
network. But if it precedes the protocol version negotiation the
protocol version number is moot.

I've been asked to throttle the discussion in the interest of reducing
list volume. I think the issue is pretty clearly addressed at this
point, but feel free to follow up directly and/or via the libbitcoin
development list (copied).

e


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [bitcoin-dev] BIP151 protocol incompatibility

[Jonas Schnelli]

[-- Attachment #1.1: Type: text/plain, Size: 7515 bytes --]


>> This indeed is not ideal for compatibility checks, but increases security.
> The issue I raised is that it is not backward compatible. It sounds like
> you agree but consider it a fair trade. My suggesting was that the BIP
> be updated to reflect the lack of compatibility.
It's still backward compatible. All (?) SPV clients and full node
implementation would still work if BIP151 has been implemented.
Isn't that backward compatibility?
>> I could not find a protocol specification that said communication must
>> be terminated when messages are transmitted before the version handshake
>> has been done.
> It doesn't need to be specified, most of Bitcoin is unspecified. The
> version handshake establishes the negotiated version. It is not possible
> to determine if a message is of the negotiated version before the
> version is negotiated. All messages apart from this one have followed
> that rule.
Yes. But encryption negotiation must be done before the version
handshake (security).
>
>> Also. BIP151 clearly says that the requesting peer needs to initiate the
>> encryption (encinit).
> An incoming connection will be dropped due to invalid protocol and
> potentially banned depending on the implementation.
This is not true. If the connecting peer (assume the SPV client) will
not request encryption, the responding peer (the node) will not enforce
or ask for encryption.
This is clearly written in the BIP.
>> It could very likely be possible that the initial responding peer tries
>> to initiate a encryption session which would mean that BIP151 was not
>> implemented correctly.
>> Correct me if I'm wrong please.
> I did consider the possibility, but there's this:
>
> "Encryption initialization must happen before sending any other messages
> to the responding peer (encinit message after a version message must be
> ignored)."
>
> https://github.com/bitcoin/bips/blob/master/bip-0151.mediawiki#specification
>
> The BIP does not define "responding" and "requesting" peers, but:
>
> "A peer that supports encryption must accept encryption requests from
> all peers... The responding peer accepts the encryption request by
> sending a encack message."
>
> This implies the requesting peer is the peer that sends the message. You
> seem to be saying that the requesting peer is the one that initiated
> the connection and the responding peer is the connection receiver. If
> this is the case it should be more clearly documented. But in the
> case I experienced the "requester" of an encrypted session was also
> the "receiver" of the connection.
I think the BIP makes this very clear. IMO you are trying to hide your
standpoint behind a wired interpretations of the BIP.

From the BIP:
«To request encrypted communication, the requesting peer generates an EC
ephemeral-session-keypair and sends an encinit message to the responding
peer and waits for a encack message. The responding node must do the
same encinit/encack interaction for the opposite communication direction.»

This seems to be pretty clear to me. You can interpret the "requesting
peer" and "responding peer" per message interaction. But then the whole
BIP would make no sense.

I'm happy if you can do a PR on the BIP that makes the wording better.
This would actually be a productive step.
>
>>>> Or do I make a mistake somewhere?
>>> Yes, the ordering of the messages. New messages can only be added after
>>> the handshake negotiates the higher version. Otherwise the handshake is
>>> both irrelevant (as Pieter is implying) and broken (for all existing
>>> protocol versions).
>> I could not find evidence of the protocol specification that would
>> forbid (=terminate connection) such messages and I think allowing
>> unknown-messages before the version handshake makes the protocol flexible.
> Flexible is certainly one word for it. Another way to describe it is
> dirty. Allowing invalid messages in a protocol encourages protocol
> incompatibility. You end up with various implementations and eventually
> have no way of knowing how they are impacted by changes. There could be
> a range of peers inter-operating with the full network while running
> their own sub-protocols. Given the network is public and strong
> identification of peers is undesirable, the invalid messages would
> reasonably just get sent to everyone. So over time, what is the
> protocol? Due to certain "flexibility" it is already a hassle to
> properly implement.
Then you would have to go after all BIPs deployed this way. This
argument has nothing to do with BIP151 it questions the whole new
protocol features deployment.
Again, check this code part:

https://github.com/bitcoin/bitcoin/blob/a06ede9a138d0fb86b0de17c42b936d9fe6e2158/src/net_processing.cpp#L2595


>
>> Are there any reasons we should drop peers if they send us unknown, but
>> correctly formatted p2p packages (magic, checksum, etc.) before the
>> version handshake, ... but not drop them if we have received unknown
>> messages after the version handshake?
> There is no reason to treat invalid messages differently based on where
> they occur in the communication. After the handshake the agreed version
> is known to both peers. As a result there is never a reason for an
> invalid message to be sent. Therefore it is always proper to drop a peer
> that sends an invalid message.
That's up to the implementation. But the current flexibility exists
because we not drop.
Again, see above.
>> I can't see that a such spec. would reduce the DOS attack vector?
> This was previously addressed (immediately below).
No. I'd like to hear from you why the DOS attack vector would be larger
if the encryption neg. would be after the version handshake.
>
>>>>> As for DOS, waste of bandwidth is not something to be ignored. If a peer
>>>>> is flooding a node with addr message the node can manage it because it
>>>>> understands the semantics of addr messages. If a node is required to
>>>>> allow any message that it cannot understand it has no recourse. It
>>>>> cannot determine whether it is under attack or if the behavior is
>>>>> correct and for proper continued operation must be ignored.
>>>> How do you threat any other not known message types?
>>> You may be more familiar with non-validating peers. If a message type is
>>> not known it is an invalid message and the peer is immediately dropped.
>>> We started seeing early drops in handshakes with bcoin nodes because of
>>> this issue.
> Yes, this is the purpose of version negotiation, which is why there are
> version and verack messages. And this is also why, in the satoshi
> client, two of the above messages are sent from the verack handler. The
> feefilter message is sent dynamically but only if the peer's version
> allows it.

Again. Encryption – for the sake of security – must be the first
interaction.
This is exceptional for BIP151 and I'd like to hear the real downsides
of doing that.
>
>> You can't link a (unimplemented) specification (improvement process) to
>> a protocol version before deployment. Or can you?
> I'm not sure I follow your question. The BIP should presumably declare a
> version number if one is necessary.
What? You want to define protocol version number in draft improvement
specifications?
How should that be possible?
It's like defining a new HTML version number if you propose/draft a new
video streaming format.


</jonas>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]