Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix fork publish inactive #4091

Merged
merged 4 commits into from
Feb 8, 2023

Conversation

dsiganos
Copy link
Contributor

In test case node.fork_publish_inactive, there is a race condition.
The election and the processing of block send2 happen in parallel.
Usually the election happens first and the send2 block is added to the election.
However, if the send2 block is processed before the election is started then the election does not notice the send2 block.

The test case can be made to pass by ensuring the election is started before the send2 is processed.
However, is this a problem with the test case or this is a problem with the node handling of forks?

@dsiganos dsiganos added the unit test Related to a new, changed or fixed unit test label Jan 31, 2023
@dsiganos dsiganos added this to the V25.0 milestone Jan 31, 2023
@dsiganos dsiganos self-assigned this Jan 31, 2023
@dsiganos
Copy link
Contributor Author

NOTES:

  • If I ensure the election starts before the send2 is processed then the test case works reliably.
  • The block processor tries to add send2 to an election by doing nano::active_transactions::publish() when it resolves to nano::process_result::fork.

clemahieu
clemahieu previously approved these changes Feb 1, 2023
LeandroVCastro
LeandroVCastro previously approved these changes Feb 1, 2023
pwojcikdev
pwojcikdev previously approved these changes Feb 8, 2023
Copy link
Contributor

@pwojcikdev pwojcikdev left a comment

Choose a reason for hiding this comment

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

If you can leave a TODO comment explaining the concerns regarding this behavior it will be great (same as in this PR description)

@dsiganos dsiganos merged commit eb17031 into nanocurrency:develop Feb 8, 2023
@dsiganos dsiganos deleted the fix_fork_publish_inactive branch February 8, 2023 18:35
# for free to join this conversation on GitHub. Already have an account? # 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.

4 participants