Merge #3688

3688: Block diffusion pipelining r=nfrisby a=nfrisby

This PR adds block diffusion pipelining, primarily by finally introducing the long-existing concept of _the tentative chain_ into the code.

Main changes:

- add the tentative chain header to the ChainDB state.

- have the `Follower`s for the Node-To-Node ChainSync server follow the tentative chain instead of only the selected chain, thus sending the tentative header before the underlying block has been validated

- this now means some honest nodes will send us an invalid block, when the block is tentative, so we adjust ChainSync and BlockFetch clients to allow that in limited number of scenarios necessary for the common pipelining scenarios

Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>
Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io>

network-mux/tools/cardano-ping.hs

@@ -127,6 +127,7 @@ supportedNodeToNodeVersions :: Word32 -> [NodeVersion]
 supportedNodeToNodeVersions magic =
   [ NodeToNodeVersionV7 magic False
   , NodeToNodeVersionV8 magic False
+  , NodeToNodeVersionV9 magic False
   ]
 
 supportedNodeToClientVersions :: Word32 -> [NodeVersion]
@@ -206,6 +207,7 @@ data NodeVersion =       NodeToClientVersionV9  Word32
                        | NodeToNodeVersionV6   Word32 Bool
                        | NodeToNodeVersionV7   Word32 Bool
                        | NodeToNodeVersionV8   Word32 Bool
+                       | NodeToNodeVersionV9   Word32 Bool
                        deriving (Eq, Ord, Show)
 
 keepAliveReqEnc :: NodeVersion -> Word16 -> CBOR.Encoding
@@ -267,6 +269,7 @@ handshakeReqEnc versions =
     encodeVersion (NodeToNodeVersionV6 magic mode) = encodeWithMode 6 magic mode
     encodeVersion (NodeToNodeVersionV7 magic mode) = encodeWithMode 7 magic mode
     encodeVersion (NodeToNodeVersionV8 magic mode) = encodeWithMode 8 magic mode
+    encodeVersion (NodeToNodeVersionV9 magic mode) = encodeWithMode 9 magic mode
 
 
     encodeWithMode :: Word -> Word32 -> Bool -> CBOR.Encoding
@@ -339,6 +342,7 @@ handshakeDec = do
              , version `testBit`  nodeToClientVersionBit ) of
              (7, False) -> decodeWithMode NodeToNodeVersionV7
              (8, False) -> decodeWithMode NodeToNodeVersionV8
+             (9, False) -> decodeWithMode NodeToNodeVersionV9
              (9, True)  -> Right . NodeToClientVersionV9 <$> CBOR.decodeWord32
              (10, True) -> Right . NodeToClientVersionV10 <$> CBOR.decodeWord32
              (11, True) -> Right . NodeToClientVersionV11 <$> CBOR.decodeWord32

ouroboros-consensus-cardano/src/Ouroboros/Consensus/Cardano/Node.hs

@@ -382,6 +382,7 @@ instance CardanoHardForkConstraints c
   supportedNodeToNodeVersions _ = Map.fromList $
       [ (NodeToNodeV_7, CardanoNodeToNodeVersion5)
       , (NodeToNodeV_8, CardanoNodeToNodeVersion5)
+      , (NodeToNodeV_9, CardanoNodeToNodeVersion5)
       ]
 
   supportedNodeToClientVersions _ = Map.fromList $
@@ -399,7 +400,7 @@ instance CardanoHardForkConstraints c
       , (NodeToClientV_12, CardanoNodeToClientVersion8)
       ]
 
-  latestReleasedNodeVersion _prx = (Just NodeToNodeV_7, Just NodeToClientV_12)
+  latestReleasedNodeVersion _prx = (Just NodeToNodeV_8, Just NodeToClientV_12)
 
 {-------------------------------------------------------------------------------
   ProtocolInfo

ouroboros-consensus-protocol/src/Ouroboros/Consensus/Protocol/TPraos.hs

@@ -17,7 +17,6 @@
 --   schedule determining slots to be produced by BFT
 module Ouroboros.Consensus.Protocol.TPraos (
     MaxMajorProtVer (..)
-  , SelfIssued (..)
   , TPraos
   , TPraosCanBeLeader (..)
   , TPraosChainSelectView (..)
@@ -270,45 +269,26 @@ data instance ConsensusConfig (TPraos c) = TPraosConfig {
 
 instance SL.PraosCrypto c => NoThunks (ConsensusConfig (TPraos c))
 
--- | Separate type instead of 'Bool' for the custom 'Ord' instance +
--- documentation.
-data SelfIssued =
-    -- | A block we produced ourself
-    SelfIssued
-    -- | A block produced by another node
-  | NotSelfIssued
-  deriving (Show, Eq)
-
-instance Ord SelfIssued where
-  compare SelfIssued    SelfIssued    = EQ
-  compare NotSelfIssued NotSelfIssued = EQ
-  compare SelfIssued    NotSelfIssued = GT
-  compare NotSelfIssued SelfIssued    = LT
-
 -- | View of the ledger tip for chain selection.
 --
 -- We order between chains as follows:
 --
 -- 1. By chain length, with longer chains always preferred.
--- 2. If the tip of each chain has the same slot number, we prefer the one tip
---    that we produced ourselves.
--- 3. If the tip of each chain was issued by the same agent, then we prefer
+-- 2. If the tip of each chain was issued by the same agent, then we prefer
 --    the chain whose tip has the highest ocert issue number.
--- 4. By the leader value of the chain tip, with lower values preferred.
+-- 3. By the leader value of the chain tip, with lower values preferred.
 data TPraosChainSelectView c = TPraosChainSelectView {
     csvChainLength :: BlockNo
   , csvSlotNo      :: SlotNo
-  , csvSelfIssued  :: SelfIssued
   , csvIssuer      :: SL.VKey 'SL.BlockIssuer c
   , csvIssueNo     :: Word64
   , csvLeaderVRF   :: VRF.OutputVRF (VRF c)
-  } deriving (Show, Eq)
+  } deriving (Show, Eq, Generic, NoThunks)
 
 instance SL.PraosCrypto c => Ord (TPraosChainSelectView c) where
   compare =
       mconcat [
           compare `on` csvChainLength
-        , whenSame csvSlotNo (compare `on` csvSelfIssued)
         , whenSame csvIssuer (compare `on` csvIssueNo)
         , compare `on` Down . csvLeaderVRF
         ]

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Config.hs

@@ -47,13 +47,9 @@ data instance BlockConfig (ShelleyBlock era) = ShelleyConfig {
       shelleyProtocolVersion  :: !SL.ProtVer
     , shelleySystemStart      :: !SystemStart
     , shelleyNetworkMagic     :: !NetworkMagic
-      -- | When chain selection is comparing two fragments, it will prefer the
-      -- fragment with a tip signed by (one of) its own key(s) (provided that
-      -- the 'BlockNo's and 'SlotNo's of the two tips are equal). For nodes that
-      -- can produce blocks, this should be set to the verification key(s)
-      -- corresponding to the node's signing key(s), to make sure we prefer
-      -- self-issued blocks. For non block producing nodes, this can be set to
-      -- the empty map.
+      -- | For nodes that can produce blocks, this should be set to the
+      -- verification key(s) corresponding to the node's signing key(s). For non
+      -- block producing nodes, this can be set to the empty map.
     , shelleyBlockIssuerVKeys :: !(Map (SL.KeyHash 'SL.BlockIssuer (EraCrypto era))
                                        (SL.VKey 'SL.BlockIssuer (EraCrypto era)))
     }

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/TPraos.hs

@@ -9,23 +9,19 @@
 
 module Ouroboros.Consensus.Shelley.Ledger.TPraos () where
 
-import           Data.Map.Strict (Map)
-import qualified Data.Map.Strict as Map
-
 import           Cardano.Crypto.VRF (certifiedOutput)
 
 import           Ouroboros.Consensus.Block
 import           Ouroboros.Consensus.Protocol.Signed
 
-import qualified Cardano.Ledger.Shelley.API as SL
 import qualified Cardano.Protocol.TPraos.BHeader as SL
 import qualified Cardano.Protocol.TPraos.OCert as SL
 
 import qualified Cardano.Protocol.TPraos.API as SL
 import           Ouroboros.Consensus.Protocol.TPraos
 import           Ouroboros.Consensus.Shelley.Eras (EraCrypto)
 import           Ouroboros.Consensus.Shelley.Ledger.Block
-import           Ouroboros.Consensus.Shelley.Ledger.Config
+import           Ouroboros.Consensus.Shelley.Ledger.Config ()
 
 {-------------------------------------------------------------------------------
   Support for Transitional Praos consensus algorithm
@@ -37,10 +33,9 @@ instance (SL.PraosCrypto (EraCrypto era), ShelleyBasedEra era)
   => BlockSupportsProtocol (ShelleyBlock era) where
   validateView _cfg (ShelleyHeader hdr _) = hdr
 
-  selectView cfg hdr@(ShelleyHeader shdr _) = TPraosChainSelectView {
+  selectView _ hdr@(ShelleyHeader shdr _) = TPraosChainSelectView {
         csvChainLength = blockNo hdr
       , csvSlotNo      = blockSlot hdr
-      , csvSelfIssued  = selfIssued
       , csvIssuer      = SL.bheaderVk hdrBody
       , csvIssueNo     = SL.ocertN . SL.bheaderOCert $ hdrBody
       , csvLeaderVRF   = certifiedOutput . SL.bheaderL $ hdrBody
@@ -49,47 +44,6 @@ instance (SL.PraosCrypto (EraCrypto era), ShelleyBasedEra era)
       hdrBody :: SL.BHBody (EraCrypto era)
       hdrBody = SL.bhbody shdr
 
-      issuerVKeys :: Map (SL.KeyHash 'SL.BlockIssuer (EraCrypto era))
-                         (SL.VKey 'SL.BlockIssuer (EraCrypto era))
-      issuerVKeys = shelleyBlockIssuerVKeys cfg
-
-      -- | Premature optimisation: we assume everywhere that 'selectView' is
-      -- cheap, so micro-optimise checking whether the issuer vkey is one of our
-      -- own vkeys.
-      --
-      -- * Equality of vkeys takes roughly 40ns
-      -- * Hashing a vkey takes roughly 850ns
-      -- * Equality of hashes takes roughly 10ns
-      --
-      -- We want to avoid the hashing of a vkey as it is more expensive than
-      -- simply doing a linear search, comparing vkeys for equality. Only when
-      -- we have to do a linear search across a large number of vkeys does it
-      -- become more efficient to first hash the vkey and look up its hash in
-      -- the map.
-      --
-      -- We could try to be clever and estimate the number of keys after which
-      -- we switch from a linear search to hashing + a O(log n) map lookup, but
-      -- we keep it (relatively) simple and optimise for the common case: 0 or 1
-      -- key.
-      selfIssued :: SelfIssued
-      selfIssued = case Map.size issuerVKeys of
-          -- The most common case: a non-block producing node
-          0 -> NotSelfIssued
-          -- A block producing node with a single set of credentials: just do an
-          -- equality check of the single VKey, skipping the more expensive
-          -- computation of the hash.
-          1 | SL.bheaderVk hdrBody `elem` issuerVKeys
-            -> SelfIssued
-            | otherwise
-            -> NotSelfIssued
-          -- When we are running with multiple sets of credentials, which should
-          -- only happen when benchmarking, do a hash lookup, as the number of
-          -- keys can grow to 100-250.
-          _ | SL.hashKey (SL.bheaderVk hdrBody) `Map.member` issuerVKeys
-            -> SelfIssued
-            | otherwise
-            -> NotSelfIssued
-
 -- TODO correct place for these two?
 type instance Signed (Header (ShelleyBlock era)) = SL.BHBody (EraCrypto era)
 

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Node.hs

@@ -79,6 +79,7 @@ import qualified Cardano.Ledger.Shelley.LedgerState as SL
                      (incrementalStakeDistr, updateStakeDistribution)
 import           Cardano.Ledger.Val (coin, inject, (<->))
 import qualified Cardano.Protocol.TPraos.API as SL
+import qualified Cardano.Protocol.TPraos.BHeader as SL
 import qualified Cardano.Protocol.TPraos.OCert as Absolute (KESPeriod (..))
 import qualified Cardano.Protocol.TPraos.OCert as SL
 
@@ -435,10 +436,49 @@ instance ShelleyBasedEra era => NodeInitStorage (ShelleyBlock era) where
 -------------------------------------------------------------------------------}
 
 instance ShelleyBasedEra era => BlockSupportsMetrics (ShelleyBlock era) where
-  isSelfIssued cfg hdr =
-       case csvSelfIssued $ selectView cfg hdr of
-         SelfIssued    -> IsSelfIssued
-         NotSelfIssued -> IsNotSelfIssued
+  -- | Premature optimisation: we assume everywhere that metrics are
+  -- cheap, so micro-optimise checking whether the issuer vkey is one of our
+  -- own vkeys.
+  --
+  -- * Equality of vkeys takes roughly 40ns
+  -- * Hashing a vkey takes roughly 850ns
+  -- * Equality of hashes takes roughly 10ns
+  --
+  -- We want to avoid the hashing of a vkey as it is more expensive than
+  -- simply doing a linear search, comparing vkeys for equality. Only when
+  -- we have to do a linear search across a large number of vkeys does it
+  -- become more efficient to first hash the vkey and look up its hash in
+  -- the map.
+  --
+  -- We could try to be clever and estimate the number of keys after which
+  -- we switch from a linear search to hashing + a O(log n) map lookup, but
+  -- we keep it (relatively) simple and optimise for the common case: 0 or 1
+  -- key.
+  isSelfIssued cfg (ShelleyHeader shdr _) = case Map.size issuerVKeys of
+      -- The most common case: a non-block producing node
+      0 -> IsNotSelfIssued
+      -- A block producing node with a single set of credentials: just do an
+      -- equality check of the single VKey, skipping the more expensive
+      -- computation of the hash.
+      1 | SL.bheaderVk hdrBody `elem` issuerVKeys
+        -> IsSelfIssued
+        | otherwise
+        -> IsNotSelfIssued
+      -- When we are running with multiple sets of credentials, which should
+      -- only happen when benchmarking, do a hash lookup, as the number of
+      -- keys can grow to 100-250.
+      _ | SL.hashKey (SL.bheaderVk hdrBody) `Map.member` issuerVKeys
+        -> IsSelfIssued
+        | otherwise
+        -> IsNotSelfIssued
+    where
+      hdrBody :: SL.BHBody (EraCrypto era)
+      hdrBody = SL.bhbody shdr
+
+      issuerVKeys :: Map (SL.KeyHash 'SL.BlockIssuer (EraCrypto era))
+                         (SL.VKey 'SL.BlockIssuer (EraCrypto era))
+      issuerVKeys = shelleyBlockIssuerVKeys cfg
+
 
 instance ShelleyBasedEra era => RunNode (ShelleyBlock era)
 

ouroboros-consensus-test/src/Test/ThreadNet/General.hs

@@ -57,6 +57,7 @@ import           Ouroboros.Consensus.Node.Run
 import           Ouroboros.Consensus.NodeId
 import           Ouroboros.Consensus.Protocol.Abstract (LedgerView)
 import           Ouroboros.Consensus.Protocol.LeaderSchedule
+import qualified Ouroboros.Consensus.Storage.ChainDB as ChainDB
 import           Ouroboros.Consensus.TypeFamilyWrappers
 
 import           Ouroboros.Consensus.Util.Condense
@@ -478,6 +479,7 @@ prop_general_internal syncity pga testOutput =
     prop_no_BlockRejections .&&.
     prop_no_unexpected_CannotForges .&&.
     prop_no_invalid_blocks .&&.
+    prop_pipelining .&&.
     propSync
       ( prop_all_common_prefix maxForkLength (Map.elems nodeChains) .&&.
         prop_all_growth .&&.
@@ -854,6 +856,39 @@ prop_general_internal syncity pga testOutput =
         , (s, blk) <- Map.toAscList nodeOutputForges
         ]
 
+    -- Check that all self-issued blocks are pipelined.
+    prop_pipelining :: Property
+    prop_pipelining = conjoin
+        [ counterexample ("Node " <> condense nid <> " did not pipeline") $
+          counterexample ("some of its blocks forged as the sole slot leader:") $
+          counterexample (condense forgedButNotPipelined) $
+          Set.null forgedButNotPipelined
+        | (nid, NodeOutput
+            { nodeOutputForges
+            , nodePipeliningEvents
+            }) <- Map.toList testOutputNodes
+        , CoreId cnid <- [nid]
+        , let tentativePoints = Set.fromList
+                [ headerPoint hdr
+                | ChainDB.SetTentativeHeader hdr <- nodePipeliningEvents
+                ]
+              forgedAsSoleLeaderPoints = Set.fromList $
+                [ blockPoint blk
+                | blk <- Map.elems nodeOutputForges
+                , let s = blockSlot blk
+                      NodeRestarts nrs = nodeRestarts
+                , getLeaderSchedule actualLeaderSchedule Map.! s == [cnid]
+                  -- When the node is restarted while it is a slot
+                  -- leader, this property is often not satisfied in
+                  -- the Byron ThreadNet tests. As diffusion
+                  -- pipelining is concerned with up-to-date,
+                  -- long-running nodes, we ignore this edge case.
+                , cnid `Map.notMember` Map.findWithDefault mempty s nrs
+                ]
+              forgedButNotPipelined =
+                forgedAsSoleLeaderPoints Set.\\ tentativePoints
+        ]
+
 {-------------------------------------------------------------------------------
   Final chains properties
 -------------------------------------------------------------------------------}