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

Confirm delta #2884

Merged
merged 12 commits into from
Nov 6, 2020
Merged

Confirm delta #2884

merged 12 commits into from
Nov 6, 2020

Conversation

clemahieu
Copy link
Contributor

online_weight_minimum is already considered in the calculation of node::delta. Using online_weight_minimum directly can cause quorum requirements to be unnecessarily high as actual online vote weight approaches online_weight_minimum.

Replacing references to online_weight_minimum with node:delta and adding tests to ensure rollbacks and confirmation is done precisely on the correct conditions.

…e::delta. Using online_weight_minimum directly can cause quorum requirements to be unnecessarily high as the online_weight_minimum is approached.

Replacing references to online_weight_minimum with node:delta and adding tests to ensure rollbacks and confirmation is done precisely on the correct conditions.
…e::delta. Using online_weight_minimum directly can cause quorum requirements to be unnecessarily high as actual online vote weight approaches online_weight_minimum.

Replacing references to online_weight_minimum with node:delta and adding tests to ensure rollbacks and confirmation is done precisely on the correct conditions.
@guilhermelawless
Copy link
Contributor

Spots that might also need a change (or at least can be optimized) as part of this:

  • peers_stake_required in the confirmation_quorum RPC response
  • gap_cache::bootstrap_check could possibly use this threshold and not online weight minimum
  • Similarly for active_transactions::inactive_votes_bootstrap_check , for legacy bootstrap when lazy is disabled

…ferences to delta (), which already considers the online_weight_minimum.
@Srayman
Copy link
Contributor

Srayman commented Aug 18, 2020

This handles the case where online weight is reduced and approaches the minimum, but what happens in the opposite scenario where a large rep comes back online or a large pending balance is received? The peers_stake would increase immediately but online_weight is trended over 2 weeks so there would be a lag before delta is updated to account for the new weight.

Should delta be the greater of 50% of online weight or 50% of peers stake to account for the rapid increase in peers weight? As the weight is reduced delta would slowly go down, but it would respond more dramatically to a rapid increase in weight since peers weight is real-time.

Alternatively modify the online weight process to move up quickly if the weight is increased but down slowly over 2 weeks if it decreases.

@zhyatt zhyatt added this to the V22.0 milestone Aug 18, 2020
@zhyatt zhyatt added the functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality label Aug 18, 2020
Moving delta on to online_reps class so all network weight calculations are done in one place.
# Conflicts:
#	nano/core_test/confirmation_height.cpp
#	nano/node/active_transactions.cpp
@clemahieu
Copy link
Contributor Author

This handles the case where online weight is reduced and approaches the minimum, but what happens in the opposite scenario where a large rep comes back online or a large pending balance is received? The peers_stake would increase immediately but online_weight is trended over 2 weeks so there would be a lag before delta is updated to account for the new weight.

Should delta be the greater of 50% of online weight or 50% of peers stake to account for the rapid increase in peers weight? As the weight is reduced delta would slowly go down, but it would respond more dramatically to a rapid increase in weight since peers weight is real-time.

Alternatively modify the online weight process to move up quickly if the weight is increased but down slowly over 2 weeks if it decreases.

@Srayman Added some new changes to address this issue.

@zhyatt zhyatt requested a review from wezrule October 6, 2020 18:47
# Conflicts:
#	nano/node/election.hpp
#	nano/node/online_reps.cpp
#	nano/node/online_reps.hpp
nano/node/online_reps.cpp Outdated Show resolved Hide resolved
nano/node/vote_processor.cpp Outdated Show resolved Hide resolved
nano/node/online_reps.cpp Outdated Show resolved Hide resolved
@SergiySW SergiySW self-requested a review November 5, 2020 15:06
nano/node/online_reps.cpp Outdated Show resolved Hide resolved
Generating timestamp within mutex to avoid issues of delays.
Removing reference to unused header.
@clemahieu clemahieu merged commit 932ee97 into develop Nov 6, 2020
@wezrule wezrule added breaking Change to node APIs (separate label) which impacts existing implementation, integrations impacted. rpc Changes related to Remote Procedure Calls labels Nov 10, 2020
SergiySW added a commit to SergiySW/raiblocks that referenced this pull request Nov 17, 2020
PR nanocurrency#2884 was originally intended to include changes to the default quorum value from 50% to 67%. This update includes this change.

Previously the online weight minimum of 60M was being applied as the floor for the number of votes required for quorum, but this caused the actual quorum % required to increase as online weight dropped towards that minimum 60M instead of staying consistent. With nanocurrency#2884 the quorum % is consistent regardless of online weight and thus the more online weight there is, the higher the vote amounts required. In order to provide similar security on the network prior to nanocurrency#2884, this update sets a 67% default value for quorum.

Given current online weight of 92M on the main network, this 67% requirements equates to ~61.6M votes required, just above the previous 60M, so little impact to confirmation times is expected.
argakiig pushed a commit that referenced this pull request Nov 23, 2020
commit f4f487e
Author: Sergey Kroshnin <sergiysw@gmail.com>
Date:   Wed Nov 18 01:39:51 2020 +0300

    Remove inline variable from cpp file

commit 9a37835
Author: Sergey Kroshnin <sergiysw@gmail.com>
Date:   Tue Nov 17 20:25:34 2020 +0300

    Update online_weight_quorum default to 67

    PR #2884 was originally intended to include changes to the default quorum value from 50% to 67%. This update includes this change.

    Previously the online weight minimum of 60M was being applied as the floor for the number of votes required for quorum, but this caused the actual quorum % required to increase as online weight dropped towards that minimum 60M instead of staying consistent. With #2884 the quorum % is consistent regardless of online weight and thus the more online weight there is, the higher the vote amounts required. In order to provide similar security on the network prior to #2884, this update sets a 67% default value for quorum.

    Given current online weight of 92M on the main network, this 67% requirements equates to ~61.6M votes required, just above the previous 60M, so little impact to confirmation times is expected.
argakiig pushed a commit that referenced this pull request Jan 5, 2021
* Update online_weight_quorum default to 67

PR #2884 was originally intended to include changes to the default quorum value from 50% to 67%. This update includes this change.

Previously the online weight minimum of 60M was being applied as the floor for the number of votes required for quorum, but this caused the actual quorum % required to increase as online weight dropped towards that minimum 60M instead of staying consistent. With #2884 the quorum % is consistent regardless of online weight and thus the more online weight there is, the higher the vote amounts required. In order to provide similar security on the network prior to #2884, this update sets a 67% default value for quorum.

Given current online weight of 92M on the main network, this 67% requirements equates to ~61.6M votes required, just above the previous 60M, so little impact to confirmation times is expected.

* Remove inline variable from cpp file
@zhyatt zhyatt added the documentation This item indicates the need for or supplies updated or expanded documentation label May 12, 2021
@zhyatt zhyatt deleted the confirm_delta branch September 21, 2021 21:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
breaking Change to node APIs (separate label) which impacts existing implementation, integrations impacted. documentation This item indicates the need for or supplies updated or expanded documentation functionality quality improvements This item indicates the need for or supplies a better way to implement existing functionality rpc Changes related to Remote Procedure Calls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants