-
Notifications
You must be signed in to change notification settings - Fork 291
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.
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
internal: add ban manager. #2554
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the fact this is breaking the API of connmgr
without being associated with a major module version bump, I don't believe the ban score can or should be moved anyway. It is specifically related to connection managed and it is something that wallet also needs and uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. While I definitely understand and agree with the notion of a ban manager, perhaps I'm just being a bit dense, but, as I further describe inline, I really don't understand the overall design here. It seems to me like everything is exactly backwards. I would expect the ban manager to manage the banning of the peers, but instead it seems like there is a whole bunch of things that I would expect to be the responsibility of the ban manager, like whitelisting and tracking of the ban score, being handled by each individual peer.
1ba8376
to
74f7bcb
Compare
74f7bcb
to
d0171d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much better with the updates. I have various comments for some outstanding things and once those are addressed I think this will be good enough. There are various other improvements I can see that would be nice for the future, but I think it's better to just focus on getting the broad strokes in first.
05cbf79
to
26ebca4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updates look good overall and address almost everything. The only major thing I see is that it really should be using the peer instances as the key. I have several comments inline to explain better. I didn't comment on every case, but it should be enough to show what I mean.
e7a688b
to
1908649
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to test it and assert the new banning conditions, but otherwise, just a few nits left. It's looking solid in terms of implementation.
efe3f7e
to
6ea0925
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this yet, but reading the code, I don't see anywhere that it is adding the peers to the ban manager when they connect nor removing them when they disconnect. I also don't see a function to actually ban the peer being specified in the config which means it would certainly panic for invoking a nil function.
server.go
Outdated
func (sp *serverPeer) OnHeaders(_ *peer.Peer, msg *wire.MsgHeaders) { | ||
func (sp *serverPeer) OnHeaders(p *peer.Peer, msg *wire.MsgHeaders) { | ||
// Ban peers sending empty headers requests. | ||
if len(msg.Headers) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revert this change for this PR. I'm fairly certain it is possible for an empty headers message to be sent in valid circumstances when reaching the chain tip during sync and this would end up banning them due to no fault of their own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was marked resolved, but it isn't. It still needs to be removed.
After going back and forth on this, please go ahead and put the code in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updates have addresses all the comments, but I see there are quite a few more issues like it's still not setting a callback and it isn't actually keeping track of the banned peers no disconnecting them when they connect.
I'm going to update the test cases to cover the missing functionality you pointed out and implement accordingly. |
server.go
Outdated
func (sp *serverPeer) OnHeaders(_ *peer.Peer, msg *wire.MsgHeaders) { | ||
func (sp *serverPeer) OnHeaders(p *peer.Peer, msg *wire.MsgHeaders) { | ||
// Ban peers sending empty headers requests. | ||
if len(msg.Headers) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was marked resolved, but it isn't. It still needs to be removed.
Alright, so I'm still running into issues with this and banning still isn't working properly in all caes. Given the impending release, I really don't want to spend any more time on this. Please revert all changes to everything except for the files under That way we'll have the primary work on the ban manager itself as a starting point for moving forward, but it will not be used for now until we've had time to iterate on the design and properly integrate it. |
This refactors peer banning functionality and ban score tracking into a ban manager. Associated tests have been added.
5b7577a
to
fbd50e4
Compare
This refactors peer banning functionality and ban score tracking into a ban manager. Associated tests have been added.
Integrating the ban manger will be done in a subsequent PR.