Merge #1168

1168: use pool production tip to compute performance r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1158 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [ ] I have used pool production tip to compute performanceinstead of the node tip. The production worker might not have caught up with the network yet and therefore, yield statistics that are slightly off



# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <matthias.benkort@gmail.com>

lib/core/src/Cardano/Pool/Metrics.hs

@@ -89,6 +89,8 @@ import Control.Monad.Trans.Except
     ( ExceptT (..), mapExceptT, runExceptT, throwE, withExceptT )
 import Control.Tracer
     ( contramap )
+import Data.Functor
+    ( (<&>) )
 import Data.Generics.Internal.VL.Lens
     ( view, (^.) )
 import Data.List
@@ -139,7 +141,7 @@ data StakePool = StakePool
     , stake :: Quantity "lovelace" Word64
     , production :: Quantity "block" Word64
     , apparentPerformance :: Double
-    } deriving (Generic)
+    } deriving (Show, Generic)
 
 --------------------------------------------------------------------------------
 -- Stake Pool Monitoring
@@ -251,6 +253,7 @@ data ErrListStakePools
      = ErrMetricsIsUnsynced (Quantity "percent" Percentage)
      | ErrListStakePoolsMetricsInconsistency ErrMetricsInconsistency
      | ErrListStakePoolsErrNetworkTip ErrNetworkTip
+     deriving (Show)
 
 newStakePoolLayer
     :: Trace IO StakePoolLayerMsg
@@ -275,22 +278,28 @@ newStakePoolLayer tr db@DBLayer{..} nl metadataDir = StakePoolLayer
         let nodeEpoch = nodeTip ^. #slotId . #epochNumber
         let genesisEpoch = block0 ^. #header . #slotId . #epochNumber
 
-        (distr, prod) <- liftIO . atomically $ (,)
+        (distr, prod, prodTip) <- liftIO . atomically $ (,,)
             <$> (Map.fromList <$> readStakeDistribution nodeEpoch)
-            <*> (count <$> readPoolProduction nodeEpoch)
+            <*> readPoolProduction nodeEpoch
+            <*> readPoolProductionTip
 
         when (Map.null distr || Map.null prod) $ do
-            computeProgress nodeTip >>= throwE . ErrMetricsIsUnsynced
+            liftIO $ logTrace tr $ MsgComputedProgress prodTip nodeTip
+            throwE $ ErrMetricsIsUnsynced $ computeProgress prodTip nodeTip
 
         if nodeEpoch == genesisEpoch
         then do
             seed <- liftIO $ atomically readSystemSeed
-            combineWith (sortArbitrarily seed) distr prod mempty
+            combineWith (sortArbitrarily seed) distr (count prod) mempty
 
         else do
-            let tip = nodeTip ^. #slotId
-            perfs <- liftIO $ readPoolsPerformances db epochLength tip
-            combineWith (pure . sortByPerformance) distr prod perfs
+            let sl = prodTip ^. #slotId
+            perfs <- liftIO $ readPoolsPerformances db epochLength sl
+            combineWith (pure . sortByPerformance) distr (count prod) perfs
+
+    readPoolProductionTip = readPoolProductionCursor 1 <&> \case
+        []  -> header block0
+        h:_ -> h
 
     -- For each pool, look up its metadata. If metadata could not be found for a
     -- pool, the result will be 'Nothing'.
@@ -330,11 +339,6 @@ newStakePoolLayer tr db@DBLayer{..} nl metadataDir = StakePoolLayer
             Left e ->
                 throwE $ ErrListStakePoolsMetricsInconsistency e
 
-    poolProductionTip :: IO (Maybe BlockHeader)
-    poolProductionTip = atomically $ readPoolProductionCursor 1 >>= \case
-        [x] -> return $ Just x
-        _ -> return Nothing
-
     sortByPerformance :: [StakePool] -> [StakePool]
     sortByPerformance = sortOn (Down . apparentPerformance)
 
@@ -352,20 +356,16 @@ newStakePoolLayer tr db@DBLayer{..} nl metadataDir = StakePoolLayer
         StakePool{poolId,stake,production,apparentPerformance}
 
     computeProgress
-        :: BlockHeader -- ^ The node tip, which respresents 100%.
-        -> ExceptT e IO (Quantity "percent" Percentage)
-    computeProgress nodeTip = liftIO $ do
-        mDbTip <- poolProductionTip
-        logTrace tr $ MsgComputedProgress mDbTip nodeTip
-        pure $ Quantity $ maybe minBound (`progress` nodeTip) mDbTip
-
-    progress :: BlockHeader -> BlockHeader -> Percentage
-    progress tip target =
-        let
-            s0 = getQuantity $ tip ^. #blockHeight
-            s1 = getQuantity $ target ^. #blockHeight
-        in toEnum $ round $ 100 * (toD s0) / (toD s1)
+        :: BlockHeader -- ^ ... / denominator
+        -> BlockHeader -- ^ numerator /...
+        -> Quantity "percent" Percentage
+    computeProgress prodTip nodeTip =
+        Quantity $ if s1 == 0
+            then minBound
+            else toEnum $ round $ 100 * (toD s0) / (toD s1)
       where
+        s0 = getQuantity $ prodTip ^. #blockHeight
+        s1 = getQuantity $ nodeTip ^. #blockHeight
         toD :: Integral i => i -> Double
         toD = fromIntegral
 
@@ -586,7 +586,7 @@ data StakePoolLayerMsg
     | MsgMetadataUsing PoolId PoolOwner StakePoolMetadata
     | MsgMetadataMissing PoolId
     | MsgMetadataMultiple PoolId [(PoolOwner, StakePoolMetadata)]
-    | MsgComputedProgress (Maybe BlockHeader) BlockHeader
+    | MsgComputedProgress BlockHeader BlockHeader
     deriving (Show, Eq)
 
 instance DefinePrivacyAnnotation StakePoolLayerMsg
@@ -605,14 +605,13 @@ instance ToText StakePoolLayerMsg where
         MsgRegistry msg -> toText msg
         MsgListStakePoolsBegin -> "Listing stake pools"
         MsgMetadataUnavailable -> "Stake pool metadata is unavailable"
-        MsgComputedProgress (Just dbTip) nodeTip -> mconcat
+        MsgComputedProgress prodTip nodeTip -> mconcat
             [ "The node tip is:\n"
             , pretty nodeTip
             , ",\nbut the last pool production stored in the db"
             , " is from:\n"
-            , pretty dbTip
+            , pretty prodTip
             ]
-        MsgComputedProgress Nothing _nodeTip -> ""
         MsgMetadataUsing pid owner _ ->
             "Using stake pool metadata from " <>
             toText owner <> " for " <> toText pid

lib/core/test/unit/Cardano/Pool/MetricsSpec.hs

@@ -19,19 +19,24 @@ module Cardano.Pool.MetricsSpec
 
 import Prelude
 
+import Cardano.BM.Data.Tracer
+    ( nullTracer )
 import Cardano.Pool.DB
     ( DBLayer (..) )
 import Cardano.Pool.DB.MVar
     ( newDBLayer )
 import Cardano.Pool.Metadata
-    ( StakePoolMetadata (..), sameStakePoolMetadata )
+    ( StakePoolMetadata (..), envVarMetadataRegistry, sameStakePoolMetadata )
 import Cardano.Pool.Metrics
     ( Block (..)
+    , ErrListStakePools (..)
+    , StakePoolLayer (..)
     , StakePoolLayerMsg (..)
     , associateMetadata
     , calculatePerformance
     , combineMetrics
     , monitorStakePools
+    , newStakePoolLayer
     )
 import Cardano.Wallet.DummyTarget.Primitive.Types
     ( genesisParameters )
@@ -59,7 +64,7 @@ import Cardano.Wallet.Primitive.Types
     , slotSucc
     )
 import Cardano.Wallet.Unsafe
-    ( unsafeFromText )
+    ( unsafeFromText, unsafeRunExceptT )
 import Control.Concurrent.Async
     ( race_ )
 import Control.Concurrent.MVar
@@ -69,7 +74,7 @@ import Control.Monad
 import Control.Monad.Trans.Class
     ( lift )
 import Control.Monad.Trans.Except
-    ( ExceptT (..) )
+    ( ExceptT (..), runExceptT )
 import Control.Monad.Trans.State.Strict
     ( StateT, evalStateT, get, modify' )
 import Data.Function
@@ -88,6 +93,8 @@ import Data.Text.Class
     ( toText )
 import Data.Word
     ( Word32, Word64 )
+import System.Environment
+    ( setEnv )
 import Test.Hspec
     ( Spec, describe, it, shouldBe, shouldContain, shouldSatisfy )
 import Test.QuickCheck
@@ -143,6 +150,10 @@ spec = do
         it "records all stake pool registrations in the database"
             $ property prop_trackRegistrations
 
+    describe "listStakePools" $ do
+        it "can't list on empty database" test_emptyDatabaseNotSynced
+        it "report correct progress when not synced" test_notSyncedProgress
+
     associateMetadataSpec
 
 {-------------------------------------------------------------------------------
@@ -326,12 +337,59 @@ prop_trackRegistrations test = monadicIO $ do
                 pure header0
             -- These params are basically unused and completely arbitrary.
             , staticBlockchainParameters =
-                (block0, mockBlockchainParameters
-                    { getEpochStability = Quantity 2 })
+                ( block0
+                , mockBlockchainParameters { getEpochStability = Quantity 2 }
+                )
             }
 
 data instance Cursor RegistrationsTest = Cursor BlockHeader
 
+test_emptyDatabaseNotSynced :: IO ()
+test_emptyDatabaseNotSynced = do
+    setEnv envVarMetadataRegistry "-"
+    db@DBLayer{..} <- newDBLayer
+    -- NOTE The directory below isn't use, the test should fail much before
+    let spl = newStakePoolLayer nullTracer db nl "/dev/null"
+    res <- runExceptT $ listStakePools spl
+    case res of
+        Left (ErrMetricsIsUnsynced (Quantity p)) -> p `shouldBe` toEnum 0
+        _ -> fail $ "got something else than expected: " <> show res
+  where
+    nl = mockNetworkLayer
+        { networkTip =
+            pure header0
+        , staticBlockchainParameters =
+            ( block0
+            -- v arbitrary but defined.
+            , mockBlockchainParameters { getEpochLength = EpochLength 10 }
+            )
+        }
+
+test_notSyncedProgress :: IO ()
+test_notSyncedProgress = do
+    setEnv envVarMetadataRegistry "-"
+    db@DBLayer{..} <- newDBLayer
+    atomically $ unsafeRunExceptT $
+        putPoolProduction prodTip (PoolId "Pool & The Gang")
+    -- NOTE The directory below isn't use, the test should fail much before
+    let spl = newStakePoolLayer nullTracer db nl "/dev/null"
+    res <- runExceptT $ listStakePools spl
+    case res of
+        Left (ErrMetricsIsUnsynced (Quantity p)) -> p `shouldBe` toEnum 33
+        _ -> fail $ "got something else than expected: " <> show res
+  where
+    nodeTip = header0 { blockHeight = Quantity 42 }
+    prodTip = header0 { blockHeight = Quantity 14 }
+    nl = mockNetworkLayer
+        { networkTip =
+            pure nodeTip
+        , staticBlockchainParameters =
+            ( block0
+            -- v arbitrary but defined.
+            , mockBlockchainParameters { getEpochLength = EpochLength 10 }
+            )
+        }
+
 {-------------------------------------------------------------------------------
                                  Mock Data
 -------------------------------------------------------------------------------}
@@ -354,7 +412,8 @@ mockNetworkLayer = NetworkLayer
         \_ -> error "mockNetworkLayer: postTx"
     , staticBlockchainParameters =
         ( error "mockNetworkLayer: genesis block"
-        , mockBlockchainParameters )
+        , mockBlockchainParameters
+        )
     , stakeDistribution =
         error "mockNetworkLayer: stakeDistribution"
     , getAccountBalance =