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.

Sign up for GitHub

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

Jump to bottom

Rework circuit breaking for components on fraud #2041

Closed
Wondertan opened this issue Apr 7, 2023 · 0 comments · Fixed by #2091
Closed

Rework circuit breaking for components on fraud #2041

Wondertan opened this issue Apr 7, 2023 · 0 comments · Fixed by #2091
Labels
area:fraud needs:triage

Comments

@Wondertan
Copy link
Member

Wondertan commented Apr 7, 2023 •
edited
Loading

The current implementation of circuit breaking is fragile and obscure. Currently, It follows a functional approach, and we just wrap the Start method of a component to subscribe for a specific fraud type and also give its Stop method, so that once fraud happens, the component is automatically stopped.

  • The Lifecycle logic is hardly readable
  • Requires two contexts in params which is an antipattern
  • Is not responsive.

Instead, we should introduce a new component, something like FraudLifecycler, with an API that allows registering callbacks for fraud events of a specific type. This will:

  • Detangle the logic
  • Separate the lifecycle context from the start context
  • Enable responsiveness for its internal events
@Wondertan Wondertan mentioned this issue Apr 7, 2023
Open
@Wondertan Wondertan mentioned this issue Apr 17, 2023
Merged
Wondertan added a commit that referenced this issue Apr 18, 2023
@Wondertan
refactor(nodebuiler): introduce ServiceBreaker and update FX (#2091)
bbe68bb 
This PR solves two problems:
* Updates FX and fixes issues we observed in [the
PR](#1801)
* Fixes #2041

Surprisingly, those two problems were related, so I decided to fix them
once and for all. The issue with FX happens after [this
change](uber-go/fx#983). The outcome of this
change is summarized:
> In other words, lifecycle hook annotations can no longer pull in extra
> dependencies outside of things on which
> the annotated constructor is dependent, results that the annotated
> constructor provides, context.Context
> object which is injected by Lifecycle, and the Lifecycle object itself.

Our current code does pull extra dependencies in the service having
`modfraud.Lifecycle` on them, like
[DASer](https://github.com/celestiaorg/celestia-node/blob/main/nodebuilder/das/module.go#L47).
Specifically, it pulls FraudService, and this is no longer allowed. This
forces us to rewrite the fraud lifecycling and here is the solution,
which additionally satisfies #2041.

This also unblocks
#2040, which is now
implemented in celestiaorg/go-fraud#1

I tried to split FX update and the refactor into two diff PRs. However,
the solution does not work with the old FX version, so we have to couple
those.

The chain here is that I needed a new version of FX that fixes
`OnStart/OnStop` hooks, and updating created the whole story. The PR
that does clean-ups basing on new version of FX will com right after.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:fraud needs:triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(nodebuiler): introduce ServiceBreaker and update FX celestiaorg/celestia-node
1 participant
@Wondertan