Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

User-manual ‘disconnect’ adds a peer to a blacklist #2664

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ChrisSchinnerl
Copy link
Contributor

This PR will cause a user initiated disconnect to blacklist a peer. The peer will only be blacklisted if it was actually connected to the node. For blacklisting nodes that are not connected we should probably add a separate endpoint (if we decide that we want that feature).
The blacklist is persisted in the blacklist.json file.

// Do the same for the blacklist.
if loadErr := g.loadBlacklist(); loadErr != nil && !os.IsNotExist(loadErr) {
return nil, loadErr
}
Copy link
Member

Choose a reason for hiding this comment

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

all persist loading should happen in load

// Disconnect terminates a connection to a peer.
Disconnect(NetAddress) error

// DisconnectManual is a Disconnect wrapper for a user-initiated disconnect
DisconnectManual(NetAddress) error
Copy link
Member

Choose a reason for hiding this comment

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

why do we need these? Wouldn't a Blacklist method make more sense? It's not clear to me that a user-initiated disconnect always implies that the user wants to blacklist the peer.

As always, we may want to check how bitcoind solves these problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think bitcoind has a setban function that allows a user to ban a node for a certain period of time.
If we don't like this behaviour we can also change this PR to introduce a new endpoint. I started it because it was one of the todos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DavidVorick any thoughts on that?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants