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

fix(bridge): include fluffy executable in docker image #1169

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

njgheorghita
Copy link
Collaborator

What was wrong?

In our bridge, we support using fluffy as the bridge client. This is done by passing an "executable path" via cli. This is possible when you run the bridge locally, but it's not possible when running a fluffy bridge via docker (eg. in kurtosis).

Also, our method for adding bootstrap enrs to the fluffy executable was broken.

How was it fixed?

  • Included the fluffy executable inside the bridge docker image
  • Fixed the bootstrap enrs cli arg formatting for fluffy

To-Do

@njgheorghita njgheorghita force-pushed the bridge-fluffy-docker branch 2 times, most recently from 9b6dddd to ebd9f60 Compare February 14, 2024 19:17
@KolbyML
Copy link
Member

KolbyML commented Feb 14, 2024

Our bridge requires clients to support trace_gossip to be used

Fluffy doesn't support trace gossip

@KolbyML
Copy link
Member

KolbyML commented Feb 14, 2024

So if fluffy can't be used, why include it in the dockerfile. I am not against making fluffy a usable option it just currently doesn't work.

@njgheorghita
Copy link
Collaborator Author

Ahhh, ok good to know. These were just the initial steps to getting the fluffy executable to run as a bridge client. I hadn't quite reached the part to validate that the client was successfully gossiping content yet.

Ummm..

  1. We could update the bridge to support simple gossip endpoints instead of trace_gossip.
  2. I've heard @kdeme mention they're working on their own bridge.
  • Is it likely that fluffy will support a trace_gossip endpoint down the road?
  • What's the status of your bridge? Would you be 👍 / 👎 on removing support for fluffy from this bridge implementation?

IIRC, we've decided for leaving fluffy support inside this bridge implementation in the past. Maybe this was before fluffy was making progress on their own bridge implementation? Obviously, this bridge's fluffy integration is broken / has been for some time. Now seems like a good time to revisit that decision.

@KolbyML
Copy link
Member

KolbyML commented Feb 14, 2024

Our bridge code for limiting the amount of current content being gossiped at a time wouldn't work with gossip, it only works with trace_gossip. And gossip doesn't provide enough information to make it work no? Of course you can make a bridge which only uses gossip, but making a bridge using gossip uses a different design with different guarantees no?

@KolbyML
Copy link
Member

KolbyML commented Feb 14, 2024

That is to say if we made the backfill bridge use gossip there would be no way to limit how many utp connections are happening at a time other then allocating arbitrarytime allocations you think it will be done in, but that would be fairly inaccurate prone to error and you are likely to have either too many or not enough open at a time. Where using trace_gossip we have guarantees of knowing when something is done or not.

As in gossip is non-blocking so it will return instantly. Which doesn't give you an indication if the uTP stream is done.

Trace_gossip is blocking and returns once the gossips are done, which then can be used to limit the amount of overall gossips at a time

In short trace_gossip gives the bridge more control over what is happening which is a good thing, when without it can be challenging to properly load the portal client with data to gossip

@njgheorghita
Copy link
Collaborator Author

Yeah, I think that's a good point. Imo, this depends on fluffy and what their plans are for their bridge / support of trace_gossip

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

Successfully merging this pull request may close these issues.

None yet

2 participants