Add LoE to the ChainDB QSM test #1119

Niols · 2024-05-27T14:29:17Z

Closes #541

This PR adds a new instruction to the ChainDB q-s-m test, which updates the Limit on Eagerness (LoE) fragment and retriggers chain selection. This requires a model implementation of the LoE.

As a preparatory change, we add a way to trigger chain selection synchronously (ie such that we can wait for it to finish), which is only used in tests.

amesgen

The test failure in getChain_addChain makes sense: The test is making sure that for every chain bc, adding its blocks to an empty model causes the selection to be bc. With the LoE enabled and at Genesis (as is the case in the counterexamples), this of course fails when the chain is sufficiently long.

One way to fix this would be to discard these test cases, or even just disable the LoE completely for them.

The failure of alwaysPickPreferredChain seems to be for a very similar reason.

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/API.hs

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model/Test.hs

Niols · 2024-05-29T12:24:14Z

The test failure in getChain_addChain makes sense: The test is making sure that for every chain bc, adding its blocks to an empty model causes the selection to be bc. With the LoE enabled and at Genesis (as is the case in the counterexamples), this of course fails when the chain is sufficiently long.

One way to fix this would be to discard these test cases, or even just disable the LoE completely for them.

The failure of alwaysPickPreferredChain seems to be for a very similar reason.

Indeed, it was as simple as that. Fixed in 660b331. Could you check it, and in particular tell me if the comment of alwaysPickPreferredChain is correct? (The one for getChain_addChain is exactly what you wrote.)

Niols · 2024-05-29T13:38:50Z

Now rebased on top of #1125.

Niols · 2024-05-30T10:33:24Z

After 14 hours of tests, I find a counter-example to this PR. Reproducible on branch niols/loe-chaindb-qsm at commit 2da9483 with:

cabal run ouroboros-consensus:storage-test -- \
  -p '/ChainDB q-s-m/' \
  --quickcheck-tests 500_000 \
  --quickcheck-replay="(SMGen 16524770743761658109 16657631389868279761,58)"

Niols · 2024-05-30T10:59:34Z

Actually, upon inspecting this bug, it turns out that this failure is not LoE-related. The LoE is very much disabled and it looks like this has to do with GCing or streaming. I don't exactly know what this should do, but the behaviour of the model looks wrong to me.

Niols · 2024-05-30T13:09:06Z

Yup, I can reproduce with the same seed on genesis/milestone-13 and main with an additional:

+source-repository-package
+  type: git
+  location: https://github.com/UnkindPartition/tasty
+  tag: dcbf32078133aa2b569774c417cc49a49f5c573b
+  subdir: quickcheck
+  --sha256: 0rwfcv0iq50ckzdgdssgkil6g9n76w1k6yxs0q4fdrkf5nlh4vv5

in cabal.project. I don't understand why I manage to reproduce when the code of the test changed so much, but oh well.

Niols · 2024-05-31T18:02:40Z

As noted by @amesgen, this is probably #299.

amesgen

Really nice to finally get this

cabal.project

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model.hs

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model/Test.hs

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model.hs

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs

This PR improves the code documentation of Genesis, and implements some fixes and optimizations. ### Documentation, refactoring, and testing The code documentation of GDD, CSJ, and LoE is better connected in cb5b69a. Additionally, cd032b1 brings small changes to the LoE, motivated by issues found while integrating the LoE into the ChainDB q-s-m tests (see #1119). 2f7152b enables CSJ in all of the uniform tests and tests run more tries now. Shrinking of peer schedules has been simplified in 627634f, and 6b44bfb adds an extra field to the `PointSchedule` type to specify the time at the end of a test, allowing to clean up a lot of schedules. ### Profiling and optimizations We consider our baseline, the fastest possible syncing, to be the scenario in which the syncing peer connects to a caught up peer and syncs using Praos. From the profiling of Genesis on mainnet data, the GDD governor and chain selection was revealed as one of the major sinks of CPU time when enabling Genesis and syncing from multiple peers (between 2 and 30 peers). Chain selection has been made idempotent again, to save on useless recomputations in 82376e6. Idempotency had been lost in #1015. 64f3da3 also calls to change selection less often, by avoiding calls when the LoE fragment doesn't change in a significant way. The overhead of GDD is eliminated in a592c65 by replacing a frequent call to `AnchoredFragment.takeWhileOldest` with a faster variant. With these changes, the only significant overhead of syncing from multiple peers comes from the BlockFetch logic, which is the topic of ongoing work. ### CSJ tests The testing code has been adapted in a969b37 to allow to specify multiple honest peers in CSJ tests. CSJ tests are the first time we care of running multiple honest peers to check that header are downloaded from only one of them. We also added a test in 1d7fc95 to check that headers are downloaded from at most one peer even in the presence of adversarial peers. ### GSM integration In b525959 Limit on Eagerness is enabled only while GSM is in syncing state, and 83b12ee does similarly for LoP. f69fd1f makes possible to toggle genesis components with environment variables. This was helpful to measure the overheads of the individual components.

This is useful for tests. Co-authored-by: Nicolas “Niols” Jeannerod <nicolas.jeannerod@tweag.io>

dnadales

LGTM, but we should wait for the synchronous review during a video call.

dnadales · 2024-08-12T11:32:53Z

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model.hs

-    , maxClockSkew     = maxClockSkew m
-    , isOpen           = True
-    }
+addBlock cfg blk m =


Would a pattern match on ignoreBlock be more readable than an if?

I condensed this code a bit 👍

diff --git a/ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model.hs b/ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model.hs index 2c8f5c7eef..05f7cabe74 100644 --- a/ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model.hs +++ b/ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/Model.hs @@ -402,14 +402,10 @@ addBlock :: forall blk. LedgerSupportsProtocol blk -> blk -> Model blk -> Model blk addBlock cfg blk m = - if ignoreBlock - then m - else - chainSelection cfg $ - m - { volatileDbBlocks = - Map.insert (blockHash blk) blk (volatileDbBlocks m) - } + | ignoreBlock = m + | otherwise = chainSelection cfg m { + volatileDbBlocks = Map.insert (blockHash blk) blk (volatileDbBlocks m) + } where secParam = configSecurityParam cfg immBlockNo = immutableBlockNo secParam m

dnadales · 2024-08-12T11:40:18Z

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs

@@ -890,6 +909,10 @@ generator genBlock m@Model {..} = At <$> frequency
    , (if empty then 1 else 10, return GetMaxSlotNo)
    , (if empty then 1 else 10, genGetIsValid)

+    , (case loe of
+         LoEDisabled   -> (0, return $ UpdateLoE $ AF.Empty AF.AnchorGenesis)


Wouldn't setting the frequency to 0 be equivalent to not generating this case at all?

Yes, we just use 0 here for convenience as we are inside of a list literal here; a similar pattern is used in a few other places; I slightly refactored this to at least not use a bogus value on the right.

diff --git a/ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs b/ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs index 510ebac6df..184f018a4b 100644 --- a/ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs +++ b/ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs @@ -909,9 +909,10 @@ generator loe genBlock m@Model {..} = At <$> frequency , (if empty then 1 else 10, return GetMaxSlotNo) , (if empty then 1 else 10, genGetIsValid) - , (case loe of - LoEDisabled -> (0, return $ UpdateLoE $ AF.Empty AF.AnchorGenesis) - LoEEnabled () -> (if empty then 1 else 10, UpdateLoE <$> genLoEFragment)) + , let freq = case loe of + LoEDisabled -> 0 + LoEEnabled () -> if empty then 1 else 10 + in (freq, UpdateLoE <$> genLoEFragment) -- Iterators , (if empty then 1 else 10, uncurry Stream <$> genBounds)

dnadales · 2024-08-12T11:47:08Z

ouroboros-consensus/changelog.d/20240724_170351_alexander.esgen_loe_chaindb_qsm.md

@@ -0,0 +1,3 @@
+### Breaking
+
+- ChainDB: allow to trigger chain selection synchronously


Should we update the PR description accordingly?

... and maybe provide a rationale on how this change relates to adding LoE to the ChainDB tests.

Good point, updated the PR description. The reason for adding this is just testing (also see the first commit message).

Co-authored-by: Nicolas “Niols” Jeannerod <nicolas.jeannerod@tweag.io>

Niols added the Genesis PRs related to Genesis testing and implementation label May 27, 2024

Niols requested review from facundominguez and amesgen May 27, 2024 14:29

amesgen reviewed May 28, 2024

View reviewed changes

Niols force-pushed the niols/loe-chaindb-qsm branch from 419866f to 656c31d Compare May 29, 2024 12:30

Niols mentioned this pull request May 29, 2024

Small tweaks to the LoE #1125

Merged

Niols force-pushed the niols/loe-chaindb-qsm branch 2 times, most recently from 80db2f9 to 2da9483 Compare May 29, 2024 13:38

Niols changed the base branch from genesis/milestone-13 to niols/fix-loe May 29, 2024 13:39

Niols force-pushed the niols/fix-loe branch from 2f4b3d4 to 56b86f3 Compare May 31, 2024 11:30

Niols force-pushed the niols/loe-chaindb-qsm branch 2 times, most recently from 41085a1 to 41d8a9d Compare May 31, 2024 14:54

Niols force-pushed the niols/fix-loe branch 2 times, most recently from e85da74 to cd032b1 Compare June 4, 2024 12:08

Base automatically changed from niols/fix-loe to genesis/milestone-13 June 4, 2024 12:08

Niols mentioned this pull request Jun 4, 2024

Genesis milestone 13: documentation / performance optimization #1113

Merged

Niols force-pushed the niols/loe-chaindb-qsm branch from 41d8a9d to a0a819d Compare June 17, 2024 17:31

amesgen reviewed Jul 10, 2024

View reviewed changes

facundominguez force-pushed the genesis/milestone-13 branch 2 times, most recently from 5680c0b to 53ea76c Compare July 11, 2024 17:28

Base automatically changed from genesis/milestone-13 to main July 11, 2024 19:12

amesgen force-pushed the niols/loe-chaindb-qsm branch 2 times, most recently from cd0f027 to 15a9be3 Compare July 25, 2024 08:17

ChainDB: allow to trigger chain selection synchronously

6442d81

This is useful for tests. Co-authored-by: Nicolas “Niols” Jeannerod <nicolas.jeannerod@tweag.io>

amesgen force-pushed the niols/loe-chaindb-qsm branch from 15a9be3 to c0ecc23 Compare July 25, 2024 12:18

amesgen marked this pull request as ready for review August 9, 2024 13:55

amesgen requested review from nfrisby, jasagredo, fraser-iohk, dnadales and RenateEilers as code owners August 9, 2024 13:55

dnadales approved these changes Aug 12, 2024

View reviewed changes

ChainDB q-s-m test: add the LoE

1357c1b

Co-authored-by: Nicolas “Niols” Jeannerod <nicolas.jeannerod@tweag.io>

amesgen force-pushed the niols/loe-chaindb-qsm branch from c0ecc23 to 1357c1b Compare August 30, 2024 15:17

amesgen enabled auto-merge August 30, 2024 15:17

amesgen added this pull request to the merge queue Aug 30, 2024

Merged via the queue into main with commit 0cae17c Aug 30, 2024
15 checks passed

amesgen deleted the niols/loe-chaindb-qsm branch August 30, 2024 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LoE to the ChainDB QSM test #1119

Add LoE to the ChainDB QSM test #1119

Niols commented May 27, 2024 •

edited by amesgen

Loading

amesgen left a comment

Niols commented May 29, 2024

Niols commented May 29, 2024

Niols commented May 30, 2024

Niols commented May 30, 2024

Niols commented May 30, 2024

Niols commented May 31, 2024

amesgen left a comment

dnadales left a comment

dnadales Aug 12, 2024

amesgen Aug 30, 2024

dnadales Aug 12, 2024

amesgen Aug 30, 2024

dnadales Aug 12, 2024

dnadales Aug 12, 2024

amesgen Aug 30, 2024

		@@ -0,0 +1,3 @@
		### Breaking

		- ChainDB: allow to trigger chain selection synchronously

Add LoE to the ChainDB QSM test #1119

Add LoE to the ChainDB QSM test #1119

Conversation

Niols commented May 27, 2024 • edited by amesgen Loading

amesgen left a comment

Choose a reason for hiding this comment

Niols commented May 29, 2024

Niols commented May 29, 2024

Niols commented May 30, 2024

Niols commented May 30, 2024

Niols commented May 30, 2024

Niols commented May 31, 2024

amesgen left a comment

Choose a reason for hiding this comment

dnadales left a comment

Choose a reason for hiding this comment

dnadales Aug 12, 2024

Choose a reason for hiding this comment

amesgen Aug 30, 2024

Choose a reason for hiding this comment

dnadales Aug 12, 2024

Choose a reason for hiding this comment

amesgen Aug 30, 2024

Choose a reason for hiding this comment

dnadales Aug 12, 2024

Choose a reason for hiding this comment

dnadales Aug 12, 2024

Choose a reason for hiding this comment

amesgen Aug 30, 2024

Choose a reason for hiding this comment

Niols commented May 27, 2024 •

edited by amesgen

Loading