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

A0-2884: Refactor alerter #315

Merged
merged 14 commits into from
Aug 21, 2023
Merged

A0-2884: Refactor alerter #315

merged 14 commits into from
Aug 21, 2023

Conversation

woocash2
Copy link
Contributor

@woocash2 woocash2 commented Aug 5, 2023

Refactor alerter to make it possible for synchronous simulation.

  • Separate the alerts module into synchronous handler and service with a single async method (Alerter is now Handler and Service contains the run logic)
  • Make possible to create Service and Handler before running the service

A delicate change: changed signature of rmc and io to not take keychain by reference but by value. This was needed because keychain is now passed to Service::new where it is passed further to Handler (old Alerter) and IO. It can't be passed by reference to them because of the lifetime issues. Instead we pass cloned keychain to both of them by value.

If we really want keychain to remain passed by reference we can probably implement it by using Rc.

@github-actions
Copy link

github-actions bot commented Aug 5, 2023

Please make sure the following happened

  • Appropriate tests created
  • Infrastructure updated accordingly
  • Updated existing documentation
  • New documentation created
  • Version bumped if breaking changes

Copy link
Contributor

@timorl timorl left a comment

Choose a reason for hiding this comment

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

You should be able to construct the Handler and use that for the simulation rather than Service. Plus some other comments about the general architecture of this kind of split, mostly that logging should happen in the Service.

consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/runway/mod.rs Outdated Show resolved Hide resolved
rmc/src/lib.rs Outdated Show resolved Hide resolved
@woocash2
Copy link
Contributor Author

woocash2 commented Aug 14, 2023

you're right, so it's already possible to create Handler and call its methods, but I suppose that some component for simulation and message aggregation is needed.
I'll proceed with adjusting errors, logging and access to methods and I'll add the simulation in the next PR

@woocash2 woocash2 requested a review from timorl August 16, 2023 10:26
Copy link
Contributor

@timorl timorl left a comment

Choose a reason for hiding this comment

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

Still some comments.

consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/service.rs Show resolved Hide resolved
consensus/src/alerts/service.rs Outdated Show resolved Hide resolved
@woocash2 woocash2 requested a review from timorl August 16, 2023 16:33
Copy link
Contributor

@fixxxedpoint fixxxedpoint left a comment

Choose a reason for hiding this comment

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

LGTM. Suggestion: consider implementing a more direct approach instead of simulation. I don't know how exactly your interactions with Handler will look like but in general a safer approach would be a more direct interaction with it, e.g. add a new constructor to Handler that initializes its internal state properly based on persisted state. This way it should be more safe if somebody decides to change Handler's internals. With the simulation approach, you risk ending in some undefined state (since it's not a full simulation - imagine someone add some more steps to protocol, or it buffers something internally, etc.). Using directly a new constructor assures you (kind of) that the Handler is in correct state after initialization.

consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
consensus/src/alerts/service.rs Outdated Show resolved Hide resolved
consensus/src/alerts/service.rs Outdated Show resolved Hide resolved
@woocash2
Copy link
Contributor Author

LGTM. Suggestion: consider implementing a more direct approach instead of simulation. I don't know how exactly your interactions with Handler will look like but in general a safer approach would be a more direct interaction with it, e.g. add a new constructor to Handler that initializes its internal state properly based on persisted state. This way it should be more safe if somebody decides to change Handler's internals. With the simulation approach, you risk ending in some undefined state (since it's not a full simulation - imagine someone add some more steps to protocol, or it buffers something internally, etc.). Using directly a new constructor assures you (kind of) that the Handler is in correct state after initialization.

I think it is possible. Our goal is to obtain a handler which witnessed some events in a particular order, and a Service with some of its IO channels partially filled, also depending on the sequence of events.

We can have Handler::new which creates the Handler in a particular state, and returns the sequence of messages to be passed to Service::new which will fill its channels itself.

That would be nicer indeed.

consensus/src/alerts/handler.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@timorl timorl left a comment

Choose a reason for hiding this comment

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

The exiting thing feels wrong, I suspect it should rather be an error propagated outwards, but I really don't want to make you rewrite this again so I guess it can stay.

consensus/src/alerts/service.rs Outdated Show resolved Hide resolved
@woocash2 woocash2 merged commit 0ee89dc into main Aug 21, 2023
11 checks passed
@woocash2 woocash2 deleted the A0-2884-alerter-refactor branch August 21, 2023 09:29
@woocash2 woocash2 restored the A0-2884-alerter-refactor branch August 21, 2023 13:54
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.

3 participants