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.

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

Extend and expose PeerSelectionCounters #4836

Closed
wants to merge 4 commits into from

Conversation

bolt12
Copy link
Contributor

@bolt12 bolt12 commented Mar 19, 2024

Description

Closes #4815

@bolt12 bolt12 self-assigned this Mar 19, 2024
@bolt12 bolt12 requested a review from a team as a code owner March 19, 2024 11:13
@bolt12 bolt12 added node-to-node Issues and PRs relate to node-to-node protocols bootstrap Issues / PRs related to bootstrap peers labels Mar 19, 2024
@bolt12 bolt12 force-pushed the bolt12/inform-consensus-layer branch from 0bd3e0d to 92107cf Compare March 19, 2024 16:03
@bolt12 bolt12 requested a review from coot March 19, 2024 16:03
@bolt12 bolt12 force-pushed the bolt12/inform-consensus-layer branch from 92107cf to a591104 Compare March 21, 2024 10:58
@bolt12 bolt12 changed the title Adds eicIsLocalRootPeer to ExpandedInitiatorContext Extend and expose PeerSelectionCounters Mar 21, 2024
@bolt12 bolt12 force-pushed the bolt12/inform-consensus-layer branch 2 times, most recently from 6adebdf to 59574c8 Compare March 21, 2024 16:33
@bolt12 bolt12 force-pushed the bolt12/inform-consensus-layer branch from 59574c8 to 47e943c Compare March 25, 2024 11:20
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.

The withDelayCancellable & modifyPeersBasedOnCounter contain a bug: they update the targets and wait on the changes in the same stm transaction - changes in a single stm tx are not visible to the outside world until the transaction is committed, as a result we never update the targets.

I think it will be much better to write this code in a different way. We need a function which:

  • update the targets to the specified value
  • then block on the current counters until they reach the
    So I suggest that we implement:
updateTargets :: MonadSTM m
              => Tracer m (TracePeerSelection peeraddr)
              -> StrictTVar m PeerSelectionTargets
              -> StrictTVar m PeerSelectionCounters
              -> DiffTime
              -- ^ timeout
              -> (PeerSelectionTargets -> PeerSelectionTargets)
              -- ^ update counters function
              -> m ()

it should look something like (Haskell pseudocode):

updateTargets tracer peerSelectionVar countersVar timeout modifyTargets = do
 -- update targets
 targets <- atomically $ do modifyTVar peerSelectionVar modifyTargets
                            readTVar peerSelectionVar
 -- create timeout
 delayVar <- registerDelay timeout
 -- block until counters reached the targets, or the timeout fires
 atomically $ runFirstToFinish $
   FirstToFinish (readTVar countersVar >>= check . didCountersReachedTargets targets)
   <>
   FIrstToFinish (readTVar delayVar >>= check)               

Note that it runs in two STM transactions, so the outbound governor will have a chance to notice and act on modified targets.

Comment on lines 9 to 11
-- For Peer Selection Governor this flag isn't much useful since one can do a
-- lookup to LocalRootPeers set. But for miniprotocols this is useful
-- information to carry around, namely for bootstrap peers optimisations.
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
-- For Peer Selection Governor this flag isn't much useful since one can do a
-- lookup to LocalRootPeers set. But for miniprotocols this is useful
-- information to carry around, namely for bootstrap peers optimisations.
-- This data structure is passed to mini-protocol callbacks; the Genesis state
-- machine relies on this information. See
-- [issue#4815](https://github.com/IntersectMBO/ouroboros-network/issues/4815).

@coot
Copy link
Contributor

coot commented Apr 6, 2024

Thanks @bolt12, I am closing it in favour of #4846 which was based on this PR.

@coot coot closed this Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bootstrap Issues / PRs related to bootstrap peers node-to-node Issues and PRs relate to node-to-node protocols
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Inform Consensus Layer whether a bootstrap peer should be considered remote
2 participants