Skip to content

Fix confirmation_height.election_winner_details_clearing#3741

Merged
dsiganos merged 1 commit intonanocurrency:developfrom
dsiganos:fix_election_winner_details_clearing
Feb 15, 2022
Merged

Fix confirmation_height.election_winner_details_clearing#3741
dsiganos merged 1 commit intonanocurrency:developfrom
dsiganos:fix_election_winner_details_clearing

Conversation

@dsiganos
Copy link
Copy Markdown
Contributor

There is a race condition between a block getting confirmed and setting
of statistics counters. In this case, the counter is incremented in an
observer callback that is executed after the block is confirmed.

One could say that the problem is that the stat counters should updated
before the block is confirmed but I think that is unrealistic to achieve
and possibly not even the right approach.

Converting the check to an ASSERT_TIMELY fixes the problem.
It seems likely that other test cases will have this problem too.

There is a race condition between a block getting confirmed and setting
of statistics counters. In this case, the counter is incremented in an
observer callback that is executed after the block is confirmed.

One could say that the problem is that the stat counters should updated
before the block is confirmed but I think that is unrealistic to achieve
and possibly not even the right approach.

Converting the check to an ASSERT_TIMELY fixes the problem.
It seems likely that other test cases will have this problem too.
@dsiganos dsiganos added the unit test Related to a new, changed or fixed unit test label Feb 15, 2022
@theohax
Copy link
Copy Markdown
Contributor

theohax commented Feb 15, 2022

quorum_minimum_update_weight_before_quorum_checks failed in the CI but it looks like a scheduler flush thing, I'll have a look at it -- I see it's not yet on the list

@dsiganos dsiganos merged commit e51b3d6 into nanocurrency:develop Feb 15, 2022
@dsiganos dsiganos deleted the fix_election_winner_details_clearing branch February 15, 2022 15:56
@zhyatt zhyatt added this to the V24.0 milestone Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

unit test Related to a new, changed or fixed unit test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants