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

Fix GetIsValid bugs (once and for all?) #279

Closed
wants to merge 2 commits into from
Closed

Fix GetIsValid bugs (once and for all?) #279

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8633b90
ChainDB QSM: don't use the currently selected chain as a candidate.
jorisdral Aug 4, 2023
d3c9999
ChainDB QSM: always record valid blocks.
jorisdral Aug 7, 2023
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
113 changes: 7 additions & 106 deletions ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,11 @@ addBlock cfg blk m = Model {
immutableChainHashes `isPrefixOf`
map blockHash (Chain.toOldestFirst fork)

consideredCandidates = filter (extendsImmutableChain . fst) candidates
-- Considered candidates are candidates that extend the immutable part of
-- the chain. Furthermore, the current selected chain is not considered as a
-- candidate.
Comment on lines +427 to +429
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could say that sortChains takes the current chain separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we add your suggestion in #279 (comment), then the comment can be reverted to what it previously was.

Comment on lines +427 to +429
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- Considered candidates are candidates that extend the immutable part of
-- the chain. Furthermore, the current selected chain is not considered as a
-- candidate.
-- Considered candidates are candidates that extend the immutable part of
-- the chain, __except for__ the current selected chain.

consideredCandidates = filter ((/= CPS.chainState (cps m)) . fst)
Copy link
Member

Choose a reason for hiding this comment

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

With the other changes, is this now actually necessary to make any tests pass? Again, motivation is that this would be a difference to #284.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I don't think it's necessary. If the current chain is the best candidate, then chain selection is idempotent (not sure if it's the right word). Including the current chain if it isn't the best candidate shouldn't have any effect on the outcome.

Copy link
Member

Choose a reason for hiding this comment

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

If we use consideredCandidates for valid' as in https://github.com/input-output-hk/ouroboros-consensus/pull/279/files#r1291126148, that would also motivate not filtering out the current chain.

$ filter (extendsImmutableChain . fst) candidates

newChain :: Chain blk
newLedger :: ExtLedgerState blk
Expand All @@ -436,109 +440,10 @@ addBlock cfg blk m = Model {
(currentChain m)
$ consideredCandidates

-- See note [Getting the valid blocks] just below
valid' = foldl
(Chain.foldChain (\s b -> Set.insert (blockHash b) s))
(valid m)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this to extend the previous valid m? The tests seem to pass even with

Suggested change
(valid m)
Set.empty

Motivation is that this would be a difference to #284.

Copy link
Contributor Author

@jorisdral jorisdral Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm hesitant towards this, because the model could "forget" valid blocks if the blocks are on a chain that isn't connected to the Genesis block anymore. That was the motivation for IntersectMBO/ouroboros-network#3651 (comment).

Assume we use Set.empty instead. Test.O.C.ChainDB.StateMachine mentions the following about garbage collection:

https://github.com/input-output-hk/ouroboros-consensus/blob/f658b1986cd0035ffad84e04cda52c905cba8b56/ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs#L466-L474

Garbage collection on the model is instantaneous, while garbage collection on the SUT is not. The real VolatileDB knows about the validity of all blocks in it, even if those blocks are disconnected.

https://github.com/input-output-hk/ouroboros-consensus/blob/f658b1986cd0035ffad84e04cda52c905cba8b56/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/API.hs#L209-L211

If we ask the SUT if a disconnected block is valid, and it returns Just True, then the model will return Nothing (the model doesn't know), because the model only collected valid, connected blocks, and this results in a test failure. By extending valid m, we can be reasonably sure that valid' is a non-strict superset of the set of valid blocks that the SUT knows about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm now also unsure whether we should have the Map.member hash (volatileDbBlocks m) check in Model.getIsValid, because it would suffer from the same non-determinism

Copy link
Member

Choose a reason for hiding this comment

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

Very good point, I went back to 90b3bdf, where I can reproduce IntersectMBO/ouroboros-network#3389. Now it turns out that c8a6186 (what I originally thought would be enough as the entire fix, ie that was the fifth commit in #284 before I force-pushed it) does not fix the bug.

Instead, maintaining the set of valid blocks cumulatively does fix the bug, I pushed that know to #284. Note that it does not restrict the model getIsValid to just blocks in the VolDB.

I'm now also unsure whether we should have the Map.member hash (volatileDbBlocks m) check in Model.getIsValid, because it would suffer from the same non-determinism

So I think we could indeed remove this.

(takeWhile (not . Chain.isPrefixOf newChain)
(map fst consideredCandidates) ++ [newChain])

-- = Getting the valid blocks
--
-- The chain selection algorithms implemented by the model and by the SUT differ
-- but have the same outcome.We illustrate this with an example. Imagine having
-- the following candidate chains where @v@ represents a valid block and @x@
-- represents an invalid block:
--
-- > C0: vvvvvxxxxx
-- > C1: vvvvvvvx
-- > C2: vvv
--
-- For candidate Cx, we will call CxV the valid prefix and CxI the invalid suffix.
--
-- The chain selection algorithm will run whenever we add a block, although it
-- will only select a new chain when adding a block results in a chain that is
-- longer than the currently selected chain. Note that the chain selection
-- algorithm doesn't know beforehand the validity of the blocks in the
-- candidates. The process it follows will be:
--
-- 1. Sort the chains by 'SelectView'. Note that for Praos this will trivially
-- imply first consider the candidates by length.
--
-- > sortedCandidates == [C0, C1, C2]
--
-- 2. Until a candidate is found to be valid and longer than the currently selected
-- chain, take the head of the (sorted) list of candidates and validate the
-- blocks in it one by one.
--
-- If a block in the candidate is found to be invalid, the candidate is
-- truncated, added back to the list, and the algorithm starts again at step 1.
-- The valid blocks in the candidate are recorded in the set of known-valid
-- blocks, so that the next time they are applied, it is known that applying
-- said block can't fail and therefore some checks can be skipped. The invalid
-- blocks in the candidate are recorded in the set of known-invalid blocks so
-- that they are not applied again.
--
-- The steps on the example are as follows:
--
-- 1. Start with the sorted candidate chains: [C0, C1, C2]
-- 2. Validate first chain C0 resulting in C0V and C0I.
-- 3. Append C0V to the list of remaining candidates: [C1, C2] ++ [C0V]
-- 4. Add the valid blocks to the state:
-- > knownValid = append C0V knownValid
-- 5. Add the invalid blocks to the state:
-- > knownInvalid = append C0I knownInvalid
-- 6. Re-sort list
-- > sortBy `selectView` [C1, C2, C0V] == [C1, C0V, C2]
-- 7. Validate first chain C1 resulting in C1V and C1I.
-- 8. Append C1V to the list of remaining candidates: [C0V, C2] ++ [C1V]
-- 9. Add the valid blocks to the state:
-- > knownValid = append C1V knownValid
-- 10. Add the invalid blocks to the state:
-- > knownInvalid = append C1I knownInvalid
-- 11. Re-sort list
-- > sortBy `selectView` [C0V, C2, C1V] == [C1V, C0V, C2]
-- 12. Validate first chain C1V, which is fully valid and returned.
--
-- 3. If such a candidate is found, the algorithm will return it as a result.
-- Otherwise, the algorithm will return a 'Nothing'.
--
-- > chainSelection [C0, C1, C2] = Just C1V
--
-- On the other hand, the chain selection on the model takes some shortcuts to
-- achieve the same result:
--
-- 1. 'validChains' will return the list of candidates sorted by 'SelectView' and
-- each candidate is truncated to its valid prefix.
--
-- > validChains [C0, C1, C2] = (invalid == C0I + C1I, candidates == [C0V, C1V, C2])
--
-- 2. 'selectChain' will sort the chains by 'SelectView' but note that now it will
-- use the 'SelectView' of the already truncated candidate.
--
-- > selectChain [C0V, C1V, C2] = listToMaybe (sortBy `selectView` [C0V, C1V, C2])
-- > = listToMaybe ([C1V, C0V, C2])
-- > = Just C1V
--
-- The selected candidate will be the same one that the chain selection
-- algorithm would choose. However, as the chain selection algorithm will
-- consider the candidates as they were sorted by 'SelectView' on the
-- non-truncated candidates, blocks in 'C0V' are also considered valid by the
-- real algorithm.
--
-- To get as a result a set of valid blocks that mirrors the one from the
-- real algorithm, the model can process the list of candidates returned by
-- 'validChains' until it find the one 'selectChain' chose as these will be
-- the ones that the real algorithm would test and re-add to the list once
-- truncated.
--
-- > knownInvalid = append (C0I + C1I) knownInvalid
-- > knownValid = foldl append knownValid (takeWhile (/= C1V) candidates ++ [C1V])
--
-- Note that the set of known valid blocks is equivalent to the set computed
-- by real algorithm, but the set of known invalid blocks is a superset of
-- the ones known by the real algorithm. See the note
-- Ouroboros.Storage.ChainDB.StateMachine.[Invalid blocks].
(map fst candidates)
Copy link
Member

Choose a reason for hiding this comment

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

As valid m is cumulative, shouldn't it always be enough to use consideredCandidates here? Both the model and the SUT will only ever validate blocks that are on a chain that extends the immutable chain.

I am asking this both as this is a difference to #284, and as consideredChains is currently only used as an input to selectChain, but not to build valid'.


addBlocks :: (Eq blk, LedgerSupportsProtocol blk)
=> TopLevelConfig blk
Expand Down Expand Up @@ -873,12 +778,8 @@ validChains cfg m bs =
-- first, which results in the valid chain A->B', which is then chosen
-- over the equally preferable A->B as it will be the first in the list
-- after a stable sort.
sortChains $ pruneKnownInvalid <$> chains bs
sortChains $ chains bs
Copy link
Member

Choose a reason for hiding this comment

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

Is the comment above still accurate? Wouldn't the reader be puzzled about why we don't filter the invalid chains first as in the SUT?

where
pruneKnownInvalid chain =
-- We don't want known invalid blocks to influence the sorting of candidate chains.
Chain.takeWhile (\b -> blockHash b `Map.notMember` (invalid m)) chain

sortChains :: [Chain blk] -> [Chain blk]
sortChains =
sortBy $ flip (
Expand Down
Loading