Skip to content

Commit

Permalink
ChainDB.Iterator: fix EBB bug
Browse files Browse the repository at this point in the history 
Closes #1435.

See the comment for more information.
  • Loading branch information
@mrBliss
mrBliss committed Jan 17, 2020
1 parent 4096cd6 commit e2dd054
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 14 deletions.
46 changes: 36 additions & 10 deletions ouroboros-consensus/src/Ouroboros/Storage/ChainDB/Impl/Iterator.hs
View file
Expand Up @@ -18,8 +18,8 @@ module Ouroboros.Storage.ChainDB.Impl.Iterator
) where

import Control.Monad (unless, when)
import Control.Monad.Except (ExceptT (..), lift, runExceptT,
throwError)
import Control.Monad.Except (ExceptT (..), catchError, lift,
runExceptT, throwError)
import Data.Functor (($>))
import Data.List.NonEmpty (NonEmpty)
import qualified Data.List.NonEmpty as NE
Expand All @@ -38,6 +38,7 @@ import Ouroboros.Network.Block (pattern BlockPoint,
SlotNo, StandardHash, atSlot, withHash)
import Ouroboros.Network.Point (WithOrigin (..))

import Ouroboros.Consensus.Block (IsEBB (..))
import Ouroboros.Consensus.Util.IOLike
import Ouroboros.Consensus.Util.ResourceRegistry (ResourceRegistry)

Expand Down Expand Up @@ -257,24 +258,49 @@ newIterator itEnv@IteratorEnv{..} getItEnv registry from to = do
=> ExceptT (UnknownRange blk) m (DeserialisableIterator m blk)
start = lift (ImmDB.getTipInfo itImmDB) >>= \case
Origin -> findPathInVolDB
At (tipSlot, tipHash, _tipIsEBB) ->
At (tipSlot, tipHash, tipIsEBB) ->
case atSlot endPoint `compare` tipSlot of
-- The end point is < the tip of the ImmutableDB
LT -> streamFromImmDB
EQ | withHash endPoint == tipHash
-- The end point == the tip of the ImmutableDB
-> streamFromImmDB
| otherwise
-- The end point /= the tip of the ImmutableDB. Either the
-- fork is too old, or the end point (or the tip of the
-- ImmutableDB) is an EBB. For example, the tip of the
-- ImmutableDB is an EBB and the end point is the regular
-- block after it.

-- The end point /= the tip of the ImmutableDB.
--
-- There are three possibilities:
--
-- 1. The end point is the regular block (or a different EBB?)
-- with the same slot number as the EBB which is currently at
-- the tip of the ImmutableDB. In this case, we must look in
-- the VolatileDB for the block, as we know it is not in the
-- ImmutableDB.
--
-- 2. The end point is the EBB with the same slot number as the
-- regular block which is currently at the tip of the
-- ImmutableDB. In this case, we can stream from just the
-- ImmutableDB, as the end point is in the ImmutableDB.
--
-- 3. The fork is too old, i.e., the end point refers to a block
-- (or EBB) not on the current chain, but with the same slot
-- number as our tip.
--
-- We don't know upfront with which of the three cases we're
-- dealing, as we don't know which block the end point refers to
-- without trying to read it from the ImmutableDB or VolatileDB.
-- The important thing is to also try the ImmutableDB in case 2.
| IsEBB <- tipIsEBB -- Case 1 or 3
-> findPathInVolDB
| otherwise -- Case 2 or 3
-> streamFromImmDB `fallbackTo` findPathInVolDB
-- The end point is > the tip of the ImmutableDB
GT -> findPathInVolDB

-- | PRECONDITION: the upper bound > the tip of the ImmutableDB
fallbackTo :: ExceptT e m a -> ExceptT e m a -> ExceptT e m a
fallbackTo a b = a `catchError` const b

-- | PRECONDITION: the upper bound >= the tip of the ImmutableDB.
-- Greater or /equal/, because of EBBs :(
findPathInVolDB :: HasCallStack
=> ExceptT (UnknownRange blk) m (DeserialisableIterator m blk)
findPathInVolDB = do
Expand Down
18 changes: 18 additions & 0 deletions ouroboros-consensus/test-consensus/Test/ThreadNet/RealPBFT.hs
View file
Expand Up @@ -146,6 +146,24 @@ tests = testGroup "RealPBFT" $
, slotLengths = defaultSlotLengths
, initSeed = Seed {getSeed = (17927476716858194849,11935807562313832971,15925564353519845641,3835030747036900598,2802397826914039548)}
}
, testProperty "BlockFetch live lock due to an EBB at the ImmutableDB tip, Issue #1435" $
once $
let ncn = NumCoreNodes 4 in
-- c0's ImmDB is T > U > V. Note that U is an EBB and U and V are
-- both in slot 50. When its BlockFetchServer tries to stream T and
-- U using a ChainDB.Iterator, instead of looking in the
-- ImmutableDB, we end up looking in the VolatileDB and incorrectly
-- return ForkTooOld. The client keeps on requesting this block
-- range, resulting in a live lock.
prop_simple_real_pbft_convergence ProduceEBBs (SecurityParam 5) TestConfig
{ numCoreNodes = ncn
, numSlots = NumSlots 58
, nodeJoinPlan = NodeJoinPlan $ Map.fromList [(CoreNodeId 0,SlotNo 3),(CoreNodeId 1,SlotNo 3),(CoreNodeId 2,SlotNo 5),(CoreNodeId 3,SlotNo 57)]
, nodeRestarts = noRestarts
, nodeTopology = meshNodeTopology ncn
, slotLengths = defaultSlotLengths
, initSeed = Seed (11044330969750026700,14522662956180538128,9026549867550077426,3049168255170604478,643621447671665184)
}
, -- RealPBFT runs are slow, so do 10x less of this narrow test
adjustOption (\(QuickCheckTests i) -> QuickCheckTests $ max 1 $ i `div` 10) $
testProperty "re-delegation via NodeRekey" $ \seed w ->
Expand Down
91 changes: 87 additions & 4 deletions ouroboros-consensus/test-storage/Test/Ouroboros/Storage/ChainDB/Iterator.hs
View file
Expand Up @@ -33,9 +33,8 @@ import Ouroboros.Consensus.Util.ResourceRegistry

import Ouroboros.Storage.ChainDB.API (Iterator (..), IteratorId (..),
IteratorResult (..), StreamFrom (..), StreamTo (..),
UnknownRange, deserialiseIterator)
import Ouroboros.Storage.ChainDB.Impl.ImmDB (ImmDB, getPointAtTip,
mkImmDB)
UnknownRange (..), deserialiseIterator)
import Ouroboros.Storage.ChainDB.Impl.ImmDB (ImmDB, mkImmDB)
import Ouroboros.Storage.ChainDB.Impl.Iterator (IteratorEnv (..),
newIterator)
import Ouroboros.Storage.ChainDB.Impl.Types (TraceIteratorEvent (..))
Expand All @@ -61,6 +60,10 @@ tests :: TestTree
tests = testGroup "Iterator"
[ testProperty "#773 bug in example 1" prop_773_bug
, testProperty "#773 correct example 2" prop_773_working
, testProperty "#1445 bug" prop_1435_bug
, testProperty "#1445 other case 1" prop_1435_other_case1
, testProperty "#1445 other case 2" prop_1435_other_case2
, testProperty "#1445 other case 3" prop_1435_other_case3
]

-- These tests focus on the implementation of the ChainDB iterators, which are
Expand Down Expand Up @@ -142,6 +145,87 @@ prop_773_working = prop_general_test
(StreamToInclusive (blockPoint e))
(Right (map Right [a, b, c, d, e]))

-- | Requested stream = U -> U where U is an EBB and V is a regular block in
-- the same slot.
--
-- ImmDB VolDB
-- Hash U -> V
--
prop_1435_bug :: Property
prop_1435_bug = prop_general_test
TestSetup
{ immutable = Chain.fromOldestFirst [u, v]
, volatile = []
}
(StreamFromInclusive (blockPoint u))
(StreamToInclusive (blockPoint u))
(Right (map Right [u]))
where
u = firstEBB TestBody { tbForkNo = 0, tbIsValid = True }
v = mkNextBlock u 0 TestBody { tbForkNo = 0, tbIsValid = True }

-- | Requested stream = V -> V where U is an EBB and V is a regular block in
-- the same slot.
--
-- ImmDB VolDB
-- Hash U -> V
--
prop_1435_other_case1 :: Property
prop_1435_other_case1 = prop_general_test
TestSetup
{ immutable = Chain.fromOldestFirst [u, v]
, volatile = []
}
(StreamFromInclusive (blockPoint v))
(StreamToInclusive (blockPoint v))
(Right (map Right [v]))
where
u = firstEBB TestBody { tbForkNo = 0, tbIsValid = True }
v = mkNextBlock u 0 TestBody { tbForkNo = 0, tbIsValid = True }

-- | Requested stream = Z -> Z where U is an EBB, V is a regular block in
-- the same slot, and Z has the same slot as U and V, but doesn't exist in
-- either DB.
--
-- ImmDB VolDB
-- Hash U -> V
--
prop_1435_other_case2 :: Property
prop_1435_other_case2 = prop_general_test
TestSetup
{ immutable = Chain.fromOldestFirst [u, v]
, volatile = []
}
(StreamFromInclusive (blockPoint z))
(StreamToInclusive (blockPoint z))
(Left (ForkTooOld (StreamFromInclusive (blockPoint z))))
where
u = firstEBB TestBody { tbForkNo = 0, tbIsValid = True }
v = mkNextBlock u 0 TestBody { tbForkNo = 0, tbIsValid = True }
z = mkNextBlock u 0 TestBody { tbForkNo = 1, tbIsValid = True }

-- | Requested stream = Z -> Z where U is an EBB, V is a regular block in
-- the same slot, and Z has the same slot as U and V, but doesn't exist in
-- either DB.
--
-- ImmDB VolDB
-- Hash U V
--
prop_1435_other_case3 :: Property
prop_1435_other_case3 = prop_general_test
TestSetup
{ immutable = Chain.fromOldestFirst [u]
, volatile = [v]
}
(StreamFromInclusive (blockPoint z))
(StreamToInclusive (blockPoint z))
(Left (ForkTooOld (StreamFromInclusive (blockPoint z))))
where
u = firstEBB TestBody { tbForkNo = 0, tbIsValid = True }
v = mkNextBlock u 0 TestBody { tbForkNo = 0, tbIsValid = True }
z = mkNextBlock u 0 TestBody { tbForkNo = 1, tbIsValid = True }


-- | The general property test
prop_general_test
:: TestSetup
Expand Down Expand Up @@ -256,7 +340,6 @@ initIteratorEnv TestSetup { immutable, volatile } tracer = do
return IteratorEnv
{ itImmDB = immDB
, itVolDB = volDB
, itGetImmDBTip = getPointAtTip immDB
, itIterators = iters
, itNextIteratorId = nextIterId
, itTracer = tracer
Expand Down

0 comments on commit e2dd054

Please sign in to comment.