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

Fire callback & add to history after confirmation height is set #2233

Conversation

wezrule
Copy link
Contributor

@wezrule wezrule commented Aug 21, 2019

After a block has won an election, the roots are removed but the blocks/dependents are not removed until confirmation height is now set, same with dependent elections.

Moved existing confirmation height tests to their own file, as they are getting quite large (1000+ lines). Added 2 new tests which check that the callback/confirmation history is set correctly for inactive/quorum and dependent elections observer stats.

Added pause/unpause functionality to the confirmation height processor, so that I can more easily prevent it confirming any blocks until desired.

Should be used in conjunction with #2156 to be completely consistent.

Note for reviewers:
Consider the following situation, a simple chain with 2 un-cemented send blocks:
1 - The frontier is being confirmed with an active election, it gets added to the conf height processor and then iterates over both blocks and it about to write the confirmation height of the account to include both blocks.
2 - block_confirm is called on the dependent block and is not marked as confirmed yet so an election is started.
3 - The confirmation height processor in the first step now sets the confirmation height on the frontier and the dependent block, callbacks and confirmation history are updated for both blocks.
4 - Election in 2 finishes, what should happen?

In master this will fire off the callback and add to the confirmation history due to the finished election, but now we only want to do this after setting the confirmation height. The confirmation height for the block is already set and will only fire a duplicate callback in json_handler::block_confirm if the block is confirmed at that point in time. Is this an issue? Should the confirmation height processor detect the already confirmed block and if from a confirmed election always fire the callback as well? (for consistent duplicates at least?).

@wezrule wezrule added the quality improvements This item indicates the need for or supplies changes that improve maintainability label Aug 21, 2019
@wezrule wezrule added this to the V20.0 milestone Aug 21, 2019
@wezrule wezrule self-assigned this Aug 21, 2019
Copy link
Contributor

@cryptocode cryptocode left a comment

Choose a reason for hiding this comment

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

Recent idempotency clarifications in https://docs.nano.org/integration-guides/block-confirmation-tracking/ should make the possible dupes mentioned in the description acceptable. As long as there is no risk of lost observer calls (to trigger websocket notifications), this LGTM. No need to trigger to get duplicates for consistency's sake IMO.

@wezrule wezrule force-pushed the only_fire_callback_after_confirmation_height_set branch from ae09f46 to b3aacfc Compare September 10, 2019 15:46
nano/node/node.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality improvements This item indicates the need for or supplies changes that improve maintainability
Projects
No open projects
V20
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants