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

POC: chainsync client as pipe producer #5027

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tdammers
Copy link
Contributor

Description

reasonably detailed description of the pull request

Checklist

Quality

  • Commit sequence makes sense and have useful messages, see ref.
  • New tests are added and existing tests are updated.
  • Self-reviewed the PR.

Maintenance

  • Linked an issue or added the PR to the current sprint of ouroboros-network project.
  • Added labels.
  • Updated changelog files.
  • The documentation has been properly updated, see ref.

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

This looks like a great start. Some comments below.

I think the next thing to try will be to use the cardano-api and try this with a test pipe consumer that just spits blocks out to stdout or something.

import Ouroboros.Network.Protocol.ChainSync.Type
import Pipes

data ChainSyncResponse header point tip
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest renaming header everywhere to block. This will be intended to be used with blocks. With the node-to-client protocol, the chain sync provides blocks (whereas for the node-to-node it's used at type header).

Comment on lines 18 to 19
| IntersectFound point tip
| IntersectNotFound tip
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a good start but I think where we want to go next is to not include the intersect finding in the pipe event stream, but to do all the intersection finding first and then yield the pipe producer (if the intersection finding is successful) for streaming the blocks.

So in the end we want just the roll forwards and roll backwards as pipe events.

And later we might want to rename it something like:

data BlockOrRollback block = NextBlock block | Rollback Point

-> [point]
-> Producer (ChainSyncResponse header point tip) m dstate
chainSyncClientProducer driver known =
snd <$> runPeerWithDriver driver peer
Copy link
Contributor

Choose a reason for hiding this comment

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

I have to admit, I'm struggling to understand if this can plausibly work in the way we want it to.

I would have thought that runPeerWithDriver will run the peer to the end and not produce anything until it is done, which would not match what we want. My intuition is we'd need to break up runPeerWithDriver and integrate it into this function (fortunately it's not long).

But on the other hand, what you have has the right type. So I'm confused :-)

We might need to just try it out.

(ChainSyncResponse header point tip)
m
(ClientStIdle header point tip (Producer (ChainSyncResponse header point tip) m) ())
runPeer =
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, this is the initial intersection finding phase, before going into the main streaming phase. A little renaming would make this clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants