Skip to content

Conversation

@adlrocha
Copy link
Contributor

@adlrocha adlrocha commented May 31, 2024

Closes #245

This PR:

  • Adds a SkipToInstance interface implemented by Receiver that can be used to force progress into a specific instance determined by a finality certificate.
  • It simplifies Participant.Start() to leverage SkipToInstance under the hood to trigger the beginning of new instances. SkipToInstance spawns a new instance asynchronously by setting an alarm for time.Now() so it is immediately picked up by the host, and the instance is started. Start() under other hand called beginInstance asynchronously and had a lot of redundant code with ReceiveAlarm. By using SkipToInstance in Start we deduplicate some code and handle for all cases the beginning of new instances in the same way.

Notes:

  • Depending on how finality certificates are validated and handled by the host, and how the instance (and future) power tables are computed and provided to the host, the API simplified a lot and instead of passing a FinalityInfo we may be able to just pass the instance number we want to skip provided by the latest finality certificate.
  • We may not need to add ReceiveFinalityCertificate as part of the Receiver api if we decide we won't use it for testings, it may be a public function from the participant consumed by the host.

Future Work

  • Host integration of ReceiveFinality
  • Introduce timeout and condition to trigger the pulling of finality certificates.

@Kubuxu Kubuxu requested a review from anorth June 3, 2024 15:03
@codecov
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.15%. Comparing base (c414912) to head (6bfba82).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
- Coverage   85.66%   85.15%   -0.51%     
==========================================
  Files          14       14              
  Lines        1458     1469      +11     
==========================================
+ Hits         1249     1251       +2     
- Misses        130      138       +8     
- Partials       79       80       +1     
Files Coverage Δ
gpbft/participant.go 88.23% <88.23%> (-1.98%) ⬇️

... and 1 file with indirect coverage changes

@adlrocha
Copy link
Contributor Author

adlrocha commented Jun 3, 2024

@anorth, would really appreciate your early feedback here to see if this makes sense to you and to see what are you suggested next steps (maybe to start with the host integration of finality certificates)? Thanks!

Copy link
Member

@anorth anorth 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 like this won't be too tricky to pull off, thanks. I think we can simplify a bit from this initial proposal.

@adlrocha adlrocha marked this pull request as ready for review June 4, 2024 08:15
@adlrocha adlrocha requested review from Kubuxu, anorth and masih June 4, 2024 09:33
@adlrocha
Copy link
Contributor Author

adlrocha commented Jun 4, 2024

Thank you for your feedback and the additional context, @anorth. I think I addressed all your comments. Please let me know if there is any outstanding issue or it is good to merge.

@adlrocha adlrocha requested a review from masih June 4, 2024 13:52
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Thanks. Looks like this will work well, nearly there.

// the start of the first instance.
// This way we handle in an homogeneous way the start of
// new instances
func (p *Participant) Start() {
Copy link
Member

Choose a reason for hiding this comment

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

Please restore the panic handler on all exploded methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restored. Would you mind elaborating a bit on why this is needed? I think I may have misunderstood the purpose of that code. I was assuming it would be handled by the alarm handler. Thanks!

}

// Triggers the start to the instance defined as an argument.
func (p *Participant) SkipToInstance(i uint64) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a panic handler here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to add an error that covered the error handling here and I don't think it is possible for SkipToInstance to ever return an error (unless I am missing something). Am I missing something? I am wondering if I should remove the error and panic handling here.

@adlrocha
Copy link
Contributor Author

adlrocha commented Jun 6, 2024

@masih, @anorth, would really appreciate a final ✔️ to merge this one. It would be great if I can avoid having to rebase a few times before merging as we may progress :) Thanks!

Copy link
Member

@masih masih left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@anorth anorth added this pull request to the merge queue Jun 6, 2024
Merged via the queue into main with commit ea55876 Jun 6, 2024
@anorth anorth deleted the f3-245-force-finality branch June 6, 2024 23:44
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.

Provide API for host to force progress from finality certificates

4 participants