neutrino: don't connect to peer which deny us relay #190
Conversation
In this commit, we fix an existing bug that would allow us to connect to the peer that explicitly signals they don't accept transaction relay. When we send our version message we explicitly set the `DisableTxRelay` bool to `false`, but then fail to check this value for any remote peers. With this change, we'll disconnect a peer in the `OnVersion` call back if they signal no transaction relay.
| log.Debugf("%v does not allow transaction relay, disconecting", | ||
| sp) | ||
|
|
||
| sp.Disconnect() |
There was a problem hiding this comment.
Should we also ban them to prevent connecting to them again?
There was a problem hiding this comment.
IMO no, as this can change for them with just a restart. In the end we aren't fundamentally incompatible. I think we also saw this issue in the past at times when it seemed like things weren't being broadcast, it may have been the case that we were connected to a node running in blocks only mode.
There was a problem hiding this comment.
IMO no, as this can change for them with just a restart.
Can't the same be said about filter support?
Any reason to not return a Reject message with the reason?
There was a problem hiding this comment.
By just disconnecting without banning, don't we risk going into a connection loop? How likely are we to attempt connecting to this peer again?
There was a problem hiding this comment.
Seems likely to happen if the address manager continues to return it as part of newAddressFunc, which is very aggressive at the moment.
|
A node without transaction relay could still be providing filters. That's how I found this :-) |
|
We could opt to allow the connection and track which peers we can relay transactions to, though we'd want to make sure that we have enough peers so that transactions are actually relayed when the time comes. |
Possibly, but IMO in order to ensure out transactions are well propagated, we should only be connected to peers that can relay our transaction. The current transaction broadcast package also has this assumption baked in. |
|
@Roasbeef, remember to re-request review from reviewers for your latest update |
2 similar comments
|
@Roasbeef, remember to re-request review from reviewers for your latest update |
|
@Roasbeef, remember to re-request review from reviewers for your latest update |
|
!lightninglabs-deploy mute |
In this commit, we fix an existing bug that would allow us to connect to
the peer that explicitly signals they don't accept transaction relay.
When we send our version message we explicitly set the
DisableTxRelaybool to
false, but then fail to check this value for any remote peers.With this change, we'll disconnect a peer in the
OnVersioncall backif they signal no transaction relay.