Removed hasTxIdsToAcknowledge function. Fix test

`hasTxIdsToAcknowledge` is not used anywhere in the code so it is
removed.

`filterActivePeers` is improved by making its decision logic more closed
to `pickTxsToDownload`.

`filterActivePeers` test is fixed, since it doesn't hold under the new
logic:

  `filterActivePeers` will not compute a decision for peers which have
  `requestedTxIdsInflight` and `makeDecisions` computes non-empty decisions
  for peers with no `requestedTxIdsInflight`. So:

  1. "The set of active peers is a superset of peers for which a decision
     was made" this is not true since it is possible that a non active
     peer has a legitimate decision, but due to our race-condition
     protection condition we just don't generate it.
  2. "The set of active peer which can acknowledge txids is a subset of
     peers for which a decision was made" this is removed since
     hasTxIdsToAcknowledge function is removed
  3. "Decisions made from the results of `filterActivePeers` is the same
     as from the original set" this isn't true because of what I said
     above

  So I refactored the test to  check that the number of filtered decisions
  is a subset of the total number of decisions, which I believe to be a
  more accurate test for the current logic

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/TxSubmission/Common.hs

@@ -95,7 +95,6 @@ tests = testGroup "Ouroboros.Network.TxSubmission.Common"
         ]
       ]
     , testProperty "acknowledgeTxIds"           prop_acknowledgeTxIds
-    , testProperty "hasTxIdsToAcknowledge"      prop_hasTxIdsToAcknowledge
     , testProperty "receivedTxIdsImpl"          prop_receivedTxIdsImpl
     , testProperty "collectTxsImpl"             prop_collectTxsImpl
     , testProperty "numTxIdsToRequest"          prop_numTxIdsToRequest
@@ -845,7 +844,7 @@ prop_acknowledgeTxIds :: ArbDecisionContextWithReceivedTxIds
                       -> Property
 prop_acknowledgeTxIds (ArbDecisionContextWithReceivedTxIds policy SharedDecisionContext { sdcSharedTxState = st } ps _ _ _) =
     case TXS.acknowledgeTxIds policy st ps of
-      (numTxIdsToAck, _, txs, TXS.RefCountDiff { TXS.txIdsToAck }, ps') ->
+      (numTxIdsToAck, txIdsToRequest, txs, TXS.RefCountDiff { TXS.txIdsToAck }, ps') | txIdsToRequest > 0 ->
              counterexample "number of tx ids to ack must agree with RefCountDiff"
              ( fromIntegral numTxIdsToAck
                ===
@@ -869,26 +868,13 @@ prop_acknowledgeTxIds (ArbDecisionContextWithReceivedTxIds policy SharedDecision
                          , Just _ <- maybeToList $ txid `Map.lookup` bufferedTxs st
                          ]
              in getTxId `map` txs === acked)
+      _otherwise -> property True
   where
     stripSuffix :: Eq a => [a] -> [a] -> Maybe [a]
     stripSuffix as suffix =
         reverse <$> reverse suffix `stripPrefix` reverse as
 
 
--- | Verify that `hasTxIdsToAcknowledge` and `acknowledgeTxIds` are compatible.
---
---
-prop_hasTxIdsToAcknowledge
-  :: ArbDecisionContextWithReceivedTxIds
-  -> Property
-prop_hasTxIdsToAcknowledge (ArbDecisionContextWithReceivedTxIds policy SharedDecisionContext { sdcSharedTxState = st } ps _ _ _) =
-    case ( TXS.hasTxIdsToAcknowledge st ps
-         , TXS.acknowledgeTxIds policy st ps
-         ) of
-      (canAck, (numTxIdsToAck, _, _, _, _)) ->
-        canAck === (numTxIdsToAck > 0)
-
-
 -- | Verify 'inboundStateInvariant' when acknowledging a sequence of txs.
 --
 prop_receivedTxIdsImpl
@@ -2047,22 +2033,6 @@ prop_makeDecisions_collectTxs
 
 -- | `filterActivePeers` should not change decisions made by `makeDecisions`
 --
---
--- This test checks the following properties:
---
--- In what follows, the set of active peers is defined as the keys of the map
--- returned by `filterActivePeers`.
---
--- 1. The set of active peers is a superset of peers for which a decision was
---    made;
--- 2. The set of active peer which can acknowledge txids is a subset of peers
---    for which a decision was made;
--- 3. Decisions made from the results of `filterActivePeers` is the same as from
---    the original set.
---
--- Ad 2. a stronger property is not possible. There can be a peer for which
--- a decision was not taken but which is an active peer.
---
 prop_filterActivePeers_not_limitting_decisions
     :: ArbDecisionContexts TxId
     -> Property
@@ -2079,25 +2049,13 @@ prop_filterActivePeers_not_limitting_decisions
                    ,"active decisions: " ++ show decisionsOfActivePeers
                    ,"                  " ++ show activePeers]) $
 
-    counterexample ("found non-active peers for which decision can be made: "
-                     ++ show (decisionPeers  Set.\\ activePeers)
+    counterexample ("active peers does not restrict the total number of valid decisions available"
+                     ++ show (decisionsOfActivePeers  Map.\\ decisions)
                    )
-                   (decisionPeers  `Set.isSubsetOf` activePeers)
-    .&&.
-    counterexample ("found an active peer which can acknowledge txids "
-                     ++ "for which decision was not made: "
-                     ++ show (activePeersAck Set.\\ decisionPeers))
-                   (activePeersAck `Set.isSubsetOf` decisionPeers)
-    .&&.
-    counterexample "decisions from active peers are not equal to decisions from all peers"
-                   (decisions === decisionsOfActivePeers)
+                   (Map.keysSet decisionsOfActivePeers  `Set.isSubsetOf` Map.keysSet decisions)
   where
     activePeersMap    = TXS.filterActivePeers policy st
     activePeers       = Map.keysSet activePeersMap
-    -- peers which are active & can acknowledge txids
-    activePeersAck    = activePeers
-                        `Set.intersection`
-                        Map.keysSet (Map.filter (TXS.hasTxIdsToAcknowledge st) (peerTxStates st))
     (_, decisionsOfActivePeers)
                       = TXS.makeDecisions policy sharedCtx activePeersMap
 

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound/Decision.hs

@@ -444,7 +444,9 @@ filterActivePeers
     TxDecisionPolicy { maxUnacknowledgedTxIds,
                        txsSizeInflightPerPeer,
                        maxTxsSizeInflight,
-                       txInflightMultiplicity }
+                       txInflightMultiplicity,
+                       maxNumTxIdsToRequest
+                     }
     SharedTxState { peerTxStates,
                     bufferedTxs,
                     inflightTxs,
@@ -460,12 +462,30 @@ filterActivePeers
 
     fn :: PeerTxState txid tx -> Bool
     fn PeerTxState { unacknowledgedTxIds,
-                     requestedTxIdsInflight } =
+                     requestedTxIdsInflight,
+                     unknownTxs
+                   } =
         --    hasTxIdsToAcknowledge st ps ||
            requestedTxIdsInflight == 0 -- document why it's not <= maxTxIdsInFlightPerPeer
         && requestedTxIdsInflight + numOfUnacked <= maxUnacknowledgedTxIds
+        && txIdsToRequest > 0
       where
-        numOfUnacked   = fromIntegral (StrictSeq.length unacknowledgedTxIds)
+        -- Split `unacknowledgedTxIds'` into the longest prefix of `txid`s which
+        -- can be acknowledged and the unacknowledged `txid`s.
+        (acknowledgedTxIds, _) =
+          StrictSeq.spanl (\txid -> txid `Map.member` bufferedTxs
+                                 || txid `Set.member` unknownTxs
+                          )
+                          unacknowledgedTxIds
+        numOfUnacked = fromIntegral (StrictSeq.length unacknowledgedTxIds)
+        numOfAcked   = StrictSeq.length acknowledgedTxIds
+        unackedAndRequested = numOfUnacked + requestedTxIdsInflight
+        txIdsToRequest =
+            assert (unackedAndRequested <= maxUnacknowledgedTxIds) $
+            assert (requestedTxIdsInflight <= maxNumTxIdsToRequest) $
+            (maxUnacknowledgedTxIds - unackedAndRequested + fromIntegral numOfAcked)
+            `min`
+            (maxNumTxIdsToRequest - requestedTxIdsInflight)
 
     gn :: PeerTxState txid tx -> Bool
     gn PeerTxState { unacknowledgedTxIds,
@@ -475,9 +495,10 @@ filterActivePeers
                      availableTxIds,
                      unknownTxs } =
         -- hasTxIdsToAcknowledge st ps ||
-           (    requestedTxIdsInflight == 0
-             && requestedTxIdsInflight + numOfUnacked <= maxUnacknowledgedTxIds
-           )
+          (    requestedTxIdsInflight == 0
+            && requestedTxIdsInflight + numOfUnacked <= maxUnacknowledgedTxIds
+            &&  txIdsToRequest > 0
+          )
         || (underSizeLimit && not (Map.null downloadable))
       where
         numOfUnacked   = fromIntegral (StrictSeq.length unacknowledgedTxIds)
@@ -487,6 +508,22 @@ filterActivePeers
             `Map.withoutKeys` unknownTxs
             `Map.withoutKeys` unrequestable
 
+        -- Split `unacknowledgedTxIds'` into the longest prefix of `txid`s which
+        -- can be acknowledged and the unacknowledged `txid`s.
+        (acknowledgedTxIds, _) =
+          StrictSeq.spanl (\txid -> txid `Map.member` bufferedTxs
+                                 || txid `Set.member` unknownTxs
+                          )
+                          unacknowledgedTxIds
+        numOfAcked   = StrictSeq.length acknowledgedTxIds
+        unackedAndRequested = numOfUnacked + requestedTxIdsInflight
+        txIdsToRequest =
+            assert (unackedAndRequested <= maxUnacknowledgedTxIds) $
+            assert (requestedTxIdsInflight <= maxNumTxIdsToRequest) $
+            (maxUnacknowledgedTxIds - unackedAndRequested + fromIntegral numOfAcked)
+            `min`
+            (maxNumTxIdsToRequest - requestedTxIdsInflight)
+
 --
 -- Auxiliary functions
 --

ouroboros-network/src/Ouroboros/Network/TxSubmission/Inbound/State.hs

@@ -15,7 +15,6 @@ module Ouroboros.Network.TxSubmission.Inbound.State
   , receivedTxIds
   , collectTxs
   , acknowledgeTxIds
-  , hasTxIdsToAcknowledge
     -- * Debug output
   , DebugSharedTxState (..)
     -- * Internals, only exported for testing purposes:
@@ -219,25 +218,6 @@ instance ( NoThunks peeraddr
 -- Pure public API
 --
 
--- | Check if a peer can acknowledge at least one `txid`.
---
-hasTxIdsToAcknowledge
-    :: forall peeraddr txid tx.
-       Ord txid
-    => SharedTxState peeraddr txid tx
-    -> PeerTxState txid tx
-    -> Bool
-hasTxIdsToAcknowledge
-    SharedTxState { bufferedTxs }
-    PeerTxState { unacknowledgedTxIds, unknownTxs }
-    =
-    -- We just need to look at the front of the unacknowledged `txid`s.
-    case unacknowledgedTxIds of
-      txid StrictSeq.:<| _ -> txid `Map.member` bufferedTxs
-                           || txid `Set.member` unknownTxs
-      _                    -> False
-
-
 acknowledgeTxIds
     :: forall peeraddr tx txid.
        Ord txid