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

Jump to bottom

Internal amendment vetoes: #3458

Closed
wants to merge 1 commit into from
Closed

Internal amendment vetoes: #3458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 22 additions & 14 deletions src/ripple/app/main/Application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,26 +1342,34 @@ ApplicationImp::setup()

// Configure the amendments the server supports
{
auto const& sa = detail::supportedAmendments();
std::vector<std::string> saHashes;
saHashes.reserve(sa.size());
for (auto const& name : sa)
{
auto const f = getRegisteredFeature(name);
BOOST_ASSERT(f);
if (f)
saHashes.push_back(to_string(*f) + " " + name);
}
Section supportedAmendments("Supported Amendments");
supportedAmendments.append(saHashes);
auto buildAmendmentList =
[](Section section, std::vector<std::string> const& amendments) {
std::vector<std::string> hashes;
hashes.reserve(amendments.size());
for (auto const& name : amendments)
{
auto const f = getRegisteredFeature(name);
assert(f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious: How do we decide when to use BOOST_ASSERT vs assert? I guess they are synonymous?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're basically synonymous, but the boost version adds the dependency on boost. In general, AFAIK, we're moving away from BOOST_ASSERT, but nobody has bothered to do a global search and replace.

if (f)
hashes.push_back(to_string(*f) + " " + name);
}
section.append(hashes);
return section;
};
Section const supportedAmendments = buildAmendmentList(
Section("Supported Amendments"), detail::supportedAmendments());
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 getting to be enough places that use supportedAmendments() that it may be time to move them out of the detail namespace. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm torn. This example isn't new new place where supportedAmendments() is called. In fact, it looks like this is the only place outside of test code where it's called. I think the only calls I added are in Feature_test.cpp. Leaving it inside detail signals that it shouldn't be called casually.

My opinion is to leave it.


Section const downVotedAmendments = buildAmendmentList(
config_->section(SECTION_VETO_AMENDMENTS),
detail::downVotedAmendments());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider moving downVotedAmendments() out of the detail namespace as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wherever they end up, they should definitely be left together.


Section enabledAmendments = config_->section(SECTION_AMENDMENTS);
Section const enabledAmendments = config_->section(SECTION_AMENDMENTS);

m_amendmentTable = make_AmendmentTable(
config().AMENDMENT_MAJORITY_TIME,
supportedAmendments,
enabledAmendments,
config_->section(SECTION_VETO_AMENDMENTS),
downVotedAmendments,
logs_->journal("Amendments"));
}

Expand Down
65 changes: 37 additions & 28 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 @@ -221,7 +221,7 @@ class AmendmentTableImpl final : public AmendmentTable
beast::Journal const j_;

// Finds or creates state. Must be called with mutex_ locked.
AmendmentState*
AmendmentState&
add(uint256 const& amendment, std::lock_guard<std::mutex> const& sl);

// Finds existing state. Must be called with mutex_ locked.
Expand Down Expand Up @@ -315,25 +315,30 @@ AmendmentTableImpl::AmendmentTableImpl(

for (auto const& a : parseSection(supported))
{
if (auto s = add(a.first, sl))
{
JLOG(j_.debug()) << "Amendment " << a.first << " is supported.";
AmendmentState& s = add(a.first, sl);

if (!a.second.empty())
s->name = a.second;
JLOG(j_.debug()) << "Amendment " << a.first << " (" << a.second
<< ") is supported.";

s->supported = true;
}
if (!a.second.empty())
s.name = a.second;
else
s.name = featureToName(a.first);

s.supported = true;
}

for (auto const& a : parseSection(enabled))
{
if (auto s = add(a.first, sl))
if (AmendmentState* const s = get(a.first, sl))
{
JLOG(j_.debug()) << "Amendment " << a.first << " is enabled.";
JLOG(j_.debug()) << "Amendment " << a.first << " (" << a.second
<< ") is enabled.";

if (!a.second.empty())
s->name = a.second;
else
s->name = featureToName(a.first);

s->supported = true;
s->enabled = true;
Expand All @@ -343,25 +348,28 @@ AmendmentTableImpl::AmendmentTableImpl(
for (auto const& a : parseSection(vetoed))
{
// Unknown amendments are effectively vetoed already
if (auto s = get(a.first, sl))
if (AmendmentState* const s = get(a.first, sl))
{
JLOG(j_.info()) << "Amendment " << a.first << " is vetoed.";
JLOG(j_.debug()) << "Amendment " << a.first << " (" << a.second
<< ") is vetoed.";

if (!a.second.empty())
s->name = a.second;
else
s->name = featureToName(a.first);

s->vetoed = true;
}
}
}

AmendmentState*
AmendmentState&
AmendmentTableImpl::add(
uint256 const& amendmentHash,
std::lock_guard<std::mutex> const&)
{
// call with the mutex held
return &amendmentMap_[amendmentHash];
return amendmentMap_[amendmentHash];
}

AmendmentState*
Expand Down Expand Up @@ -406,19 +414,19 @@ bool
AmendmentTableImpl::veto(uint256 const& amendment)
{
std::lock_guard sl(mutex_);
auto s = add(amendment, sl);
AmendmentState& s = add(amendment, sl);

if (s->vetoed)
if (s.vetoed)
return false;
s->vetoed = true;
s.vetoed = true;
return true;
}

bool
AmendmentTableImpl::unVeto(uint256 const& amendment)
{
std::lock_guard sl(mutex_);
auto s = get(amendment, sl);
AmendmentState* const s = get(amendment, sl);

if (!s || !s->vetoed)
return false;
Expand All @@ -430,14 +438,14 @@ bool
AmendmentTableImpl::enable(uint256 const& amendment)
{
std::lock_guard sl(mutex_);
auto s = add(amendment, sl);
AmendmentState& s = add(amendment, sl);

if (s->enabled)
if (s.enabled)
return false;

s->enabled = true;
s.enabled = true;

if (!s->supported)
if (!s.supported)
{
JLOG(j_.error()) << "Unsupported amendment " << amendment
<< " activated.";
Expand All @@ -451,15 +459,15 @@ bool
AmendmentTableImpl::isEnabled(uint256 const& amendment) const
{
std::lock_guard sl(mutex_);
auto s = get(amendment, sl);
AmendmentState const* s = get(amendment, sl);
return s && s->enabled;
}

bool
AmendmentTableImpl::isSupported(uint256 const& amendment) const
{
std::lock_guard sl(mutex_);
auto s = get(amendment, sl);
AmendmentState const* s = get(amendment, sl);
return s && s->supported;
}

Expand Down Expand Up @@ -493,6 +501,7 @@ AmendmentTableImpl::doValidation(std::set<uint256> const& enabled) const
(enabled.count(e.first) == 0))
{
amendments.push_back(e.first);
JLOG(j_.info()) << "Voting for amendment " << e.second.name;
}
}
}
Expand Down Expand Up @@ -612,12 +621,12 @@ AmendmentTableImpl::doValidatedLedger(
firstUnsupportedExpected_.reset();
for (auto const& [hash, time] : majority)
{
auto s = add(hash, sl);
AmendmentState& s = add(hash, sl);

if (s->enabled)
if (s.enabled)
continue;

if (!s->supported)
if (!s.supported)
{
JLOG(j_.info()) << "Unsupported amendment " << hash
<< " reached majority at " << to_string(time);
Expand Down Expand Up @@ -683,7 +692,7 @@ AmendmentTableImpl::getJson(uint256 const& amendmentID) const

{
std::lock_guard sl(mutex_);
auto a = get(amendmentID, sl);
AmendmentState const* a = get(amendmentID, sl);
if (a)
injectJson(jAmendment, amendmentID, *a, sl);
}
Expand Down
27 changes: 23 additions & 4 deletions src/ripple/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,15 @@
* 2) add a uint256 declaration for the feature to the bottom of this file
* 3) add a uint256 definition for the feature to the corresponding source
* file (Feature.cpp)
* 4) if the feature is going to be supported in the near future, add its
* sha512half value and name (matching exactly the featureName here) to
* the supportedAmendments in Feature.cpp.
* 4) use the uint256 as the parameter to `view.rules.enabled()` to
* control flow into new code that this feature limits.
* 5) if the feature development is COMPLETE, and the feature is ready to be
* SUPPORTED, add its name (matching exactly the featureName here) to the
* supportedAmendments in Feature.cpp.
* 6) if the feature is not ready to be ENABLED, add its name (matching exactly
* the featureName here) to the downVotedAmendments in Feature.cpp.
* In general, any new amendments added to supportedAmendments should also be
* added to downVotedAmendments for at least one full release cycle.
*
*/

Expand Down Expand Up @@ -136,12 +142,22 @@ class FeatureCollections

uint256 const&
bitsetIndexToFeature(size_t i) const;

std::string
featureToName(uint256 const& f) const;
};

/** Amendments that this server supports, but doesn't enable by default */
/** Amendments that this server supports and will vote for by default.
Whether they are enabled depends on the Rules defined in the validated
ledger */
std::vector<std::string> const&
supportedAmendments();

/** Amendments that this server won't vote for by default. Overrides the default
vote behavior of `supportedAmendments()` */
std::vector<std::string> const&
downVotedAmendments();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tested how downVotedAmendments() interacts with unit tests? I think that we want to run unit tests with the downVotedAmendments() enabled. Is that your expectation as well? I suppose we can test (and) fix that when this capability is integrated with the Negative UNL or with Tickets...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good question. There is the initFee function in TxQ_test.cpp where it runs up to a flag ledger to make a fee change pseudo transaction, but it also creates all the supported amendment pseudo transactions. I don't check the individual amendments, but I do check the counts, accounting for downVotedAmendments(), and the counts match, regardless of which or how many amendments I put into the down vote list. I think that's sufficient.


} // namespace detail

boost::optional<uint256>
Expand All @@ -153,6 +169,9 @@ featureToBitsetIndex(uint256 const& f);
uint256
bitsetIndexToFeature(size_t i);

std::string
featureToName(uint256 const& f);

class FeatureBitset
: private std::bitset<detail::FeatureCollections::numFeatures()>
{
Expand Down
60 changes: 58 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 @@ -74,14 +74,32 @@ detail::FeatureCollections::bitsetIndexToFeature(size_t i) const
return features[i];
}

std::string
detail::FeatureCollections::featureToName(uint256 const& f) const
Copy link
Collaborator

Choose a reason for hiding this comment

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

FeatureCollections::featureToName() is untouched by the unit tests. That probably ought to be fixed.

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.

{
auto const i = featureToIndex.find(f);
assert(featureToIndex.size() == numFeatures());
return i == featureToIndex.end() ? to_string(f) : featureNames[i->second];
}

static detail::FeatureCollections const featureCollections;

/** Amendments that this server supports, but doesn't enable by default */
/** Amendments that this server supports and will vote for by default.
Whether they are enabled depends on the Rules defined in the validated
ledger */
std::vector<std::string> const&
detail::supportedAmendments()
{
// Commented out amendments will be supported in a future release (and
// uncommented at that time).
// uncommented at that time). Including an amendment here indicates
// that development of the feature is complete. Any future behavior
// changes will require another amendment.
//
// In general, any new non-fix amendments added to this list should also be
// added to downVotedAmendments for at least one full release cycle to
// prevent rippled from automatically voting to enable that amendment by
// default for some time. Fix amendments should be carefully considered
// based on the risk and severity of the thing they are fixing.
//
// There are also unconditionally supported amendments in the list.
// Those are amendments that were enabled some time ago and the
Expand Down Expand Up @@ -137,6 +155,38 @@ detail::supportedAmendments()
return supported;
}

/** Amendments that this server won't vote for by default. Overrides the default
vote behavior of `supportedAmendments()` */
std::vector<std::string> const&
detail::downVotedAmendments()
{
// Amendment names included in this list should also be present and
// uncommented in supportedAmendments above. This allows a particular
// version of rippled to be able to understand the amendment if it gets
// enabled, but not vote for the amendment by default. Additionally, some
// tests will fail if an entry in this list is not in supportedAmendments.
//
// Note that this list can be overridden by the "feature" admin RPC command.
//
// For example, if amendment FOO is first complete and released in 2.1, but
// down voted (included in this list) until 2.3 when it is removed from this
// list, then when it is finally enabled by receiving votes from the
// majority of UNL validators some time after 2.3 is released, versions 2.1
// and later will know how to handle the new behavior and will NOT be
// amendment blocked.
//
// Suggested format for entries in the string vector:
// "AmendmentName", // Added in 2.1, planned to enable in 2.3
//
// To enable automatically voting for the amendment, simply remove it from
// this list. There should never be a need to comment entries out aside from
// testing.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice comment!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

static std::vector<std::string> const vetoed{
"CryptoConditionsSuite", // Added in 0.60.0, incomplete, do not enable
};
return vetoed;
}

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

boost::optional<uint256>
Expand All @@ -157,6 +207,12 @@ bitsetIndexToFeature(size_t i)
return featureCollections.bitsetIndexToFeature(i);
}

std::string
featureToName(uint256 const& f)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This featureToName() is also untouched by the unit tests.

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.

{
return featureCollections.featureToName(f);
}

// clang-format off

uint256 const
Expand Down
Loading