Added unban peer feature #286
Conversation
|
Hello @Roasbeef and @positiveblue . I have added the test |
banman/store_test.go
Outdated
| @@ -123,4 +123,10 @@ func TestBanStore(t *testing.T) { | |||
| // We'll query for second IP network again as it should now be unknown | |||
| // to the BanStore. We should expect not to find anything regarding it. | |||
| checkBanStore(ipNet2, false, 0, 0) | |||
|
|
|||
| //Unban ipNet1 | |||
There was a problem hiding this comment.
Style nit: comments should be complete sentences.
neutrino.go
Outdated
| } | ||
| s.connManager.Connect(&connmgr.ConnReq{ | ||
| Addr: tcpAddr, | ||
| Permanent: true, |
There was a problem hiding this comment.
Perhaps this should be optional? So a bool that controls if we'll try to permanently connecvt to them after banning.
There was a problem hiding this comment.
I don't think this defer is necessary?
There was a problem hiding this comment.
Are you saying we should skip the defer function and just connect the peer that has just been unbanned?
banman/store.go
Outdated
|
|
||
| var ipNetBuf bytes.Buffer | ||
| if err := encodeIPNet(&ipNetBuf, ipNet); err != nil { | ||
| return fmt.Errorf("unable to encode %v: %v", ipNet, err) |
There was a problem hiding this comment.
I think it has the right line length?
There was a problem hiding this comment.
Should be wrapped to 80 columns.
neutrino.go
Outdated
| } | ||
| s.connManager.Connect(&connmgr.ConnReq{ | ||
| Addr: tcpAddr, | ||
| Permanent: true, |
There was a problem hiding this comment.
I don't think this defer is necessary?
|
@Chinwendu20 still interested in finishing this up? |
|
Thanks @Roasbeef wow, did not notice the comments |
Chinwendu20
force-pushed
the
UBPeer
branch
3 times, most recently
from
c4ac8ac
to
e420c11
Compare
banman/store.go
Outdated
|
|
||
| var ipNetBuf bytes.Buffer | ||
| if err := encodeIPNet(&ipNetBuf, ipNet); err != nil { | ||
| return fmt.Errorf("unable to encode %v: %v", ipNet, err) |
There was a problem hiding this comment.
Should be wrapped to 80 columns.
| return fmt.Errorf("unable to unban peer: %v", err) | ||
| } | ||
|
|
||
| return s.ConnectNode(addr, parmanent) |
There was a problem hiding this comment.
sorry for my earlier suggestion, I think this should be in a goroutine so we aren't blocking?
cc @Roasbeef
There was a problem hiding this comment.
I think the blocking you are referring to might emanate from here?
Lines 351 to 360 in 9130a5e
From what I have seen from other exported functions on the chainservice, I do not think they take into account this blocking?
There was a problem hiding this comment.
See above in BanPeer, the disconnect is in a goroutine
There was a problem hiding this comment.
Thanks for pointing this out. The Disconnect in that function is quite different as it is not a chain service method like what I was referring to. I was referring to other exported chainservice methods just like the ConnectNode function used in UnbanPeer that send a message to the query channel and how I have seen that they do not take into account a potential blocking.
I am not sure if we should continue with that practice.
There was a problem hiding this comment.
It depends on how it's used in practice, which it's not here. So I'm fine with it as-is and we can adjust it later based on usage
Signed-off-by: Ononiwu Maureen <amaka013@gmail.com>
Signed-off-by: Ononiwu Maureen <amaka013@gmail.com>
|
@guggero I think we can merge this now? |
Changes:
Link to issue: #253
Link to previous PR for context:
#270