Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use exponential distribution for P2P session timeouts #1244

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
8 changes: 2 additions & 6 deletions src/Chainweb/Time.hs
Original file line number Diff line number Diff line change
Expand Up @@ -326,10 +326,8 @@ secondsToTimeSpan :: Num a => Seconds -> TimeSpan a
secondsToTimeSpan (Seconds s) = scaleTimeSpan s second
{-# INLINE secondsToTimeSpan #-}

-- | Assumes that the `TimeSpan` contains milliseconds.
--
timeSpanToSeconds :: Integral a => TimeSpan a -> Seconds
timeSpanToSeconds (TimeSpan ms) = Seconds . int $ ms `div` 1000000
timeSpanToSeconds (TimeSpan us) = Seconds . int $ us `div` 1000000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're here -- maybe make these functions strict?

{-# INLINE timeSpanToSeconds #-}

secondsToText :: Seconds -> T.Text
Expand Down Expand Up @@ -361,10 +359,8 @@ microsToTimeSpan :: Num a => Micros -> TimeSpan a
microsToTimeSpan (Micros us) = scaleTimeSpan us microsecond
{-# INLINE microsToTimeSpan #-}

-- | Assumes that the `TimeSpan` contains milliseconds.
--
timeSpanToMicros :: Integral a => TimeSpan a -> Micros
timeSpanToMicros (TimeSpan ms) = Micros . int $ ms * 1000
timeSpanToMicros (TimeSpan us) = Micros . int $ us
{-# INLINE timeSpanToMicros #-}

microsToText :: Micros -> T.Text
Expand Down
31 changes: 25 additions & 6 deletions src/P2P/Node.hs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
{-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE DerivingStrategies #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE MultiWayIf #-}
{-# LANGUAGE NumericUnderscores #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE RankNTypes #-}
{-# LANGUAGE ScopedTypeVariables #-}
Expand Down Expand Up @@ -585,26 +587,43 @@ newSession conf node = do
let env = peerClientEnv node newPeerInfo
(info, newSes) <- mask $ \restore -> do
now <- getCurrentTimeIntegral
t <- R.randomRIO
( round (0.9 * timeoutMs)
, round (1.1 * timeoutMs)
)
!newSes <- async $ restore $ timeout t
t <- sessionTimeout $ secondsToTimeSpan $ _p2pConfigSessionTimeout conf
!newSes <- async $ restore $ timeout (int $ timeSpanToMicros t)
$ _p2pNodeClientSession node (loggFun node) env newPeerInfo
incrementActiveSessionCount peerDb newPeerInfo
!info <- atomically $ addSession node newPeerInfo newSes now
return (info, newSes)
logg node Debug $ "Started peer session " <> showSessionId newPeerInfo newSes
loggFun node Info $ JsonLog info
where
TimeSpan timeoutMs = secondsToTimeSpan @Double (_p2pConfigSessionTimeout conf)
peerDb = _p2pNodePeerDb node

syncFromPeer_ pinfo
| _p2pConfigPrivate conf = return True
| _p2pNodeDoPeerSync node = syncFromPeer node pinfo
| otherwise = return True

-- | (Roughly) exponentially distributed timespans with the given expectation
-- within the range of a a tenth of the expectation and ten times the
-- expectation.
--
-- The expected value of the actual distribution gets increasinly imprecise
-- as the input value gets smaller, because a minimum result of 5 seconds
-- is implemented. Input values of less than 30s should be avoided.
--
sessionTimeout :: TimeSpan Int -> IO (TimeSpan Int)
sessionTimeout expected = do
x <- exponential (1 / us) :: IO Double
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use System.Random.MWC instead, it's better in basically every way -- including already shipping with a truncated exponential distribution. I used it for exactly this in https://github.com/kadena-io/chainweb-node/pull/1254/files, you could crib the code from there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to use it. However, I run into compatibility issues with our rather old nix snapshot. I don't remember the details. However, this implementation should be the same than what is used in mac-random. Also, I think, modern versions of the random package have much better performance than previous versions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lars -- mwc-random was already in the deps list?

return $! microsToTimeSpan $ round $ max lower $ min upper x
where
us = int $ timeSpanToMicros expected
lower = max 5_000_000 (us / 10) -- at least 5 seconds
upper = us * 10

exponential rate = do
!x <- R.getStdRandom (R.randomR (0, 1))
return $! - log x / rate

-- | Monitor and garbage collect sessions
--
awaitSessions :: P2pNode -> IO ()
Expand Down
22 changes: 22 additions & 0 deletions test/Chainweb/Test/Roundtrips.hs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import Crypto.Hash.Algorithms
import Data.Aeson
import qualified Data.ByteString.Lazy as BL
import qualified Data.HashMap.Strict as HM
import Data.Int
import qualified Data.Text as T

import Test.QuickCheck
Expand Down Expand Up @@ -68,6 +69,7 @@ tests = testGroup "roundtrip tests"
, hasTextRepresentationTests
, jsonRoundtripTests
, jsonKeyRoundtripTests
, timeSpanTests
]

-- -------------------------------------------------------------------------- --
Expand Down Expand Up @@ -196,6 +198,7 @@ jsonTestCases f =
[ testProperty "Time Micros" $ f @(Time Micros)
, testProperty "TimeSpan Micros" $ f @(TimeSpan Micros)
, testProperty "Seconds" $ f @Seconds
, testProperty "Micros" $ f @Micros
, testProperty "ChainId" $ f @ChainId
, testProperty "ChainwebVersion" $ f @ChainwebVersion
, testProperty "Nonce" $ f @Nonce
Expand Down Expand Up @@ -344,6 +347,7 @@ hasTextRepresentationTests = testGroup "HasTextRepresentation roundtrips"
, testProperty "ChainId" $ prop_iso' @_ @ChainId fromText toText
, testProperty "BlockHash" $ prop_iso' @_ @BlockHash fromText toText
, testProperty "Seconds" $ prop_iso' @_ @Seconds fromText toText
, testProperty "Micros" $ prop_iso' @_ @Micros fromText toText
, testProperty "Hostname" $ prop_iso' @_ @Hostname fromText toText
, testProperty "Port" $ prop_iso' @_ @Port fromText toText
, testProperty "HostAddress" $ prop_iso' @_ @HostAddress fromText toText
Expand All @@ -357,3 +361,21 @@ hasTextRepresentationTests = testGroup "HasTextRepresentation roundtrips"
, testProperty "ChainDatabaseGcConfig" $ prop_iso' @_ @ChainDatabaseGcConfig fromText toText
, testProperty "MerkleRootType" $ prop_iso' @_ @MerkleRootType fromText toText
]

-- -------------------------------------------------------------------------- --
-- Time

timeSpanTests :: TestTree
timeSpanTests = testGroup "TimeSpan roundtrips"
[ testProperty "timeSpanToMicros Int" $ prop_iso @(TimeSpan Int) microsToTimeSpan timeSpanToMicros
, testProperty "timeSpanToMicros Int64" $ prop_iso @(TimeSpan Int64) microsToTimeSpan timeSpanToMicros
, testProperty "timeSpanToMicros Micros" $ prop_iso @(TimeSpan Micros) microsToTimeSpan timeSpanToMicros

, testProperty "microsToTimeSpan Int" $ prop_iso @_ @(TimeSpan Int) timeSpanToMicros microsToTimeSpan
, testProperty "microsToTimeSpan Int64" $ prop_iso @_ @(TimeSpan Int64) timeSpanToMicros microsToTimeSpan
, testProperty "microsToTimeSpan Micros" $ prop_iso @_ @(TimeSpan Micros) timeSpanToMicros microsToTimeSpan

, testProperty "secondsToTimeSpan Int" $ prop_iso @_ @(TimeSpan Int) timeSpanToSeconds secondsToTimeSpan
, testProperty "secondsToTimeSpan Int64" $ prop_iso @_ @(TimeSpan Int64) timeSpanToSeconds secondsToTimeSpan
, testProperty "secondsToTimeSpan Seconds" $ prop_iso @_ @(TimeSpan Micros) timeSpanToSeconds secondsToTimeSpan
]