Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

neutrino: don't connect to peer which deny us relay #190

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

neutrino: don't connect to peer which deny us relay #190

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions neutrino.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,17 @@ func (sp *ServerPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) *wire.MsgRej
// the local clock to keep the network time in sync.
sp.server.timeSource.AddTimeSample(sp.Addr(), msg.Timestamp)

// If the peer doesn't allow us to relay any transactions to them, then
// we won't add them as a peer, as they aren't of much use to us.
if msg.DisableRelayTx {
log.Debugf("%v does not allow transaction relay, disconecting",
sp)

sp.Disconnect()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also ban them to prevent connecting to them again?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@wpaulino wpaulino Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems likely to happen if the address manager continues to return it as part of newAddressFunc, which is very aggressive at the moment.


return nil
}

// Check to see if the peer supports the latest protocol version and
// service bits required to service us. If not, then we'll disconnect
// so we can find compatible peers.
Expand Down