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.

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

Added unban peer feature #286

Merged
merged 2 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions banman/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ type Store interface {

// Status returns the ban status for a given IP network.
Status(*net.IPNet) (Status, error)

// UnbanIPNet removes the ban imposed on the specified peer.
UnbanIPNet(ipNet *net.IPNet) error
}

// NewStore returns a Store backed by a database.
Expand Down Expand Up @@ -136,6 +139,38 @@ func (s *banStore) BanIPNet(ipNet *net.IPNet, reason Reason, duration time.Durat
})
}

// UnbanIPNet removes a ban record for the IP network within the store.
func (s *banStore) UnbanIPNet(ipNet *net.IPNet) error {
err := walletdb.Update(s.db, func(tx walletdb.ReadWriteTx) error {
banStore := tx.ReadWriteBucket(banStoreBucket)
if banStore == nil {
return ErrCorruptedStore
}

banIndex := banStore.NestedReadWriteBucket(banBucket)
Crypt-iQ marked this conversation as resolved.
Show resolved Hide resolved
if banIndex == nil {
return ErrCorruptedStore
}

reasonIndex := banStore.NestedReadWriteBucket(reasonBucket)
if reasonIndex == nil {
return ErrCorruptedStore
}

var ipNetBuf bytes.Buffer
if err := encodeIPNet(&ipNetBuf, ipNet); err != nil {
return fmt.Errorf("unable to encode %v: %v", ipNet,
err)
}

k := ipNetBuf.Bytes()

return removeBannedIPNet(banIndex, reasonIndex, k)
})

return err
}

// addBannedIPNet adds an entry to the ban store for the given IP network.
func addBannedIPNet(banIndex, reasonIndex walletdb.ReadWriteBucket,
ipNetKey []byte, reason Reason, duration time.Duration) error {
Expand Down
7 changes: 7 additions & 0 deletions banman/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/btcsuite/btcwallet/walletdb"
_ "github.com/btcsuite/btcwallet/walletdb/bdb"
"github.com/lightninglabs/neutrino/banman"
"github.com/stretchr/testify/require"
)

// createTestBanStore creates a test Store backed by a boltdb instance.
Expand Down Expand Up @@ -123,4 +124,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)

// Test UnbanIPNet.
require.NoError(t, banStore.UnbanIPNet(ipNet1))
Crypt-iQ marked this conversation as resolved.
Show resolved Hide resolved

// We would now check that ipNet1 is indeed unbanned.
checkBanStore(ipNet1, false, 0, 0)
}
18 changes: 18 additions & 0 deletions neutrino.go
Original file line number Diff line number Diff line change
Expand Up @@ -1081,6 +1081,24 @@ func (s *ChainService) BanPeer(addr string, reason banman.Reason) error {
return s.banStore.BanIPNet(ipNet, reason, BanDuration)
}

// UnbanPeer connects and unbans a previously banned peer.
func (s *ChainService) UnbanPeer(addr string, parmanent bool) error {
log.Infof("UnBanning peer %v", addr)

ipNet, err := banman.ParseIPNet(addr, nil)
if err != nil {
return fmt.Errorf("unable to parse IP network for peer %v: %v",
addr, err)
}

err = s.banStore.UnbanIPNet(ipNet)
if err != nil {
return fmt.Errorf("unable to unban peer: %v", err)
}

return s.ConnectNode(addr, parmanent)
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for my earlier suggestion, I think this should be in a goroutine so we aren't blocking?

cc @Roasbeef

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 the blocking you are referring to might emanate from here?

neutrino/notifications.go

Lines 351 to 360 in 9130a5e

select {
case s.query <- connectNodeMsg{
addr: addr,
permanent: permanent,
reply: replyChan,
}:
return <-replyChan
case <-s.quit:
return nil
}

From what I have seen from other exported functions on the chainservice, I do not think they take into account this blocking?

Copy link
Contributor

Choose a reason for hiding this comment

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

See above in BanPeer, the disconnect is in a goroutine

Copy link
Contributor Author

@Chinwendu20 Chinwendu20 May 20, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

// IsBanned returns true if the peer is banned, and false otherwise.
func (s *ChainService) IsBanned(addr string) bool {
ipNet, err := banman.ParseIPNet(addr, nil)
Expand Down
Loading