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: Implement Finalize stage in Play-Grandpa-Round. #750

Merged
merged 5 commits into from
Feb 7, 2025

Conversation

hMitov
Copy link
Collaborator

@hMitov hMitov commented Feb 7, 2025

Description

  • Implemented Finalize stage in Play-Grandpa-Round
  • Added callback to GrandpaRound, that should be included in the attemptToFinalize method implementation

Fixes #729

@hMitov hMitov self-assigned this Feb 7, 2025
@hMitov hMitov added this to the Fruzhin Phase 3 - M2. GRANDPA milestone Feb 7, 2025

private boolean isRoundReadyToEnd(GrandpaRound round) {
BlockHeader finalized = round.getFinalizedBlock();
Copy link
Collaborator

@Grigorov-Georgi Grigorov-Georgi Feb 7, 2025

Choose a reason for hiding this comment

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

You should use try-catch instead of null check for this one.

    public BlockHeader getFinalizedBlock() {
        if (finalizedBlock == null) throw new GrandpaGenericException("Finalized block has not been set.");
        return finalizedBlock;
    }

Copy link
Collaborator Author

@hMitov hMitov Feb 7, 2025

Choose a reason for hiding this comment

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

The idea here is that if finalized is null, it means that round is not ready to be finalized. The method should return false(we are in waiting for preCommits as not eligible to finalize) and then the callback should be passed to the GrandpaRound handler. Then on each preCommit message, we attemptToFinalize the round and execute the logic from the callback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Return false in the catch if getter throws an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I probably didn't describe well what I meant, but I guess after @Zurcusa's clarification you get the point.

return;
}

Runnable onFinalizeHandler = () -> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we have this inline when we invoke setOnFinalizeHandler

try {
finalized = round.getFinalizedBlock();
} catch (GrandpaGenericException e) {
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a warning log here.

Copy link

sonarqubecloud bot commented Feb 7, 2025

@hMitov hMitov merged commit bd77340 into dev Feb 7, 2025
3 checks passed
@hMitov hMitov deleted the 729-Implement-finalization-stage branch February 7, 2025 14:26
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.

Implement finalization stage.
3 participants