Skip to content

Fix fork publish inactive#4091

Merged
dsiganos merged 4 commits intonanocurrency:developfrom
dsiganos:fix_fork_publish_inactive
Feb 8, 2023
Merged

Fix fork publish inactive#4091
dsiganos merged 4 commits intonanocurrency:developfrom
dsiganos:fix_fork_publish_inactive

Conversation

@dsiganos
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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.

4 participants