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.

Sign up for GitHub

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

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

Closed
wants to merge 12 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
29b74b2
Use base 10 for majority vote calculation
gregtatcam May 18, 2020
7d90194
Remove seconds option from amendment majority time configuration
gregtatcam Jun 2, 2020
492272c
Fix clang
gregtatcam Jun 2, 2020
7482d74
Fix min majority time config
gregtatcam Jun 2, 2020
5e989ec
Move majority threshold calc into AmendmentSet
gregtatcam Jun 11, 2020
63370d1
Include <chrono> in Config.h
gregtatcam Jun 13, 2020
e0f516f
Fix amendment threshold calculation
gregtatcam Jun 14, 2020
9f6aefc
Fix Amendment calculation with one trusted validator
gregtatcam Jun 15, 2020
f3c752c
Fix amendment majority time Config unit test
gregtatcam Jun 20, 2020
2c429dd
Refactor:
gregtatcam Jun 23, 2020
69ea2d4
Clang fix
gregtatcam Jun 23, 2020
d7cb90c
Clang fix
gregtatcam Jun 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/ripple/app/main/Application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@
namespace ripple {

// 204/256 about 80%
static int const MAJORITY_FRACTION(204);
static int const MAJORITY_FRACTION_OLD(204);
// 8/10 = 80%
static int const MAJORITY_FRACTION(8);
Copy link
Contributor

@nbougalis nbougalis Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


//------------------------------------------------------------------------------

Expand Down Expand Up @@ -1533,8 +1535,8 @@ ApplicationImp::setup()
Section enabledAmendments = config_->section(SECTION_AMENDMENTS);

m_amendmentTable = make_AmendmentTable(
weeks{2},
MAJORITY_FRACTION,
config().AMENDMENT_MAJORITY_TIME,
{MAJORITY_FRACTION_OLD, MAJORITY_FRACTION},
supportedAmendments,
enabledAmendments,
config_->section(SECTION_VETO_AMENDMENTS),
Expand Down
10 changes: 9 additions & 1 deletion src/ripple/app/misc/AmendmentTable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@

namespace ripple {

struct MajorityFraction
{
int old_ = 0;
int new_ = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

};

/** The amendment table stores the list of enabled and potential amendments.
Individuals amendments are voted on by validators during the consensus
process.
Expand Down Expand Up @@ -99,6 +105,7 @@ class AmendmentTable
// inject pseudo-transactions
virtual std::map<uint256, std::uint32_t>
doVoting(
Rules const& rules,
NetClock::time_point closeTime,
std::set<uint256> const& enabledAmendments,
majorityAmendments_t const& majorityAmendments,
Expand Down Expand Up @@ -130,6 +137,7 @@ class AmendmentTable
{
// Ask implementation what to do
auto actions = doVoting(
lastClosedLedger->rules(),
lastClosedLedger->parentCloseTime(),
getEnabledAmendments(*lastClosedLedger),
getMajorityAmendments(*lastClosedLedger),
Expand Down Expand Up @@ -164,7 +172,7 @@ class AmendmentTable
std::unique_ptr<AmendmentTable>
make_AmendmentTable(
std::chrono::seconds majorityTime,
int majorityFraction,
MajorityFraction const& majorityFraction,
Section const& supported,
Section const& enabled,
Section const& vetoed,
Expand Down
26 changes: 16 additions & 10 deletions src/ripple/app/misc/impl/AmendmentTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <ripple/app/misc/AmendmentTable.h>
#include <ripple/core/ConfigSections.h>
#include <ripple/core/DatabaseCon.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/STValidation.h>
#include <ripple/protocol/TxFlags.h>
#include <ripple/protocol/jss.h>
Expand Down Expand Up @@ -148,8 +149,8 @@ class AmendmentTableImpl final : public AmendmentTable
std::chrono::seconds const majorityTime_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


// 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the comment

MajorityFraction const majorityFraction_;

// The results of the last voting round - may be empty if
// we haven't participated in one yet.
Expand Down Expand Up @@ -187,7 +188,7 @@ class AmendmentTableImpl final : public AmendmentTable
public:
AmendmentTableImpl(
std::chrono::seconds majorityTime,
int majorityFraction,
MajorityFraction const& majorityFraction,
Section const& supported,
Section const& enabled,
Section const& vetoed,
Expand Down Expand Up @@ -237,6 +238,7 @@ class AmendmentTableImpl final : public AmendmentTable

std::map<uint256, std::uint32_t>
doVoting(
Rules const& rules,
NetClock::time_point closeTime,
std::set<uint256> const& enabledAmendments,
majorityAmendments_t const& majorityAmendments,
Expand All @@ -247,7 +249,7 @@ class AmendmentTableImpl final : public AmendmentTable

AmendmentTableImpl::AmendmentTableImpl(
std::chrono::seconds majorityTime,
int majorityFraction,
MajorityFraction const& majorityFraction,
Section const& supported,
Section const& enabled,
Section const& vetoed,
Expand All @@ -258,7 +260,7 @@ AmendmentTableImpl::AmendmentTableImpl(
, unsupportedEnabled_(false)
, j_(journal)
{
assert(majorityFraction_ != 0);
assert(majorityFraction_.old_ != 0 && majorityFraction_.new_ != 0);

std::lock_guard sl(mutex_);

Expand Down Expand Up @@ -461,6 +463,7 @@ AmendmentTableImpl::getDesired() const

std::map<uint256, std::uint32_t>
AmendmentTableImpl::doVoting(
Rules const& rules,
NetClock::time_point closeTime,
std::set<uint256> const& enabledAmendments,
majorityAmendments_t const& majorityAmendments,
Expand Down Expand Up @@ -489,8 +492,10 @@ AmendmentTableImpl::doVoting(
}
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@nbougalis nbougalis Jun 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

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

JLOG(j_.debug()) << "Received " << vote->mTrustedValidations
<< " trusted validations, threshold is: "
Expand All @@ -507,8 +512,9 @@ AmendmentTableImpl::doVoting(
{
NetClock::time_point majorityTime = {};

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


{
auto const it = majorityAmendments.find(entry.first);
Expand Down Expand Up @@ -666,7 +672,7 @@ AmendmentTableImpl::getJson(uint256 const& amendmentID) const
std::unique_ptr<AmendmentTable>
make_AmendmentTable(
std::chrono::seconds majorityTime,
int majorityFraction,
MajorityFraction const& majorityFraction,
Section const& supported,
Section const& enabled,
Section const& vetoed,
Expand Down
4 changes: 4 additions & 0 deletions src/ripple/core/Config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <ripple/basics/BasicConfig.h>
#include <ripple/basics/FeeUnits.h>
#include <ripple/basics/base_uint.h>
#include <ripple/basics/chrono.h>
nbougalis marked this conversation as resolved.
Show resolved Hide resolved
#include <ripple/beast/net/IPEndpoint.h>
#include <ripple/beast/utility/Journal.h>
#include <ripple/protocol/SystemParameters.h> // VFALCO Breaks levelization
Expand Down Expand Up @@ -172,6 +173,9 @@ class Config : public BasicConfig
// Compression
bool COMPRESSION = false;

// Amendment majority time
std::chrono::seconds AMENDMENT_MAJORITY_TIME = weeks{2};

// Thread pool configuration
std::size_t WORKERS = 0;

Expand Down
1 change: 1 addition & 0 deletions src/ripple/core/ConfigSections.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ struct ConfigSection
#define SECTION_INSIGHT "insight"
#define SECTION_IPS "ips"
#define SECTION_IPS_FIXED "ips_fixed"
#define SECTION_AMENDMENT_MAJORITY_TIME "amendment_majority_time"
#define SECTION_NETWORK_QUORUM "network_quorum"
#define SECTION_NODE_SEED "node_seed"
#define SECTION_NODE_SIZE "node_size"
Expand Down
25 changes: 25 additions & 0 deletions src/ripple/core/impl/Config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,31 @@ Config::loadFromString(std::string const& fileContents)
if (getSingleSection(secConfig, SECTION_COMPRESSION, strTemp, j_))
COMPRESSION = beast::lexicalCastThrow<bool>(strTemp);

if (getSingleSection(
secConfig, SECTION_AMENDMENT_MAJORITY_TIME, strTemp, j_))
{
using namespace std::chrono;
boost::regex const re(
"^\\s*(\\d+)\\s*(seconds|minutes|hours|days|weeks)\\s*(\\s+.*)?$");
boost::smatch match;
if (!boost::regex_match(strTemp, match, re))
Throw<std::runtime_error>(
"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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

if (boost::iequals(match[2], "seconds"))
AMENDMENT_MAJORITY_TIME = seconds(duration);
else if (boost::iequals(match[2], "minutes"))
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);
}

// Do not load trusted validator configuration for standalone mode
if (!RUN_STANDALONE)
{
Expand Down
4 changes: 3 additions & 1 deletion src/ripple/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ class FeatureCollections
"RequireFullyCanonicalSig",
"fix1781", // XRPEndpointSteps should be included in the circular
// payment check
"HardenedValidations"};
"HardenedValidations",
"fix3396"}; // Fix Amendment majority calculation

std::vector<uint256> features;
boost::container::flat_map<uint256, std::size_t> featureToIndex;
Expand Down Expand Up @@ -367,6 +368,7 @@ extern uint256 const fixQualityUpperBound;
extern uint256 const featureRequireFullyCanonicalSig;
extern uint256 const fix1781;
extern uint256 const featureHardenedValidations;
extern uint256 const fix3396;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest renaming this to fixAmendmentMajorityCalc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


} // namespace ripple

Expand Down
6 changes: 4 additions & 2 deletions src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ detail::supportedAmendments()
"fixQualityUpperBound",
"RequireFullyCanonicalSig",
"fix1781",
"HardenedValidations"};
"HardenedValidations",
"fix3396"};
return supported;
}

Expand Down Expand Up @@ -181,7 +182,8 @@ uint256 const
fixQualityUpperBound = *getRegisteredFeature("fixQualityUpperBound"),
featureRequireFullyCanonicalSig = *getRegisteredFeature("RequireFullyCanonicalSig"),
fix1781 = *getRegisteredFeature("fix1781"),
featureHardenedValidations = *getRegisteredFeature("HardenedValidations");
featureHardenedValidations = *getRegisteredFeature("HardenedValidations"),
fix3396 = *getRegisteredFeature("fix3396");

// The following amendments have been active for at least two years. Their
// pre-amendment code has been removed and the identifiers are deprecated.
Expand Down
Loading