Skip to content

Commit

Permalink
Add the ability to mark amendments as obsolete:
Browse files Browse the repository at this point in the history 
* Prevents the server from ever voting on the given amendment,
  regardless of operator configuration.
* Also prevents the "feature" command from changing the amendment's
  vote.
* Incidentally, resolves XRPLF#4014.
  • Loading branch information
@ximinez
ximinez committed Dec 16, 2022
1 parent c3a9f3d commit 0e2fae1
Show file tree
Hide file tree
Showing 7 changed files with 326 additions and 140 deletions.
4 changes: 2 additions & 2 deletions src/ripple/app/misc/AmendmentTable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ class AmendmentTable
struct FeatureInfo
{
FeatureInfo() = delete;
FeatureInfo(std::string const& n, uint256 const& f, DefaultVote v)
FeatureInfo(std::string const& n, uint256 const& f, VoteBehavior v)
: name(n), feature(f), vote(v)
{
}

std::string const name;
uint256 const feature;
DefaultVote const vote;
VoteBehavior const vote;
};

virtual ~AmendmentTable() = default;
Expand Down
41 changes: 32 additions & 9 deletions src/ripple/app/misc/impl/AmendmentTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,19 +333,31 @@ AmendmentTableImpl::AmendmentTableImpl(
}();

// Parse supported amendments
for (auto const& [name, amendment, defaultVote] : supported)
for (auto const& [name, amendment, votebehavior] : supported)
{
AmendmentState& s = add(amendment, lock);

s.name = name;
s.supported = true;
s.vote = defaultVote == DefaultVote::yes ? AmendmentVote::up
: AmendmentVote::down;
switch (votebehavior)
{
case VoteBehavior::DefaultYes:
s.vote = AmendmentVote::up;
break;

case VoteBehavior::DefaultNo:
s.vote = AmendmentVote::down;
break;

case VoteBehavior::Obsolete:
s.vote = AmendmentVote::obsolete;
break;
}

JLOG(j_.debug()) << "Amendment " << amendment << " (" << s.name
<< ") is supported and will be "
<< (s.vote == AmendmentVote::up ? "up" : "down")
<< " voted if not enabled on the ledger.";
<< " voted by default if not enabled on the ledger.";
}

hash_set<uint256> detect_conflict;
Expand Down Expand Up @@ -420,7 +432,9 @@ AmendmentTableImpl::AmendmentTableImpl(
<< amend_hash << "} is downvoted.";
if (!amendment_name->empty())
s->name = *amendment_name;
s->vote = *vote;
// An obsolete amendment's vote can never be changed
if (s->vote != AmendmentVote::obsolete)
s->vote = *vote;
}
}
else // up-vote
Expand All @@ -431,7 +445,9 @@ AmendmentTableImpl::AmendmentTableImpl(
<< amend_hash << "} is upvoted.";
if (!amendment_name->empty())
s.name = *amendment_name;
s.vote = *vote;
// An obsolete amendment's vote can never be changed
if (s.vote != AmendmentVote::obsolete)
s.vote = *vote;
}
});
}
Expand Down Expand Up @@ -489,6 +505,7 @@ AmendmentTableImpl::persistVote(
std::string const& name,
AmendmentVote vote) const
{
assert(vote != AmendmentVote::obsolete);
auto db = db_.checkoutDb();
voteAmendment(*db, amendment, name, vote);
}
Expand All @@ -499,7 +516,7 @@ AmendmentTableImpl::veto(uint256 const& amendment)
std::lock_guard lock(mutex_);
AmendmentState& s = add(amendment, lock);

if (s.vote == AmendmentVote::down)
if (s.vote != AmendmentVote::up)
return false;
s.vote = AmendmentVote::down;
persistVote(amendment, s.name, s.vote);
Expand All @@ -512,7 +529,7 @@ AmendmentTableImpl::unVeto(uint256 const& amendment)
std::lock_guard lock(mutex_);
AmendmentState* const s = get(amendment, lock);

if (!s || s->vote == AmendmentVote::up)
if (!s || s->vote != AmendmentVote::down)
return false;
s->vote = AmendmentVote::up;
persistVote(amendment, s->name, s->vote);
Expand Down Expand Up @@ -734,7 +751,13 @@ AmendmentTableImpl::injectJson(
v[jss::name] = fs.name;

v[jss::supported] = fs.supported;
v[jss::vetoed] = fs.vote == AmendmentVote::down;
if (!fs.enabled)
{
if (fs.vote == AmendmentVote::obsolete)
v[jss::vetoed] = "Obsolete";
else
v[jss::vetoed] = fs.vote == AmendmentVote::down;
}
v[jss::enabled] = fs.enabled;

if (!fs.enabled && lastVote_)
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/rdb/Wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ createFeatureVotes(soci::session& session);

// For historical reasons the up-vote and down-vote integer representations
// are unintuitive.
enum class AmendmentVote : int { up = 0, down = 1 };
enum class AmendmentVote : int { obsolete = -1, up = 0, down = 1 };

/**
* @brief readAmendments Reads all amendments from the FeatureVotes table.
Expand Down
14 changes: 7 additions & 7 deletions src/ripple/protocol/Feature.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,17 @@
* for the feature at the bottom
* 2) Add a uint256 definition for the feature to the corresponding source
* file (Feature.cpp). Use `registerFeature` to create the feature with
* the feature's name, `Supported::no`, and `DefaultVote::no`. This
* the feature's name, `Supported::no`, and `VoteBehavior::DefaultNo`. This
* should be the only place the feature's name appears in code as a string.
* 3) Use the uint256 as the parameter to `view.rules.enabled()` to
* control flow into new code that this feature limits.
* 4) If the feature development is COMPLETE, and the feature is ready to be
* SUPPORTED, change the `registerFeature` parameter to Supported::yes.
* 5) When the feature is ready to be ENABLED, change the `registerFeature`
* parameter to `DefaultVote::yes`.
* parameter to `VoteBehavior::DefaultYes`.
* In general, any newly supported amendments (`Supported::yes`) should have
* a `DefaultVote::no` for at least one full release cycle. High priority
* bug fixes can be an exception to this rule of thumb.
* a `VoteBehavior::DefaultNo` for at least one full release cycle. High
* priority bug fixes can be an exception to this rule of thumb.
*
* When a feature has been enabled for several years, the conditional code
* may be removed, and the feature "retired". To retire a feature:
Expand All @@ -55,7 +55,7 @@
* section at the end of the file.
* 3) CHANGE the name of the variable to start with "retired".
* 4) CHANGE the parameters of the `registerFeature` call to `Supported::yes`
* and `DefaultVote::no`.
* and `VoteBehavior::DefaultNo`.
* The feature must remain registered and supported indefinitely because it
* still exists in the ledger, but there is no need to vote for it because
* there's nothing to vote for. If it is removed completely from the code, any
Expand All @@ -66,7 +66,7 @@

namespace ripple {

enum class DefaultVote : bool { no = false, yes };
enum class VoteBehavior : int { Obsolete = -1, DefaultNo = 0, DefaultYes };

namespace detail {

Expand All @@ -79,7 +79,7 @@ static constexpr std::size_t numFeatures = 53;
/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
ledger */
std::map<std::string, DefaultVote> const&
std::map<std::string, VoteBehavior> const&
supportedAmendments();

/** Amendments that this server won't vote for by default.
Expand Down
119 changes: 66 additions & 53 deletions src/ripple/protocol/impl/Feature.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,10 @@ enum class Supported : bool { no = false, yes };
// updated.
//
// Generally, amendments which introduce new features should be set as
// "DefaultVote::no" whereas in rare cases, amendments that fix critical
// bugs should be set as "DefaultVote::yes", if off-chain consensus is
// reached amongst reviewers, validator operators, and other participants.
// "VoteBehavior::DefaultNo" whereas in rare cases, amendments that fix
// critical bugs should be set as "VoteBehavior::DefaultYes", if off-chain
// consensus is reached amongst reviewers, validator operators, and other
// participants.

class FeatureCollections
{
Expand Down Expand Up @@ -115,7 +116,7 @@ class FeatureCollections
// name, index, and uint256 feature identifier
boost::multi_index::multi_index_container<Feature, feature_indexing>
features;
std::map<std::string, DefaultVote> supported;
std::map<std::string, VoteBehavior> supported;
std::size_t upVotes = 0;
std::size_t downVotes = 0;
mutable std::atomic<bool> readOnly = false;
Expand Down Expand Up @@ -163,7 +164,7 @@ class FeatureCollections
registerFeature(
std::string const& name,
Supported support,
DefaultVote vote);
VoteBehavior vote);

/** Tell FeatureCollections when registration is complete. */
bool
Expand All @@ -181,7 +182,7 @@ class FeatureCollections
/** Amendments that this server supports.
Whether they are enabled depends on the Rules defined in the validated
ledger */
std::map<std::string, DefaultVote> const&
std::map<std::string, VoteBehavior> const&
supportedAmendments() const
{
return supported;
Expand Down Expand Up @@ -230,11 +231,11 @@ uint256
FeatureCollections::registerFeature(
std::string const& name,
Supported support,
DefaultVote vote)
VoteBehavior vote)
{
check(!readOnly, "Attempting to register a feature after startup.");
check(
support == Supported::yes || vote == DefaultVote::no,
support == Supported::yes || vote == VoteBehavior::DefaultNo,
"Invalid feature parameters. Must be supported to be up-voted.");
Feature const* i = getByName(name);
if (!i)
Expand All @@ -254,7 +255,7 @@ FeatureCollections::registerFeature(
{
supported.emplace(name, vote);

if (vote == DefaultVote::yes)
if (vote == VoteBehavior::DefaultYes)
++upVotes;
else
++downVotes;
Expand Down Expand Up @@ -315,7 +316,7 @@ static FeatureCollections featureCollections;
/** Amendments that this server supports.
Whether they are enabled depends on the Rules defined in the validated
ledger */
std::map<std::string, DefaultVote> const&
std::map<std::string, VoteBehavior> const&
detail::supportedAmendments()
{
return featureCollections.supportedAmendments();
Expand Down Expand Up @@ -344,7 +345,7 @@ getRegisteredFeature(std::string const& name)
}

uint256
registerFeature(std::string const& name, Supported support, DefaultVote vote)
registerFeature(std::string const& name, Supported support, VoteBehavior vote)
{
return featureCollections.registerFeature(name, support, vote);
}
Expand All @@ -354,7 +355,7 @@ registerFeature(std::string const& name, Supported support, DefaultVote vote)
uint256
retireFeature(std::string const& name)
{
return registerFeature(name, Supported::yes, DefaultVote::no);
return registerFeature(name, Supported::yes, VoteBehavior::Obsolete);
}

/** Tell FeatureCollections when registration is complete. */
Expand Down Expand Up @@ -390,9 +391,9 @@ Takes the name of a feature, whether it's supported, and the default vote. Will
register the feature, and create a variable whose name is "feature" plus the
feature name.
*/
#define REGISTER_FEATURE(fName, supported, defaultvote) \
uint256 const feature##fName = \
registerFeature(#fName, supported, defaultvote)
#define REGISTER_FEATURE(fName, supported, votebehavior) \
uint256 const feature##fName = \
registerFeature(#fName, supported, votebehavior)

#pragma push_macro("REGISTER_FIX")
#undef REGISTER_FIX
Expand All @@ -402,55 +403,67 @@ Takes the name of a feature, whether it's supported, and the default vote. Will
register the feature, and create a variable whose name is the unmodified feature
name.
*/
#define REGISTER_FIX(fName, supported, defaultvote) \
uint256 const fName = registerFeature(#fName, supported, defaultvote)
#define REGISTER_FIX(fName, supported, votebehavior) \
uint256 const fName = registerFeature(#fName, supported, votebehavior)

// clang-format off

// All known amendments must be registered either here or below with the
// "retired" amendments
REGISTER_FEATURE(OwnerPaysFee, Supported::no, DefaultVote::no);
REGISTER_FEATURE(Flow, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(FlowCross, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(CryptoConditionsSuite, Supported::yes, DefaultVote::no);
REGISTER_FIX (fix1513, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(DepositAuth, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(Checks, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fix1571, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fix1543, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fix1623, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(DepositPreauth, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(OwnerPaysFee, Supported::no, VoteBehavior::DefaultNo);
REGISTER_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(FlowCross, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fix1513, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(DepositAuth, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fix1571, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fix1543, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fix1623, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes);
// Use liquidity from strands that consume max offers, but mark as dry
REGISTER_FIX (fix1515, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fix1578, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(MultiSignReserve, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixTakerDryOfferRemoval, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixMasterKeyAsRegularKey, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixCheckThreading, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixPayChanRecipientOwnerDir, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(DeletableAccounts, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fix1515, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fix1578, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fixTakerDryOfferRemoval, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fixMasterKeyAsRegularKey, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fixCheckThreading, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fixPayChanRecipientOwnerDir, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(DeletableAccounts, Supported::yes, VoteBehavior::DefaultYes);
// fixQualityUpperBound should be activated before FlowCross
REGISTER_FIX (fixQualityUpperBound, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(RequireFullyCanonicalSig, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixQualityUpperBound, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(RequireFullyCanonicalSig, Supported::yes, VoteBehavior::DefaultYes);
// fix1781: XRPEndpointSteps should be included in the circular payment check
REGISTER_FIX (fix1781, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(HardenedValidations, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixAmendmentMajorityCalc, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(NegativeUNL, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(TicketBatch, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(FlowSortStrands, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixSTAmountCanonicalize, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixRmSmallIncreasedQOffers, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(CheckCashMakesTrustLine, Supported::yes, DefaultVote::no);
REGISTER_FEATURE(NonFungibleTokensV1, Supported::yes, DefaultVote::no);
REGISTER_FEATURE(ExpandedSignerList, Supported::yes, DefaultVote::no);
REGISTER_FIX (fixNFTokenDirV1, Supported::yes, DefaultVote::no);
REGISTER_FIX (fixNFTokenNegOffer, Supported::yes, DefaultVote::no);
REGISTER_FEATURE(NonFungibleTokensV1_1, Supported::yes, DefaultVote::no);
REGISTER_FIX (fixTrustLinesToSelf, Supported::yes, DefaultVote::no);
REGISTER_FIX (fix1781, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(HardenedValidations, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fixAmendmentMajorityCalc, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(NegativeUNL, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(TicketBatch, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(FlowSortStrands, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fixSTAmountCanonicalize, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FIX (fixRmSmallIncreasedQOffers, Supported::yes, VoteBehavior::DefaultYes);
REGISTER_FEATURE(CheckCashMakesTrustLine, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FEATURE(ExpandedSignerList, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FEATURE(NonFungibleTokensV1_1, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixTrustLinesToSelf, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixRemoveNFTokenAutoTrustLine, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(ImmediateOfferKilled, Supported::yes, DefaultVote::no);

// The following amendments are obsolete, but must remain supported
// because they could potentially get enabled.
//
// Obsolete features are (usually) not in the ledger, and may have code
// controlled by the feature. They need to be supported because at some
// time in the past, the feature was supported and votable, but never
// passed. So the feature needs to be supported in case it is ever
// enabled (added to the ledger).
//
// If a feature remains obsolete for long enough that no clients are able
// to vote for it, the feature can be removed (entirely?) from the code.
REGISTER_FEATURE(CryptoConditionsSuite, Supported::yes, VoteBehavior::Obsolete);
REGISTER_FEATURE(NonFungibleTokensV1, Supported::yes, VoteBehavior::Obsolete);
REGISTER_FIX (fixNFTokenDirV1, Supported::yes, VoteBehavior::Obsolete);
REGISTER_FIX (fixNFTokenNegOffer, Supported::yes, VoteBehavior::Obsolete);

// The following amendments have been active for at least two years. Their
// pre-amendment code has been removed and the identifiers are deprecated.
// All known amendments and amendments that may appear in a validated
Expand Down
Loading

0 comments on commit 0e2fae1

Please sign in to comment.