Address review comments

docs/network-spec/miniprotocols.tex

@@ -1399,8 +1399,11 @@ \subsection{Server Implementation Details}
 function application all the way to diffusion and share the relevant parts of
 \texttt{PeerSelectionState} with this function via a \texttt{TVar}.
 
-\subsection{CDDL encoding specification}
-\lstinputlisting[style=cddl]{../../ouroboros-network-protocols/test-cddl/specs/peer-sharing.cddl}
+\subsection{CDDL encoding specification ($11$ to $12$)}\label{peersharing-cddl}
+\lstinputlisting[style=cddl]{../../ouroboros-network-protocols/test-cddl/specs/peer-sharing-v11-12.cddl}
+
+\subsection{CDDL encoding specification ($\geq 13$)}\label{peersharing-cddl}
+\lstinputlisting[style=cddl]{../../ouroboros-network-protocols/test-cddl/specs/peer-sharing-v13.cddl}
 
 \section{Pipelining of Mini Protocols}
 \label{pipelining}

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
 

ouroboros-network-api/ouroboros-network-api.cabal

@@ -45,6 +45,7 @@ library
                        Ouroboros.Network.PeerSelection.PeerMetric.Type
                        Ouroboros.Network.PeerSelection.PeerAdvertise
                        Ouroboros.Network.PeerSelection.PeerSharing
+                       Ouroboros.Network.PeerSelection.PeerSharing.Codec
                        Ouroboros.Network.PeerSelection.RelayAccessPoint
   default-language:    Haskell2010
   build-depends:       base              >=4.14 && <4.19,

ouroboros-network-api/src/Ouroboros/Network/NodeToNode/Version.hs

@@ -23,9 +23,7 @@ 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 (..))
 
 -- | Enumeration of node to node protocol versions.
 --
@@ -62,7 +60,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 +132,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
@@ -167,7 +164,7 @@ nodeToNodeCodecCBORTerm version
           | x >= 0
           , x <= 0xffffffff
           , peerSharing >= 0
-          , peerSharing <= 2
+          , peerSharing <= 1
           = Right
               NodeToNodeVersionData {
                   networkMagic = NetworkMagic (fromIntegral x),
@@ -182,7 +179,7 @@ nodeToNodeCodecCBORTerm version
                 }
           | x < 0 || x > 0xffffffff
           = Left $ T.pack $ "networkMagic out of bound: " <> show x
-          | otherwise -- peerSharing < 0 || peerSharing > 2
+          | otherwise -- peerSharing < 0 || peerSharing > 1
           = Left $ T.pack $ "peerSharing is out of bound: " <> show peerSharing
         decodeTerm _ t
           = Left $ T.pack $ "unknown encoding: " ++ show t
@@ -267,7 +264,6 @@ nodeToNodeCodecCBORTerm version
 
 data ConnectionMode = UnidirectionalMode | DuplexMode
 
-
 -- | Check whether a version enabling diffusion pipelining has been
 -- negotiated.
 --

ouroboros-network-api/src/Ouroboros/Network/PeerSelection/PeerSharing.hs

@@ -1,24 +1,11 @@
 {-# 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
 -- to share this peer's address?
@@ -32,69 +19,30 @@ 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

ouroboros-network-api/src/Ouroboros/Network/PeerSelection/PeerSharing/Codec.hs

@@ -0,0 +1,100 @@
+module Ouroboros.Network.PeerSelection.PeerSharing.Codec
+  ( encodePortNumber
+  , decodePortNumber
+  , encodeRemoteAddress
+  , decodeRemoteAddress
+  ) where
+
+import qualified Codec.CBOR.Decoding as CBOR
+import qualified Codec.CBOR.Encoding as CBOR
+
+import           Network.Socket (PortNumber, SockAddr (..))
+import           Ouroboros.Network.NodeToNode.Version (NodeToNodeVersion (..))
+
+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 0 (w1, w2, w3, w4) 0)
+      _ -> fail ("Serialise.decode.SockAddr unexpected tok " ++ show tok)
+

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs

@@ -1844,11 +1844,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

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

ouroboros-network-protocols/CHANGELOG.md

@@ -14,9 +14,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