Skip to content

Conversation

KonradStaniec
Copy link
Collaborator

#350 Introduced regression due to change from if/else to switch. break which earlier was jumping out of the loop, after change would only exit the switch statement.

Fix:

  • add finalizationLoop label and use it during break
  • add proper test to check that finalization in consecutive.

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.

Nice fix!

Copy link
Contributor

@RafilxTenfen RafilxTenfen left a comment

Choose a reason for hiding this comment

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

Should we also remove the continue?

		case fpSet == nil && !ib.Finalized:
			// does not have finality providers, non-finalised: not finalisable,
			// increment the next height to finalise and continue
			k.setNextHeightToFinalize(ctx, ib.Height+1)
			continue

edit: Ops, it goes to the for to continue, understood

@KonradStaniec KonradStaniec merged commit aaac0d4 into main Jan 2, 2025
21 checks passed
@KonradStaniec KonradStaniec deleted the konradstaniec/fix-block-tallying branch January 2, 2025 13:51
KonradStaniec added a commit that referenced this pull request Jan 2, 2025
#350 Introduced regression
due to change from if/else to switch. `break` which earlier was jumping
out of the loop, after change would only exit the `switch` statement.

Fix:
- add `finalizationLoop` label and use it during `break`
- add proper test to check that finalization in consecutive.
KonradStaniec added a commit that referenced this pull request Jan 2, 2025
#350 Introduced regression
due to change from if/else to switch. `break` which earlier was jumping
out of the loop, after change would only exit the `switch` statement.

Fix:
- add `finalizationLoop` label and use it during `break`
- add proper test to check that finalization in consecutive.
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.

3 participants