From e8067f741f9fa50eae6959892044fc0b335fc1c5 Mon Sep 17 00:00:00 2001
From: Matt Joiner <anacrolix@gmail.com>
Date: Mon, 25 Oct 2021 16:36:58 +1100
Subject: [PATCH] Always count unhandled requests as pending

Fixes https://github.com/anacrolix/torrent/issues/679.
---
 peerconn.go   | 38 +++++++++-----------------------------
 requesting.go |  5 ++---
 2 files changed, 11 insertions(+), 32 deletions(-)

diff --git a/peerconn.go b/peerconn.go
index e17d211680..8e1f654d7a 100644
--- a/peerconn.go
+++ b/peerconn.go
@@ -1076,15 +1076,16 @@ func (c *PeerConn) mainReadLoop() (err error) {
 			if !c.fastEnabled() {
 				c.deleteAllRequests()
 			} else {
-				c.actualRequestState.Requests.Iterate(func(x uint32) bool {
-					if !c.peerAllowedFast.Contains(x / c.t.chunksPerRegularPiece()) {
-						c.t.pendingRequests.Dec(x)
-					}
-					return true
-				})
+				// We don't decrement pending requests here, let's wait for the peer to either
+				// reject or satisfy the outstanding requests. Additionally some peers may unchoke
+				// us and resume where they left off, we don't want to have piled on to those chunks
+				// in the meanwhile. I think a peers ability to abuse this should be limited: they
+				// could let us request a lot of stuff, then choke us and never reject, but they're
+				// only a single peer, our chunk balancing should smooth over this abuse.
 			}
 			c.peerChoking = true
-			// We can then reset our interest.
+			// We can now reset our interest. I think we do this after setting the flag in case the
+			// peerImpl updates synchronously (webseeds?).
 			c.updateRequests("choked")
 			c.updateExpectingChunks()
 		case pp.Unchoke:
@@ -1099,7 +1100,6 @@ func (c *PeerConn) mainReadLoop() (err error) {
 			c.actualRequestState.Requests.Iterate(func(x uint32) bool {
 				if !c.peerAllowedFast.Contains(x / c.t.chunksPerRegularPiece()) {
 					preservedCount++
-					c.t.pendingRequests.Inc(x)
 				}
 				return true
 			})
@@ -1169,24 +1169,6 @@ func (c *PeerConn) mainReadLoop() (err error) {
 		case pp.AllowedFast:
 			torrent.Add("allowed fasts received", 1)
 			log.Fmsg("peer allowed fast: %d", msg.Index).AddValues(c).SetLevel(log.Debug).Log(c.t.logger)
-			pieceIndex := msg.Index.Int()
-			// If we have outstanding requests that aren't currently counted toward the combined
-			// outstanding request count, increment them.
-			if c.peerAllowedFast.CheckedAdd(msg.Index.Uint32()) && c.peerChoking &&
-				// The check here could be against having the info, but really what we need to know
-				// is if there are any existing requests.
-				!c.actualRequestState.Requests.IsEmpty() {
-
-				i := c.actualRequestState.Requests.Iterator()
-				i.AdvanceIfNeeded(t.pieceRequestIndexOffset(pieceIndex))
-				for i.HasNext() {
-					r := i.Next()
-					if r >= t.pieceRequestIndexOffset(pieceIndex+1) {
-						break
-					}
-					c.t.pendingRequests.Inc(r)
-				}
-			}
 			c.updateRequests("PeerConn.mainReadLoop allowed fast")
 		case pp.Extended:
 			err = c.onReadExtendedMsg(msg.ExtendedID, msg.ExtendedPayload)
@@ -1534,9 +1516,7 @@ func (c *Peer) deleteRequest(r RequestIndex) bool {
 		f(PeerRequestEvent{c, c.t.requestIndexToRequest(r)})
 	}
 	c.updateExpectingChunks()
-	if !c.peerChoking || c.peerAllowedFast.Contains(r/c.t.chunksPerRegularPiece()) {
-		c.t.pendingRequests.Dec(r)
-	}
+	c.t.pendingRequests.Dec(r)
 	return true
 }
 
diff --git a/requesting.go b/requesting.go
index c1ae8adac5..74d1d682e3 100644
--- a/requesting.go
+++ b/requesting.go
@@ -144,9 +144,8 @@ func (p *peerRequests) Less(i, j int) bool {
 		if current {
 			ret--
 		}
-		// I have a hunch that this could trigger for requests for chunks that are choked and not
-		// allowed fast, since the current conn shouldn't already be included. It's a very specific
-		// circumstance, and if it triggers I will fix it.
+		// See https://github.com/anacrolix/torrent/issues/679 for possible issues. This should be
+		// resolved.
 		if ret < 0 {
 			panic(ret)
 		}