From 7d69fb9ba6a009817abdc62bc30b25ddc938225e Mon Sep 17 00:00:00 2001
From: Olaoluwa Osuntokun <laolu32@gmail.com>
Date: Thu, 16 Jul 2020 18:05:45 -0700
Subject: [PATCH] peer: prevent last block height going backwards

This modifies the UpdateLastBlockHeight function to ensure the new
height is after the existing height before updating it in order to
prevent it from going backwards so it properly matches the intent of the
function which is to report the latest known block height for the peer.

Without this change, the value will properly start out at the latest
known block height reported by the peer during version negotiation,
however, it will be set to lower values when syncing from the peer due
to requesting old blocks and blindly updating the height.

It also adds a test to ensure proper functionality.

This is a backport of https://github.com/decred/dcrd/pull/1747
---
 peer/peer.go      |  4 +++
 peer/peer_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/peer/peer.go b/peer/peer.go
index 82010f3b..5b083a74 100644
--- a/peer/peer.go
+++ b/peer/peer.go
@@ -495,6 +495,10 @@ func (p *Peer) String() string {
 // This function is safe for concurrent access.
 func (p *Peer) UpdateLastBlockHeight(newHeight int32) {
 	p.statsMtx.Lock()
+	if newHeight <= p.lastBlock {
+		p.statsMtx.Unlock()
+		return
+	}
 	log.Tracef("Updating last block height of peer %v from %v to %v",
 		p.addr, p.lastBlock, newHeight)
 	p.lastBlock = newHeight
diff --git a/peer/peer_test.go b/peer/peer_test.go
index fff0ce3f..6cc3113d 100644
--- a/peer/peer_test.go
+++ b/peer/peer_test.go
@@ -917,6 +917,72 @@ func TestDuplicateVersionMsg(t *testing.T) {
 	}
 }
 
+// TestUpdateLastBlockHeight ensures the last block height is set properly
+// during the initial version negotiation and is only allowed to advance to
+// higher values via the associated update function.
+func TestUpdateLastBlockHeight(t *testing.T) {
+	// Create a pair of peers that are connected to each other using a fake
+	// connection and the remote peer starting at height 100.
+	const remotePeerHeight = 100
+	verack := make(chan struct{})
+	peerCfg := peer.Config{
+		Listeners: peer.MessageListeners{
+			OnVerAck: func(p *peer.Peer, msg *wire.MsgVerAck) {
+				verack <- struct{}{}
+			},
+		},
+		UserAgentName:    "peer",
+		UserAgentVersion: "1.0",
+		ChainParams:      &chaincfg.MainNetParams,
+		Services:         0,
+	}
+	remotePeerCfg := peerCfg
+	remotePeerCfg.NewestBlock = func() (*chainhash.Hash, int32, error) {
+		return &chainhash.Hash{}, remotePeerHeight, nil
+	}
+	inConn, outConn := pipe(
+		&conn{laddr: "10.0.0.1:9108", raddr: "10.0.0.2:9108"},
+		&conn{laddr: "10.0.0.2:9108", raddr: "10.0.0.1:9108"},
+	)
+	localPeer, err := peer.NewOutboundPeer(&peerCfg, inConn.laddr)
+	if err != nil {
+		t.Fatalf("NewOutboundPeer: unexpected err: %v\n", err)
+	}
+	localPeer.AssociateConnection(outConn)
+	inPeer := peer.NewInboundPeer(&remotePeerCfg)
+	inPeer.AssociateConnection(inConn)
+
+	// Wait for the veracks from the initial protocol version negotiation.
+	for i := 0; i < 2; i++ {
+		select {
+		case <-verack:
+		case <-time.After(time.Second):
+			t.Fatal("verack timeout")
+		}
+	}
+
+	// Ensure the latest block height starts at the value reported by the remote
+	// peer via its version message.
+	if height := localPeer.LastBlock(); height != remotePeerHeight {
+		t.Fatalf("wrong starting height - got %d, want %d", height,
+			remotePeerHeight)
+	}
+
+	// Ensure the latest block height is not allowed to go backwards.
+	localPeer.UpdateLastBlockHeight(remotePeerHeight - 1)
+	if height := localPeer.LastBlock(); height != remotePeerHeight {
+		t.Fatalf("height allowed to go backwards - got %d, want %d", height,
+			remotePeerHeight)
+	}
+
+	// Ensure the latest block height is allowed to advance.
+	localPeer.UpdateLastBlockHeight(remotePeerHeight + 1)
+	if height := localPeer.LastBlock(); height != remotePeerHeight+1 {
+		t.Fatalf("height not allowed to advance - got %d, want %d", height,
+			remotePeerHeight+1)
+	}
+}
+
 func init() {
 	// Allow self connection when running the tests.
 	peer.TstAllowSelfConns()