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

feat: batch processing non-finalised blocks #57

Merged
merged 4 commits into from
Sep 21, 2024

Conversation

lesterli
Copy link
Collaborator

Summary

This PR fixes the issue #55

Test Plan

make lint
make test
make test-e2e-babylon --run ^TestFastSync$
make test-e2e-op

Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

This is an algorithm-level change so let's get approval from @gitferry : What do you think about submitting finality signatures for all blocks if multiple blocks are in the channel?

Also, if this idea is approved, how about we merge it to main?

Comment on lines 258 to 259
default:
goto processBlocks
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to avoid the goto statement, as it seems not a good practice.

Here default is hit when the channel becomes empty. Can we replace this goto with break then?

Copy link
Collaborator Author

@lesterli lesterli Sep 20, 2024

Choose a reason for hiding this comment

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

Using a boolean fetchMoreBlocks, it replaced goto statement.

@bap2pecs
Copy link
Collaborator

Also, if this idea is approved, how about we merge it to main?

yes and also #54. we can cherry pick the PRs

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

In general I like the batching solution. Left some suggestion in code-wise. We'd better add a test case to ensure the solution works

@@ -194,52 +193,51 @@ func (fp *FinalityProviderInstance) IsRunning() bool {
func (fp *FinalityProviderInstance) finalitySigSubmissionLoop() {
defer fp.wg.Done()

var targetHeight uint64
for {
select {
case b := <-fp.poller.GetBlockInfoChan():
Copy link
Member

Choose a reason for hiding this comment

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

I think we can put it in the default case where we have getAllBlocksFromChan() to drain the channel and then try to process each block and do batch send

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean to put the var targetHeight uint64 in the default case of the inner loop?

Copy link
Member

Choose a reason for hiding this comment

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

I meant the entire block fetching logic can be put in the default case so that we can have a method to fetch them all other than fetch one, and the rest

Copy link
Collaborator Author

@lesterli lesterli Sep 20, 2024

Choose a reason for hiding this comment

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

Got it. Updated.

// due to lack of voting power or public randomness, so we may
// have gaps during processing
pollerBlocks := []*types.BlockInfo{b}
fetchMoreBlocks := true
Copy link
Member

Choose a reason for hiding this comment

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

we probably don't need this flag. We can use for {} and inject break in default case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it is removed.

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

LGTM! We should test this case but can be done in a separate PR

@lesterli lesterli merged commit b1ab7ea into fix/poller-stuck Sep 21, 2024
3 of 8 checks passed
@bap2pecs
Copy link
Collaborator

@lesterli have we tested this using our demo app to see how long it takes after this change?

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.

4 participants