Some BlockFetch decline decisions are not revisited after completeFetchBatch

[nfrisby]

While developing infra-slot network latencies (PR #1131), I'm seeing a violation of our current variant of Chain Growth. This Issue is complete when that test case is no longer a failure.

The current repro, which might not work directly on future master, is as follows in Test.Dynamic.Praos. (If it unexpectedly does not fail on a branch with latencies, my only suggestion is to let the latency seed vary and hope it discovers the failure.)

            testPraos' TestConfig
              { numCoreNodes
              , numSlots
              , nodeJoinPlan = NodeJoinPlan (Map.fromList [(CoreNodeId 0,SlotNo {unSlotNo = 24}),(CoreNodeId 1,SlotNo {unSlotNo = 29}),(CoreNodeId 2,SlotNo {unSlotNo = 29})])
              , nodeTopology = NodeTopology (Map.fromList [(CoreNodeId 0,Set.fromList []),(CoreNodeId 1,Set.fromList [CoreNodeId 0]),(CoreNodeId 2,Set.fromList [CoreNodeId 1])])
              , latencySeed = InjectLatencies (seedSMGen 9511500888644978603 10563634114029886209)
              }
                Seed {getSeed = (10580735904357354200,1771287816820322801,4449307331544544775,3207572460394768516,9310143438266696584)}

The specific failure happens because network latency introduces a non-negligible duration (never before seen in the test suite) between MsgBlock and MsgBatchDone. A particular node makes a FetchDeclineConcurrencyLimit decision (due to peerFetchReqsInFlight = 1) after receiving MsgBlock but before receiving MsgBatchDone (which sets peerFetchReqsInFlight = 0). The BlockFetch decision re-evaluation trigger currently does not include peerFetchReqsInFlight: MsgBlock would trigger a re-evaluation but MsgBatchDone does not. Because of that decision, nodes c1 and c2 do not adopt the chain that c0 forged, which is the longest chain in the network. That's a violation of our currently-very-strict variant of Chain Growth.

Apparent ways to resolve this Issue:

• Ensure MsgBatchDone triggers re-evaluation (i.e. expand the content of FetchStateFingerprint to include the peerFetchReqsInFlight fields; and perhaps similar for other bases for decline decisions?)
• @dcoutts on Slack came up with the idea of adding an "Idle" status to the PeerStatus, which is already part of the FetchStateFingerprint.
• Accept this current behavior as-is, and adjust the Chain Growth variation Property to anticipate this.

[nfrisby]

I added the Bug label. That might be an overstatement, but at the very least Duncan agreed on Slack that the current behavior is not as prompt as we'd prefer.

[mrBliss]

@nfrisby What's the status on this?

[nfrisby]

I responded on Slack. Summary: Duncan had planned to do it, though I have a commit on my branch that is more "surgical" than what he had discussed, if we want to go with that for now.