From 2c4ace022e455033381337aafa3e0ce75db709e6 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 20 Jan 2020 19:02:19 +0300 Subject: [PATCH] network/config: redesign ping timeout handling a bit 1) Make timeout a timeout, don't do magic ping counts. 2) Drop additional timer from the main peer's protocol loop, create it dynamically and make it disconnect the peer. 3) Don't expose the ping counter to the outside, handle more logic inside the Peer. Relates to #430. --- config/protocol.mainnet.yml | 4 +- config/protocol.privnet.docker.four.yml | 4 +- config/protocol.privnet.docker.one.yml | 4 +- config/protocol.privnet.docker.single.yml | 4 +- config/protocol.privnet.docker.three.yml | 4 +- config/protocol.privnet.docker.two.yml | 4 +- config/protocol.privnet.yml | 4 +- config/protocol.testnet.yml | 4 +- config/protocol.unit_testnet.yml | 4 +- pkg/network/helper_test.go | 16 +++---- pkg/network/peer.go | 11 +++-- pkg/network/server.go | 9 ++-- pkg/network/tcp_peer.go | 56 ++++++++++++----------- 13 files changed, 67 insertions(+), 61 deletions(-) diff --git a/config/protocol.mainnet.yml b/config/protocol.mainnet.yml index 2fd9c94b6..2e50755ae 100644 --- a/config/protocol.mainnet.yml +++ b/config/protocol.mainnet.yml @@ -50,8 +50,8 @@ ApplicationConfiguration: Relay: true DialTimeout: 3 ProtoTickInterval: 2 - PingInterval: 60 - PingTimeout: 60 + PingInterval: 30 + PingTimeout: 90 MaxPeers: 100 AttemptConnPeers: 20 MinPeers: 5 diff --git a/config/protocol.privnet.docker.four.yml b/config/protocol.privnet.docker.four.yml index 4981f4fee..2fe502d32 100644 --- a/config/protocol.privnet.docker.four.yml +++ b/config/protocol.privnet.docker.four.yml @@ -41,8 +41,8 @@ ApplicationConfiguration: Relay: true DialTimeout: 3 ProtoTickInterval: 2 - PingInterval: 60 - PingTimeout: 60 + PingInterval: 30 + PingTimeout: 90 MaxPeers: 10 AttemptConnPeers: 5 MinPeers: 3 diff --git a/config/protocol.privnet.docker.one.yml b/config/protocol.privnet.docker.one.yml index 28f7e9a3e..95792063c 100644 --- a/config/protocol.privnet.docker.one.yml +++ b/config/protocol.privnet.docker.one.yml @@ -41,8 +41,8 @@ ApplicationConfiguration: Relay: true DialTimeout: 3 ProtoTickInterval: 2 - PingInterval: 60 - PingTimeout: 60 + PingInterval: 30 + PingTimeout: 90 MaxPeers: 10 AttemptConnPeers: 5 MinPeers: 3 diff --git a/config/protocol.privnet.docker.single.yml b/config/protocol.privnet.docker.single.yml index 3c12813aa..fbc946ac8 100644 --- a/config/protocol.privnet.docker.single.yml +++ b/config/protocol.privnet.docker.single.yml @@ -35,8 +35,8 @@ ApplicationConfiguration: Relay: true DialTimeout: 3 ProtoTickInterval: 2 - PingInterval: 60 - PingTimeout: 60 + PingInterval: 30 + PingTimeout: 90 MaxPeers: 10 AttemptConnPeers: 5 MinPeers: 0 diff --git a/config/protocol.privnet.docker.three.yml b/config/protocol.privnet.docker.three.yml index d0b83f35a..b36dae766 100644 --- a/config/protocol.privnet.docker.three.yml +++ b/config/protocol.privnet.docker.three.yml @@ -41,8 +41,8 @@ ApplicationConfiguration: Relay: true DialTimeout: 3 ProtoTickInterval: 2 - PingInterval: 60 - PingTimeout: 60 + PingInterval: 30 + PingTimeout: 90 MaxPeers: 10 AttemptConnPeers: 5 MinPeers: 3 diff --git a/config/protocol.privnet.docker.two.yml b/config/protocol.privnet.docker.two.yml index 9936ae13d..8406c4a19 100644 --- a/config/protocol.privnet.docker.two.yml +++ b/config/protocol.privnet.docker.two.yml @@ -41,8 +41,8 @@ ApplicationConfiguration: Relay: true DialTimeout: 3 ProtoTickInterval: 2 - PingInterval: 60 - PingTimeout: 60 + PingInterval: 30 + PingTimeout: 90 MaxPeers: 10 AttemptConnPeers: 5 MinPeers: 3 diff --git a/config/protocol.privnet.yml b/config/protocol.privnet.yml index 247faee82..6166bd308 100644 --- a/config/protocol.privnet.yml +++ b/config/protocol.privnet.yml @@ -41,8 +41,8 @@ ApplicationConfiguration: Relay: true DialTimeout: 3 ProtoTickInterval: 2 - PingInterval: 60 - PingTimeout: 60 + PingInterval: 30 + PingTimeout: 90 MaxPeers: 10 AttemptConnPeers: 5 MinPeers: 3 diff --git a/config/protocol.testnet.yml b/config/protocol.testnet.yml index 6bc778911..f936dc261 100644 --- a/config/protocol.testnet.yml +++ b/config/protocol.testnet.yml @@ -50,8 +50,8 @@ ApplicationConfiguration: Relay: true DialTimeout: 3 ProtoTickInterval: 2 - PingInterval: 60 - PingTimeout: 60 + PingInterval: 30 + PingTimeout: 90 MaxPeers: 100 AttemptConnPeers: 20 MinPeers: 5 diff --git a/config/protocol.unit_testnet.yml b/config/protocol.unit_testnet.yml index 7fe53358c..09cfef518 100644 --- a/config/protocol.unit_testnet.yml +++ b/config/protocol.unit_testnet.yml @@ -40,8 +40,8 @@ ApplicationConfiguration: Relay: true DialTimeout: 3 ProtoTickInterval: 2 - PingInterval: 60 - PingTimeout: 60 + PingInterval: 30 + PingTimeout: 90 MaxPeers: 50 AttemptConnPeers: 5 MinPeers: 1 diff --git a/pkg/network/helper_test.go b/pkg/network/helper_test.go index f41685f78..30ba4321a 100644 --- a/pkg/network/helper_test.go +++ b/pkg/network/helper_test.go @@ -160,7 +160,7 @@ type localPeer struct { handshaked bool t *testing.T messageHandler func(t *testing.T, msg *Message) - pingSent int + pingSent int } func newLocalPeer(t *testing.T) *localPeer { @@ -206,9 +206,6 @@ func (p *localPeer) Version() *payload.Version { func (p *localPeer) LastBlockIndex() uint32 { return p.lastBlockIndex } -func (p *localPeer) UpdateLastBlockIndex(newIndex uint32) { - p.lastBlockIndex = newIndex -} func (p *localPeer) HandleVersion(v *payload.Version) error { p.version = v return nil @@ -225,11 +222,14 @@ func (p *localPeer) HandleVersionAck() error { p.handshaked = true return nil } -func (p *localPeer) GetPingSent() int { - return p.pingSent +func (p *localPeer) SendPing() error { + p.pingSent++ + return nil } -func (p *localPeer) UpdatePingSent(newValue int) { - p.pingSent = newValue +func (p *localPeer) HandlePong(pong *payload.Ping) error { + p.lastBlockIndex = pong.LastBlockIndex + p.pingSent-- + return nil } func (p *localPeer) Handshaked() bool { diff --git a/pkg/network/peer.go b/pkg/network/peer.go index 3fe9cb23d..d063c5ddf 100644 --- a/pkg/network/peer.go +++ b/pkg/network/peer.go @@ -36,8 +36,12 @@ type Peer interface { EnqueueHPPacket([]byte) error Version() *payload.Version LastBlockIndex() uint32 - UpdateLastBlockIndex(lbIndex uint32) Handshaked() bool + + // SendPing enqueues a ping message to be sent to the peer and does + // appropriate protocol handling like timeouts and outstanding pings + // management. + SendPing() error SendVersion(*Message) error SendVersionAck(*Message) error // StartProtocol is a goroutine to be run after the handshake. It @@ -45,6 +49,7 @@ type Peer interface { StartProtocol() HandleVersion(*payload.Version) error HandleVersionAck() error - GetPingSent() int - UpdatePingSent(int) + + // HandlePong checks pong contents against Peer's state and updates it. + HandlePong(pong *payload.Ping) error } diff --git a/pkg/network/server.go b/pkg/network/server.go index 6c98ed665..3c7e942e8 100644 --- a/pkg/network/server.go +++ b/pkg/network/server.go @@ -29,7 +29,6 @@ const ( maxBlockBatch = 200 maxAddrsToSend = 200 minPoolCount = 30 - defaultPingLimit = 4 ) var ( @@ -373,12 +372,10 @@ func (s *Server) handlePing(p Peer, ping *payload.Ping) error { // handlePing processes pong request. func (s *Server) handlePong(p Peer, pong *payload.Ping) error { - pingSent := p.GetPingSent() - if pingSent == 0 { - return errors.New("pong message wasn't expected") + err := p.HandlePong(pong) + if err != nil { + return err } - p.UpdatePingSent(pingSent - 1) - p.UpdateLastBlockIndex(pong.LastBlockIndex) if s.chain.HeaderHeight() < pong.LastBlockIndex { return s.requestHeaders(p) } diff --git a/pkg/network/tcp_peer.go b/pkg/network/tcp_peer.go index 329a51130..5683cfe39 100644 --- a/pkg/network/tcp_peer.go +++ b/pkg/network/tcp_peer.go @@ -25,7 +25,9 @@ const ( ) var ( - errStateMismatch = errors.New("tried to send protocol message before handshake completed") + errStateMismatch = errors.New("tried to send protocol message before handshake completed") + errPingPong = errors.New("ping/pong timeout") + errUnexpectedPong = errors.New("pong message wasn't expected") ) // TCPPeer represents a connected remote node in the @@ -51,7 +53,8 @@ type TCPPeer struct { wg sync.WaitGroup // number of sent pings. - pingSent int + pingSent int + pingTimer *time.Timer } // NewTCPPeer returns a TCPPeer structure based on the given connection. @@ -191,7 +194,6 @@ func (p *TCPPeer) StartProtocol() { } timer := time.NewTimer(p.server.ProtoTickInterval) - pingTimer := time.NewTimer(p.server.PingTimeout) for { select { case <-p.done: @@ -210,20 +212,12 @@ func (p *TCPPeer) StartProtocol() { } else { diff := time.Now().UTC().Unix() - p.server.getLastBlockTime() if diff > int64(p.server.PingInterval/time.Second) { - p.UpdatePingSent(p.GetPingSent() + 1) - err = p.EnqueueMessage(NewMessage(p.server.Net, CMDPing, payload.NewPing(p.server.id, p.server.chain.HeaderHeight()))) + err = p.SendPing() } } if err == nil { timer.Reset(p.server.ProtoTickInterval) } - case <-pingTimer.C: - if p.GetPingSent() > defaultPingLimit { - err = errors.New("ping/pong timeout") - } else { - pingTimer.Reset(p.server.PingTimeout) - p.UpdatePingSent(0) - } } if err != nil { timer.Stop() @@ -350,23 +344,33 @@ func (p *TCPPeer) LastBlockIndex() uint32 { return p.lastBlockIndex } -// UpdateLastBlockIndex updates last block index. -func (p *TCPPeer) UpdateLastBlockIndex(newIndex uint32) { +// SendPing sends a ping message to the peer and does appropriate accounting of +// outstanding pings and timeouts. +func (p *TCPPeer) SendPing() error { p.lock.Lock() - defer p.lock.Unlock() - p.lastBlockIndex = newIndex + p.pingSent++ + if p.pingTimer == nil { + p.pingTimer = time.AfterFunc(p.server.PingTimeout, func() { + p.Disconnect(errPingPong) + }) + } + p.lock.Unlock() + return p.EnqueueMessage(NewMessage(p.server.Net, CMDPing, payload.NewPing(p.server.id, p.server.chain.HeaderHeight()))) } -// GetPingSent returns flag whether ping was sent or not. -func (p *TCPPeer) GetPingSent() int { - p.lock.RLock() - defer p.lock.RUnlock() - return p.pingSent -} - -// UpdatePingSent updates pingSent value. -func (p *TCPPeer) UpdatePingSent(newValue int) { +// HandlePong handles a pong message received from the peer and does appropriate +// accounting of outstanding pings and timeouts. +func (p *TCPPeer) HandlePong(pong *payload.Ping) error { p.lock.Lock() defer p.lock.Unlock() - p.pingSent = newValue + if p.pingTimer != nil && !p.pingTimer.Stop() { + return errPingPong + } + p.pingTimer = nil + p.pingSent-- + if p.pingSent < 0 { + return errUnexpectedPong + } + p.lastBlockIndex = pong.LastBlockIndex + return nil }