Creates 'cardano-node' sublibrary #5082

bolt12 · 2025-02-20T13:09:36Z

Two of the steps in #5065.

This PR:

Creates a sublibrary under ouroboros-network that contains all cardano-node specific data types and definitions.
Moves NetworkTopology data definition and auxiliary functions to the 'cardano-node' sublibrary.

The idea is that everything network related currently in cardano-node that requires integration, should live in this sublibrary. This will make maintance and future integration work easier.

crocodile-dentist

lgtm

crocodile-dentist · 2025-02-25T10:24:54Z

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/PeerSelection/Cardano/MockEnvironment.hs

@@ -91,13 +91,14 @@ import Cardano.Network.PeerSelection.LocalRootPeers
 import Cardano.Network.PeerSelection.PeerTrustable (PeerTrustable)
 import Cardano.Network.Types (LedgerStateJudgement (..),
           NumberOfBigLedgerPeers (..))
+import Ouroboros.Cardano.Network.ExtraRootPeers qualified as Cardano
+import Ouroboros.Cardano.Network.ExtraRootPeers qualified as ExtraPeers


Why not make this imported qualified once as Cardano.ExtraPeers like Cardano.PublicRootPeers below? This pattern is in a few other places in this patch so I just mention it once here.

I want to qualify ExtraPeers data type with Cardano to make it obvious it is an ExtraPeer for Cardano, but then I want to qualify ExtraPeers module definitions with ExtraPeers to make it obvious they are coming from ExtraPeers

It's also imported twice with different qualifications, are we sure it's a good idea?

I think it helps clarity when reading the code and understanding where things are coming from.. We also can't have everything qualified under Cardano since some record fields will clash

crocodile-dentist · 2025-02-25T10:31:56Z

ouroboros-network/src/Ouroboros/Cardano/Node/Topology.hs

+-- | This function returns false if non-trustable peers are configured
+--
+isValidTrustedPeerConfiguration :: CardanoNetworkTopology -> Bool
+isValidTrustedPeerConfiguration


The node also has a check whether the options are compatible with Genesis, ie. we have UseBootstrapPeers enabled but so is Genesis - I'd have to take another look at the node code but maybe that can be moved here as well?

Ah, I see, I'll double check! I'll be making an integration branch soon and expect to catch more things to add here

coot

Thanks @bolt12, some suggestions with possible improvements follow.

coot · 2025-02-25T10:49:55Z

ouroboros-network/cardano-node/Ouroboros/Cardano/Network/PublicRootPeers.hs

+import Data.Set qualified as Set
+import Ouroboros.Cardano.Network.ExtraRootPeers (ExtraPeers (..))


We usually space seprate library imports from local ones:

Suggested change

import Data.Set qualified as Set

import Ouroboros.Cardano.Network.ExtraRootPeers (ExtraPeers (..))

import Data.Set qualified as Set

import Ouroboros.Cardano.Network.ExtraRootPeers (ExtraPeers (..))

coot · 2025-02-25T13:34:06Z

ouroboros-network/src/Ouroboros/Network/Diffusion.hs

-
 socketAddressType :: Socket.SockAddr -> Maybe AddressType
 socketAddressType Socket.SockAddrInet {}  = Just IPv4Address
 socketAddressType Socket.SockAddrInet6 {} = Just IPv6Address
 socketAddressType Socket.SockAddrUnix {}  = Nothing

-


These double empty lines shouldn't be removed.

coot · 2025-02-25T13:34:56Z

ouroboros-network/cardano-node/Ouroboros/Cardano/Network/PublicRootPeers.hs

+{-# OPTIONS_GHC -Wno-orphans #-}
+module Ouroboros.Cardano.Network.PublicRootPeers where


Suggested change

{-# OPTIONS_GHC -Wno-orphans #-}

module Ouroboros.Cardano.Network.PublicRootPeers where

{-# OPTIONS_GHC -Wno-orphans #-}

module Ouroboros.Cardano.Network.PublicRootPeers where

coot · 2025-02-25T13:40:07Z

ouroboros-network/cardano-node/Ouroboros/Cardano/Network/PublicRootPeers.hs

@@ -0,0 +1,132 @@
+{-# LANGUAGE FlexibleInstances    #-}


From which module in main this code is coming from?

what do you mean? This is required for the instances that are defined in this module:

e.g.:

instance ( Ord peeraddr ) => Semigroup (CardanoPublicRootPeers peeraddr) where (<>) = merge

coot · 2025-02-25T13:42:00Z

ouroboros-network/cardano-node/Ouroboros/Cardano/Network/Types.hs

+module Ouroboros.Cardano.Network.Types where
+
+data ChurnMode = ChurnModeBulkSync
+               | ChurnModeNormal
+               deriving Show


Why not move this to Ouroboros.Cardano.PeerSelection.Churn? Types at this level is a bit general for churn specific stuff, isn't it?

Yes that's a much better thing to do!

coot · 2025-02-25T14:05:07Z

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/PeerSelection/Cardano/MockEnvironment.hs

@@ -91,13 +91,14 @@ import Cardano.Network.PeerSelection.LocalRootPeers
 import Cardano.Network.PeerSelection.PeerTrustable (PeerTrustable)
 import Cardano.Network.Types (LedgerStateJudgement (..),
           NumberOfBigLedgerPeers (..))
+import Ouroboros.Cardano.Network.ExtraRootPeers qualified as Cardano
+import Ouroboros.Cardano.Network.ExtraRootPeers qualified as ExtraPeers


It's also imported twice with different qualifications, are we sure it's a good idea?

coot · 2025-02-25T14:05:41Z

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/PeerSelection/Cardano/PublicRootPeers.hs

-                                => Gen (PublicRootPeers (Cardano.ExtraPeers peeraddr) peeraddr)
+                                => Gen (CardanoPublicRootPeers peeraddr)


coot · 2025-02-25T14:06:31Z

ouroboros-network/src/Ouroboros/Network/Diffusion/Topology.hs

+import Data.Map qualified as Map
+import Data.Map.Strict (Map)
+import Ouroboros.Network.Diffusion.Configuration (DiffusionMode)


Let's group imports

Suggested change

import Data.Map qualified as Map

import Data.Map.Strict (Map)

import Ouroboros.Network.Diffusion.Configuration (DiffusionMode)

import Data.Map qualified as Map

import Data.Map.Strict (Map)

import Ouroboros.Network.Diffusion.Configuration (DiffusionMode)

coot · 2025-02-25T14:09:04Z

ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/Types.hs

@@ -1797,7 +1796,6 @@ data TracePeerSelection extraDebugState extraFlags extraPeers peeraddr =
     --

     | TraceChurnWait          DiffTime
-     | TraceChurnMode          ChurnMode


So we have an extra tracer just for ChurnMode; When we'll be integrating with cardano-node it should be configured afterTracePeerSelection (no need for a separate option for enabling / disabling it).

coot · 2025-02-25T14:09:41Z

ouroboros-network/cardano-node/Ouroboros/Cardano/Network/PeerSelection/Governor/Types.hs

 import Control.Concurrent.Class.MonadSTM
 import Data.Set (Set)
 import Data.Set qualified as Set


We could organise the imports better.

This is a Cardano specific type and this is a step to remove any lingering cardano-node details from the network code and move them to the cardano-node sublibrary.

bolt12 self-assigned this Feb 20, 2025

bolt12 requested a review from a team as a code owner February 20, 2025 13:09

bolt12 force-pushed the bolt12/topology branch from 6d24443 to 7d6fc86 Compare February 21, 2025 16:23

bolt12 changed the title ~~Moved Topology files to ouroboros-network~~ Creates 'cardano-node' sublibrary Feb 21, 2025

bolt12 force-pushed the bolt12/topology branch from 7d6fc86 to d9c4151 Compare February 21, 2025 17:08

bolt12 requested review from coot and crocodile-dentist February 24, 2025 15:43

crocodile-dentist approved these changes Feb 25, 2025

View reviewed changes

coot requested changes Feb 25, 2025

View reviewed changes

bolt12 force-pushed the bolt12/topology branch 3 times, most recently from 8b6facd to cfcdb12 Compare February 25, 2025 16:20

bolt12 requested a review from coot February 25, 2025 16:29

bolt12 force-pushed the bolt12/topology branch from cfcdb12 to d4cbca6 Compare February 26, 2025 12:37

bolt12 added 6 commits February 27, 2025 09:55

Moved Topology files to ouroboros-network

72ef4ac

Remove TraceChurnMode from ouroboros-network code

b49fec0

This is a Cardano specific type and this is a step to remove any lingering cardano-node details from the network code and move them to the cardano-node sublibrary.

Move ExtraPeers to standalone module

c737dcc

This is a Cardano specific type and this is a step to remove any lingering cardano-node details from the network code and move them to the cardano-node sublibrary.

Created cardano-node sublibrary

f83e18c

Address Review Feedback

ce6462b

Update CHANGELOG

7b2cd34

bolt12 force-pushed the bolt12/topology branch from d4cbca6 to 7b2cd34 Compare February 27, 2025 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creates 'cardano-node' sublibrary #5082

Creates 'cardano-node' sublibrary #5082

bolt12 commented Feb 20, 2025 •

edited

Loading

crocodile-dentist left a comment

crocodile-dentist Feb 25, 2025

bolt12 Feb 25, 2025

coot Feb 25, 2025

bolt12 Feb 25, 2025

crocodile-dentist Feb 25, 2025

bolt12 Feb 25, 2025

coot left a comment

coot Feb 25, 2025

coot Feb 25, 2025

coot Feb 25, 2025

coot Feb 25, 2025

bolt12 Feb 25, 2025

coot Feb 25, 2025

bolt12 Feb 25, 2025

coot Feb 25, 2025

coot Feb 25, 2025

coot Feb 25, 2025

coot Feb 25, 2025

bolt12 Feb 25, 2025

coot Feb 25, 2025

		import Data.Set qualified as Set
		import Ouroboros.Cardano.Network.ExtraRootPeers (ExtraPeers (..))

		{-# OPTIONS_GHC -Wno-orphans #-}
		module Ouroboros.Cardano.Network.PublicRootPeers where

		=> Gen (PublicRootPeers (Cardano.ExtraPeers peeraddr) peeraddr)
		=> Gen (CardanoPublicRootPeers peeraddr)

Creates 'cardano-node' sublibrary #5082

Are you sure you want to change the base?

Creates 'cardano-node' sublibrary #5082

Conversation

bolt12 commented Feb 20, 2025 • edited Loading

crocodile-dentist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolt12 commented Feb 20, 2025 •

edited

Loading