Skip to content

Commit

Permalink
Revert "fix: stabilize voting threshold for amendment majority mechan…
Browse files Browse the repository at this point in the history 
…ism (#4410)"

This reverts commit 2bb8de0.

Manual testing showed some unexpected behavior.  We're removing the
commit since the release date is close and fixing the problem is
risky.  A fixed commit will be put into a later release.
  • Loading branch information
@scottschurr
scottschurr committed Nov 3, 2023
1 parent 09e0f10 commit b28286b
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 511 deletions.
3 changes: 0 additions & 3 deletions src/ripple/app/main/Application.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1333,9 +1333,6 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline)
<< "Invalid entry in [" << SECTION_VALIDATOR_LIST_SITES << "]";
return false;
}

// Tell the AmendmentTable who the trusted validators are.
m_amendmentTable->trustChanged(validators_->getQuorumKeys().second);
}
//----------------------------------------------------------------------
//
Expand Down
4 changes: 0 additions & 4 deletions src/ripple/app/misc/AmendmentTable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,6 @@ class AmendmentTable
std::set<uint256> const& enabled,
majorityAmendments_t const& majority) = 0;

// Called when the set of trusted validators changes.
virtual void
trustChanged(hash_set<PublicKey> const& allTrusted) = 0;

// Called by the consensus code when we need to
// inject pseudo-transactions
virtual std::map<uint256, std::uint32_t>
Expand Down
5 changes: 0 additions & 5 deletions src/ripple/app/misc/NetworkOPs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1853,12 +1853,7 @@ NetworkOPsImp::beginConsensus(uint256 const& networkClosed)
app_.getHashRouter());

if (!changes.added.empty() || !changes.removed.empty())
{
app_.getValidations().trustChanged(changes.added, changes.removed);
// Update the AmendmentTable so it tracks the current validators.
app_.getAmendmentTable().trustChanged(
app_.validators().getQuorumKeys().second);
}

mConsensus.startRound(
app_.timeKeeper().closeTime(),
Expand Down
213 changes: 26 additions & 187 deletions src/ripple/app/misc/impl/AmendmentTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,155 +67,6 @@ parseSection(Section const& section)
return names;
}

/** TrustedVotes records the most recent votes from trusted validators.
We keep a record in an effort to avoid "flapping" while amendment voting
is in process.
If a trusted validator loses synchronization near a flag ledger their
amendment votes may be lost during that round. If the validator is a
bit flaky, then this can cause an amendment to appear to repeatedly
gain and lose support.
TrustedVotes addresses the problem by holding on to the last vote seen
from every trusted validator. So if any given validator is off line near
a flag ledger we can assume that they did not change their vote.
If we haven't seen any STValidations from a validator for several hours we
lose confidence that the validator hasn't changed their position. So
there's a timeout. We remove upVotes if they haven't been updated in
several hours.
*/
class TrustedVotes
{
private:
static constexpr NetClock::time_point maxTimeout =
NetClock::time_point::max();

// Associates each trusted validator with the last votes we saw from them
// and an expiration for that record.
struct UpvotesAndTimeout
{
std::vector<uint256> upVotes;
NetClock::time_point timeout = maxTimeout;
};
hash_map<PublicKey, UpvotesAndTimeout> recordedVotes_;

public:
TrustedVotes() = default;
TrustedVotes(TrustedVotes const& rhs) = delete;
TrustedVotes&
operator=(TrustedVotes const& rhs) = delete;

// Called when the list of trusted validators changes.
//
// Call with AmendmentTable::mutex_ locked.
void
trustChanged(
hash_set<PublicKey> const& allTrusted,
std::lock_guard<std::mutex> const& lock)
{
decltype(recordedVotes_) newRecordedVotes;
newRecordedVotes.reserve(allTrusted.size());

// Make sure every PublicKey in allTrusted is represented in
// recordedVotes_. Also make sure recordedVotes_ contains
// no additional PublicKeys.
for (auto& trusted : allTrusted)
{
if (recordedVotes_.contains(trusted))
{
// Preserve this validator's previously saved voting state.
newRecordedVotes.insert(recordedVotes_.extract(trusted));
}
else
{
// New validators have a starting position of no on everything.
// Add the entry with an empty vector and maxTimeout.
newRecordedVotes[trusted];
}
}
// The votes of any no-longer-trusted validators will be destroyed
// when changedTrustedVotes goes out of scope.
recordedVotes_.swap(newRecordedVotes);
}

// Called when we receive the latest votes.
//
// Call with AmendmentTable::mutex_ locked.
void
recordVotes(
Rules const& rules,
std::vector<std::shared_ptr<STValidation>> const& valSet,
NetClock::time_point const closeTime,
std::lock_guard<std::mutex> const& lock)
{
// When we get an STValidation we save the upVotes it contains, but
// we also set an expiration for those upVotes. The following constant
// controls the timeout.
//
// There really is no "best" timeout to choose for when we finally
// lose confidence that we know how a validator is voting. But part
// of the point of recording validator votes is to avoid flapping of
// amendment votes. A 24h timeout says that we will change the local
// record of a validator's vote to "no" 24h after the last vote seen
// from that validator. So flapping due to that validator being off
// line will happen less frequently than every 24 hours.
using namespace std::chrono_literals;
static constexpr NetClock::duration expiresAfter = 24h;

// Walk all validations and replace previous votes from trusted
// validators with these newest votes.
for (auto const& val : valSet)
{
// If this validation comes from one of our trusted validators...
if (auto const iter = recordedVotes_.find(val->getSignerPublic());
iter != recordedVotes_.end())
{
iter->second.timeout = closeTime + expiresAfter;
if (val->isFieldPresent(sfAmendments))
{
auto const& choices = val->getFieldV256(sfAmendments);
iter->second.upVotes.assign(choices.begin(), choices.end());
}
else
{
// This validator does not upVote any amendments right now.
iter->second.upVotes.clear();
}
}
}

// Now remove any expired records from recordedVotes_.
std::for_each(
recordedVotes_.begin(),
recordedVotes_.end(),
[&closeTime](decltype(recordedVotes_)::value_type& votes) {
if (closeTime > votes.second.timeout)
{
votes.second.timeout = maxTimeout;
votes.second.upVotes.clear();
}
});
}

// Return the information needed by AmendmentSet to determine votes.
//
// Call with AmendmentTable::mutex_ locked.
[[nodiscard]] std::pair<int, hash_map<uint256, int>>
getVotes(Rules const& rules, std::lock_guard<std::mutex> const& lock) const
{
hash_map<uint256, int> ret;
for (auto& validatorVotes : recordedVotes_)
{
for (uint256 const& amendment : validatorVotes.second.upVotes)
{
ret[amendment] += 1;
}
}
return {recordedVotes_.size(), ret};
}
};

/** Current state of an amendment.
Tells if a amendment is supported, enabled or vetoed. A vetoed amendment
means the node will never announce its support.
Expand Down Expand Up @@ -253,9 +104,30 @@ class AmendmentSet
// number of votes needed
int threshold_ = 0;

void
computeThreshold(int trustedValidations, Rules const& rules)
public:
AmendmentSet(
Rules const& rules,
std::vector<std::shared_ptr<STValidation>> const& valSet)
: rules_(rules)
{
// process validations for ledger before flag ledger
for (auto const& val : valSet)
{
if (val->isTrusted())
{
if (val->isFieldPresent(sfAmendments))
{
auto const choices = val->getFieldV256(sfAmendments);
std::for_each(
choices.begin(),
choices.end(),
[&](auto const& amendment) { ++votes_[amendment]; });
}

++trustedValidations_;
}
}

threshold_ = !rules_.enabled(fixAmendmentMajorityCalc)
? std::max(
1L,
Expand All @@ -271,22 +143,6 @@ class AmendmentSet
postFixAmendmentMajorityCalcThreshold.den));
}

public:
AmendmentSet(
Rules const& rules,
TrustedVotes const& trustedVotes,
std::lock_guard<std::mutex> const& lock)
: rules_(rules)
{
// process validations for ledger before flag ledger.
auto [trustedCount, newVotes] = trustedVotes.getVotes(rules, lock);

trustedValidations_ = trustedCount;
votes_.swap(newVotes);

computeThreshold(trustedValidations_, rules);
}

bool
passes(uint256 const& amendment) const
{
Expand Down Expand Up @@ -347,9 +203,6 @@ class AmendmentTableImpl final : public AmendmentTable
hash_map<uint256, AmendmentState> amendmentMap_;
std::uint32_t lastUpdateSeq_;

// Record of the last votes seen from trusted validators.
TrustedVotes previousTrustedVotes_;

// Time that an amendment must hold a majority for
std::chrono::seconds const majorityTime_;

Expand Down Expand Up @@ -441,9 +294,6 @@ class AmendmentTableImpl final : public AmendmentTable
std::set<uint256> const& enabled,
majorityAmendments_t const& majority) override;

void
trustChanged(hash_set<PublicKey> const& allTrusted) override;

std::vector<uint256>
doValidation(std::set<uint256> const& enabledAmendments) const override;

Expand Down Expand Up @@ -783,14 +633,8 @@ AmendmentTableImpl::doVoting(
<< ": " << enabledAmendments.size() << ", "
<< majorityAmendments.size() << ", " << valSet.size();

std::lock_guard lock(mutex_);

// Keep a record of the votes we received.
previousTrustedVotes_.recordVotes(rules, valSet, closeTime, lock);
auto vote = std::make_unique<AmendmentSet>(rules, valSet);

// Tally the most recent votes.
auto vote =
std::make_unique<AmendmentSet>(rules, previousTrustedVotes_, lock);
JLOG(j_.debug()) << "Received " << vote->trustedValidations()
<< " trusted validations, threshold is: "
<< vote->threshold();
Expand All @@ -799,6 +643,8 @@ AmendmentTableImpl::doVoting(
// the value of the flags in the pseudo-transaction
std::map<uint256, std::uint32_t> actions;

std::lock_guard lock(mutex_);

// process all amendments we know of
for (auto const& entry : amendmentMap_)
{
Expand Down Expand Up @@ -894,13 +740,6 @@ AmendmentTableImpl::doValidatedLedger(
firstUnsupportedExpected_ = *firstUnsupportedExpected_ + majorityTime_;
}

void
AmendmentTableImpl::trustChanged(hash_set<PublicKey> const& allTrusted)
{
std::lock_guard lock(mutex_);
previousTrustedVotes_.trustChanged(allTrusted, lock);
}

void
AmendmentTableImpl::injectJson(
Json::Value& v,
Expand Down
Loading

0 comments on commit b28286b

Please sign in to comment.