Skip to content

server: relax server connection dropping #1351

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

Closed
wants to merge 1 commit into from
Closed

server: relax server connection dropping #1351

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
24 changes: 20 additions & 4 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ type server struct {
outboundPeers map[string]*peer

peerConnectedListeners map[string][]chan<- struct{}
peerSequence map[string]byte
Copy link
Member

Choose a reason for hiding this comment

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

When's the sequence for a peer to be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I think there are cases where we can safely delete and not alter correctness. Will do some research.

Fwiw we can always delete upon returning to 0


persistentPeers map[string]struct{}
persistentPeersBackoff map[string]time.Duration
Expand Down Expand Up @@ -243,6 +244,7 @@ func newServer(listenAddrs []string, chanDB *channeldb.DB, cc *chainControl,
inboundPeers: make(map[string]*peer),
outboundPeers: make(map[string]*peer),
peerConnectedListeners: make(map[string][]chan<- struct{}),
peerSequence: make(map[string]byte),

globalFeatures: lnwire.NewFeatureVector(globalFeatures,
lnwire.GlobalFeatures),
Expand Down Expand Up @@ -1587,14 +1589,28 @@ func (s *server) peerConnected(conn net.Conn, connReq *connmgr.ConnReq,
// utilize the ordering of the local and remote public key. If we didn't use
// such a tie breaker, then we risk _both_ connections erroneously being
// dropped.
func shouldDropLocalConnection(local, remote *btcec.PublicKey) bool {
func (s *server) shouldDropLocalConnection(local, remote *btcec.PublicKey) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Does this no longer need these two symptom targeting commits?

18de558
418267e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, still need those. This algo assumes that we are correctly closing only one connection

localPubBytes := local.SerializeCompressed()
remotePubPbytes := remote.SerializeCompressed()

// The connection that comes from the node with a "smaller" pubkey
// should be kept. Therefore, if our pubkey is "greater" than theirs, we
// should drop our established connection.
return bytes.Compare(localPubBytes, remotePubPbytes) > 0
supposedToDrop := bytes.Compare(localPubBytes, remotePubPbytes) > 0
Copy link
Member

Choose a reason for hiding this comment

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

godoc comment for the method hasn't been updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


pubStr := string(remotePubPbytes)

// Lookup the current sequence number for this peer. If no entry is
// found, we start in state 0. We take the states mod 3 so that we
// the sequence eventually terminates, without making assumptions about
// the starting states of the peers.
state := s.peerSequence[pubStr]
s.peerSequence[pubStr] = (state + 1) % 3

// Stay honest to the protocol only if we are in state 0.
stayHonest := state == 0

return supposedToDrop == stayHonest
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very hard to understand without having read the description of the protocol beforehand. Maybe better to write code/comments as "we choose A<B in state 0, and B>A in state 1 and 2"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you squint, this is actually computing the xor of the two values. Though it could be documented/explained better :)

}

// InboundPeerConnected initializes a new peer in response to a new inbound
Expand Down Expand Up @@ -1657,7 +1673,7 @@ func (s *server) InboundPeerConnected(conn net.Conn) {
// we'll close out this connection s.t there's only a single
// connection between us.
localPub := s.identityPriv.PubKey()
if !shouldDropLocalConnection(localPub, nodePub) {
if !s.shouldDropLocalConnection(localPub, nodePub) {
srvrLog.Warnf("Received inbound connection from "+
"peer %x, but already connected, dropping conn",
nodePub.SerializeCompressed())
Expand Down Expand Up @@ -1754,7 +1770,7 @@ func (s *server) OutboundPeerConnected(connReq *connmgr.ConnReq, conn net.Conn)
// so, in order to ensure we don't have any duplicate
// connections.
localPub := s.identityPriv.PubKey()
if shouldDropLocalConnection(localPub, nodePub) {
if s.shouldDropLocalConnection(localPub, nodePub) {
srvrLog.Warnf("Established outbound connection to "+
"peer %x, but already connected, dropping conn",
nodePub.SerializeCompressed())
Expand Down