Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Relax overly strict disconnection rule for known-invalid blocks #3726

Merged
merged 3 commits into from
May 3, 2022
Merged

Relax overly strict disconnection rule for known-invalid blocks #3726

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
78f35a2
Consolidate checks whether pipelining is enabled
amesgen Apr 28, 2022
a51e003
Relax overly strict disconnection rule for known-invalid blocks
amesgen Apr 28, 2022
2c1fcd1
When pipelining a block, check that its predecessor is not known-invalid
amesgen Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions ouroboros-consensus/src/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import qualified Ouroboros.Network.AnchoredFragment as AF
import qualified Ouroboros.Network.AnchoredSeq as AS
import Ouroboros.Network.Block (Tip, getTipBlockNo)
import Ouroboros.Network.Mux (ControlMessage (..), ControlMessageSTM)
import Ouroboros.Network.NodeToNode.Version (isPipeliningEnabled)
import Ouroboros.Network.PeerSelection.PeerMetric.Type
(HeaderMetricsTracer)
import Ouroboros.Network.Protocol.ChainSync.ClientPipelined
Expand Down Expand Up @@ -440,7 +441,7 @@ chainSyncClient mkPipelineDecision0 tracer cfg
, getPastLedger
, getIsInvalidBlock
}
_version
version
controlMessageSTM
headerMetricsTracer
varCandidate = ChainSyncClientPipelined $
Expand Down Expand Up @@ -725,12 +726,13 @@ chainSyncClient mkPipelineDecision0 tracer cfg
rollForward mkPipelineDecision n hdr theirTip
= Stateful $ \kis -> traceException $ do
now <- getMonotonicTime
-- Reject the block if invalid
let hdrHash = headerHash hdr
hdrPoint = headerPoint hdr
isInvalidBlock <- atomically $ forgetFingerprint <$> getIsInvalidBlock
whenJust (isInvalidBlock hdrHash) $ \reason ->
disconnect $ InvalidBlock hdrPoint reason
unless (isPipeliningEnabled version) $ do
-- Reject the block if invalid
isInvalidBlock <- atomically $ forgetFingerprint <$> getIsInvalidBlock
whenJust (isInvalidBlock hdrHash) $ \reason ->
disconnect $ InvalidBlock hdrPoint reason
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still check for extending invalid chains though right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that happens here, in a background thread:

https://github.com/input-output-hk/ouroboros-network/blob/6d11459940a5d9a06a0109b75e56bc37a03cf940/ouroboros-consensus/src/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs#L1013-L1017

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes ok, makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think it's reliable to use the Watcher for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: While we don't think it is strictly necessary, I will go ahead and add a check that at this place in the code that ensures that the parent hash is not known-invalid.


-- Get the ledger view required to validate the header
-- NOTE: This will block if we are too far behind.
Expand Down Expand Up @@ -1012,7 +1014,7 @@ invalidBlockRejector tracer version getIsInvalidBlock getCandidate =
-- it's explicit, only skip it if it's annotated as tentative
mapM_ (uncurry disconnect) $ firstJust
(\hdr -> (hdr,) <$> isInvalidBlock (headerHash hdr))
( (if enablePipelining then drop 1 else id)
( (if isPipeliningEnabled version then drop 1 else id)
$ AF.toNewestFirst theirFrag
)

Expand All @@ -1022,9 +1024,6 @@ invalidBlockRejector tracer version getIsInvalidBlock getCandidate =
traceWith tracer $ TraceException ex
throwIO ex

enablePipelining :: Bool
enablePipelining = version >= NodeToNodeV_8

-- | Auxiliary data type used as an intermediary result in 'rollForward'.
data IntersectCheck blk =
-- | The upstream chain no longer intersects with our current chain because
Expand Down
2 changes: 1 addition & 1 deletion ouroboros-consensus/src/Ouroboros/Consensus/Network/NodeToNode.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ mkApps kernel Tracers {..} mkCodecs ByteLimits {..} genChainSyncTimeout ReportPe
bracketWithPrivateRegistry
(chainSyncHeaderServerFollower
(getChainDB kernel)
( if version >= NodeToNodeV_8
( if isPipeliningEnabled version
then ChainDB.TentativeChain
else ChainDB.SelectedChain
)
Expand Down
7 changes: 4 additions & 3 deletions ouroboros-network/src/Ouroboros/Network/BlockFetch/ClientRegistry.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ import Control.Tracer (Tracer)

import Ouroboros.Network.BlockFetch.ClientState
import Ouroboros.Network.DeltaQ
import Ouroboros.Network.NodeToNode.Version (NodeToNodeVersion (..))
import Ouroboros.Network.NodeToNode.Version (NodeToNodeVersion (..),
isPipeliningEnabled)



Expand Down Expand Up @@ -100,8 +101,8 @@ bracketFetchClient (FetchClientRegistry ctxVar
-- allocate the policy specific for this peer's negotiated version
policy <- do
let pipeliningEnabled
| version >= NodeToNodeV_8 = ReceivingTentativeBlocks
| otherwise = NotReceivingTentativeBlocks
| isPipeliningEnabled version = ReceivingTentativeBlocks
| otherwise = NotReceivingTentativeBlocks
mkPolicy pipeliningEnabled

stateVars <- newFetchClientStateVars
Expand Down
1 change: 1 addition & 0 deletions ouroboros-network/src/Ouroboros/Network/NodeToNode.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ module Ouroboros.Network.NodeToNode
, Handshake
, LocalAddresses (..)
, Socket
, isPipeliningEnabled
-- ** Error Policies and Peer state
, ErrorPolicies (..)
, remoteNetworkErrorPolicy
Expand Down
7 changes: 7 additions & 0 deletions ouroboros-network/src/Ouroboros/Network/NodeToNode/Version.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ module Ouroboros.Network.NodeToNode.Version
, ConnectionMode (..)
, nodeToNodeVersionCodec
, nodeToNodeCodecCBORTerm
-- * Feature checks
, isPipeliningEnabled
) where

import Data.Text (Text)
Expand Down Expand Up @@ -120,3 +122,8 @@ nodeToNodeCodecCBORTerm _version


data ConnectionMode = UnidirectionalMode | DuplexMode

-- | Check whether a version enabling diffusion pipelining has been
-- negotiated.
isPipeliningEnabled :: NodeToNodeVersion -> Bool
isPipeliningEnabled v = v >= NodeToNodeV_8