Refactor Feature name management and creation:

* Only require adding the new feature names in one place. (Also need to
  increment a counter, but a check on startup will catch that.)
* Allows rippled to have the code to support a given amendment, but
  not vote for it by default. This allows the amendment to be enabled in
  a future version without necessarily amendment blocking these older
  versions.
* The default vote is carried with the amendment name in the list of
  supported amendments.
* The amendment table is constructed with the amendment and default
  vote.
This commit is contained in:
Edward Hennis
2020-06-13 22:56:36 -04:00
committed by manojsdoshi
parent 1ca8898703
commit 4a9bd7ed6d
13 changed files with 837 additions and 490 deletions

View File

@@ -86,11 +86,31 @@ private:
return cfg;
}
static std::vector<AmendmentTable::FeatureInfo>
makeDefaultYes(std::vector<std::string> const& amendments)
{
std::vector<AmendmentTable::FeatureInfo> result;
result.reserve(amendments.size());
for (auto const& a : amendments)
{
result.emplace_back(a, amendmentId(a), DefaultVote::yes);
}
return result;
}
static std::vector<AmendmentTable::FeatureInfo>
makeDefaultYes(uint256 const amendment)
{
std::vector<AmendmentTable::FeatureInfo> result{
{to_string(amendment), amendment, DefaultVote::yes}};
return result;
}
// All useful amendments are supported amendments.
// Enabled amendments are typically a subset of supported amendments.
// Vetoed amendments should be supported but not enabled.
// Unsupported amendments may be added to the AmendmentTable.
std::vector<std::string> const supported_{
std::vector<std::string> const supportedYes_{
"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k",
"l", "m", "n", "o", "p", "q", "r", "s", "t", "u"};
std::vector<std::string> const
@@ -100,6 +120,7 @@ private:
std::vector<std::string> const unsupportedMajority_{"y", "z"};
Section const emptySection;
std::vector<AmendmentTable::FeatureInfo> const emptyYes;
test::SuiteJournal journal;
@@ -112,7 +133,7 @@ public:
makeTable(
Application& app,
std::chrono::seconds majorityTime,
Section const& supported,
std::vector<AmendmentTable::FeatureInfo> const& supported,
Section const& enabled,
Section const& vetoed)
{
@@ -124,7 +145,7 @@ public:
makeTable(
test::jtx::Env& env,
std::chrono::seconds majorityTime,
Section const& supported,
std::vector<AmendmentTable::FeatureInfo> const& supported,
Section const& enabled,
Section const& vetoed)
{
@@ -137,7 +158,7 @@ public:
return makeTable(
env.app(),
majorityTime,
makeSection(supported_),
makeDefaultYes(supportedYes_),
makeSection(enabled_),
makeSection(vetoed_));
}
@@ -149,7 +170,7 @@ public:
test::jtx::Env env{*this, makeConfig()};
auto table = makeTable(env, weeks(1));
for (auto const& a : supported_)
for (auto const& a : supportedYes_)
{
BEAST_EXPECT(table->isSupported(amendmentId(a)));
}
@@ -174,7 +195,7 @@ public:
test::jtx::Env env{*this, makeConfig()};
auto table = makeTable(env, weeks(1));
for (auto const& a : supported_)
for (auto const& a : supportedYes_)
BEAST_EXPECT(table->find(a) == amendmentId(a));
for (auto const& a : enabled_)
BEAST_EXPECT(table->find(a) == amendmentId(a));
@@ -207,7 +228,8 @@ public:
void
testBadConfig()
{
auto const section = makeSection(supported_);
auto const yesVotes = makeDefaultYes(supportedYes_);
auto const section = makeSection(vetoed_);
auto const id = to_string(amendmentId(enabled_[0]));
testcase("Bad Config");
@@ -219,12 +241,13 @@ public:
try
{
test::jtx::Env env{*this, makeConfig()};
if (makeTable(env, weeks(2), test, emptySection, emptySection))
if (makeTable(env, weeks(2), yesVotes, test, emptySection))
fail("Accepted only amendment ID");
}
catch (...)
catch (std::exception const& e)
{
pass();
BEAST_EXPECT(
e.what() == "Invalid entry '" + id + "' in [Test]");
}
}
@@ -235,12 +258,14 @@ public:
try
{
test::jtx::Env env{*this, makeConfig()};
if (makeTable(env, weeks(2), test, emptySection, emptySection))
if (makeTable(env, weeks(2), yesVotes, test, emptySection))
fail("Accepted extra arguments");
}
catch (...)
catch (std::exception const& e)
{
pass();
BEAST_EXPECT(
e.what() ==
"Invalid entry '" + id + " Test Name' in [Test]");
}
}
@@ -254,12 +279,13 @@ public:
try
{
test::jtx::Env env{*this, makeConfig()};
if (makeTable(env, weeks(2), test, emptySection, emptySection))
if (makeTable(env, weeks(2), yesVotes, test, emptySection))
fail("Accepted short amendment ID");
}
catch (...)
catch (std::exception const& e)
{
pass();
BEAST_EXPECT(
e.what() == "Invalid entry '" + sid + " Name' in [Test]");
}
}
@@ -273,12 +299,13 @@ public:
try
{
test::jtx::Env env{*this, makeConfig()};
if (makeTable(env, weeks(2), test, emptySection, emptySection))
if (makeTable(env, weeks(2), yesVotes, test, emptySection))
fail("Accepted long amendment ID");
}
catch (...)
catch (std::exception const& e)
{
pass();
BEAST_EXPECT(
e.what() == "Invalid entry '" + sid + " Name' in [Test]");
}
}
@@ -293,12 +320,13 @@ public:
try
{
test::jtx::Env env{*this, makeConfig()};
if (makeTable(env, weeks(2), test, emptySection, emptySection))
if (makeTable(env, weeks(2), yesVotes, test, emptySection))
fail("Accepted non-hex amendment ID");
}
catch (...)
catch (std::exception const& e)
{
pass();
BEAST_EXPECT(
e.what() == "Invalid entry '" + sid + " Name' in [Test]");
}
}
}
@@ -311,27 +339,27 @@ public:
test::jtx::Env env{*this, makeConfig()};
std::unique_ptr<AmendmentTable> table = makeTable(env, weeks(2));
// Note which entries are enabled.
// Note which entries are enabled
std::set<uint256> allEnabled;
// Subset of amendments to enable
allEnabled.insert(amendmentId(supported_[0]));
allEnabled.insert(amendmentId(enabled_[0]));
allEnabled.insert(amendmentId(vetoed_[0]));
for (auto const& a : enabled_)
allEnabled.insert(amendmentId(a));
for (uint256 const& a : allEnabled)
table->enable(a);
BEAST_EXPECT(table->enable(a));
// So far all enabled amendments are supported.
BEAST_EXPECT(!table->hasUnsupportedEnabled());
// Verify all enables are enabled and nothing else.
for (std::string const& a : supported_)
for (std::string const& a : supportedYes_)
{
uint256 const supportedID = amendmentId(a);
BEAST_EXPECT(
table->isEnabled(supportedID) ==
(allEnabled.find(supportedID) != allEnabled.end()));
bool const enabled = table->isEnabled(supportedID);
bool const found = allEnabled.find(supportedID) != allEnabled.end();
BEAST_EXPECTS(
enabled == found,
a + (enabled ? " enabled " : " disabled ") +
(found ? " found" : " not found"));
}
// All supported and unVetoed amendments should be returned as desired.
@@ -347,14 +375,14 @@ public:
// 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(supportedYes_[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<uint256> const desired = table->getDesired();
BEAST_EXPECT(
@@ -363,7 +391,7 @@ public:
}
// Veto all supported amendments. Now desired should be empty.
for (std::string const& a : supported_)
for (std::string const& a : supportedYes_)
{
table->veto(amendmentId(a));
}
@@ -505,7 +533,7 @@ public:
test::jtx::Env env{*this};
auto table =
makeTable(env, weeks(2), emptySection, emptySection, emptySection);
makeTable(env, weeks(2), emptyYes, emptySection, emptySection);
std::vector<std::pair<uint256, int>> votes;
std::vector<uint256> ourVotes;
@@ -566,11 +594,7 @@ public:
test::jtx::Env env{*this};
auto table = makeTable(
env,
weeks(2),
emptySection,
emptySection,
makeSection(testAmendment));
env, weeks(2), emptyYes, emptySection, makeSection(testAmendment));
auto const validators = makeValidators(10);
@@ -629,7 +653,11 @@ public:
test::jtx::Env env{*this};
auto table = makeTable(
env, weeks(2), makeSection(supported_), emptySection, emptySection);
env,
weeks(2),
makeDefaultYes(supportedYes_),
emptySection,
emptySection);
auto const validators = makeValidators(10);
std::vector<std::pair<uint256, int>> votes;
@@ -647,13 +675,13 @@ public:
ourVotes,
enabled,
majority);
BEAST_EXPECT(ourVotes.size() == supported_.size());
BEAST_EXPECT(ourVotes.size() == supportedYes_.size());
BEAST_EXPECT(enabled.empty());
for (auto const& i : supported_)
for (auto const& i : supportedYes_)
BEAST_EXPECT(majority.find(amendmentId(i)) == majority.end());
// Now, everyone votes for this feature
for (auto const& i : supported_)
for (auto const& i : supportedYes_)
votes.emplace_back(amendmentId(i), validators.size());
// Week 2: We should recognize a majority
@@ -666,10 +694,10 @@ public:
ourVotes,
enabled,
majority);
BEAST_EXPECT(ourVotes.size() == supported_.size());
BEAST_EXPECT(ourVotes.size() == supportedYes_.size());
BEAST_EXPECT(enabled.empty());
for (auto const& i : supported_)
for (auto const& i : supportedYes_)
BEAST_EXPECT(majority[amendmentId(i)] == weekTime(weeks{2}));
// Week 5: We should enable the amendment
@@ -682,7 +710,7 @@ public:
ourVotes,
enabled,
majority);
BEAST_EXPECT(enabled.size() == supported_.size());
BEAST_EXPECT(enabled.size() == supportedYes_.size());
// Week 6: We should remove it from our votes and from having a majority
doRound(
@@ -694,9 +722,9 @@ public:
ourVotes,
enabled,
majority);
BEAST_EXPECT(enabled.size() == supported_.size());
BEAST_EXPECT(enabled.size() == supportedYes_.size());
BEAST_EXPECT(ourVotes.empty());
for (auto const& i : supported_)
for (auto const& i : supportedYes_)
BEAST_EXPECT(majority.find(amendmentId(i)) == majority.end());
}
@@ -711,7 +739,7 @@ public:
auto table = makeTable(
env,
weeks(2),
makeSection(testAmendment),
makeDefaultYes(testAmendment),
emptySection,
emptySection);
@@ -782,7 +810,7 @@ public:
auto table = makeTable(
env,
weeks(8),
makeSection(testAmendment),
makeDefaultYes(testAmendment),
emptySection,
emptySection);

View File

@@ -136,11 +136,10 @@ class TxQ1_test : public beast::unit_test::suite
for (auto i = env.current()->seq(); i <= 257; ++i)
env.close();
// The ledger after the flag ledger creates all the
// fee (1) and amendment (supportedAmendments().size())
// fee (1) and amendment (numUpVotedAmendments())
// 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::numUpVotedAmendments();
auto const flagMaxQueue = ledgersInQueue * flagPerLedger;
checkMetrics(env, 0, flagMaxQueue, 0, flagPerLedger, 256);
@@ -4157,8 +4156,7 @@ public:
if (!getMajorityAmendments(*env.closed()).empty())
break;
}
auto expectedPerLedger =
ripple::detail::supportedAmendments().size() + 1;
auto expectedPerLedger = ripple::detail::numUpVotedAmendments() + 1;
checkMetrics(env, 0, 5 * expectedPerLedger, 0, expectedPerLedger, 256);
// Now wait 2 weeks modulo 256 ledgers for the amendments to be
@@ -4213,79 +4211,49 @@ public:
// These particular amendments don't impact any of the queued
// transactions, so we won't see any change in the transaction
// outcomes. But code coverage is affected.
env.close(closeDuration);
expectedInQueue -= expectedPerLedger + 2;
++expectedPerLedger;
checkMetrics(
env,
expectedInQueue,
5 * expectedPerLedger,
expectedPerLedger + 1,
expectedPerLedger,
256);
do
{
auto const expectedPerAccount = expectedInQueue / 6;
auto const expectedRemainder = expectedInQueue % 6;
BEAST_EXPECT(env.seq(alice) == seqAlice - expectedPerAccount);
BEAST_EXPECT(
env.seq(bob) ==
seqBob - expectedPerAccount - (expectedRemainder > 4 ? 1 : 0));
BEAST_EXPECT(
env.seq(carol) ==
seqCarol - expectedPerAccount -
(expectedRemainder > 3 ? 1 : 0));
BEAST_EXPECT(
env.seq(daria) ==
seqDaria - expectedPerAccount -
(expectedRemainder > 2 ? 1 : 0));
BEAST_EXPECT(
env.seq(ellie) ==
seqEllie - expectedPerAccount -
(expectedRemainder > 1 ? 1 : 0));
BEAST_EXPECT(
env.seq(fiona) ==
seqFiona - expectedPerAccount -
(expectedRemainder > 0 ? 1 : 0));
}
env.close(closeDuration);
auto expectedInLedger = expectedInQueue;
expectedInQueue =
(expectedInQueue > expectedPerLedger + 2
? expectedInQueue - (expectedPerLedger + 2)
: 0);
++expectedPerLedger;
checkMetrics(
env,
0,
5 * expectedPerLedger,
expectedInLedger,
expectedPerLedger,
256);
{
auto const expectedPerAccount = expectedInQueue / 6;
auto const expectedRemainder = expectedInQueue % 6;
BEAST_EXPECT(env.seq(alice) == seqAlice - expectedPerAccount);
BEAST_EXPECT(
env.seq(bob) ==
seqBob - expectedPerAccount - (expectedRemainder > 4 ? 1 : 0));
BEAST_EXPECT(
env.seq(carol) ==
seqCarol - expectedPerAccount -
(expectedRemainder > 3 ? 1 : 0));
BEAST_EXPECT(
env.seq(daria) ==
seqDaria - expectedPerAccount -
(expectedRemainder > 2 ? 1 : 0));
BEAST_EXPECT(
env.seq(ellie) ==
seqEllie - expectedPerAccount -
(expectedRemainder > 1 ? 1 : 0));
BEAST_EXPECT(
env.seq(fiona) ==
seqFiona - expectedPerAccount -
(expectedRemainder > 0 ? 1 : 0));
}
env.close(closeDuration);
auto expectedInLedger = expectedInQueue;
expectedInQueue =
(expectedInQueue > expectedPerLedger + 2
? expectedInQueue - (expectedPerLedger + 2)
: 0);
expectedInLedger -= expectedInQueue;
++expectedPerLedger;
checkMetrics(
env,
expectedInQueue,
5 * expectedPerLedger,
expectedInLedger,
expectedPerLedger,
256);
{
auto const expectedPerAccount = expectedInQueue / 6;
auto const expectedRemainder = expectedInQueue % 6;
BEAST_EXPECT(env.seq(alice) == seqAlice - expectedPerAccount);
BEAST_EXPECT(
env.seq(bob) ==
seqBob - expectedPerAccount -
(expectedRemainder > 4 ? 1 : 0));
BEAST_EXPECT(
env.seq(carol) ==
seqCarol - expectedPerAccount -
(expectedRemainder > 3 ? 1 : 0));
BEAST_EXPECT(
env.seq(daria) ==
seqDaria - expectedPerAccount -
(expectedRemainder > 2 ? 1 : 0));
BEAST_EXPECT(
env.seq(ellie) ==
seqEllie - expectedPerAccount -
(expectedRemainder > 1 ? 1 : 0));
BEAST_EXPECT(
env.seq(fiona) ==
seqFiona - expectedPerAccount -
(expectedRemainder > 0 ? 1 : 0));
}
} while (expectedInQueue > 0);
}
void

View File

@@ -73,8 +73,9 @@ supported_amendments()
auto const& sa = ripple::detail::supportedAmendments();
std::vector<uint256> feats;
feats.reserve(sa.size());
for (auto const& s : sa)
for (auto const& [s, vote] : sa)
{
(void)vote;
if (auto const f = getRegisteredFeature(s))
feats.push_back(*f);
else

View File

@@ -812,6 +812,7 @@ public:
auto const missingSomeFeatures =
supported_amendments() - featureMultiSignReserve - featureFlow;
BEAST_EXPECT(missingSomeFeatures.count() == (supported.count() - 2));
{
// a Env supported_features_except is missing *only* those features
Env env{*this, missingSomeFeatures};

View File

@@ -26,6 +26,77 @@ namespace ripple {
class Feature_test : public beast::unit_test::suite
{
void
testInternals()
{
testcase("internals");
std::map<std::string, DefaultVote> const& supported =
ripple::detail::supportedAmendments();
BEAST_EXPECT(
supported.size() ==
ripple::detail::numDownVotedAmendments() +
ripple::detail::numUpVotedAmendments());
std::size_t up = 0, down = 0;
for (std::pair<std::string const, DefaultVote> const& amendment :
supported)
{
if (amendment.second == DefaultVote::no)
++down;
else
{
if (BEAST_EXPECT(amendment.second == DefaultVote::yes))
++up;
}
}
BEAST_EXPECT(down == ripple::detail::numDownVotedAmendments());
BEAST_EXPECT(up == ripple::detail::numUpVotedAmendments());
}
void
testFeatureLookups()
{
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, vote] : supported)
{
(void)vote;
auto const registered = getRegisteredFeature(feature);
if (BEAST_EXPECT(registered))
{
BEAST_EXPECT(featureToName(*registered) == feature);
BEAST_EXPECT(
bitsetIndexToFeature(featureToBitsetIndex(*registered)) ==
*registered);
}
}
// Test an arbitrary unknown feature
uint256 zero{0};
BEAST_EXPECT(featureToName(zero) == to_string(zero));
BEAST_EXPECT(
featureToName(zero) ==
"0000000000000000000000000000000000000000000000000000000000000000");
// Test looking up an unknown feature
BEAST_EXPECT(!getRegisteredFeature("unknown"));
// Test a random sampling of the variables. If any of these get retired
// or removed, swap out for any other feature.
BEAST_EXPECT(featureToName(featureOwnerPaysFee) == "OwnerPaysFee");
BEAST_EXPECT(featureToName(featureFlow) == "Flow");
BEAST_EXPECT(featureToName(featureNegativeUNL) == "NegativeUNL");
BEAST_EXPECT(featureToName(fix1578) == "fix1578");
BEAST_EXPECT(
featureToName(fixTakerDryOfferRemoval) ==
"fixTakerDryOfferRemoval");
}
void
testNoParams()
{
@@ -34,6 +105,9 @@ class Feature_test : public beast::unit_test::suite
using namespace test::jtx;
Env env{*this};
std::map<std::string, DefaultVote> const& votes =
ripple::detail::supportedAmendments();
auto jrr = env.rpc("feature")[jss::result];
if (!BEAST_EXPECT(jrr.isMember(jss::features)))
return;
@@ -41,13 +115,15 @@ 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 =
!(votes.at(feature[jss::name].asString()) == DefaultVote::yes);
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(),
@@ -67,6 +143,9 @@ class Feature_test : public beast::unit_test::suite
BEAST_EXPECTS(jrr[jss::status] == jss::success, "status");
jrr.removeMember(jss::status);
BEAST_EXPECT(jrr.size() == 1);
BEAST_EXPECT(
jrr.isMember("586480873651E106F1D6339B0C4A8945BA705A777F3F4524626FF"
"1FC07EFE41D"));
auto feature = *(jrr.begin());
BEAST_EXPECTS(feature[jss::name] == "MultiSignReserve", "name");
@@ -121,6 +200,9 @@ class Feature_test : public beast::unit_test::suite
Env env{
*this, FeatureBitset(featureDepositAuth, featureDepositPreauth)};
std::map<std::string, DefaultVote> const& votes =
ripple::detail::supportedAmendments();
auto jrr = env.rpc("feature")[jss::result];
if (!BEAST_EXPECT(jrr.isMember(jss::features)))
return;
@@ -135,11 +217,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 =
!(votes.at((*it)[jss::name].asString()) == DefaultVote::yes);
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 +282,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::map<std::string, DefaultVote> const& votes =
ripple::detail::supportedAmendments();
jrr = env.rpc("feature")[jss::result];
if (!BEAST_EXPECT(jrr.isMember(jss::features)))
@@ -206,9 +292,15 @@ class Feature_test : public beast::unit_test::suite
{
if (!BEAST_EXPECT(feature.isMember(jss::name)))
return;
bool expectVeto =
!(votes.at(feature[jss::name].asString()) == DefaultVote::yes);
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 +310,12 @@ 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] == 2540);
BEAST_EXPECTS(
expectVeto || feature[jss::majority] == 2540,
"Majority: " + feature[jss::majority].asString());
}
}
@@ -273,6 +367,8 @@ public:
void
run() override
{
testInternals();
testFeatureLookups();
testNoParams();
testSingleFeature();
testInvalidFeature();