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

A0-3185: split rmc #347

Merged
merged 38 commits into from
Oct 13, 2023
Merged

A0-3185: split rmc #347

merged 38 commits into from
Oct 13, 2023

Conversation

woocash2
Copy link
Contributor

@woocash2 woocash2 commented Sep 14, 2023 •
edited
Loading

Splits the rmc logic into Service of 1 async method and synchronous Handler.
The split assures that rmc can be safely initialized from backup state and provides consistency with the design of our other components.

  • Rewritten unit tests for rmc::Service
  • Ran blockchain and ordering examples to verify the behavior after the changes

Not tested yet:

  • Usage with aleph-node
  • Scenarios with forks which should trigger alerts and thus rmcs

@woocash2 woocash2 changed the title A0 3185 split rmc A0-3185: split rmc Sep 14, 2023
@github-actions
Copy link

Please make sure the following happened

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

@woocash2 woocash2 force-pushed the A0-3185-split-rmc branch 3 times, most recently from f591911 to 8557014 Compare September 26, 2023 14:46
@woocash2 woocash2 marked this pull request as ready for review October 2, 2023 07:53
timorleph
timorleph approved these changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@timorleph timorleph left a comment

Choose a reason for hiding this comment

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

Nice.

rmc/src/handler.rs Outdated Show resolved Hide resolved
rmc/src/lib.rs Outdated Show resolved Hide resolved
rmc/src/service.rs Outdated Show resolved Hide resolved
fixxxedpoint
fixxxedpoint requested changes Oct 4, 2023
View reviewed changes
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

rmc/src/handler.rs Outdated Show resolved Hide resolved
rmc/src/handler.rs Outdated Show resolved Hide resolved
rmc/src/service.rs Outdated Show resolved Hide resolved
rmc/src/service.rs Outdated Show resolved Hide resolved
rmc/src/service.rs Outdated Show resolved Hide resolved
fixxxedpoint
fixxxedpoint requested changes Oct 5, 2023
View reviewed changes
rmc/src/service.rs Outdated Show resolved Hide resolved
rmc/src/service.rs Outdated Show resolved Hide resolved
timorleph
timorleph approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@timorleph timorleph left a comment

Choose a reason for hiding this comment

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

Couple minor comments. Looks nice, good interface change after all.

consensus/src/alerts/mod.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
rmc/src/handler.rs Show resolved Hide resolved
rmc/src/handler.rs Show resolved Hide resolved
rmc/src/lib.rs Outdated Show resolved Hide resolved
rmc/src/handler.rs Show resolved Hide resolved
timorleph
timorleph reviewed Oct 11, 2023
View reviewed changes
rmc/src/lib.rs Outdated Show resolved Hide resolved
timorleph
timorleph approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@timorleph timorleph left a comment

Choose a reason for hiding this comment

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

One naming nit, other than that great! <3

rmc/src/handler.rs Outdated Show resolved Hide resolved
fixxxedpoint
fixxxedpoint approved these changes Oct 13, 2023
View reviewed changes
rmc/src/service.rs Outdated Show resolved Hide resolved
@woocash2 woocash2 merged commit ccb0a3a into main Oct 13, 2023
12 checks passed
@woocash2 woocash2 deleted the A0-3185-split-rmc branch October 13, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timorleph timorleph timorleph approved these changes

@fixxxedpoint fixxxedpoint fixxxedpoint approved these changes

@timorl timorl Awaiting requested review from timorl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@woocash2 @fixxxedpoint @timorleph