Fix Peer Sharing issue #4642

- Remove `PeerSharingPrivate` option and renamed others to
  `PeerSharingDisabled` and `PeerSharingEnabled`
- Add test to check that the bug is fixed
- Add new `NodeToNodeV_13`
- Renamed spec files and added V13 CDDL handshake and
  `NodeToNodeVersionData` specs

docs/network-spec/miniprotocols.tex

@@ -584,10 +584,15 @@ \subsubsection{Node to node handshake mini-protocol}
 \subsubsection{Node to client handshake mini-protocol}
 \lstinputlisting[style=cddl]{../../ouroboros-network-protocols/test-cddl/specs/handshake-node-to-client.cddl}
 
-\subsection{CDDL encoding specification ($\geq 11$)}\label{handshake-cddl}
+\subsection{CDDL encoding specification ($11$ to $12$)}\label{handshake-cddl}
 
 \subsubsection{Node to node handshake mini-protocol}
-\lstinputlisting[style=cddl]{../../ouroboros-network-protocols/test-cddl/specs/handshake-node-to-node-v11.cddl}
+\lstinputlisting[style=cddl]{../../ouroboros-network-protocols/test-cddl/specs/handshake-node-to-node-v11-12.cddl}
+
+\subsection{CDDL encoding specification ($\geq 13$)}\label{handshake-cddl}
+
+\subsubsection{Node to node handshake mini-protocol}
+\lstinputlisting[style=cddl]{../../ouroboros-network-protocols/test-cddl/specs/handshake-node-to-node-v13.cddl}
 
 \section{Chain-Sync mini-protocol}
 \label{chain-sync-protocol}

ouroboros-network-api/CHANGELOG.md

@@ -4,6 +4,12 @@
 
 ### Breaking changes
 
+- 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
+  type.
+
 ### Non-breaking changes
 
 

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

@@ -23,7 +23,8 @@ import           Ouroboros.Network.Handshake.Acceptable (Accept (..),
                      Acceptable (..))
 import           Ouroboros.Network.Handshake.Queryable (Queryable (..))
 import           Ouroboros.Network.Magic
-import           Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..))
+import           Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..),
+                     combinePeerSharing)
 
 
 -- | Enumeration of node to node protocol versions.
@@ -58,6 +59,10 @@ data NodeToNodeVersion
     -- ^ Changes:
     --
     -- * Enable @CardanoNodeToNodeVersion7@, i.e., Conway
+    | NodeToNodeV_13
+    -- ^ Changes:
+    --
+    -- * Adds a fix for PeerSharing handshake negotiation
   deriving (Eq, Ord, Enum, Bounded, Show, Typeable)
 
 nodeToNodeVersionCodec :: CodecCBORTerm (Text, Maybe Int) NodeToNodeVersion
@@ -69,13 +74,15 @@ nodeToNodeVersionCodec = CodecCBORTerm { encodeTerm, decodeTerm }
     encodeTerm NodeToNodeV_10 = CBOR.TInt 10
     encodeTerm NodeToNodeV_11 = CBOR.TInt 11
     encodeTerm NodeToNodeV_12 = CBOR.TInt 12
+    encodeTerm NodeToNodeV_13 = CBOR.TInt 13
 
     decodeTerm (CBOR.TInt 7) = Right NodeToNodeV_7
     decodeTerm (CBOR.TInt 8) = Right NodeToNodeV_8
     decodeTerm (CBOR.TInt 9) = Right NodeToNodeV_9
     decodeTerm (CBOR.TInt 10) = Right NodeToNodeV_10
     decodeTerm (CBOR.TInt 11) = Right NodeToNodeV_11
     decodeTerm (CBOR.TInt 12) = Right NodeToNodeV_12
+    decodeTerm (CBOR.TInt 13) = Right NodeToNodeV_13
     decodeTerm (CBOR.TInt n) = Left ( T.pack "decode NodeToNodeVersion: unknonw tag: "
                                         <> T.pack (show n)
                                     , Just n
@@ -127,7 +134,8 @@ instance Acceptable NodeToNodeVersionData where
       = Accept NodeToNodeVersionData
           { networkMagic  = networkMagic local
           , diffusionMode = diffusionMode local `min` diffusionMode remote
-          , peerSharing   = peerSharing remote
+          , peerSharing   = combinePeerSharing (peerSharing local)
+                                               (peerSharing remote)
           , query         = query local || query remote
           }
       | otherwise
@@ -140,7 +148,7 @@ instance Queryable NodeToNodeVersionData where
 
 nodeToNodeCodecCBORTerm :: NodeToNodeVersion -> CodecCBORTerm Text NodeToNodeVersionData
 nodeToNodeCodecCBORTerm version
-  | version >= NodeToNodeV_11 =
+  | version >= NodeToNodeV_13 =
     let encodeTerm :: NodeToNodeVersionData -> CBOR.Term
         encodeTerm NodeToNodeVersionData { networkMagic, diffusionMode, peerSharing, query }
           = CBOR.TList $
@@ -149,9 +157,49 @@ nodeToNodeCodecCBORTerm version
                              InitiatorOnlyDiffusionMode         -> True
                              InitiatorAndResponderDiffusionMode -> False)
               , CBOR.TInt (case peerSharing of
-                             NoPeerSharing      -> 0
-                             PeerSharingPrivate -> 1
-                             PeerSharingPublic  -> 2)
+                             PeerSharingDisabled -> 0
+                             PeerSharingEnabled  -> 1)
+              , CBOR.TBool query
+              ]
+
+        decodeTerm :: NodeToNodeVersion -> CBOR.Term -> Either Text NodeToNodeVersionData
+        decodeTerm _ (CBOR.TList [CBOR.TInt x, CBOR.TBool diffusionMode, CBOR.TInt peerSharing, CBOR.TBool query])
+          | x >= 0
+          , x <= 0xffffffff
+          , Just ps <- case peerSharing of
+                        0 -> Just PeerSharingDisabled
+                        1 -> Just PeerSharingEnabled
+                        _ -> Nothing
+          = Right
+              NodeToNodeVersionData {
+                  networkMagic = NetworkMagic (fromIntegral x),
+                  diffusionMode = if diffusionMode
+                                  then InitiatorOnlyDiffusionMode
+                                  else InitiatorAndResponderDiffusionMode,
+                  peerSharing = ps,
+                  query = query
+                }
+          | x < 0 || x > 0xffffffff
+          = Left $ T.pack $ "networkMagic out of bound: " <> show x
+          | otherwise -- peerSharing < 0 || peerSharing > 1
+          = Left $ T.pack $ "peerSharing is out of bound: " <> show peerSharing
+        decodeTerm _ t
+          = Left $ T.pack $ "unknown encoding: " ++ show t
+     in CodecCBORTerm {encodeTerm, decodeTerm = decodeTerm version }
+  | version >= NodeToNodeV_11
+  , version <= NodeToNodeV_12 =
+    let encodeTerm :: NodeToNodeVersionData -> CBOR.Term
+        encodeTerm NodeToNodeVersionData { networkMagic, diffusionMode, peerSharing, query }
+          = CBOR.TList
+              [ CBOR.TInt (fromIntegral $ unNetworkMagic networkMagic)
+              , CBOR.TBool (case diffusionMode of
+                             InitiatorOnlyDiffusionMode         -> True
+                             InitiatorAndResponderDiffusionMode -> False)
+                          -- Need to be careful mapping here since older
+                          -- versions will map PeerSharingPrivate to 1.
+              , CBOR.TInt (case peerSharing of
+                             PeerSharingDisabled -> 0
+                             PeerSharingEnabled  -> 2)
               , CBOR.TBool query
               ]
 
@@ -161,23 +209,27 @@ nodeToNodeCodecCBORTerm version
           , x <= 0xffffffff
           , peerSharing >= 0
           , peerSharing <= 2
+            -- This means if an older version node with
+            -- NodeToNodeV_{11,12} talks with a >NodeToNodeV_13
+            -- one it will map PeerSharingPrivate to PeerSharingDisabled
+          , Just ps <- case peerSharing of
+                        0 -> Just PeerSharingDisabled
+                        1 -> Just PeerSharingDisabled
+                        2 -> Just PeerSharingEnabled
+                        _ -> Nothing
           = Right
               NodeToNodeVersionData {
                   networkMagic = NetworkMagic (fromIntegral x),
                   diffusionMode = if diffusionMode
                                   then InitiatorOnlyDiffusionMode
                                   else InitiatorAndResponderDiffusionMode,
-                  peerSharing = case peerSharing of
-                                  0 -> NoPeerSharing
-                                  1 -> PeerSharingPrivate
-                                  2 -> PeerSharingPublic
-                                  _ -> error "decodeTerm: impossible happened!",
+                  peerSharing = ps,
                   query = query
                 }
           | x < 0 || x > 0xffffffff
           = Left $ T.pack $ "networkMagic out of bound: " <> show x
           | otherwise -- peerSharing < 0 || peerSharing > 2
-          = Left $ T.pack $ "peerSharing out of bound: " <> show peerSharing
+          = Left $ T.pack $ "Either peerSharing is out of bound: " <> show peerSharing
         decodeTerm _ t
           = Left $ T.pack $ "unknown encoding: " ++ show t
      in CodecCBORTerm {encodeTerm, decodeTerm = decodeTerm version }
@@ -203,7 +255,7 @@ nodeToNodeCodecCBORTerm version
                                   else InitiatorAndResponderDiffusionMode
                   -- By default older versions do not participate in Peer
                   -- Sharing, since they do not support the new miniprotocol
-                , peerSharing = NoPeerSharing
+                , peerSharing = PeerSharingDisabled
                 , query = False
                 }
           | otherwise

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

@@ -4,7 +4,7 @@
 
 module Ouroboros.Network.PeerSelection.PeerSharing
   ( PeerSharing (..)
-  , combinePeerInformation
+  , combinePeerSharing
   , encodePortNumber
   , decodePortNumber
   , encodeRemoteAddress
@@ -18,8 +18,6 @@ import           Data.Aeson.Types (FromJSON (..), ToJSON (..), Value (..),
 import qualified Data.Text as Text
 import           GHC.Generics (Generic)
 import           Network.Socket (PortNumber, SockAddr (..))
-import           Ouroboros.Network.PeerSelection.PeerAdvertise
-                     (PeerAdvertise (..))
 import           Text.Read (readMaybe)
 
 -- | Is a peer willing to participate in Peer Sharing? If yes are others allowed
@@ -29,11 +27,9 @@ import           Text.Read (readMaybe)
 --
 -- NOTE: This information is only useful if P2P flag is enabled.
 --
-data PeerSharing = NoPeerSharing -- ^ Peer does not participate in Peer Sharing
-                                 -- at all
-                 | PeerSharingPrivate -- ^ Peer participates in Peer Sharing but
-                                      -- its address should be private
-                 | PeerSharingPublic -- ^ Peer participates in Peer Sharing
+data PeerSharing = PeerSharingDisabled -- ^ Peer does not participate in Peer Sharing
+                                       -- at all
+                 | PeerSharingEnabled -- ^ Peer participates in Peer Sharing
   deriving  (Eq, Show, Read, Generic)
 
 instance FromJSON PeerSharing where
@@ -46,21 +42,13 @@ instance FromJSON PeerSharing where
 instance ToJSON PeerSharing where
   toJSON = String . Text.pack . show
 
--- Combine a 'PeerSharing' value and a 'PeerAdvertise' value into a
--- resulting 'PeerSharing' that can be used to decide if we should
--- share or not the given Peer. According to the following rules:
+-- | Combine two 'PeerSharing' values
 --
--- - If no PeerSharing value is known then there's nothing we can assess
--- - If a peer is not participating in Peer Sharing ignore all other information
--- - If a peer said it wasn't okay to share its address, respect that no matter what.
--- - If a peer was privately configured with DoNotAdvertisePeer respect that no matter
--- what.
---
-combinePeerInformation :: PeerSharing -> PeerAdvertise -> PeerSharing
-combinePeerInformation NoPeerSharing      _                  = NoPeerSharing
-combinePeerInformation PeerSharingPrivate _                  = PeerSharingPrivate
-combinePeerInformation PeerSharingPublic  DoNotAdvertisePeer = PeerSharingPrivate
-combinePeerInformation _                         _           = PeerSharingPublic
+-- 'PeerSharingDisabled' is the absorbing element
+combinePeerSharing :: PeerSharing -> PeerSharing -> PeerSharing
+combinePeerSharing PeerSharingDisabled _ = PeerSharingDisabled
+combinePeerSharing _ PeerSharingDisabled = PeerSharingDisabled
+combinePeerSharing _ _                   = PeerSharingEnabled
 
 encodePortNumber :: PortNumber -> CBOR.Encoding
 encodePortNumber = CBOR.encodeWord16 . fromIntegral

ouroboros-network-framework/CHANGELOG.md

@@ -6,6 +6,8 @@
 
 ### Non-breaking changes
 
+* Update code to accommodate changes on `PeerSharing` data type.
+
 ## 0.10.0.0 -- 2023-10-26
 
 ### Breaking changes

ouroboros-network-framework/demo/connection-manager.hs

@@ -247,7 +247,7 @@ withBidirectionalConnectionManager snocket makeBearer socket
               acceptedConnectionsSoftLimit = maxBound,
               acceptedConnectionsDelay     = 0
             },
-          cmGetPeerSharing = \_ -> NoPeerSharing
+          cmGetPeerSharing = \_ -> PeerSharingDisabled
         }
         (makeConnectionHandler
           muxTracer

ouroboros-network-framework/sim-tests/Test/Ouroboros/Network/ConnectionManager.hs

@@ -773,7 +773,7 @@ prop_valid_transitions (SkewedBool bindToLocalAddress) scheduleMap =
                 },
               cmTimeWaitTimeout = testTimeWaitTimeout,
               cmOutboundIdleTimeout = testOutboundIdleTimeout,
-              cmGetPeerSharing = \_ -> NoPeerSharing
+              cmGetPeerSharing = \_ -> PeerSharingDisabled
             }
             connectionHandler
             (\_ -> HandshakeFailure)

ouroboros-network-framework/sim-tests/Test/Ouroboros/Network/Server2/Sim.hs

@@ -153,7 +153,6 @@ tests =
 -- Server tests
 --
 
-
 prop_unidirectional_Sim :: ClientAndServerData Int
                         -> Property
 prop_unidirectional_Sim clientAndServerData =

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

@@ -141,7 +141,8 @@ data ConnectionManagerArguments handlerTrace socket peerAddr handle handleError
         cmPrunePolicy         :: PrunePolicy peerAddr (STM m),
         cmConnectionsLimits   :: AcceptedConnectionsLimit,
 
-        -- | How to extract PeerSharing information from versionData
+        -- | How to extract remote side's PeerSharing information from
+        -- versionData
         cmGetPeerSharing      :: versionData -> PeerSharing
       }
 
@@ -1843,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

@@ -26,7 +26,8 @@ import           Network.TypedProtocol.Codec
 
 import           Ouroboros.Network.CodecCBORTerm
 import           Ouroboros.Network.ConnectionManager.Types (DataFlow (..))
-import           Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..))
+import           Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..),
+                     combinePeerSharing)
 import           Ouroboros.Network.Protocol.Handshake.Codec
 import           Ouroboros.Network.Protocol.Handshake.Type
 import           Ouroboros.Network.Protocol.Handshake.Version
@@ -82,8 +83,8 @@ data DataFlowProtocolData =
   deriving (Eq, Show)
 
 instance Acceptable DataFlowProtocolData where
-  acceptableVersion (DataFlowProtocolData local _) (DataFlowProtocolData remote ps) =
-    Accept (DataFlowProtocolData (local `min` remote) ps)
+  acceptableVersion (DataFlowProtocolData local lps) (DataFlowProtocolData remote rps) =
+    Accept (DataFlowProtocolData (local `min` remote) (combinePeerSharing lps rps))
 
 instance Queryable DataFlowProtocolData where
   queryVersion (DataFlowProtocolData _ _) = False
@@ -92,20 +93,25 @@ dataFlowProtocolDataCodec :: UnversionedProtocol -> CodecCBORTerm Text DataFlowP
 dataFlowProtocolDataCodec _ = CodecCBORTerm {encodeTerm, decodeTerm}
     where
       encodeTerm :: DataFlowProtocolData -> CBOR.Term
-      encodeTerm (DataFlowProtocolData Unidirectional NoPeerSharing)      = CBOR.TList [CBOR.TBool False, CBOR.TInt 0]
-      encodeTerm (DataFlowProtocolData Unidirectional PeerSharingPrivate) = CBOR.TList [CBOR.TBool False, CBOR.TInt 1]
-      encodeTerm (DataFlowProtocolData Unidirectional PeerSharingPublic)  = CBOR.TList [CBOR.TBool False, CBOR.TInt 2]
-      encodeTerm (DataFlowProtocolData Duplex NoPeerSharing)      = CBOR.TList [CBOR.TBool True, CBOR.TInt 0]
-      encodeTerm (DataFlowProtocolData Duplex PeerSharingPrivate) = CBOR.TList [CBOR.TBool True, CBOR.TInt 1]
-      encodeTerm (DataFlowProtocolData Duplex PeerSharingPublic)  = CBOR.TList [CBOR.TBool True, CBOR.TInt 2]
+      encodeTerm (DataFlowProtocolData Unidirectional ps) =
+        let peerSharing = case ps of
+              PeerSharingDisabled -> 0
+              PeerSharingEnabled  -> 1
+         in CBOR.TList [CBOR.TBool False, CBOR.TInt peerSharing]
+      encodeTerm (DataFlowProtocolData Duplex ps) =
+        let peerSharing = case ps of
+              PeerSharingDisabled -> 0
+              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"
 
       decodeTerm :: CBOR.Term -> Either Text DataFlowProtocolData
-      decodeTerm (CBOR.TList [CBOR.TBool False, CBOR.TInt 0]) = Right (DataFlowProtocolData Unidirectional NoPeerSharing)
-      decodeTerm (CBOR.TList [CBOR.TBool False, CBOR.TInt 1]) = Right (DataFlowProtocolData Unidirectional PeerSharingPrivate)
-      decodeTerm (CBOR.TList [CBOR.TBool False, CBOR.TInt 2]) = Right (DataFlowProtocolData Unidirectional PeerSharingPublic)
-      decodeTerm (CBOR.TList [CBOR.TBool True, CBOR.TInt 0])  = Right (DataFlowProtocolData Duplex NoPeerSharing)
-      decodeTerm (CBOR.TList [CBOR.TBool True, CBOR.TInt 1])  = Right (DataFlowProtocolData Duplex PeerSharingPrivate)
-      decodeTerm (CBOR.TList [CBOR.TBool True, CBOR.TInt 2])  = Right (DataFlowProtocolData Duplex PeerSharingPublic)
+      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 t                  = Left $ T.pack $ "unexpected term: " ++ show t
 
 dataFlowProtocol :: DataFlow
@@ -114,7 +120,7 @@ dataFlowProtocol :: DataFlow
                              DataFlowProtocolData
                              app
 dataFlowProtocol dataFlow =
-    simpleSingletonVersions UnversionedProtocol (DataFlowProtocolData dataFlow NoPeerSharing)
+    simpleSingletonVersions UnversionedProtocol (DataFlowProtocolData dataFlow PeerSharingDisabled)
 
 -- | 'Handshake' codec used in various tests.
 --

ouroboros-network-protocols/CHANGELOG.md

@@ -14,6 +14,10 @@
 * 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
+  relative to the initiator and responder side.
+- Adds cddl specs and tests for `NodeToNodeV_13` and handshake
+
 ## 0.5.2.0 -- 2023-09-08
 
 ### Non-breaking changes