Remove want-block sent tracking from sessionWantSender

[gammazero]

Tracking want-blocks that were sent to a peer is not necessary to do in sessionwantsender. This is already handled in peermanager/peerwantmanager.

Removing this tracking reduces unnecessary memory use.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.45%. Comparing base (ddfe266) to head (68f9efc).
Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main     #759      +/-   ##
==========================================
- Coverage   60.51%   60.45%   -0.06%     
==========================================
  Files         245      244       -1     
  Lines       31134    31102      -32     
==========================================
- Hits        18841    18804      -37     
- Misses      10618    10620       +2     
- Partials     1675     1678       +3     

Files with missing lines	Coverage Δ
...tswap/client/internal/session/sessionwantsender.go	96.83% <100.00%> (-0.12%)	⬇️

... and 8 files with indirect coverage changes

[hsanjuan]

Are we sure removing this has no effect?

swbt was introduced here: ipfs/go-bitswap@b3a47bc#diff-92a0b168d045013a05e87675741bb3cfa356a674c6f1535fbe88fc8e89d31e53

It is not explained what exactly motivated it, but it sounds like it is somehow keeping track of some wants that would otherwise not be sent?

[guillaumemichel]

It seems that the only place where the use of sentWantBlocksTracker is questionable is getPiggybackWantHaves.

// sendNextWants sends wants to peers according to the latest information
// about which peers have / dont have blocks
func (sws *sessionWantSender) sendNextWants(newlyAvailable []peer.ID) {
	toSend := make(allWants)


	for c, wi := range sws.wants {
		// Ensure we send want-haves to any newly available peers
		for _, p := range newlyAvailable {
			toSend.forPeer(p).wantHaves.Add(c)
		}


		// We already sent a want-block to a peer and haven't yet received a
		// response yet
		if wi.sentTo != "" {
			continue
		}


		// All the peers have indicated that they don't have the block
		// corresponding to this want, so we must wait to discover more peers
		if wi.bestPeer == "" {
			// TODO: work this out in real time instead of using bestP?
			continue
		}


		// Send a want-block to the chosen peer
		toSend.forPeer(wi.bestPeer).wantBlocks.Add(c)


		// Send a want-have to each other peer
		for _, op := range sws.spm.Peers() {
			if op != wi.bestPeer {
				toSend.forPeer(op).wantHaves.Add(c)
			}
		}
	}


	// Send any wants we've collected
	sws.sendWants(toSend)
	...
}

getPiggybackWantHaves seems to cover a specific corner case (I doubt it was done voluntarily though).

1. A new WANT(k) is added to the client for cid k
2. A WANT_BLOCK is sent to wi.bestPeer, and WANT_HAVEs are sent to all other peers in sws.spm.Peers()
3. wi.bestPeer becomes unavailable, triggering wi.sentTo to be reset. (this is the only place where wi.sentTo can be reset).
4. At the next call of sendNextWants, WANT_HAVE(k) will be sent to all peers in sws.spm.Peers() again (as long as not peers have return a DONT_HAVE)
5. getPiggybackWantHaves should add WANT_HAVE(kk) for all kk in sws.wants where kk != k and WANT_HAVE(kk) wasn't sent to that peer before, for peers that are part of sws.spm.Peers() but NOT newlyAvailable

However I would say that this set of peers will always be empty.

1. If a peer was part of sws.spm.Peers() at the time the new WANT(k) has been added, then it would already have received a WANT_HAVE.
2. If it wasn't part of sws.spm.Peers() at the time the new WANT(k) has been added , then it means that it would have been part of newlyAvailable at some point, and hence would have received the WANT_HAVE.

So I would say that getPiggybackWantHaves and the whole sentWantBlocksTracker logic can be removed safely, without impacting Bitswap behaviour.

[gammazero]

@guillaumemichel Thank you for this careful review and analysis of the logic.