Amendment table clean up (Improve amendment processing and activation logic) #3428

gregtatcam · 2020-06-02T12:47:28Z

Use base 10 for majority vote calculation
Add majority timer configuration
FIXES: #3396

Add majority timer configuration FIXES: XRPLF#3396

nbougalis

This looks good, but I left a couple of suggestions to request one change: do not allow the wait time to be specified as seconds in the config file and impose a 15-minute floor in the selected value. The other change is at your discretion.

nbougalis · 2020-06-02T16:33:50Z

src/ripple/core/impl/Config.cpp

+                "Invalid " SECTION_AMENDMENT_MAJORITY_TIME
+                ", must be: [0-9]+ [seconds|minutes|hours|days|weeks]");
+        std::uint32_t duration =
+            beast::lexicalCastThrow<std::uint32_t>(match[1].str());


I would not allow seconds: there's no real plausible scenario for such a short interval. In fact, I think I'd reject any interval smaller than 15 minutes.

nbougalis · 2020-06-02T16:43:38Z

src/ripple/app/main/Application.cpp

+static int const MAJORITY_FRACTION_OLD(204);
+// 8/10 = 80%
+static int const MAJORITY_FRACTION(8);


These values, along with the default value for the majority time should probably migrate into SystemParametersInfo.h:

/** The minimum amount of support an amendment should have. @note This value is used by legacy code and will become obsolete once the fix3396 amendment activates. */ constexpr int const amendmentMajorityFractionOld = 204; /** The minimum amount of support an amendment must have */ constexpr int const amendmentMajorityPercent = 80; /** The minimum amount of time an amendment must hold a majority */ constexpr std::chrono::seconds const defaultAmendmentMajorityTime = std::chrono::weeks{2};

Done, except for amendmentMajorityPercent, which is currently not used. Added instead amendmentMajorityFraction.

Make 15 minutes the min amendment majority time Move amendment related const to SystemParameters.h

nbougalis · 2020-06-02T19:03:27Z

src/ripple/core/impl/Config.cpp

            AMENDMENT_MAJORITY_TIME = minutes(duration);
        else if (boost::iequals(match[2], "hours"))
            AMENDMENT_MAJORITY_TIME = hours(duration);
        else if (boost::iequals(match[2], "days"))
            AMENDMENT_MAJORITY_TIME = days(duration);
        else if (boost::iequals(match[2], "weeks"))
            AMENDMENT_MAJORITY_TIME = weeks(duration);
+
+        if (AMENDMENT_MAJORITY_TIME < defaultAmendmentMajorityTime)


I think this check is wrong. It will prevent anything less than 2 weeks. I'd simply hard-code std::chrono::minutes(15) here.

scottschurr

I left some ideas for you to think about. All of them are open for discussion.

scottschurr · 2020-06-04T22:22:46Z

src/ripple/app/misc/AmendmentTable.h

+    int old_ = 0;
+    int new_ = 0;


I'm not keen on the names old_ and new_. If I'm reading the code correctly, the way the numbers are interpreted is significantly different. Perhaps...

old_ -> per256

new_ -> per10fix3396

Or perhaps you have a better idea for more descriptive names...

Also, in general, around this code base, if a member variable is public (like in struct MajorityFraction) then we don't put trailing underscores on it. The trailing underscores are usually reserved for private or protected data.

I think it's best to use std::ratio here:

struct AmendmentSupermajorityThreshold { std::ratio<204, 256> const pre3396; std::ratio<80, 100> const post3396; };

The nice thing is that std::ratio tells you exactly what's happening and reduces the that risk that someone changes the numerator, while leaving the denominator (which is currently hidden in AmendmentTableImpl) unchanged.

Of course, it's important to note that the compiler actually reduces std::ratio<204, 256> from 204/256 to 51/64 and std::ratio<80,100> from 80/100 to 4/5 but even with those reductions the arithmetic should be precise and match the existing math. See this gist.

scottschurr · 2020-06-04T23:06:24Z

src/ripple/app/misc/impl/AmendmentTable.cpp

@@ -148,8 +149,8 @@ class AmendmentTableImpl final : public AmendmentTable
    std::chrono::seconds const majorityTime_;

    // The amount of support that an amendment must receive
-    // 0 = 0% and 256 = 100%
-    int const majorityFraction_;
+    // 0 = 0% and 256 = 100%(old) 8 = 100%(new)


I think this comment is wrong? Doesn't the current implementation result in "10 = 100%(new) "?

That's right. But I think the comment with the arithmetic is unnecessary here, especially if we use the std::ratio thing above.

Removed the comment

scottschurr · 2020-06-05T19:15:48Z

src/ripple/app/misc/impl/AmendmentTable.cpp

-    vote->mThreshold =
-        std::max(1, (vote->mTrustedValidations * majorityFraction_) / 256);
+    vote->mThreshold = rules.enabled(ripple::fix3396)
+        ? std::max(1, (vote->mTrustedValidations * majorityFraction_.new_) / 10)


There are two different places where fix3396 comes into play. Here. And down on line 515 where a >= becomes a >.

I'd like to suggest consolidating those two places into a single place; here. You can do that by rounding up the division instead of doing the usual C/C++ truncate toward zero.

It's worth noting that this change, either way it is done, has non-trivial impact on how amendments behave on a network with a small number of validators. It means that with small numbers of trusted validators there must be 100% agreement before any amendment will pass. For example, if we require at least 80% agreement between validators, then with 2, 3, or 4 trusted validators there must be 100% agreement for any amendment to pass.

I agree that's the right call to make given the current state of the network.

I think by doing the rounding up here, rather than tweaking the comparison on line 515, you'll make the intent easier to understand for future maintainers. There are a couple of different ways to achieve rounding up. Here's one way to do it that avoids floating point:

auto amendmentThreshold = [this] (int trustedValidations) { std::div_t const d = std::div (trustedValidations * majorityFraction_.new_, 10); return std::clamp (d.quot + (d.rem > 0 ? 1 : 0), 1, trustedValidations); };

There are other ways to code it as well.

While we're here, I have a mild preference for using 100 as the divisor rather than 10. That would allow you to rename amendmentMajorityFraction = 8 to amendmentMajorityPercent = 80; which is way easier to read and understand.

I think people accustomed to western math will also recognize percentages much faster than they will recognize fractions of 10.

I appreciate the well-thought out comment (and agree with the 8 -> 80 change), but I find the division code confusing to read. Once I read it carefully, I understand what's happening, but I'm not confident it behaves identically pre-amendment. I am hoping we can do something even better:

We should push all this logic into AmendmentSet and add bool passes(uint256) const to its interface:

struct AmendmentSet { private: // How many yes votes each amendment received hash_map<uint256, int> votes_; // number of trusted validations int mTrustedValidations = 0; // number of votes needed int mThreshold; Rules const& rules; public: AmendmentSet( Rules const& rules, MajorityFraction const& majorityFraction, std::vector<std::shared_ptr<STValidation>> const& valSet) { for (auto const& val : valSet) { if (val->isTrusted()) { std::set<uint256> ballot; if (val->isFieldPresent(sfAmendments)) { auto const choices = val->getFieldV256(sfAmendments); ballot.insert(choices.begin(), choices.end()); } ++mTrustedValidations; for (auto const& amendment : ballot) ++votes_[amendment]; } } mThreshold = rules.enabled(fix3396) ? std::max(1, (vote->mTrustedValidations * majorityFraction.pre3396.num) / majorityFraction.pre3396.den) : std::max(1, (vote->mTrustedValidations * majorityFraction.post3396.num) / majorityFraction.post3396.den); } bool passes(uint256 const& amendment) const { auto const& it = votes_.find(amendment); if (it == votes_.end()) return false; // Before this fix, it was possible for an amendment to activate with a percentage // slightly less than 80% because we compared for "greater than or equal to" // instead of strictly "greater than". if (!rules.enabled(fix3396)) return it->second >= fix3396; return it->second > fix3396; } ... other stuff };

scottschurr · 2020-06-05T19:59:09Z

src/ripple/app/misc/impl/AmendmentTable.cpp

@@ -148,8 +149,8 @@ class AmendmentTableImpl final : public AmendmentTable
    std::chrono::seconds const majorityTime_;


I can't comment on a line that hasn't been modified. And I know this is not your doing. I'm looking at the protected: on line 141. AmendmentTableImpl is final, yes? So protected data really doesn't make much sense. Perhaps you can make that private while you're in here?

scottschurr · 2020-06-05T20:18:32Z

src/ripple/app/misc/impl/AmendmentTable.cpp

+        bool const hasValMajority = rules.enabled(ripple::fix3396)
+            ? (vote->votes(entry.first) > vote->mThreshold)
+            : (vote->votes(entry.first) >= vote->mThreshold);


If you go with rounding up when calculating the threshold, then this change should be reverted.

Updated per Nik's suggestion of pushing the logic into AmendmentSet.

scottschurr · 2020-06-05T21:22:41Z

src/test/app/AmendmentTable_test.cpp

            {
-                if ((256 * i) < (validators.size() * amendment.second))
+                if (auto yes = enabled3396() ? nVotes >= i : nVotes > i; yes)


I think you can lose yes. The following is working for me:

if (enabled3396() ? nVotes >= i : nVotes > i)

scottschurr · 2020-06-05T21:27:24Z

src/test/app/AmendmentTable_test.cpp

@@ -630,7 +643,7 @@ class AmendmentTable_test final : public beast::unit_test::suite
                enabled,
                majority);

-            if (i < 13)
+            if (i < 13)  // 13 => 13/16 = 0.8125 => > 80%


Thanks for adding this comment. It helps.

scottschurr · 2020-06-05T22:25:06Z

src/test/app/AmendmentTable_test.cpp

@@ -788,6 +802,9 @@ class AmendmentTable_test final : public beast::unit_test::suite
        testDetectMajority();
        testLostMajority();
        testHasUnsupported();
+        enable3396(true);
+        testDetectMajority();
+        testLostMajority();


I'd like to suggest a different way of enabling and disabling the amendment you want to test with/without. Here are a couple of unit tests files that have a similar problem:

https://github.com/ripple/rippled/blob/develop/src/test/app/Offer_test.cpp#L5155-L5227

https://github.com/ripple/rippled/blob/develop/src/test/app/Flow_test.cpp#L1354-L1388

These tests, and other similar tests in the code base, pass the amendment of interest into the test as a parameter. Those of us who structure our tests this way feel like it's easier to understand and maintain than if the current amendments are kept in a member variable of the test.

If you were to take this model, then your test file might change to look something like this:

void testAll (uint256 const& feat) { testConstruct(feat); testGet(feat); testBadConfig(feat); testEnableVeto(feat); testNoOnUnknown(feat); testNoOnVetoed(feat); testVoteEnable(feat); testDetectMajority(feat); testLostMajority(feat); testHasUnsupported(feat); } void run() override { testAll ({}); testAll (fix3396); }

And doRound might look like this:

// Execute a pretend consensus round for a flag ledger void doRound( uint256 const& feat, AmendmentTable& table, ... if (feat == fix3396 ? nVotes >= i : nVotes > i) ... auto actions = table.doVoting( Rules({feat}), roundTime, enabled, majority, validations); ...

I think this change might improve the maintainability of your tests. Thanks for listening.

scottschurr · 2020-06-05T23:55:43Z

src/test/core/Config_test.cpp

+        testcase("amendment");
+
+        std::vector<std::pair<std::string, std::uint32_t>> units = {
+            {"seconds", 1},


The good news is that this test now fails for

seconds because the unit is not supported and

minutes because 10 minutes is too short.

I suggest you add those to the tests that throw.

You might also want to put in a test...

at precisely 15 minutes (see it work) and another

at 15 minutes minus one second (see it throw).

scottschurr · 2020-06-06T00:52:10Z

src/test/app/AmendmentTable_test.cpp

-            if (i < 8)
-            {
+            if (i < 4)  // 16 - 3 = 13 => 13/16 = 0.8125 => > 80%
+            {           // 16 - 4 = 12 => 12/16 = 0.75 => < 80%


Nice comment.

Replace majority fraction with std::ratio Reference super majority threshold constant directly in AmendmentTable instead of passing majority fraction into constructor Rename fix3396 feature to fixAmendmentMajorityCalc Add test config test cases Refactor tests with/without amendment feature

nbougalis

A couple of μNits to address, but overall looks good. Thanks.

src/ripple/protocol/SystemParameters.h

src/ripple/core/Config.h

src/ripple/protocol/SystemParameters.h

Fix super majority threshold typo 205 -> 204

nbougalis · 2020-06-13T18:25:02Z

Weird unit test failures happening...

gregtatcam · 2020-06-13T19:03:18Z

Weird unit test failures happening...

I have been testing AmendmentTable only. It does fail on other tests. I'll take a look.

gregtatcam · 2020-06-14T00:53:52Z

Weird unit test failures happening...

I have been testing AmendmentTable only. It does fail on other tests. I'll take a look.

The problem is that in some tests (for inst Feature_test::testWithMajorities()), we have one trusted validator and one vote. Consequently the threshold is max(1, 1*80/100) = 1. And because of the change for post 3396 fix the votes should be greater than the threshold, which is false and the test fails. Pre 3396 fix, the votes should be greater or equal to the threshold, which was true and the test succeeded. I'm investigating how to address this issue in the unit tests.

nbougalis

👍

ximinez

Veto: Unit test failures in CI. I have not investigated further

1485.6s, 185 suites, 1188 cases, 632056 tests total, 1 failure

nbougalis · 2020-06-19T02:01:08Z

@gregtatcam, please cherry-pick 2bc3898 to remove the "per-256" reporting in the feature RPC call. This is nominally an API change (removing the vote) response, but it's not documented and the value (a) is confusing and (b) can be derived from the other information returned.

nbougalis · 2020-06-19T02:22:56Z

The issue identified by @ximinez can be fixed by cherry-picking c228f9f

Fix Windows compile Remove per-256 response in Feature rpc test

scottschurr

👍 Looks good. I left two additional suggestions, but I leave them to your discretion.

scottschurr · 2020-06-22T19:33:35Z

src/ripple/app/misc/impl/AmendmentTable.cpp

@@ -97,25 +98,73 @@ struct AmendmentSet
 private:
    // How many yes votes each amendment received
    hash_map<uint256, int> votes_;
+    Rules const& rules_;


I like the way that the decision making code has moved into AmendmentSet. However I find it a bit disorienting to read AmendmentSet in its current hybrid struct/class form. I suggest that you push AmendmentSet all the way over to being a proper class.

Declare it as a class, not a struct.

Make member variables mTrustedValidations and mThreshold private and move them up to the top of the class declaration next to votes_ and rules_.

Rename mTrustedValidations and mThreshold to trustedValidations_ and threshold_ respectively.

Provide const accessor methods trustedValidations() and threshold().

Fix up the few places in this file that were directly referencing mTrustedValidations and mThreshold to use the newly added accessor methods.

These changes don't affect functionality, they just make the class more self consistent and easier to comprehend IMO. So your call.

scottschurr · 2020-06-22T20:19:10Z

src/ripple/protocol/SystemParameters.h

+constexpr std::ratio<204, 256> amendmentSuperMajorityThresholdPre3396;
+
+constexpr std::ratio<80, 100> amendmentSuperMajorityThresholdPost3396;


Now that the fix has been renamed these names don't make as much sense as they used to. There aren't any other mentions of 3396 in the source code. I don't have any great suggestions, but maybe...

preFixAmendmentMajorityCalcThreshold and postFixAmendmentMajorityCalcThreshold or

preFixAmendmentMajorityCalcSuperMajority and postFixAmendmentMajorityCalcSuperMajority

would be a little easier to figure out. You may have better ideas than these.

- make AmendmentSet a class - change amendmentSuperMajorityThreshold[Pre,Post]3396 to [pre,post]FixAmendmentMajorityCaltThreshold

scottschurr

👍

intelliot · 2023-08-04T17:39:01Z

Released in 1.6.0 https://xrpl.org/blog/2020/rippled-1.6.0.html

Amendment activated in April 2021

Use base 10 for majority vote calculation

29b74b2

Add majority timer configuration FIXES: XRPLF#3396

gregtatcam assigned scottschurr and nbougalis Jun 2, 2020

nbougalis suggested changes Jun 2, 2020

View reviewed changes

gregtatcam added 2 commits June 2, 2020 14:53

Remove seconds option from amendment majority time configuration

7d90194

Make 15 minutes the min amendment majority time Move amendment related const to SystemParameters.h

Fix clang

492272c

nbougalis reviewed Jun 2, 2020

View reviewed changes

Fix min majority time config

7482d74

scottschurr reviewed Jun 6, 2020

View reviewed changes

nbougalis suggested changes Jun 13, 2020

View reviewed changes

src/ripple/protocol/SystemParameters.h Outdated Show resolved Hide resolved

src/ripple/core/Config.h Outdated Show resolved Hide resolved

src/ripple/protocol/SystemParameters.h Show resolved Hide resolved

Include <chrono> in Config.h

63370d1

Fix super majority threshold typo 205 -> 204

Fix amendment threshold calculation

e0f516f

Fix Amendment calculation with one trusted validator

9f6aefc

nbougalis approved these changes Jun 18, 2020

View reviewed changes

ximinez requested changes Jun 18, 2020

View reviewed changes

Fix amendment majority time Config unit test

f3c752c

Fix Windows compile Remove per-256 response in Feature rpc test

carlhua requested a review from ximinez June 22, 2020 14:17

ximinez approved these changes Jun 22, 2020

View reviewed changes

ximinez self-requested a review June 22, 2020 17:24

carlhua requested a review from scottschurr June 22, 2020 18:50

scottschurr approved these changes Jun 22, 2020

View reviewed changes

carlhua added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 23, 2020

gregtatcam added 3 commits June 23, 2020 12:07

Refactor:

2c429dd

- make AmendmentSet a class - change amendmentSuperMajorityThreshold[Pre,Post]3396 to [pre,post]FixAmendmentMajorityCaltThreshold

Clang fix

69ea2d4

Clang fix

d7cb90c

scottschurr approved these changes Jun 23, 2020

View reviewed changes

This was referenced Jun 25, 2020

Proposed 1.6.0-b8 #3459

Closed

Proposed 1.6.0-b8 #3461

Closed

Proposed 1.6.0-b8 #3462

Closed

Proposed 1.6.0-b8 #3483

Merged

manojsdoshi closed this in #3483 Jun 30, 2020

intelliot changed the title ~~Amendment table clean up~~ Amendment table clean up (Improve amendment processing and activation logic) Feb 2, 2021

		@@ -148,8 +149,8 @@ class AmendmentTableImpl final : public AmendmentTable
		std::chrono::seconds const majorityTime_;

		constexpr std::ratio<204, 256> amendmentSuperMajorityThresholdPre3396;

		constexpr std::ratio<80, 100> amendmentSuperMajorityThresholdPost3396;

Amendment table clean up (Improve amendment processing and activation logic) #3428

Amendment table clean up (Improve amendment processing and activation logic) #3428

Conversation

gregtatcam commented Jun 2, 2020

nbougalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis Jun 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

nbougalis commented Jun 13, 2020

gregtatcam commented Jun 13, 2020

gregtatcam commented Jun 14, 2020

nbougalis left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

nbougalis commented Jun 19, 2020 • edited Loading

nbougalis commented Jun 19, 2020

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

intelliot commented Aug 4, 2023

nbougalis Jun 2, 2020 •

edited

Loading

nbougalis Jun 6, 2020 •

edited

Loading

nbougalis commented Jun 19, 2020 •

edited

Loading