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

feat(rpc): pipelined aggregator setup #487

Merged
merged 4 commits into from
Feb 6, 2025
Merged

feat(rpc): pipelined aggregator setup #487

merged 4 commits into from
Feb 6, 2025

Conversation

doscortados
Copy link

Closes #460

@doscortados doscortados requested a review from a team January 28, 2025 14:39
@doscortados doscortados changed the base branch from main to dev January 28, 2025 14:40
@ozankaymak ozankaymak requested review from mmtftr, ekrembal, ceyhunsen and ozankaymak and removed request for a team January 28, 2025 14:42
Copy link

@mmtftr mmtftr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the changes.

I believe the original issue also wants the try_join_alls to happen in background tasks so that they're happening in parallel (you can check aggregator for details)

We're also going to be working on moving the business logic out of the gRPC handlers, and moving parsing/validation logic out into trait impls so that they're more easily tested and the code has less clutter.

core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
@doscortados
Copy link
Author

Thank you for the comments @mmtftr , and sorry for my late reply!
This is rather initial version and significant re-work will follow in further commits.

At this point I see that mpsc channel isn't working out for multiplexing N-over-M streams, and I am trying out mpmc one to avoid pulling all the params in memory before streaming it to consumers.

Copy link
Member

@ceyhunsen ceyhunsen left a comment

Choose a reason for hiding this comment

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

Overall, it looks good. But apparently there is a deadlock or something like that: CI/CD tests are not finishing.

core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
@doscortados doscortados self-assigned this Feb 4, 2025
@doscortados
Copy link
Author

Thank you for your comments @mmtftr @ceyhunsen , I will address them once I get the tests pass.
Right now I figured out a "quick & dirty" way to avoid deadlock when broadcasting operator/watchtower params, but now when deadlock is no more, two tests still fail.

@doscortados doscortados force-pushed the setup-pipelines branch 3 times, most recently from 5c0ca48 to 3d716a7 Compare February 4, 2025 15:05
@doscortados
Copy link
Author

At this time the deadlock and the failing tests are fixed, I will make one more round of cleanups and mark it ready for the review.

@doscortados doscortados marked this pull request as ready for review February 5, 2025 08:08
@doscortados doscortados changed the title Aggregator setup pipelining feat(rpc): pipelined aggregator setup Feb 5, 2025
@doscortados doscortados requested a review from ekrembal February 5, 2025 08:14
Copy link
Member

@ceyhunsen ceyhunsen left a comment

Choose a reason for hiding this comment

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

It looks unnecessarily complex to me in general tbh

core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
@doscortados
Copy link
Author

doscortados commented Feb 5, 2025

It looks unnecessarily complex to me in general tbh

I totally agree @ceyhunsen ! But it also is non-trivial multiplexing M-over-N streams in a concurrent fashion. Also it is necessary to re-stream from broadcast consumer to mpsc consumer due to limitation of the client API.

Compared with existing non-concurrent implementation the only overhead is actually only two extra broadcast channels (one for setting operator params, another one for watchtowers).

UPDATE

If you know how to simplify it, I am open to suggestions by all means!

We can probably also approach the verifier cliet API to accept broadcast channel into-stream wrappers, but I didn't want to go there in scope of this PR just to avoid bloating it.

core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
Copy link

@mmtftr mmtftr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the refactor, just some small comments

core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
core/src/rpc/aggregator.rs Outdated Show resolved Hide resolved
@ekrembal ekrembal merged commit c268007 into dev Feb 6, 2025
9 checks passed
@ekrembal ekrembal deleted the setup-pipelines branch February 6, 2025 11:25
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.

Pipeline for aggregator setup
4 participants