It was possible for connections to bitcoind nodes to be closed from
their side if the OnVersion callback queued a message to send. This is
because bitcoind expects a verack message before attempting to process
any other messages. To address this, we ensure the verack is sent as
part of the handshake process, such that any queued messages happen
after the fact.
This modifies the peer code which deals with advertising service flags
via the net address fields of the version message as follows:
- For outgoing connections:
- Set the local netaddress services to what the local peer supports
- Set the remote netaddress services to 0 to indicate no services as
they are still unknown
- For incoming connections:
- Set the local netaddress services to what the local peer supports
- Set the remote netaddress services to the what was advertised by the
remote peer in its version message
This modifies the OnVersion callback to allow a reject message to be
returned in which case the message will be sent to the peer and the peer
will be disconnected.
Backported from Decred.
This modifies the negotiation logic to ensure the callback has the
opportunity to see the message before the peer is disconnected and
improves the error handling when reading the remote version message.
It also has the side effect of ensuring the protocol version is
negotiated before sending reject messages with the exception of the
first message not being a version message since negotiation is not
possible in that case.
This is being changed because it is useful for the server to see the
message regardless in order to have the opportunity to things such as
update the address manager and reject peers that don't have desired
services.
Backported from Decred.
This rearranges some of the function definitions that pertain to initial
peer version negotiation and bringup so they are more consistent with
the preferred order used throughout the codebase. In particular, the
functions are defined before they're first used and generally as close
as possible to the first use when they're defined in the same file.
There are no functional changes.
Backported from Decred.
In this commit, we patch a goroutine leak within the peer struct. This
goroutine leak can happen, if the remote side fails to actually finish
negotiation the protocol before our timeout ticker ticks. In this case,
the goroutine will be blocked on a send, as the channel is unfired. An
example trace from a btcd node I had on testnet showed:
```
3183 @ 0x42e78a 0x42e83e 0x40540b 0x4051a5 0x872f76 0x45bfc1
```
So, all instances of the goroutine failing to exit due to the remote
peer not finishing the p2p version negotiation handshake.
Our fix is simple: make the `negotiateErr' channel unbuffered. With this
simple change, we ensure that the goroutine will always exit even in the
case that the parent goroutine exists due to a timeout. # Please enter
the commit message for your changes. Lines starting
This commit modifies the base peer struct to ascertain when a peer is
able to understand the new witness encoding, and specify the peer’s
supported encoding explicitly before/after the version handshake.
This changes the nonce generated to detect self connections over to use
pseudo randoms instead of a cryptographically random nonce.
There is really not a good reason for it to be cryptographically strong,
using the prng is much faster, and the prng also doesn't burn entropy.
This removes the field that tracks whether the version has been sent
since it is no longer used after the version negotiation was separated
from the main read and write code.
Replace assignments to individual fields of wire.NetAddress with
creating the entire object at once, as one would do if the type was
immutable.
In some places this replaces the creation of a NetAddress with a
high-precision timestamp with a call to a 'constructor' that converts
the timestamp to single second precision. For consistency, the tests
have also been changed to use single-precision timestamps.
Lastly, the number of allocations in readNetAddress have been reduced by
reading the services directly into the NetAddress instead of first into
a temporary variable.
This modifies the error handling path in the peer read loop such that it
will no longer log an error when the error is io.ErrUnexpectedEOF. This
is being done because that error is almost always the result of a peer
being remotely disconnected and thus it isn't useful information to log.
However, since it might actually be due to a malformed message, a reject
message is still queued up to be sent back to the peer (which will
simply be discarded if the peer disconnected) before it is disconnected.
While it would be ideal to only log if it's not due to a disconnect, and
the code already attempts to handle that, it's not 100% possible to
detect upon the read returning an error without attempting to read again
which will not happen until the next loop iteration.
The inability for a peer to negotiate is not something that should be a
warning which implies something is wrong. On the contrary, it is quite
expected that various peers will connect (or be connected to) that are
unable to properly negotiate for a variety of reasons. One example would
be a peer that is too old.
Also, while here take care of a few style nits.
This adds a new field to the peer struct which stores the protocol
version advertised by the remote peer and updates the StatsSnapshot to
return the advertised version instead of the negotiated version.
Older nodes previously added the IP and port information to the address
manager which proved to be unreliable as an inbound connection from a
peer didn't necessarily mean the peer itself accepted inbound
connections.
This also fixes a bug where the peer package was incorrectly sending
the peer's services as its own.
This makes the enforcement of the bloom filter service bit much more
strict. In particular, it does the following:
- Moves the enforcement of the bloom filter service bit out of the peer
package and into the server so the server can ban as necessary
- Disconnect peers that send filter commands when the server is
configured to disable them regardless of the protocol version
- Bans peers that are a high enough protocol version that they are
supposed to observe the service bit is disabled, but ignore it and
send filter commands regardless.
As an added bonus, this fixes the old logic which had a bug in that it
was examining the *remote* peer's supported services in order to choose
whether or not to disconnect instead of the *local* server's supported
services.
