diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index b9acd63e748..bd4c0fc5e01 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1342,26 +1342,34 @@ ApplicationImp::setup() // Configure the amendments the server supports { - auto const& sa = detail::supportedAmendments(); - std::vector 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 const& amendments) { + std::vector hashes; + hashes.reserve(amendments.size()); + for (auto const& name : amendments) + { + auto const f = getRegisteredFeature(name); + assert(f); + if (f) + hashes.push_back(to_string(*f) + " " + name); + } + section.append(hashes); + return section; + }; + Section const supportedAmendments = buildAmendmentList( + Section("Supported Amendments"), detail::supportedAmendments()); + + Section const downVotedAmendments = buildAmendmentList( + config_->section(SECTION_VETO_AMENDMENTS), + detail::downVotedAmendments()); - 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")); } diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 2f29a2f5788..2b160aec1b3 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -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 const& sl); // Finds existing state. Must be called with mutex_ locked. @@ -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; @@ -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 const&) { // call with the mutex held - return &amendmentMap_[amendmentHash]; + return amendmentMap_[amendmentHash]; } AmendmentState* @@ -406,11 +414,11 @@ 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; } @@ -418,7 +426,7 @@ 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; @@ -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."; @@ -451,7 +459,7 @@ 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; } @@ -459,7 +467,7 @@ 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; } @@ -493,6 +501,7 @@ AmendmentTableImpl::doValidation(std::set const& enabled) const (enabled.count(e.first) == 0)) { amendments.push_back(e.first); + JLOG(j_.info()) << "Voting for amendment " << e.second.name; } } } @@ -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); @@ -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); } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 5f997042314..dd10c829736 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -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. * */ @@ -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 const& supportedAmendments(); +/** Amendments that this server won't vote for by default. Overrides the default + vote behavior of `supportedAmendments()` */ +std::vector const& +downVotedAmendments(); + } // namespace detail boost::optional @@ -153,6 +169,9 @@ featureToBitsetIndex(uint256 const& f); uint256 bitsetIndexToFeature(size_t i); +std::string +featureToName(uint256 const& f); + class FeatureBitset : private std::bitset { diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 7025117aa75..f856e482fd9 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -74,14 +74,32 @@ detail::FeatureCollections::bitsetIndexToFeature(size_t i) const return features[i]; } +std::string +detail::FeatureCollections::featureToName(uint256 const& f) const +{ + 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 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 @@ -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 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. + static std::vector const vetoed{ + "CryptoConditionsSuite", // Added in 0.60.0, incomplete, do not enable + }; + return vetoed; +} + //------------------------------------------------------------------------------ boost::optional @@ -157,6 +207,12 @@ bitsetIndexToFeature(size_t i) return featureCollections.bitsetIndexToFeature(i); } +std::string +featureToName(uint256 const& f) +{ + return featureCollections.featureToName(f); +} + // clang-format off uint256 const diff --git a/src/test/app/AmendmentTable_test.cpp b/src/test/app/AmendmentTable_test.cpp index 3b4ec47d143..c0a883671aa 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -273,20 +273,19 @@ class AmendmentTable_test final : public beast::unit_test::suite std::unique_ptr table = makeTable(weeks(2)); - // Note which entries are pre-enabled. + // Note which entries are pre-enabled std::set allEnabled; - for (std::string const& a : enabled_) + for (auto const& a : enabled_) allEnabled.insert(amendmentId(a)); // Subset of amendments to late-enable std::set lateEnabled; lateEnabled.insert(amendmentId(supported_[0])); - lateEnabled.insert(amendmentId(enabled_[0])); lateEnabled.insert(amendmentId(vetoed_[0])); // Do the late enabling. for (uint256 const& a : lateEnabled) - table->enable(a); + BEAST_EXPECT(table->enable(a)); // So far all enabled amendments are supported. BEAST_EXPECT(!table->hasUnsupportedEnabled()); @@ -314,14 +313,14 @@ class AmendmentTable_test final : public beast::unit_test::suite // Unveto an amendment that is already not vetoed. Shouldn't // hurt anything, but the values returned by getDesired() // shouldn't change. - table->unVeto(amendmentId(supported_[1])); + BEAST_EXPECT(!table->unVeto(amendmentId(supported_[1]))); BEAST_EXPECT(desired == table->getDesired()); } // UnVeto one of the vetoed amendments. It should now be desired. { uint256 const unvetoedID = amendmentId(vetoed_[0]); - table->unVeto(unvetoedID); + BEAST_EXPECT(table->unVeto(unvetoedID)); std::vector const desired = table->getDesired(); BEAST_EXPECT( diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 5550b873949..d0e4e43cb2b 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -141,9 +141,12 @@ class TxQ_test : public beast::unit_test::suite // fee (1) and amendment (supportedAmendments().size()) // pseudotransactions. The queue treats the fees on these // transactions as though they are ordinary transactions. - auto const flagPerLedger = - 1 + ripple::detail::supportedAmendments().size(); + auto const flagPerLedger = 1 + + ripple::detail::supportedAmendments().size() - + ripple::detail::downVotedAmendments().size(); auto const flagMaxQueue = ledgersInQueue * flagPerLedger; + // If this check fails, check that all the entries in + // downVotedAmendments are also in supportedAmendments. checkMetrics(env, 0, flagMaxQueue, 0, flagPerLedger, 256); // Pad a couple of txs with normal fees so the median comes @@ -4173,8 +4176,8 @@ class TxQ_test : public beast::unit_test::suite if (!getMajorityAmendments(*env.closed()).empty()) break; } - auto expectedPerLedger = - ripple::detail::supportedAmendments().size() + 1; + auto expectedPerLedger = ripple::detail::supportedAmendments().size() - + ripple::detail::downVotedAmendments().size() + 1; checkMetrics(env, 0, 5 * expectedPerLedger, 0, expectedPerLedger, 256); // Now wait 2 weeks modulo 256 ledgers for the amendments to be diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 5773f756667..9cefe3f5b7a 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -26,6 +26,47 @@ namespace ripple { class Feature_test : public beast::unit_test::suite { + void + testDownVotesSupported() + { + testcase("internal vetoes are all supported"); + + auto const supported = ripple::detail::supportedAmendments(); + auto const vetoed = ripple::detail::downVotedAmendments(); + + for (auto const& veto : vetoed) + { + BEAST_EXPECTS( + std::count(supported.begin(), supported.end(), veto) == 1, + veto); + } + } + + void + testFeatureToName() + { + testcase("featureToName"); + + // Test all the supported features. In a perfect world, this would test + // FeatureCollections::featureNames, but that's private. Leave it that + // way. + auto const supported = ripple::detail::supportedAmendments(); + + for (auto const& feature : supported) + { + auto const registered = getRegisteredFeature(feature); + if (BEAST_EXPECT(registered)) + BEAST_EXPECT(featureToName(*registered) == feature); + } + + // Test an arbitrary unknown feature + uint256 zero{0}; + BEAST_EXPECT(featureToName(zero) == to_string(zero)); + BEAST_EXPECT( + featureToName(zero) == + "0000000000000000000000000000000000000000000000000000000000000000"); + } + void testNoParams() { @@ -34,6 +75,9 @@ class Feature_test : public beast::unit_test::suite using namespace test::jtx; Env env{*this}; + std::vector const& vetoed = + ripple::detail::downVotedAmendments(); + auto jrr = env.rpc("feature")[jss::result]; if (!BEAST_EXPECT(jrr.isMember(jss::features))) return; @@ -41,13 +85,16 @@ class Feature_test : public beast::unit_test::suite { if (!BEAST_EXPECT(feature.isMember(jss::name))) return; - // default config - so all should be disabled, not vetoed, and - // supported + // default config - so all should be disabled, and + // supported. Some may be vetoed. + bool expectVeto = + std::count(vetoed.begin(), vetoed.end(), feature[jss::name]) > + 0; BEAST_EXPECTS( !feature[jss::enabled].asBool(), feature[jss::name].asString() + " enabled"); BEAST_EXPECTS( - !feature[jss::vetoed].asBool(), + feature[jss::vetoed].asBool() == expectVeto, feature[jss::name].asString() + " vetoed"); BEAST_EXPECTS( feature[jss::supported].asBool(), @@ -121,6 +168,9 @@ class Feature_test : public beast::unit_test::suite Env env{ *this, FeatureBitset(featureDepositAuth, featureDepositPreauth)}; + std::vector const& vetoed = + ripple::detail::downVotedAmendments(); + auto jrr = env.rpc("feature")[jss::result]; if (!BEAST_EXPECT(jrr.isMember(jss::features))) return; @@ -135,11 +185,13 @@ class Feature_test : public beast::unit_test::suite bool expectEnabled = env.app().getAmendmentTable().isEnabled(id); bool expectSupported = env.app().getAmendmentTable().isSupported(id); + bool expectVeto = + std::count(vetoed.begin(), vetoed.end(), (*it)[jss::name]) > 0; BEAST_EXPECTS( (*it)[jss::enabled].asBool() == expectEnabled, (*it)[jss::name].asString() + " enabled"); BEAST_EXPECTS( - !(*it)[jss::vetoed].asBool(), + (*it)[jss::vetoed].asBool() == expectVeto, (*it)[jss::name].asString() + " vetoed"); BEAST_EXPECTS( (*it)[jss::supported].asBool() == expectSupported, @@ -198,6 +250,8 @@ class Feature_test : public beast::unit_test::suite // There should be at least 5 amendments. Don't do exact comparison // to avoid maintenance as more amendments are added in the future. BEAST_EXPECT(majorities.size() >= 5); + std::vector const& vetoed = + ripple::detail::downVotedAmendments(); jrr = env.rpc("feature")[jss::result]; if (!BEAST_EXPECT(jrr.isMember(jss::features))) @@ -206,9 +260,16 @@ class Feature_test : public beast::unit_test::suite { if (!BEAST_EXPECT(feature.isMember(jss::name))) return; + bool expectVeto = + std::count(vetoed.begin(), vetoed.end(), feature[jss::name]) > + 0; BEAST_EXPECTS( - feature.isMember(jss::majority), + expectVeto ^ feature.isMember(jss::majority), feature[jss::name].asString() + " majority"); + BEAST_EXPECTS( + feature.isMember(jss::vetoed) && + feature[jss::vetoed].asBool() == expectVeto, + feature[jss::name].asString() + " vetoed"); BEAST_EXPECTS( feature.isMember(jss::count), feature[jss::name].asString() + " count"); @@ -218,10 +279,10 @@ class Feature_test : public beast::unit_test::suite BEAST_EXPECTS( feature.isMember(jss::validations), feature[jss::name].asString() + " validations"); - BEAST_EXPECT(feature[jss::count] == 1); + BEAST_EXPECT(feature[jss::count] == (expectVeto ? 0 : 1)); BEAST_EXPECT(feature[jss::threshold] == 1); BEAST_EXPECT(feature[jss::validations] == 1); - BEAST_EXPECT(feature[jss::majority] == 2740); + BEAST_EXPECT(expectVeto || feature[jss::majority] == 2740); } } @@ -273,6 +334,8 @@ class Feature_test : public beast::unit_test::suite void run() override { + testDownVotesSupported(); + testFeatureToName(); testNoParams(); testSingleFeature(); testInvalidFeature();