LedgerSupportsPeerSelection: return peers registered in ledger state #2536

mrBliss · 2020-08-24T13:26:20Z

For Shelley, this returns the stake pool relays ordered by descending stake.

mrBliss · 2020-08-24T13:26:52Z

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/PeerSelection.hs

+          $ shelleyState
+
+      relayToDomainAddress :: SL.StakePoolRelay -> DomainAddress
+      relayToDomainAddress = undefined


@coot @JaredCorduan This hole still needs to be filled in.

@coot Don't forget about this hole.

mrBliss · 2020-08-24T13:29:53Z

ouroboros-network/src/Ouroboros/Network/PeerSelection/RootPeersDNS.hs

@@ -24,6 +24,9 @@ module Ouroboros.Network.PeerSelection.RootPeersDNS (
    DNS.Domain,
    DNS.TTL,
    IPv4,
+
+    -- * Socket type re-exports
+    Socket.PortNumber,


@coot Thanks to this extra re-export and the re-exports in Ouroboros.Consensus.Ledger.SupportsPeerSelection, it should not be necessary to import anything from the network layer or network-related packages in Ouroboros.Consensus.Shelley.Ledger.PeerSelection.

mrBliss · 2020-08-25T06:55:21Z

I changed the target of this branch to master as it doesn't rely on anything in https://github.com/input-output-hk/ouroboros-network/tree/coot/connection-manager and it being part of a feature branch that won't be merged quickly will just cause bigger conflicts in the end (I'm already creating conflicts with a PR I submitted to that branch 🙁).

mrBliss · 2020-08-26T08:12:18Z

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/PeerSelection.hs

+      poolDomainAddresses :: Map (SL.KeyHash 'SL.StakePool c) [DomainAddress]
+      poolDomainAddresses =
+            Map.map (map relayToDomainAddress . toList . SL._poolRelays)
+          . SL._pParams


FYI, if we change this from SL._pParams to SL._fPParams, we get the future pool parameters.

Presumably that's the set that is updated when pools re-register (that we take snapshots from).

We should aim for:

sampling pools from an appropriate stake distribution (e.g. the one used for block production in the current epoch)

selecting the pool relays that are the most up to date, so that we can respond to changed relays more quickly than waiting 5 days

About 1: I believe the current version of this PR uses that stake distribution. It "samples" it in that it returns all registered relays for each stake pool. So the list returned by getPeers will look something like (with pool1 having the most stake, etc.):

[pool1_relay1, pool1_relay2, pool2_relay1, pool3_relay1, pool3_relay2, ..]

About 2: I suppose we can merge the relays from SL._pParams with those of SL._fPParams so we get the future relays as well. For example:

[pool1_relay1, pool1_relay2, pool1_future_relay3, ..]

Let me know what is wanted, I'm just the conduit between network and ledger.

I think we need both current and future relays. The governor should try future relays first and if they are not yet accessible then resort to the current ones.

Another point is that it would be more useful to group relays for the same core node, would that be possible? e.g. [[pool1_relay1, pool1_relay2, pool1_future_relay3], ...]. @dcoutts I think this would require some changes to the governor, but it's more useful to know in advance that which relays belong to the same stake pool, instead of relaying on tip-sample to discover that through some heuristics.

As mentioned, this PR already groups them per stake pool.

@coot Which order do you want for the relays? Future relays first, then the current ones? The other way around? You probably want the duplicates filtered out, right?

I think it makes more sense to get future relays first, and we'd like to filter out duplicates.

edsko · 2020-09-15T08:22:41Z

See discussion on slack, we might need an API of type

getPeers :: LedgerState blk -> [(PoolStake, [DomainAddress])]

mrBliss · 2020-09-28T10:06:44Z

See discussion on slack, we might need an API of type
getPeers :: LedgerState blk -> [(PoolStake, [DomainAddress])]

This PR already provides

getPeers :: LedgerState blk -> [(PoolStake, NonEmpty DomainAddress)]

mrBliss · 2020-09-29T14:20:56Z

@coot: I updated the PR to also include the relays registered in the future pool parameters.

The only thing left to do, but not for me, is relayToDomainAddress. When somebody from the network team or ledger team fills that in, I can mark this PR ready for review.

mrBliss · 2020-11-11T09:33:19Z

Rebased against master

ouroboros-consensus/src/Ouroboros/Consensus/Ledger/SupportsPeerSelection.hs

ouroboros-consensus/src/Ouroboros/Consensus/NodeKernel.hs

ouroboros-consensus/src/Ouroboros/Consensus/Ledger/SupportsPeerSelection.hs

mrBliss · 2020-11-20T12:18:40Z

ouroboros-consensus/src/Ouroboros/Consensus/NodeKernel.hs

+--
+-- If the slot is too far in the future w.r.t. the current ledger state,
+-- 'HF.PastHorizonException' will be returned.
+getSlotTime ::


WRONG: we can't use the current LedgerState if we don't know anything about the header or block that has that SlotNo. In particular, the safe zone that is appropriate for a header should extend from the intersection with our and their chain, not from our tip. Since we don't know the intersection point here, we might be incorrectly returning a UTCTime while we should be returning a PastHorizonException.

For example, if the SlotNo is of a header on a chain that forked off before the transition to Shelley on mainnet and in that chain the transition to Shelley happened ten epochs later, this conversion is completely wrong.

Alternative: introduce a Timed a which stores the header and the time corresponding to its slot (computed using the ledger state at the intersection!). It can also include the time at which the header was received by the ChainSyncClient.

Who knew time was tricky? 🤔 😬

mrBliss · 2020-12-09T16:34:29Z

Rebased and squashed some commits.

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/PeerSelection.hs

coot

I already reviewed the network changes in the PR that where merged into this branch; I skimmed through consensus changes and they looked fine to me, but that was very rudimentary; I only analysed the LedgerSupportsPeerSelection (SheelyBlock era) instance which I think is the core of what we need. @mrBliss should I look more closely into other parts?

coot · 2020-12-10T10:15:34Z

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/PeerSelection.hs

+           Map (SL.KeyHash 'SL.StakePool (EraCrypto era)) (NonEmpty StakePoolRelay)
+      poolRelayAddresses =
+          Map.unionWith
+            (\futureRelays currentRelays -> NE.nub (futureRelays <> currentRelays))


Looks good, we want both future and current relays.

coot · 2020-12-10T10:16:18Z

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/PeerSelection.hs

+           SL.PoolDistr (EraCrypto era)
+        -> [(SL.KeyHash 'SL.StakePool (EraCrypto era), PoolStake)]
+      orderByStake =
+            sortOn (Down . snd)


ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger.hs

mrBliss · 2020-12-10T10:37:28Z

I already reviewed the network changes in the PR that where merged into this branch; I skimmed through consensus changes and they looked fine to me, but that was very rudimentary; I only analysed the LedgerSupportsPeerSelection (SheelyBlock era) instance which I think is the core of what we need. @mrBliss should I look more closely into other parts?

No need, I have reviewed this with @edsko already. So from the PoV of consensus, this is ready to merge.

mrBliss · 2020-12-10T10:39:59Z

I'll fix the CI error (didn't compile after rebasing, oops)

mrBliss · 2020-12-10T10:56:08Z

I'll fix the CI error (didn't compile after rebasing, oops)

Done.

mrBliss · 2020-12-10T12:04:02Z

bors merge

iohk-bors · 2020-12-10T12:27:07Z

Merge conflict.

Fixes #2535.

A relay can either be a domain name or an IP address therefore `DomainAddress` isn't suitable. Co-authored-by: Karl Knutsson <karl.knutsson@iohk.io>

Support for randomly picking peers from the current ledger state. The peers are randomly choosen from the relays registered on chain, and the chance to pick a given stake pools relay is weighted by the amount of stake they control.

mrBliss · 2020-12-10T13:10:17Z

bors merge

iohk-bors · 2020-12-10T13:34:14Z

Build succeeded:

mrBliss added the consensus issues related to ouroboros-consensus label Aug 24, 2020

mrBliss requested a review from coot August 24, 2020 13:26

mrBliss commented Aug 24, 2020

View reviewed changes

mrBliss linked an issue Aug 24, 2020 that may be closed by this pull request

On chain root peers #2535

Closed

mrBliss changed the base branch from coot/connection-manager to master August 25, 2020 06:53

mrBliss force-pushed the mrBliss/ledger-supports-peer-selection branch from 5e96201 to 602c946 Compare August 25, 2020 06:53

mrBliss commented Aug 26, 2020

View reviewed changes

mrBliss force-pushed the mrBliss/ledger-supports-peer-selection branch from 602c946 to 225a16e Compare September 8, 2020 07:08

edsko assigned edsko and mrBliss Sep 15, 2020

mrBliss unassigned edsko Sep 28, 2020

mrBliss force-pushed the mrBliss/ledger-supports-peer-selection branch from ff1e72a to 0e6b061 Compare September 29, 2020 14:18

mrBliss force-pushed the mrBliss/ledger-supports-peer-selection branch from 0e6b061 to c8fb01c Compare November 11, 2020 09:33

mrBliss force-pushed the mrBliss/ledger-supports-peer-selection branch from c8fb01c to 248d2de Compare November 20, 2020 11:00

mrBliss commented Nov 20, 2020

View reviewed changes

mrBliss force-pushed the mrBliss/ledger-supports-peer-selection branch from 248d2de to 076cdbd Compare November 20, 2020 13:55

mrBliss force-pushed the mrBliss/ledger-supports-peer-selection branch from 09c32a3 to 1d7c50b Compare December 9, 2020 16:34

mrBliss marked this pull request as ready for review December 9, 2020 16:34

mrBliss requested a review from karknu December 9, 2020 16:40

coot reviewed Dec 10, 2020

View reviewed changes

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/PeerSelection.hs Show resolved Hide resolved

coot approved these changes Dec 10, 2020

View reviewed changes

mrBliss commented Dec 10, 2020

View reviewed changes

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger.hs Show resolved Hide resolved

mrBliss force-pushed the mrBliss/ledger-supports-peer-selection branch from 1d7c50b to 181726d Compare December 10, 2020 10:55

mrBliss and others added 4 commits December 10, 2020 14:01

ChainDB: add getImmutableLedger

fe53dd6

LedgerSupportsPeerSelection: return peers registered in ledger state

2e771ab

Fixes #2535.

Introduce RelayAddress

fcf4432

A relay can either be a domain name or an IP address therefore `DomainAddress` isn't suitable. Co-authored-by: Karl Knutsson <karl.knutsson@iohk.io>

Support for getting peers from ledger (disabled)

ce220f1

Support for randomly picking peers from the current ledger state. The peers are randomly choosen from the relays registered on chain, and the chance to pick a given stake pools relay is weighted by the amount of stake they control.

mrBliss force-pushed the mrBliss/ledger-supports-peer-selection branch from 181726d to ce220f1 Compare December 10, 2020 13:10

iohk-bors bot merged commit 02771ee into master Dec 10, 2020

iohk-bors bot deleted the mrBliss/ledger-supports-peer-selection branch December 10, 2020 13:34

karknu mentioned this pull request Dec 11, 2020

Peers from the ledger #2749

Closed

mrBliss mentioned this pull request Jan 7, 2021

On chain root peers #2535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LedgerSupportsPeerSelection: return peers registered in ledger state #2536

LedgerSupportsPeerSelection: return peers registered in ledger state #2536

mrBliss commented Aug 24, 2020

mrBliss Aug 24, 2020

mrBliss Sep 4, 2020

mrBliss Aug 24, 2020

mrBliss commented Aug 25, 2020

mrBliss Aug 26, 2020

dcoutts Aug 26, 2020

mrBliss Aug 26, 2020

coot Sep 14, 2020

mrBliss Sep 28, 2020

coot Sep 28, 2020

edsko commented Sep 15, 2020 •

edited

Loading

mrBliss commented Sep 28, 2020

mrBliss commented Sep 29, 2020

mrBliss commented Nov 11, 2020

mrBliss Nov 20, 2020

edsko Nov 20, 2020

mrBliss commented Dec 9, 2020

coot left a comment

coot Dec 10, 2020

coot Dec 10, 2020

mrBliss commented Dec 10, 2020

mrBliss commented Dec 10, 2020

mrBliss commented Dec 10, 2020

mrBliss commented Dec 10, 2020

iohk-bors bot commented Dec 10, 2020

mrBliss commented Dec 10, 2020

iohk-bors bot commented Dec 10, 2020

LedgerSupportsPeerSelection: return peers registered in ledger state #2536

LedgerSupportsPeerSelection: return peers registered in ledger state #2536

Conversation

mrBliss commented Aug 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrBliss commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsko commented Sep 15, 2020 • edited Loading

mrBliss commented Sep 28, 2020

mrBliss commented Sep 29, 2020

mrBliss commented Nov 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrBliss commented Dec 9, 2020

coot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrBliss commented Dec 10, 2020

mrBliss commented Dec 10, 2020

mrBliss commented Dec 10, 2020

mrBliss commented Dec 10, 2020

iohk-bors bot commented Dec 10, 2020

mrBliss commented Dec 10, 2020

iohk-bors bot commented Dec 10, 2020

edsko commented Sep 15, 2020 •

edited

Loading