Skip to content

Big Ledger Peer Targets for Genesis #4832

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
Merged
merged 10 commits into from
Jul 24, 2024
Merged

Big Ledger Peer Targets for Genesis #4832

merged 10 commits into from
Jul 24, 2024

Conversation

crocodile-dentist
Copy link
Contributor

@crocodile-dentist crocodile-dentist commented Mar 15, 2024 •
edited
Loading

Genesis mode support can be determined from node's configuration at startup. If this mode is disabled, the legacy peer selection method will be used, with optional support for bootstrap peers. When Genesis is enabled, peer selection must be provided with two sets of peer targets.
One set is used when it is detected that we are far from the tip of the best chain, and this set is maintained for the duration of the syncing period. Support for churning of this set is included. The other set is the default set when we are caught up, and it is analogous to the set used by the non-Genesis method.

The configuration module has been expanded to include some useful defaults for the new functionality.

Some minor refactoring.

Description

resolves #4813, resolves #3396, resolves #4609

Checklist

  • Branch
    • Updated changelog files.
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • The documentation has been properly updated
    • New tests are added if needed and existing tests are updated
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

@crocodile-dentist crocodile-dentist added Genesis outbound-governor Issues / PRs related to outbound-governor labels Mar 15, 2024
@crocodile-dentist crocodile-dentist self-assigned this Mar 15, 2024
@crocodile-dentist crocodile-dentist requested a review from a team as a code owner March 15, 2024 07:48
@crocodile-dentist crocodile-dentist marked this pull request as draft March 15, 2024 07:49
@crocodile-dentist crocodile-dentist force-pushed the crocodile-dentist/og-genesis branch 3 times, most recently from fadac6c to 44f6265 Compare March 18, 2024 10:14
@coot coot changed the title This commit introduces support for Genesis diffusion Big Ledger Peer Targets for Genesis Mar 18, 2024
@coot
Copy link
Contributor

coot commented Mar 18, 2024

It's ready for review, isn't it. I'll mark it as such.

@coot coot marked this pull request as ready for review March 18, 2024 16:06
coot
coot reviewed Mar 21, 2024
View reviewed changes
bolt12
bolt12 requested changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@bolt12 bolt12 left a comment

Choose a reason for hiding this comment

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

I think you complicated things for you with this mutex. I get that you want to have different targets depending on a given OG state but the OG only acts upon inputs, it doesn't actually output much for the outside except some public info and debugging info. So having a mutex that is kind of a smell to me.

I think what could simplify things is to move the target decision logic outside of the churn and OG, since it is static anyway (via currentTargets) and put it at the STM m PeerSelectionTargets action level. I think that's the right place to put it

@crocodile-dentist crocodile-dentist force-pushed the crocodile-dentist/og-genesis branch from 44f6265 to 1d84abb Compare April 3, 2024 13:33
@crocodile-dentist crocodile-dentist force-pushed the crocodile-dentist/og-genesis branch from 1d84abb to 6b74ed8 Compare April 12, 2024 15:31
coot
coot requested changes May 2, 2024
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

A few suggestions

@crocodile-dentist crocodile-dentist force-pushed the crocodile-dentist/og-genesis branch 9 times, most recently from 4a0b47c to cee3efb Compare May 8, 2024 09:50
@crocodile-dentist crocodile-dentist force-pushed the crocodile-dentist/og-genesis branch 5 times, most recently from dd67a38 to 26b4721 Compare May 20, 2024 16:46
@crocodile-dentist crocodile-dentist force-pushed the crocodile-dentist/og-genesis branch 4 times, most recently from 13b33e2 to 7300cf7 Compare July 16, 2024 13:40
coot
coot approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@crocodile-dentist crocodile-dentist force-pushed the crocodile-dentist/og-genesis branch 4 times, most recently from 39aa13e to 943e64b Compare July 22, 2024 09:13
@crocodile-dentist
Enable distribution of big ledger peers obtained from snapshot:
c6b06a7 
Depending on its topology configuration, a node may provide diffusion
layer with a snapshot of big ledger peers from some slot. This value
is provided as an argument when diffusion is initialized, and is
provided to ledgerPeersThread function, which contains the logic
when these peers, if any, can be provided to peer selection governor
when it requests them. This is especially useful when consensus is
ran in Genesis mode and when the node is bootstrapping so it may have
opportunity to connect to these trustworthy peers when they may not
be yet available on its own ledger.
@crocodile-dentist
Review - improve naming
33d47bb
@crocodile-dentist
Added a property test that verifies validity of peer snapshot CBOR
b494d2d 
encoding, as well as checking that ToCBOR and FromCBOR are working
together correctly.
@crocodile-dentist
Changes to LedgerPeerSnapshot serialisation tests
c4a3839 
Test is modified such that ledger peer pool relays are
equal modulo fully qualified domain names to pool relays
restored from snapshot.
Also, since stake is serialised in floating point format, comparison
to original Rational values is approximate.
@crocodile-dentist
Added ledger peer snapshot property tests
748fcf7 
Improved naming of functions related to calculating big ledger stake
distribution which were to moved public ouroboros-network-api component.
@crocodile-dentist
Support for Genesis consensus mode alternate big ledger peer targets
643b9cb 
This change equips peer selection and churn governor to
appropriately respond to change in ledger state judgment
when diffusion is running in Genesis consensus mode. A
separate targets basis is made available in this mode
which uses more hot big ledger peers when a node is syncing
up.

Additional changes in support of this were made to
the configuration module, and some other minor refactorings
were applied as well.
@crocodile-dentist
Improve naming
5770890
@crocodile-dentist
Pass LedgerPeersConsensusInterface to churn
556e063 
Read LedgerStateJudgement(*) to decide whether to use syncTargets
from ConsensusModePeerTargets when * is TooOld.
@crocodile-dentist
Don't use stale targets lingering since last churning
1ce1cc1 
In Genesis sync mode, when we aren't actively churning,
the targets basis in the TVar can become stale when
ledger state judgement flips. We don't want to use these
targets when that happens and so we add a check for this condition,
and instead set the targets in the private OG state based on
the targets appropriate for the current ledger state judgement.
When churn wakes up for the next round, it will use the appropriate
targets basis and then we can use the targets that churn modifies
and provides us with.
@crocodile-dentist
Stylish haskell
2244210
@crocodile-dentist crocodile-dentist force-pushed the crocodile-dentist/og-genesis branch from 943e64b to 2244210 Compare July 23, 2024 11:18
@crocodile-dentist crocodile-dentist dismissed bolt12’s stale review July 24, 2024 05:54

I've looked thru all suggestions and confirmed that your review comments have been addressed. However, github still won't allow it to merge because apparently it still thinks there are some changes that need to be fixed.

@crocodile-dentist crocodile-dentist added this pull request to the merge queue Jul 24, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 24, 2024
@crocodile-dentist
Merge pull request #4832 from IntersectMBO/crocodile-dentist/og-genesis
a744beb 
Big Ledger Peer Targets for Genesis
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 24, 2024
@crocodile-dentist crocodile-dentist added this pull request to the merge queue Jul 24, 2024
Merged via the queue into master with commit 29899df Jul 24, 2024
16 checks passed
@crocodile-dentist crocodile-dentist deleted the crocodile-dentist/og-genesis branch July 24, 2024 06:39
@crocodile-dentist crocodile-dentist mentioned this pull request Aug 12, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coot coot coot approved these changes

@karknu karknu karknu left review comments

@bolt12 bolt12 bolt12 left review comments

Assignees

@crocodile-dentist crocodile-dentist

Labels
Genesis outbound-governor Issues / PRs related to outbound-governor
Projects
None yet
Milestone
No milestone
4 participants
@crocodile-dentist @coot @karknu @bolt12