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

Add cemented frontier successor confirmation in request loop #2885

Conversation

wezrule
Copy link
Contributor

@wezrule wezrule commented Aug 18, 2020

This adds the following functionality:

  • New flag in elections is_optimistic which is set to true for any elections inserted from the optimistic frontiers confirmation. New confirm_req rounds are sent every 10 seconds (as opposed to 5 for normal elections), and expire after 1 minute
  • The frontiers confirmation procedure now goes: wallet frontiers -> frontiers -> expired cemented frontier successors. The last one is new and is to confirm blocks at cemented height + 1 as the network has likely not confirmed this frontier (due to sequential voting) so it is not possible to confirm blocks for this account without backtracking. If it is a receive block the source must be confirmed otherwise an election is not attempted, there is no complex dependency traversal done or any prioritisation of accounts, it simply iterates from the beginning of the accounts database in sorted order and starts elections for ones that it can. The pessimistic elections are not set to active immediately.

(Unrelated) - Moved a comment I noticed was misplaced in confirmation_height_bounded.cpp

@wezrule wezrule self-assigned this Aug 18, 2020
@wezrule wezrule marked this pull request as draft August 18, 2020 18:12
@wezrule wezrule marked this pull request as ready for review August 19, 2020 13:24
@zhyatt zhyatt added this to the V21.2 milestone Aug 19, 2020
@zhyatt zhyatt added enhancement functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality labels Aug 19, 2020
@Srayman
Copy link
Contributor

Srayman commented Aug 19, 2020

It can sometimes take 2+ rounds to complete bootstrapping frontier confirmations as it's limited to just the top 30 reps each round. If I'm reading this correctly it now extends frontiers to 1 minute in between confirm_req that could extend the frontier confirmation process quite a bit for initial bootstrap. Also as the representatives become more decentralized the number of reps required for quorum will continue to increase. Right now on the live network the top 30 reps make up 66.59M weight which is quite close to the required minimum 60M. Even though the minimum will be adjusted #2884 to be lower and more dynamic there's still the risk of the required reps needing to be more than 30.

Potential ideas

  1. Allow 2 rounds before delaying to 1 minute intervals
  2. Increase max reps above 30
  3. Dynamically adjust max reps to account for the number that are required to reach 2/3 majority for example, to account for potential network or communication issues from some nodes.

@wezrule
Copy link
Contributor Author

wezrule commented Aug 19, 2020

@Srayman made some good points. After some discussion with him we decided that optimistic elections would do 5 second requests for 1 minute then once a minute until the 5 minute election expiration time is reached. Also will wait 5 seconds before starting pessimistic elections to allow nodes enough time to generate and publish votes if they're all moving forward with the same blocks.

@wezrule wezrule force-pushed the pessimistic_frontiers_confirmation branch from 116a105 to 4b9f88f Compare August 20, 2020 10:28
@wezrule wezrule force-pushed the pessimistic_frontiers_confirmation branch from 4b9f88f to 6a1f724 Compare August 20, 2020 10:32
@guilhermelawless guilhermelawless added documentation This item indicates the need for or supplies updated or expanded documentation and removed documentation This item indicates the need for or supplies updated or expanded documentation labels Aug 27, 2020
SergiySW
SergiySW previously approved these changes Aug 27, 2020
SergiySW
SergiySW previously approved these changes Aug 27, 2020
SergiySW
SergiySW previously approved these changes Aug 31, 2020
@guilhermelawless guilhermelawless dismissed stale reviews from SergiySW and themself via da80d14 September 2, 2020 09:06
# Conflicts:
#	nano/core_test/frontiers_confirmation.cpp
#	nano/core_test/ledger.cpp
#	nano/node/active_transactions.hpp
@guilhermelawless guilhermelawless merged commit 758d620 into nanocurrency:develop Sep 2, 2020
guilhermelawless added a commit that referenced this pull request Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants