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

EnableAmendment timer starts at 77.78% (Version: [rippled 1.5.0]) #3396

Closed
gnurag opened this issue May 11, 2020 · 1 comment
Closed

EnableAmendment timer starts at 77.78% (Version: [rippled 1.5.0]) #3396

gnurag opened this issue May 11, 2020 · 1 comment
Assignees
Labels
Documentation README changes, code comments, etc.

Comments

@gnurag
Copy link

gnurag commented May 11, 2020

Issue Description

I may be reading this incorrectly - but I think the 14 day timer to enable an amendment starts at 77.78% (28 Ayes out of 36 as of today). 28 Ayes of 36 is only 77.78% agreement. Amendment process doc says this must be 80% or above.

Observed this behavior with RequireFullyCanonicalSig amendment which is hovering around 27 Ayes at the moment. Once it achieved 28 Ayes (=77.78% agreement), the validator inserted an EnableAmendment transaction and the 14 day timer started.

fullycanonicalsig-vote

Steps to Reproduce

Run:

# /opt/ripple/bin/rippled feature

on the validator when an amendment has 28 Ayes out of 36.

Expected Result

  1. EnableAmendment transaction is not inserted into the ledger.
  2. majority property is not set.

Actual Result

  1. EnableAmendment transaction is inserted into the ledger.
  2. majority property is set (see screenshot above).

Environment

rippled-1.5.0

Supporting Files

fullycanonicalsig-votes

@nbougalis
Copy link
Contributor

nbougalis commented May 13, 2020

Thanks for opening an issue. You aren't reading it incorrectly.

A couple of people had already reached out to me about the DeletableAccounts amendment, wondering how come it reached majority, since 28 validators seem to support it but (a) the consensus quorum is presently 29 and (b) 28/36 is strictly less than 80%, which is the amendment activation threshold, so I already had a chance to triage this..

The answer boils down to two things:

First, for historical and largely obsolete reasons, the calculation for the majority is done in base 256, with the quorum threshold set to (V * 204)/256 where V is the total number of trusted validations. There are currently 36 validators on Ripple's recommended UNL, so I am assuming a server running with a UNL with a size of 36 for this example, but similar calculations apply for other sizes:

So the code calculates (36 * 204) / 256. Ref:

vote->mThreshold = std::max(1,
    (vote->mTrustedValidations * majorityFraction_) / 256);

JLOG (j_.debug()) <<
    "Received " << vote->mTrustedValidations <<
    " trusted validations, threshold is: " << vote->mThreshold;

Now, if you plug that into a calculator you'll see it works out to 28.6875. So what's wrong?

The code is using integer arithmetic, which "truncates toward zero’’. Basically, you can think of it as "rounding down to the nearest integer" and what is the nearest integer that is strictly smaller? It's 28. For a little deeper dive into this check out this excellent StackOverflow answer.

The second, problem is that the code checks whether an amendment has achieved the required quorum by using greater than or equal to instead of strictly greater than. Ref:

bool const hasValMajority =
    (vote->votes (entry.first) >= vote->mThreshold);

While the behavior of the code is well-defined in terms of C/C++, it's not ideal from a human perspective and certainly complicates the understanding of quorum when it comes to amendments.

I believe that this issue needs to be addressed in a future release of rippled, preferably with the 1.6.0 release. Ironically, the fix will require an amendment. 😅

@carlhua carlhua added the Documentation README changes, code comments, etc. label May 14, 2020
gregtatcam added a commit to gregtatcam/rippled that referenced this issue Jun 2, 2020
Add majority timer configuration
FIXES: XRPLF#3396
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this issue Jun 24, 2020
 *Add majority timer configuration
 *FIXES: XRPLF#3396
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this issue Jun 25, 2020
 * The amendment ballot counting code contained a minor technical flaw, caused by the use of integer
   arithmetic and rounding semantics, that could allow amendments to reach majority with slightly less
   than 80% support. This commit introduces an amendment which, if enabled, will ensure that activation
   requires at least 80% support.

 * This commit also introduces a configuration option to adjust the amendment activation hysteresis. This
   option is useful on test networks, but should not be used on the main network as is a network-wide
   consensus parameter that should not be changed on a per-server basis; doing so can result in a hard-fork.

 * Fixes XRPLF#3396
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this issue Jun 25, 2020
 * The amendment ballot counting code contained a minor technical flaw, caused by the use of integer arithmetic and rounding semantics, that could allow amendments to reach majority with slightly less than 80% support. This commit introduces an amendment which, if enabled, will ensure that activation requires at least 80% support.

 * This commit also introduces a configuration option to adjust the amendment activation hysteresis. This option is useful on test networks, but should not be used on the main network as is a network-wide consensus parameter that should not be changed on a per-server basis; doing so can result in a hard-fork.

  * Fixes XRPLF#3396
manojsdoshi referenced this issue in manojsdoshi/rippled Jun 25, 2020
Improve amendment processing and activation logic:
* The amendment ballot counting code contained a minor technical
  flaw, caused by the use of integer arithmetic and rounding
  semantics, that could allow amendments to reach majority with
  slightly less than 80% support. This commit introduces an
  amendment which, if enabled, will ensure that activation
  requires at least 80% support.

* This commit also introduces a configuration option to adjust
  the amendment activation hysteresis. This option is useful on
  test networks, but should not be used on the main network as
  is a network-wide consensus parameter that should not be
  changed on a per-server basis; doing so can result in a
  hard-fork.

* Fixes ripple#3396
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation README changes, code comments, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants