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

Allow to rotate dynamos for blockfetch #1123

Closed
wants to merge 24 commits into from
Closed

Allow to rotate dynamos for blockfetch #1123

wants to merge 24 commits into from

Conversation

facundominguez
Copy link
Contributor

No description provided.

@facundominguez facundominguez added Genesis PRs related to Genesis testing and implementation no changelog labels May 29, 2024
@facundominguez facundominguez force-pushed the fd/demote-dynamos branch 2 times, most recently from 6dbb8cd to 450f24d Compare May 29, 2024 12:32
Niols
Niols approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@Niols Niols left a comment

Choose a reason for hiding this comment

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

Code LGTM

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
...nsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/Jumping.hs
Comment on lines +78 to +84
-- Interactions with the BlockFetch logic
-- --------------------------------------
--
-- When syncing, the BlockFetch logic will fetch blocks from the dynamo. If the
-- dynamo is responding too slowly, the BlockFetch logic can ask to change the
-- dynamo with a call to 'rotateDynamo'.
--
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention here that this is only allowed to happen on honest peers (with “honest” defined as the peer from the HAA)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This text needs to be revised, as it may not always be the case that blocks are downloaded from the dynamo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Frankly, I also don't understand my initial message. I suppose it is missing a negation, rotateDynamo is supposed to not be used on the honest peer.

...nsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/Jumping.hs Outdated Show resolved Hide resolved
...nsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/Jumping.hs Outdated Show resolved Hide resolved
...consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/State.hs Show resolved Hide resolved
...nsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/Jumping.hs Outdated Show resolved Hide resolved
@facundominguez facundominguez force-pushed the genesis/milestone-13 branch 2 times, most recently from fae3e08 to a36c25b Compare May 30, 2024 18:55
Niols and others added 4 commits May 30, 2024 19:06
@Niols @amesgen @facundominguez
Make the LoP follow the GSM states
83b12ee 
This also involves some fairly important changes to the leaky bucket and
to the leaky bucket API. These changes make the leaky bucket
significantly more robust.

Co-authored-by: Alexander Esgen <[email protected]>
@facundominguez
Only select chains whose tip is in the loe fragment
82376e6 
Also:

* Simplify chainDiffs computation in chainSelectionForBlock
* Revert "Don't allow to extend the selection by more than k blocks"
  This reverts commit c8468c2.
* Revert "Retrigger GDD when the selection changes"
  This reverts commit f990d41.
@facundominguez
Don't retrigger chain selection if the tip of the loe fragment doesn'…
64f3da3 
…t change
@facundominguez
Increase amount of tries in the Uniform tests and enable CSJ in all o…
2f7152b 
…f them
Niols and others added 4 commits May 30, 2024 19:12
@Niols @facundominguez
Allow to specify tests with multiple honest peers
a969b37 
* Rewrite `Peers` to accept arbitrary number of peers
* Actually generate honest peers in CSJ happy path
* Support a field for extra honest peers in `GenesisTest`
* Allow `uniformPoints` to generate schedules with multiple honest peers
* Adapt CSJ test to use native multiple honest peers generation
* Share partial accessor functions used in tests
* Use partial accessor to retrieve the only honest peer
@Niols @facundominguez
Add some adversaries to the “happy path” test
1d7fc95
@facundominguez
Edit documentation of LoE and GDD
cb5b69a 
Also:
* Remove ill-defined LoELimit
* Remove unused updateLoEFragStall
* Rename runGdd to runGDDGovernor
* Print GDD in traces instead of GDG
* Remove the UpdateLoEFrag callback
* Reorder functions in the Governor module
@facundominguez
Explain more the shrinking choice of test 'serve adversarial branches…
854d72e 
…' and disable timeouts as they are supposed to be
amesgen and others added 8 commits May 30, 2024 19:22
@amesgen @facundominguez
NodeKernel: integrate GSM and LoE
dd651b1 
Also:
* LoEEnabled: make payload strict to avoid NoThunks failures
* LoE: allow to dynamically en-/disable
* Depending on the state of the GSM, we want to either en- or disable the LoE.
* Refactor `runGdd` to use `Watcher`, share trigger logic
  Previously, we would duplicate the logic for when to trigger the GDD between the
  NodeKernel and the peer simulator.
* Add GDD tracing
  The `Show` instances are probably way too large ATM, but there are currently
  unused, so that isn't a pressing concern.
* LoP rate: fix typo
  500/s = 1/2ms, not 2/ms 🤦
@Niols @facundominguez
Automated watcher of CSJ state invariants
4b437c5
@amesgen @facundominguez
Allow to disable *all* Genesis components individually
f69fd1f 
Previously, it wasn't possible to eg run *just* CSJ.
@nbacquey @facundominguez
Simplify the honest peer shrinking function
627634f 
Modify the honest shrinking function by no longer speeding up the other
schedules when an honest tick is deleted. This simplifies a lot of code
in the `Shrinking` module, at the cost of no longer ensuring that shrunk
schedules preserve the overall order of events.

Additionally, we re-enable shrinking in CSJ tests

Also:
* Extend adversarial schedules when shrinking an honest one
* Document the cases when we don't use shrinking
@facundominguez
Avoid reconstructing the candidateSuffixes in densityDisconnect
a592c65
@Niols @facundominguez
Rename PeersSchedule into PointSchedule and make it a newtype
5eac5d3 
Instead of a type alias. This could help catching bugs, but, more
importantly, this paves the way to making it a data-type and adding more
fields to it, although a lot of the fixes of this commit would then
crumble immediately.
@facundominguez
Skip List/Map conversions in GDD governor
ab0da06
@facundominguez
Only get the successors of the immutable tip when reprocessing chain …
2acc4b3 
…sel for LoE
amesgen and others added 3 commits May 31, 2024 15:56
@amesgen
Add changelog fragments
e882d90
@Niols
Rework followsLoEFrag into trimToLoE
cd032b1 
This commit brings several LoE-related improvements, namely:

- The precondition of `followsLoEFrag` stipulating that the given
  candidate fragment had to intersect with the LoE fragment was
  often violated in the code, because the candidate fragments were
  anchored at the tip of the selection.

- `followsLoEFrag` used to accept or reject fragments depending on
  whether they were LoE-compliant or not. The new verson trims them
  to their longest LoE-compliant prefix. Following this change,
  `followsLoEFrag` has been renamed to `timToLoE`. It does not filter
  candidates out anymore.

- This last change makes `computeLoEMaxExtra` redundant. We get rid of
  it entirely and clean up the functions `maximalCandidates` and
  `extendWithSuccessors` accordingly.
@nbacquey @Niols
Refactor PointSchedule to support test end time
6b44bfb 
This commit refactors the `PointSchedule` type from a `newtype` of
`Peers (PeerSchedule blk)` to a datatype containing this schedule as
well as an additional field `psMinEndTime`. This field describes a
minimal absolute time that the test must reach. If all ticks are
executed before this time is reached, an extra delay is inserted.

This allows getting rid of all the places in which we added extra ticks
only for the purpose of making the test run longer. At the cost of
complexifying a bit the implementation of point schedules, this makes
the semantics much clearer.

The fact that `PointSchedule` is now a datatype makes it easy to later
add other fields, for instance a field stating the initial tip points,
instead of having to add zero-duration ticks at the beginning to set
those up, or a field describing the origin of the absolute clock,
instead of shifting the tick times.

Co-authored-by: Nicolas “Niols” Jeannerod <[email protected]>
@Niols
Copy link
Contributor

Niols commented Jun 26, 2024

When plugging this work to the new BlockFetch implementation, I find it very hard to provide a full Context because that requires to have the CSJ configuration in hand when creating the BlockFetchConsensusInterface. While doable, this requires pulling a looot of cables, while the CSJ-configuration-specific fields are not actually used in rotateDynamo. I would rather only have to provide the ChainSync handle collection directly.

@facundominguez
Copy link
Contributor Author

Sounds good to me.

@facundominguez 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
@facundominguez
Copy link
Contributor Author

superseded by #1179

@facundominguez facundominguez deleted the fd/demote-dynamos branch August 9, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Niols Niols Niols approved these changes

@nfrisby nfrisby Awaiting requested review from nfrisby nfrisby will be requested when the pull request is marked ready for review nfrisby is a code owner

@jasagredo jasagredo Awaiting requested review from jasagredo jasagredo will be requested when the pull request is marked ready for review jasagredo is a code owner

@amesgen amesgen Awaiting requested review from amesgen amesgen will be requested when the pull request is marked ready for review amesgen is a code owner

@fraser-iohk fraser-iohk Awaiting requested review from fraser-iohk fraser-iohk will be requested when the pull request is marked ready for review fraser-iohk is a code owner

@dnadales dnadales Awaiting requested review from dnadales dnadales will be requested when the pull request is marked ready for review dnadales is a code owner

Assignees
No one assigned
Labels
Genesis PRs related to Genesis testing and implementation no changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@facundominguez @Niols @amesgen @nbacquey