Address review comments

IntersectMBO · Oct 2, 2023 · 84317e9 · 84317e9
1 parent 6c03050
commit 84317e9
Show file tree

Hide file tree

Showing 14 changed files with 238 additions and 128 deletions.
diff --git a/ouroboros-network-api/CHANGELOG.md b/ouroboros-network-api/CHANGELOG.md
@@ -4,11 +4,13 @@
 
 ### Breaking changes
 
-- Remote `PeerSharingPrivate` option from the `PeerSharing` data type.
-- Rename `NoPeerSharing` and `PeerSharingPublic` to `PeerSharingDisabled` and
+* Remote `PeerSharingPrivate` option from the `PeerSharing` data type.
+* Rename `NoPeerSharing` and `PeerSharingPublic` to `PeerSharingDisabled` and
   `PeerSharingEnabled`, respectively.
-- Add new `NodeToNodeV_13` that encodes and decodes the updated `PeerSharing` flag data
+* Add new `NodeToNodeV_13` that encodes and decodes the updated `PeerSharing` flag data
   type.
+* Move remote address codec to 'src/Ouroboros/Network/NodeToNode/Version.hs'
+* Make remote address codec receive 'NodeToNodeVersion'.
 
 ### Non-breaking changes
 

diff --git a/ouroboros-network-api/src/Ouroboros/Network/NodeToNode/Version.hs b/ouroboros-network-api/src/Ouroboros/Network/NodeToNode/Version.hs
@@ -7,13 +7,19 @@ module Ouroboros.Network.NodeToNode.Version
   , ConnectionMode (..)
   , nodeToNodeVersionCodec
   , nodeToNodeCodecCBORTerm
+  , encodePortNumber
+  , decodePortNumber
+  , encodeRemoteAddress
+  , decodeRemoteAddress
   , isPipeliningEnabled
   ) where
 
 import           Data.Text (Text)
 import qualified Data.Text as T
 import           Data.Typeable (Typeable)
 
+import qualified Codec.CBOR.Decoding as CBOR
+import qualified Codec.CBOR.Encoding as CBOR
 import qualified Codec.CBOR.Term as CBOR
 
 import           Ouroboros.Network.BlockFetch.ConsensusInterface
@@ -23,9 +29,9 @@ import           Ouroboros.Network.Handshake.Acceptable (Accept (..),
                      Acceptable (..))
 import           Ouroboros.Network.Handshake.Queryable (Queryable (..))
 import           Ouroboros.Network.Magic
-import           Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..),
-                     combinePeerSharing)
+import           Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..))
 
+import           Network.Socket (PortNumber, SockAddr (..))
 
 -- | Enumeration of node to node protocol versions.
 --
@@ -62,7 +68,7 @@ data NodeToNodeVersion
     | NodeToNodeV_13
     -- ^ Changes:
     --
-    -- * Adds a fix for PeerSharing handshake negotiation
+    -- * Added `localPeerSharing` negotiation flag.
   deriving (Eq, Ord, Enum, Bounded, Show, Typeable)
 
 nodeToNodeVersionCodec :: CodecCBORTerm (Text, Maybe Int) NodeToNodeVersion
@@ -134,8 +140,7 @@ instance Acceptable NodeToNodeVersionData where
       = Accept NodeToNodeVersionData
           { networkMagic  = networkMagic local
           , diffusionMode = diffusionMode local `min` diffusionMode remote
-          , peerSharing   = combinePeerSharing (peerSharing local)
-                                               (peerSharing remote)
+          , peerSharing   = (peerSharing local) <> (peerSharing remote)
           , query         = query local || query remote
           }
       | otherwise
@@ -268,6 +273,93 @@ nodeToNodeCodecCBORTerm version
 data ConnectionMode = UnidirectionalMode | DuplexMode
 
 
+encodePortNumber :: PortNumber -> CBOR.Encoding
+encodePortNumber = CBOR.encodeWord16 . fromIntegral
+
+decodePortNumber :: CBOR.Decoder s PortNumber
+decodePortNumber = fromIntegral <$> CBOR.decodeWord16
+
+
+-- | This encoder should be faithful to the PeerSharing
+-- CDDL Specification.
+--
+-- See the network design document for more details
+--
+encodeRemoteAddress :: NodeToNodeVersion -> SockAddr -> CBOR.Encoding
+encodeRemoteAddress ntnVersion sockAddr
+  | ntnVersion >= NodeToNodeV_13 =
+    case sockAddr of
+      SockAddrInet pn w -> CBOR.encodeListLen 3
+                        <> CBOR.encodeWord 0
+                        <> CBOR.encodeWord32 w
+                        <> encodePortNumber pn
+      SockAddrInet6 pn _ (w1, w2, w3, w4) _ -> CBOR.encodeListLen 6
+                                            <> CBOR.encodeWord 1
+                                            <> CBOR.encodeWord32 w1
+                                            <> CBOR.encodeWord32 w2
+                                            <> CBOR.encodeWord32 w3
+                                            <> CBOR.encodeWord32 w4
+                                            <> encodePortNumber pn
+      SockAddrUnix _ -> error "Should never be encoding a SockAddrUnix!"
+  | otherwise =
+    case sockAddr of
+      SockAddrInet pn w -> CBOR.encodeListLen 3
+                        <> CBOR.encodeWord 0
+                        <> CBOR.encodeWord32 w
+                        <> encodePortNumber pn
+      SockAddrInet6 pn fi (w1, w2, w3, w4) si -> CBOR.encodeListLen 8
+                                            <> CBOR.encodeWord 1
+                                            <> CBOR.encodeWord32 w1
+                                            <> CBOR.encodeWord32 w2
+                                            <> CBOR.encodeWord32 w3
+                                            <> CBOR.encodeWord32 w4
+                                            <> CBOR.encodeWord32 fi
+                                            <> CBOR.encodeWord32 si
+                                            <> encodePortNumber pn
+      SockAddrUnix _ -> error "Should never be encoding a SockAddrUnix!"
+
+-- | This decoder should be faithful to the PeerSharing
+-- CDDL Specification.
+--
+-- See the network design document for more details
+--
+decodeRemoteAddress :: NodeToNodeVersion -> CBOR.Decoder s SockAddr
+decodeRemoteAddress ntnVersion
+  | ntnVersion >= NodeToNodeV_13 = do
+    _ <- CBOR.decodeListLen
+    tok <- CBOR.decodeWord
+    case tok of
+      0 -> do
+        w <- CBOR.decodeWord32
+        pn <- decodePortNumber
+        return (SockAddrInet pn w)
+      1 -> do
+        w1 <- CBOR.decodeWord32
+        w2 <- CBOR.decodeWord32
+        w3 <- CBOR.decodeWord32
+        w4 <- CBOR.decodeWord32
+        pn <- decodePortNumber
+        return (SockAddrInet6 pn 0 (w1, w2, w3, w4) 0)
+      _ -> fail ("Serialise.decode.SockAddr unexpected tok " ++ show tok)
+  | otherwise = do
+    _ <- CBOR.decodeListLen
+    tok <- CBOR.decodeWord
+    case tok of
+      0 -> do
+        w <- CBOR.decodeWord32
+        pn <- decodePortNumber
+        return (SockAddrInet pn w)
+      1 -> do
+        w1 <- CBOR.decodeWord32
+        w2 <- CBOR.decodeWord32
+        w3 <- CBOR.decodeWord32
+        w4 <- CBOR.decodeWord32
+        fi <- CBOR.decodeWord32
+        si <- CBOR.decodeWord32
+        pn <- decodePortNumber
+        return (SockAddrInet6 pn fi (w1, w2, w3, w4) si)
+      _ -> fail ("Serialise.decode.SockAddr unexpected tok " ++ show tok)
+
 -- | Check whether a version enabling diffusion pipelining has been
 -- negotiated.
 --

diff --git a/ouroboros-network-api/src/Ouroboros/Network/PeerSelection/PeerSharing.hs b/ouroboros-network-api/src/Ouroboros/Network/PeerSelection/PeerSharing.hs
@@ -1,23 +1,14 @@
 {-# LANGUAGE DeriveGeneric #-}
 
 {-# OPTIONS_GHC -Wno-orphans #-}
+{-# LANGUAGE InstanceSigs  #-}
 
-module Ouroboros.Network.PeerSelection.PeerSharing
-  ( PeerSharing (..)
-  , combinePeerSharing
-  , encodePortNumber
-  , decodePortNumber
-  , encodeRemoteAddress
-  , decodeRemoteAddress
-  ) where
+module Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..)) where
 
-import qualified Codec.CBOR.Decoding as CBOR
-import qualified Codec.CBOR.Encoding as CBOR
 import           Data.Aeson.Types (FromJSON (..), ToJSON (..), Value (..),
                      withText)
 import qualified Data.Text as Text
 import           GHC.Generics (Generic)
-import           Network.Socket (PortNumber, SockAddr (..))
 import           Text.Read (readMaybe)
 
 -- | Is a peer willing to participate in Peer Sharing? If yes are others allowed
@@ -32,69 +23,41 @@ data PeerSharing = PeerSharingDisabled -- ^ Peer does not participate in Peer Sh
                  | PeerSharingEnabled -- ^ Peer participates in Peer Sharing
   deriving  (Eq, Show, Read, Generic)
 
-instance FromJSON PeerSharing where
-  parseJSON = withText "PeerSharing" $ \t ->
-    case readMaybe (Text.unpack t) of
-      Nothing -> fail ("PeerSharing.parseJSON: could not parse value: "
-                     ++ Text.unpack t)
-      Just ps -> return ps
-
-instance ToJSON PeerSharing where
-  toJSON = String . Text.pack . show
-
 -- | Combine two 'PeerSharing' values
 --
--- 'PeerSharingDisabled' is the absorbing element
+-- 'PeerSharingEnabled' is the unit element.
+--
 combinePeerSharing :: PeerSharing -> PeerSharing -> PeerSharing
 combinePeerSharing PeerSharingDisabled _ = PeerSharingDisabled
 combinePeerSharing _ PeerSharingDisabled = PeerSharingDisabled
 combinePeerSharing _ _                   = PeerSharingEnabled
 
-encodePortNumber :: PortNumber -> CBOR.Encoding
-encodePortNumber = CBOR.encodeWord16 . fromIntegral
-
-decodePortNumber :: CBOR.Decoder s PortNumber
-decodePortNumber = fromIntegral <$> CBOR.decodeWord16
-
-
--- | This encoder should be faithful to the PeerSharing
--- CDDL Specification.
+-- | The combination of two 'PeerSharing' values forms a Monoid where the unit
+-- is 'PeerSharingEnabled'.
 --
--- See the network design document for more details
+-- This operation is used in the connection handshake.
 --
-encodeRemoteAddress :: SockAddr -> CBOR.Encoding
-encodeRemoteAddress (SockAddrInet pn w) = CBOR.encodeListLen 3
-                           <> CBOR.encodeWord 0
-                           <> CBOR.encodeWord32 w
-                           <> encodePortNumber pn
-encodeRemoteAddress (SockAddrInet6 pn _ (w1, w2, w3, w4) _) = CBOR.encodeListLen 6
-                                                <> CBOR.encodeWord 1
-                                                <> CBOR.encodeWord32 w1
-                                                <> CBOR.encodeWord32 w2
-                                                <> CBOR.encodeWord32 w3
-                                                <> CBOR.encodeWord32 w4
-                                                <> encodePortNumber pn
-encodeRemoteAddress (SockAddrUnix _) = error "Should never be encoding a SockAddrUnix!"
+instance Semigroup PeerSharing where
+  (<>) :: PeerSharing -> PeerSharing -> PeerSharing
+  (<>) = combinePeerSharing
 
--- | This decoder should be faithful to the PeerSharing
--- CDDL Specification.
+-- | The Monoid laws are witnessed by the following denotation function:
 --
--- See the network design document for more details
+-- ⟦_⟧ :: PeerSharing -> All
+-- ⟦ PeerSharingDisabled ⟧ = All False
+-- ⟦ PeerSharingEnabled ⟧  = All True
 --
-decodeRemoteAddress :: CBOR.Decoder s SockAddr
-decodeRemoteAddress = do
-  _ <- CBOR.decodeListLen
-  tok <- CBOR.decodeWord
-  case tok of
-    0 -> do
-      w <- CBOR.decodeWord32
-      pn <- decodePortNumber
-      return (SockAddrInet pn w)
-    1 -> do
-      w1 <- CBOR.decodeWord32
-      w2 <- CBOR.decodeWord32
-      w3 <- CBOR.decodeWord32
-      w4 <- CBOR.decodeWord32
-      pn <- decodePortNumber
-      return (SockAddrInet6 pn 0 (w1, w2, w3, w4) 0)
-    _ -> fail ("Serialise.decode.SockAddr unexpected tok " ++ show tok)
+instance Monoid PeerSharing where
+  mempty :: PeerSharing
+  mempty = PeerSharingEnabled
+
+instance FromJSON PeerSharing where
+  parseJSON = withText "PeerSharing" $ \t ->
+    case readMaybe (Text.unpack t) of
+      Nothing -> fail ("PeerSharing.parseJSON: could not parse value: "
+                     ++ Text.unpack t)
+      Just ps -> return ps
+
+instance ToJSON PeerSharing where
+  toJSON = String . Text.pack . show
+
diff --git a/ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs b/ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs
@@ -1817,11 +1817,11 @@ withConnectionManager ConnectionManagerArguments {
                           let connState' = OutboundDupState connId connThread handle Ticking
                               notifyInboundGov =
                                 case provenance' of
+                                  Inbound  -> False
                                   -- This is a connection to oneself; We don't
                                   -- need to notify the inbound governor, as
                                   -- it's already done by
                                   -- `includeInboundConnectionImpl`
-                                  Inbound  -> False
                                   Outbound -> True
                           writeTVar connVar connState'
                           case inboundGovernorInfoChannel of

diff --git a/ouroboros-network-framework/src/Ouroboros/Network/Protocol/Handshake/Unversioned.hs b/ouroboros-network-framework/src/Ouroboros/Network/Protocol/Handshake/Unversioned.hs
@@ -1,4 +1,5 @@
-{-# LANGUAGE NamedFieldPuns #-}
+{-# LANGUAGE NamedFieldPuns    #-}
+{-# LANGUAGE OverloadedStrings #-}
 
 -- | Unversioned protocol, used in tests and demo applications.
 --
@@ -26,8 +27,7 @@ import           Network.TypedProtocol.Codec
 
 import           Ouroboros.Network.CodecCBORTerm
 import           Ouroboros.Network.ConnectionManager.Types (DataFlow (..))
-import           Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..),
-                     combinePeerSharing)
+import           Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..))
 import           Ouroboros.Network.Protocol.Handshake.Codec
 import           Ouroboros.Network.Protocol.Handshake.Type
 import           Ouroboros.Network.Protocol.Handshake.Version
@@ -84,7 +84,7 @@ data DataFlowProtocolData =
 
 instance Acceptable DataFlowProtocolData where
   acceptableVersion (DataFlowProtocolData local lps) (DataFlowProtocolData remote rps) =
-    Accept (DataFlowProtocolData (local `min` remote) (combinePeerSharing lps rps))
+    Accept (DataFlowProtocolData (local `min` remote) (lps <> rps))
 
 instance Queryable DataFlowProtocolData where
   queryVersion (DataFlowProtocolData _ _) = False
@@ -104,14 +104,14 @@ dataFlowProtocolDataCodec _ = CodecCBORTerm {encodeTerm, decodeTerm}
               PeerSharingEnabled  -> 1
          in CBOR.TList [CBOR.TBool True, CBOR.TInt peerSharing]
 
-      toPeerSharing :: Int -> PeerSharing
-      toPeerSharing 0 = PeerSharingDisabled
-      toPeerSharing 1 = PeerSharingEnabled
-      toPeerSharing _ = error "toPeerSharing: out of bounds"
+      toPeerSharing :: Int -> Either Text PeerSharing
+      toPeerSharing 0 = Right PeerSharingDisabled
+      toPeerSharing 1 = Right PeerSharingEnabled
+      toPeerSharing _ = Left "toPeerSharing: out of bounds"
 
       decodeTerm :: CBOR.Term -> Either Text DataFlowProtocolData
-      decodeTerm (CBOR.TList [CBOR.TBool False, CBOR.TInt a]) = Right (DataFlowProtocolData Unidirectional (toPeerSharing a))
-      decodeTerm (CBOR.TList [CBOR.TBool True, CBOR.TInt a])  = Right (DataFlowProtocolData Duplex (toPeerSharing a))
+      decodeTerm (CBOR.TList [CBOR.TBool False, CBOR.TInt a]) = DataFlowProtocolData Unidirectional <$> (toPeerSharing a)
+      decodeTerm (CBOR.TList [CBOR.TBool True, CBOR.TInt a])  = DataFlowProtocolData Duplex <$> (toPeerSharing a)
       decodeTerm t                  = Left $ T.pack $ "unexpected term: " ++ show t
 
 dataFlowProtocol :: DataFlow

diff --git a/ouroboros-network-protocols/CHANGELOG.md b/ouroboros-network-protocols/CHANGELOG.md
@@ -10,9 +10,11 @@
 * Add a 3673s timeout to chainsync's StIdle state.
 * Add a 97s timeout to keepalive's StClient state.
 
-- Add a test to check that Peer Sharing values after handshake are symmetric
+* Add a test to check that Peer Sharing values after handshake are symmetric
   relative to the initiator and responder side.
-- Adds cddl specs and tests for `NodeToNodeV_13` and handshake
+* Adds cddl specs and tests for `NodeToNodeV_13` and handshake
+
+* Refactored cddl tests for `PeerSharing` to include versioning.
 
 ## 0.5.2.0 -- 2023-09-08