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-2846: aleph-bft shouldn't quit after creator's graceful exit #314

Conversation

fixxxedpoint
Copy link
Contributor

@fixxxedpoint fixxxedpoint commented Aug 4, 2023

Description

Creator of aleph-bft might decide to gracefully exit when it discovers inconsistency of its units backup compared to knowledge collected from network. In that case, aleph-bft's session should be allowed to continue instead of panicking.

Type of change

  • New feature (non-breaking change which adds functionality): allow to disable creator
  • Fix: channel returned by Terminator was already of type FusedStream. When used with futures::select! it could cause that select to panic, since its implementation returns is_terminated() == true before it return an error (after we drop parents Terminator).

…nish early, which might happen if unit-backup was purged.
…f some custom oneshot channel, to allow await task completion.
…nstead of some custom oneshot channel, to allow await task completion."

This reverts commit ef90f0f.
…is version also works, but kind of against the description of the SpawnHandle returned by substrate. Probably it is caused by setting a global panic handler on substrate's runner initialization.
…nics. This version also works, but kind of against the description of the SpawnHandle returned by substrate. Probably it is caused by setting a global panic handler on substrate's runner initialization."

This reverts commit b8f4601.
…ied about exit from our parent, we continue to disable our offspring tasks, but without communicating it to our parent.
@github-actions
Copy link

github-actions bot commented Aug 4, 2023

Please make sure the following happened

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

@ghost ghost added the pending label Aug 4, 2023
…as panicking without returning error, since oneshot::Receiver returned by Terminator was already implementing FusedFuture that behaved that way.
@fixxxedpoint fixxxedpoint force-pushed the A0-2846_purge_unit-backup_error branch 2 times, most recently from 4e9fd09 to 9988830 Compare August 6, 2023 09:57
Copy link
Contributor

@kostekIV kostekIV left a comment

Choose a reason for hiding this comment

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

nice one. LGTM

Copy link
Contributor

@lesniak43 lesniak43 left a comment

Choose a reason for hiding this comment

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

cool

@fixxxedpoint fixxxedpoint force-pushed the A0-2846_purge_unit-backup_error branch from 9988830 to 3ff040d Compare August 7, 2023 10:27
@fixxxedpoint fixxxedpoint force-pushed the A0-2846_purge_unit-backup_error branch from 3ff040d to 4f46254 Compare August 7, 2023 10:32
@fixxxedpoint fixxxedpoint merged commit 22f8191 into Cardinal-Cryptography:main Aug 7, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants