Skip to content

Conversation

@crocodile-dentist
Copy link
Contributor

@crocodile-dentist crocodile-dentist commented Apr 11, 2024

Description

Consensus Genesis syncing mode relies on having at least one honest peer to ensure a syncing node will finish on a good chain. Largest stake pools are a good proxy for honesty, and therefore these peers are the big ledger peers, which a syncing node may not be aware of when its ledger state may still be too stale. A snapshot file of these peers can be provided in a node's topology configuration, and this set will be provided to peer selection governor so it has an opportunity to make these connections.

This change modifies ledgerPeersThread to leverage this data, and appropriately randomly pick from this set and respond to peer selection governor when it makes a request for big ledger peers.

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 requested a review from a team as a code owner April 11, 2024 14:18
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch from 4c893c8 to e3cf273 Compare April 11, 2024 15:27
@crocodile-dentist crocodile-dentist self-assigned this Apr 12, 2024
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch 3 times, most recently from 30b0651 to bd190c2 Compare April 22, 2024 19:58
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch 2 times, most recently from e0f3ac8 to c074f34 Compare April 24, 2024 09:12
Copy link
Collaborator

@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.

We also need to add some tests where the simulation nodes are using ledger snapshot rather as a source of big ledger peers. Network simulation tests are located here.

@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch 8 times, most recently from 8916d88 to 062381a Compare May 2, 2024 08:01
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch from 062381a to 1d93e05 Compare May 22, 2024 10:40
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch from 1d93e05 to ba520a7 Compare May 27, 2024 12:57
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch 6 times, most recently from 9e0af35 to bdae33f Compare June 19, 2024 12:31
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch 2 times, most recently from d191be9 to b3d24b1 Compare June 24, 2024 11:12
@coot coot added the Genesis label Jul 8, 2024
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch 5 times, most recently from c2b80bf to c63e4b6 Compare July 11, 2024 10:45
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch 5 times, most recently from 304f889 to dba4e75 Compare July 18, 2024 09:55
@crocodile-dentist crocodile-dentist removed the request for review from karknu July 19, 2024 14:29
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch from dba4e75 to 149e98f Compare July 22, 2024 07:34
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.

LGTM just stylistic comments!

Comment on lines 268 to 271
let (peersStakeMap, bigPeersStakeMap, lastSnapshotSlot'') =
stakeMapWithSlotOverSource StakeMapOverSource {
ledgerSlotNo, ledgerPools, peerSnapshot,
lastSnapshotSlot, peerMap, bigPeerMap, ula}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let (peersStakeMap, bigPeersStakeMap, lastSnapshotSlot'') =
stakeMapWithSlotOverSource StakeMapOverSource {
ledgerSlotNo, ledgerPools, peerSnapshot,
lastSnapshotSlot, peerMap, bigPeerMap, ula}
let (peersStakeMap, bigPeersStakeMap, lastSnapshotSlot'') =
stakeMapWithSlotOverSource
StakeMapOverSource {
ledgerSlotNo
, ledgerPools
, peerSnapshot
, lastSnapshotSlot
, peerMap
, bigPeerMap
, ula
}

Copy link
Contributor

Choose a reason for hiding this comment

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

or

Suggested change
let (peersStakeMap, bigPeersStakeMap, lastSnapshotSlot'') =
stakeMapWithSlotOverSource StakeMapOverSource {
ledgerSlotNo, ledgerPools, peerSnapshot,
lastSnapshotSlot, peerMap, bigPeerMap, ula}
let stakeMapOverSource =
StakeMapOverSource {
ledgerSlotNo
, ledgerPools
, peerSnapshot
, lastSnapshotSlot
, peerMap
, bigPeerMap
, ula
}
(peersStakeMap, bigPeersStakeMap, lastSnapshotSlot'') =
stakeMapWithSlotOverSource stakeMapOverSource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 380 to 412
case (ledgerWithOrigin, ledgerPeers, peerSnapshot) of
(At ledgerSlotNo, LedgerPeers _ ledgerRelays, Just (LedgerPeerSnapshot (At snapshotSlotNo, accSnapshotRelays)))
| snapshotSlotNo >= ledgerSlotNo -> -- we cache the peers from the snapshot to avoid unnecessary work
case cachedSlot of
Just thatSlot | thatSlot == snapshotSlotNo ->
(peerMap, bigPeerMap, cachedSlot)
_otherwise -> (accPoolStake (map snd accSnapshotRelays), Map.fromAscList accSnapshotRelays, Just snapshotSlotNo)
| otherwise -> (accPoolStake ledgerRelays, accBigPoolStakeMap ledgerRelays, Nothing)

(_, LedgerPeers _ ledgerRelays, Nothing) -> (accPoolStake ledgerRelays, accBigPoolStakeMap ledgerRelays, Nothing)

(_, _, Just (LedgerPeerSnapshot (At snapshotSlotNo, accSnapshotRelays)))
| After slot <- ula, snapshotSlotNo >= slot -> do
case cachedSlot of
Just thatSlot | thatSlot == snapshotSlotNo ->
(peerMap, bigPeerMap, cachedSlot)
_otherwise -> (accPoolStake (map snd accSnapshotRelays), Map.fromAscList accSnapshotRelays, Just snapshotSlotNo)

_otherwise -> (Map.empty, Map.empty, Nothing)
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to keep to 80 character width lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@crocodile-dentist crocodile-dentist added this pull request to the merge queue Jul 23, 2024
@crocodile-dentist crocodile-dentist removed this pull request from the merge queue due to a manual request Jul 23, 2024
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 crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch from 149e98f to 170ae15 Compare July 23, 2024 09:41
encoding, as well as checking that ToCBOR and FromCBOR are working
together correctly.
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.
Improved naming of functions related to calculating big ledger stake
distribution which were to moved public ouroboros-network-api component.
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/provide-peers-from-snapshot branch from 170ae15 to 748fcf7 Compare July 23, 2024 11:11
@crocodile-dentist crocodile-dentist added this pull request to the merge queue Jul 23, 2024
Merged via the queue into master with commit 88bf776 Jul 23, 2024
@crocodile-dentist crocodile-dentist deleted the mwojtowicz/provide-peers-from-snapshot branch July 23, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants