Merge pull request #5209 from IntersectMBO/mw/test-false-positive

Peer selection bugfix and testing updates

Author: crocodile-dentist

cardano-diffusion/changelog.d/20251013_152200_crocodile-dentist_test_false_positive.md

@@ -0,0 +1,16 @@
+<!--
+A new scriv changelog fragment.
+
+Uncomment the section that is right (remove the HTML comment wrapper).
+For top level release notes, leave all the headers commented out.
+-->
+
+<!--
+### Breaking
+-->
+
+### Non-Breaking
+
+- fixed false positive in `prop_diffusion_target_active_below` testnet test
+- improved approach in general to target-chasing tests in diffusion testnet
+  and PeerSelection mock environment tests.

cardano-diffusion/tests/lib/Test/Cardano/Network/Diffusion/Testnet.hs

@@ -529,10 +529,7 @@ mockPeerSelectionActions' tracer
                        Cardano.PublicRootPeers.fromPublicRootPeers publicConfigPeers
                      | otherwise            ->
                        PublicRootPeers.fromLedgerPeers ledgerPeers
-                   BigLedgerPeers
-                     | Set.null ledgerPeers ->
-                       Cardano.PublicRootPeers.fromPublicRootPeers publicConfigPeers
-                     | otherwise            ->
+                   BigLedgerPeers ->
                        PublicRootPeers.fromBigLedgerPeers bigLedgerPeers
 
       traceWith tracer (TraceEnvRootsResult (Set.toList (PublicRootPeers.toSet Cardano.ExtraPeers.toSet result)))

cardano-diffusion/tests/lib/Test/Cardano/Network/PeerSelection.hs

@@ -0,0 +1,25 @@
+<!--
+A new scriv changelog fragment.
+
+Uncomment the section that is right (remove the HTML comment wrapper).
+For top level release notes, leave all the headers commented out.
+-->
+
+### Breaking
+
+- `linger` function's arm callback now returns a `Maybe Bool`
+- `keyedLinger'`s arm callback now returns a `Maybe (Set b)`
+- `keyedLinger'`'s arm callback now returns a `Maybe (Set b, DiffTime))`
+- The above changes allow those functions to reset signal state on `Nothing`
+
+### Non-Breaking
+
+- Added latch function to `Signal`
+- bugfix missed promotion/demotion opportunities in:
+  - `ActivePeers.aboveTargetBigLedgerPeers`
+  - `ActivePeers.aboveTargetOther`
+  - `EstablishedPeers.aboveTargetOther`
+  - `EstablishedPeers.aboveTargetBigLedgerPeers`
+  - `EstablishedPeers.belowTargetLocal`
+  - `EstablishedPeers.belowTargetOther`
+  - `ActivePeers.belowTargetLocal`

cardano-diffusion/tests/lib/Test/Cardano/Network/PeerSelection/MockEnvironment.hs

@@ -259,11 +259,12 @@ belowTargetLocal actions@PeerSelectionActions {
                      Set.\\ inProgressPromoteWarm
                      Set.\\ inProgressDemoteWarm
                      Set.\\ inProgressDemoteToCold
-                numPromoteInProgress = Set.size inProgressPromoteWarm
           , not (Set.null availableToPromote)
           , (HotValency hotTarget, members, membersActive) <- groupsBelowTarget
           , let membersAvailableToPromote = Set.intersection
                                               members availableToPromote
+                numPromoteInProgress      = Set.size (Set.intersection
+                                                       inProgressPromoteWarm members)
                 numMembersToPromote       = hotTarget
                                           - Set.size membersActive
                                           - numPromoteInProgress
@@ -686,20 +687,25 @@ aboveTargetBigLedgerPeers actions@PeerSelectionActions {
                           }
     -- Are we above the general target for number of active peers?
   | numActiveBigLedgerPeers > targetNumberOfActiveBigLedgerPeers
-
-    -- Would we demote any if we could?
-  , let numPeersToDemote = numActiveBigLedgerPeers
+  , let activeBigLedger = activePeers
+                              `Set.intersection` bigLedgerPeersSet
+        -- Would we demote any if we could?
+        numPeersToDemote = numActiveBigLedgerPeers
                          - targetNumberOfActiveBigLedgerPeers
                          - numDemoteInProgressBigLedgerPeers
+                         -- don't drop too many and don't fail to take an opportunity
+                         -- if there are warm peers which are async demoted
+                         - Set.size (Set.intersection
+                                       inProgressDemoteToCold
+                                       activeBigLedger)
   , numPeersToDemote > 0
 
     -- Are there any hot peers we actually can pick to demote?
     -- For the moment we say we cannot demote local root peers.
     -- TODO: review this decision. If we want to be able to demote local root
     -- peers, e.g. for churn and improved selection, then we'll need an extra
     -- mechanism to avoid promotion/demotion loops for local peers.
-  , let availableToDemote = activePeers
-                              `Set.intersection` bigLedgerPeersSet
+  , let availableToDemote = activeBigLedger
                               Set.\\ inProgressDemoteHot
                               Set.\\ inProgressDemoteToCold
                               Set.\\ LocalRootPeers.keysSet localRootPeers
@@ -890,23 +896,26 @@ aboveTargetOther actions@PeerSelectionActions {
                  }
     -- Are we above the general target for number of active peers?
   | numActivePeers > targetNumberOfActivePeers
-
-    -- Would we demote any if we could?
-  , let numPeersToDemote = numActivePeers
+  , let activeNonBig = activePeers Set.\\ bigLedgerPeersSet
+        -- Would we demote any if we could?
+        numPeersToDemote = numActivePeers
                          - targetNumberOfActivePeers
                          - numDemoteInProgress
-                         - (Set.size inProgressDemoteToCold)
+                         -- don't drop too many and don't fail to take an opportunity
+                         -- if there are warm peers which are async demoted
+                         - Set.size (Set.intersection
+                                       inProgressDemoteToCold
+                                       activeNonBig)
   , numPeersToDemote > 0
 
     -- Are there any hot peers we actually can pick to demote?
     -- For the moment we say we cannot demote local root peers.
     -- TODO: review this decision. If we want to be able to demote local root
     -- peers, e.g. for churn and improved selection, then we'll need an extra
     -- mechanism to avoid promotion/demotion loops for local peers.
-  , let availableToDemote = activePeers
+  , let availableToDemote = activeNonBig
                               Set.\\ inProgressDemoteHot
                               Set.\\ LocalRootPeers.keysSet localRootPeers
-                              Set.\\ bigLedgerPeersSet
                               Set.\\ inProgressDemoteToCold
   , not (Set.null availableToDemote)
   = Guarded Nothing $ do