AmendmentTable improvements

src/ripple/app/misc/AmendmentTable.h

@@ -37,7 +37,7 @@ public:
     virtual ~AmendmentTable() = default;
 
     virtual uint256
-    find(std::string const& name) = 0;
+    find(std::string const& name) const = 0;
 
     virtual bool
     veto(uint256 const& amendment) = 0;
@@ -46,13 +46,11 @@ public:
 
     virtual bool
     enable(uint256 const& amendment) = 0;
-    virtual bool
-    disable(uint256 const& amendment) = 0;
 
     virtual bool
-    isEnabled(uint256 const& amendment) = 0;
+    isEnabled(uint256 const& amendment) const = 0;
     virtual bool
-    isSupported(uint256 const& amendment) = 0;
+    isSupported(uint256 const& amendment) const = 0;
 
     /**
      * @brief returns true if one or more amendments on the network
@@ -61,16 +59,17 @@ public:
      * @return true if an unsupported feature is enabled on the network
      */
     virtual bool
-    hasUnsupportedEnabled() = 0;
+    hasUnsupportedEnabled() const = 0;
+
     virtual boost::optional<NetClock::time_point>
-    firstUnsupportedExpected() = 0;
+    firstUnsupportedExpected() const = 0;
 
     virtual Json::Value
-    getJson(int) = 0;
+    getJson() const = 0;
 
     /** Returns a Json::objectValue. */
     virtual Json::Value
-    getJson(uint256 const&) = 0;
+    getJson(uint256 const& amendment) const = 0;
 
     /** Called when a new fully-validated ledger is accepted. */
     void
@@ -88,7 +87,7 @@ public:
         a new validated ledger. (If it could have changed things.)
     */
     virtual bool
-    needValidatedLedger(LedgerIndex seq) = 0;
+    needValidatedLedger(LedgerIndex seq) const = 0;
 
     virtual void
     doValidatedLedger(
@@ -108,14 +107,14 @@ public:
     // Called by the consensus code when we need to
     // add feature entries to a validation
     virtual std::vector<uint256>
-    doValidation(std::set<uint256> const& enabled) = 0;
+    doValidation(std::set<uint256> const& enabled) const = 0;
 
     // The set of amendments to enable in the genesis ledger
     // This will return all known, non-vetoed amendments.
     // If we ever have two amendments that should not both be
     // enabled at the same time, we should ensure one is vetoed.
     virtual std::vector<uint256>
-    getDesired() = 0;
+    getDesired() const = 0;
 
     // The function below adapts the API callers expect to the
     // internal amendment table API. This allows the amendment

src/ripple/app/misc/README.md

@@ -71,8 +71,8 @@ Amendment must receive at least an 80% approval rate from validating nodes for
 a period of two weeks before being accepted. The following example outlines the
 process of an Amendment from its conception to approval and usage.
 
-*  A community member makes proposes to change transaction processing in some 
+*  A community member proposes to change transaction processing in some way.
-  way. The proposal is discussed amongst the community and receives its support 
+  The proposal is discussed amongst the community and receives its support
   creating a community or human consensus.
 
 *  Some members contribute their time and work to develop the Amendment.
@@ -101,7 +101,7 @@ the majority status from the ledger.
 If an amendment holds majority status for two weeks, validators will
 introduce a pseudo-transaction to enable the amendment.
 
-All amednements are assumed to be critical and irreversible. Thus there
+All amendments are assumed to be critical and irreversible. Thus there
 is no mechanism to disable or revoke an amendment, nor is there a way
 for a server to operate while an amendment it does not understand is
 enabled.

src/ripple/app/misc/impl/AmendmentTable.cpp

@@ -139,7 +139,7 @@ public:
 class AmendmentTableImpl final : public AmendmentTable
 {
 protected:
-    std::mutex mutex_;
+    mutable std::mutex mutex_;
 
     hash_map<uint256, AmendmentState> amendmentMap_;
     std::uint32_t lastUpdateSeq_;
@@ -157,6 +157,7 @@ protected:
 
     // True if an unsupported amendment is enabled
     bool unsupportedEnabled_;
+
     // Unset if no unsupported amendments reach majority,
     // else set to the earliest time an unsupported amendment
     // will be enabled.
@@ -164,16 +165,24 @@ protected:
 
     beast::Journal const j_;
 
-    // Finds or creates state
+    // Finds or creates state.  Must be called with mutex_ locked.
     AmendmentState*
-    add(uint256 const& amendment);
+    add(uint256 const& amendment, std::lock_guard<std::mutex> const& sl);
 
-    // Finds existing state
+    // Finds existing state.  Must be called with mutex_ locked.
     AmendmentState*
-    get(uint256 const& amendment);
+    get(uint256 const& amendment, std::lock_guard<std::mutex> const& sl);
 
+    AmendmentState const*
+    get(uint256 const& amendment, std::lock_guard<std::mutex> const& sl) const;
+
+    // Injects amendment json into v.  Must be called with mutex_ locked.
     void
-    setJson(Json::Value& v, uint256 const& amendment, const AmendmentState&);
+    injectJson(
+        Json::Value& v,
+        uint256 const& amendment,
+        AmendmentState const& state,
+        std::lock_guard<std::mutex> const& sl) const;
 
 public:
     AmendmentTableImpl(
@@ -185,7 +194,7 @@ public:
         beast::Journal journal);
 
     uint256
-    find(std::string const& name) override;
+    find(std::string const& name) const override;
 
     bool
     veto(uint256 const& amendment) override;
@@ -194,26 +203,25 @@ public:
 
     bool
     enable(uint256 const& amendment) override;
-    bool
-    disable(uint256 const& amendment) override;
 
     bool
-    isEnabled(uint256 const& amendment) override;
+    isEnabled(uint256 const& amendment) const override;
     bool
-    isSupported(uint256 const& amendment) override;
+    isSupported(uint256 const& amendment) const override;
 
     bool
-    hasUnsupportedEnabled() override;
+    hasUnsupportedEnabled() const override;
+
     boost::optional<NetClock::time_point>
-    firstUnsupportedExpected() override;
+    firstUnsupportedExpected() const override;
 
     Json::Value
-    getJson(int) override;
+    getJson() const override;
     Json::Value
-    getJson(uint256 const&) override;
+    getJson(uint256 const&) const override;
 
     bool
-    needValidatedLedger(LedgerIndex seq) override;
+    needValidatedLedger(LedgerIndex seq) const override;
 
     void
     doValidatedLedger(
@@ -222,10 +230,10 @@ public:
         majorityAmendments_t const& majority) override;
 
     std::vector<uint256>
-    doValidation(std::set<uint256> const& enabledAmendments) override;
+    doValidation(std::set<uint256> const& enabledAmendments) const override;
 
     std::vector<uint256>
-    getDesired() override;
+    getDesired() const override;
 
     std::map<uint256, std::uint32_t>
     doVoting(
@@ -256,7 +264,7 @@ AmendmentTableImpl::AmendmentTableImpl(
 
     for (auto const& a : parseSection(supported))
     {
-        if (auto s = add(a.first))
+        if (auto s = add(a.first, sl))
         {
             JLOG(j_.debug()) << "Amendment " << a.first << " is supported.";
 
@@ -269,7 +277,7 @@ AmendmentTableImpl::AmendmentTableImpl(
 
     for (auto const& a : parseSection(enabled))
     {
-        if (auto s = add(a.first))
+        if (auto s = add(a.first, sl))
         {
             JLOG(j_.debug()) << "Amendment " << a.first << " is enabled.";
 
@@ -284,7 +292,7 @@ AmendmentTableImpl::AmendmentTableImpl(
     for (auto const& a : parseSection(vetoed))
     {
         // Unknown amendments are effectively vetoed already
-        if (auto s = get(a.first))
+        if (auto s = get(a.first, sl))
         {
             JLOG(j_.info()) << "Amendment " << a.first << " is vetoed.";
 
@@ -297,14 +305,28 @@ AmendmentTableImpl::AmendmentTableImpl(
 }
 
 AmendmentState*
-AmendmentTableImpl::add(uint256 const& amendmentHash)
+AmendmentTableImpl::add(
+    uint256 const& amendmentHash,
+    std::lock_guard<std::mutex> const&)
 {
     // call with the mutex held
     return &amendmentMap_[amendmentHash];
 }
 
 AmendmentState*
-AmendmentTableImpl::get(uint256 const& amendmentHash)
+AmendmentTableImpl::get(
+    uint256 const& amendmentHash,
+    std::lock_guard<std::mutex> const& sl)
+{
+    // Forward to the const version of get.
+    return const_cast<AmendmentState*>(
+        std::as_const(*this).get(amendmentHash, sl));
+}
+
+AmendmentState const*
+AmendmentTableImpl::get(
+    uint256 const& amendmentHash,
+    std::lock_guard<std::mutex> const&) const
 {
     // call with the mutex held
     auto ret = amendmentMap_.find(amendmentHash);
@@ -316,7 +338,7 @@ AmendmentTableImpl::get(uint256 const& amendmentHash)
 }
 
 uint256
-AmendmentTableImpl::find(std::string const& name)
+AmendmentTableImpl::find(std::string const& name) const
 {
     std::lock_guard sl(mutex_);
 
@@ -333,7 +355,7 @@ bool
 AmendmentTableImpl::veto(uint256 const& amendment)
 {
     std::lock_guard sl(mutex_);
-    auto s = add(amendment);
+    auto s = add(amendment, sl);
 
     if (s->vetoed)
         return false;
@@ -345,7 +367,7 @@ bool
 AmendmentTableImpl::unVeto(uint256 const& amendment)
 {
     std::lock_guard sl(mutex_);
-    auto s = get(amendment);
+    auto s = get(amendment, sl);
 
     if (!s || !s->vetoed)
         return false;
@@ -357,7 +379,7 @@ bool
 AmendmentTableImpl::enable(uint256 const& amendment)
 {
     std::lock_guard sl(mutex_);
-    auto s = add(amendment);
+    auto s = add(amendment, sl);
 
     if (s->enabled)
         return false;
@@ -375,58 +397,45 @@ AmendmentTableImpl::enable(uint256 const& amendment)
 }
 
 bool
-AmendmentTableImpl::disable(uint256 const& amendment)
+AmendmentTableImpl::isEnabled(uint256 const& amendment) const
 {
     std::lock_guard sl(mutex_);
-    auto s = get(amendment);
+    auto s = get(amendment, sl);
-
-    if (!s || !s->enabled)
-        return false;
-
-    s->enabled = false;
-    return true;
-}
-
-bool
-AmendmentTableImpl::isEnabled(uint256 const& amendment)
-{
-    std::lock_guard sl(mutex_);
-    auto s = get(amendment);
     return s && s->enabled;
 }
 
 bool
-AmendmentTableImpl::isSupported(uint256 const& amendment)
+AmendmentTableImpl::isSupported(uint256 const& amendment) const
 {
     std::lock_guard sl(mutex_);
-    auto s = get(amendment);
+    auto s = get(amendment, sl);
     return s && s->supported;
 }
 
 bool
-AmendmentTableImpl::hasUnsupportedEnabled()
+AmendmentTableImpl::hasUnsupportedEnabled() const
 {
     std::lock_guard sl(mutex_);
     return unsupportedEnabled_;
 }
 
 boost::optional<NetClock::time_point>
-AmendmentTableImpl::firstUnsupportedExpected()
+AmendmentTableImpl::firstUnsupportedExpected() const
 {
     std::lock_guard sl(mutex_);
     return firstUnsupportedExpected_;
 }
 
 std::vector<uint256>
-AmendmentTableImpl::doValidation(std::set<uint256> const& enabled)
+AmendmentTableImpl::doValidation(std::set<uint256> const& enabled) const
 {
     // Get the list of amendments we support and do not
     // veto, but that are not already enabled
     std::vector<uint256> amendments;
-    amendments.reserve(amendmentMap_.size());
 
     {
         std::lock_guard sl(mutex_);
+        amendments.reserve(amendmentMap_.size());
         for (auto const& e : amendmentMap_)
         {
             if (e.second.supported && !e.second.vetoed &&
@@ -444,7 +453,7 @@ AmendmentTableImpl::doValidation(std::set<uint256> const& enabled)
 }
 
 std::vector<uint256>
-AmendmentTableImpl::getDesired()
+AmendmentTableImpl::getDesired() const
 {
     // Get the list of amendments we support and do not veto
     return doValidation({});
@@ -491,7 +500,6 @@ AmendmentTableImpl::doVoting(
     // the value of the flags in the pseudo-transaction
     std::map<uint256, std::uint32_t> actions;
 
-    {
     std::lock_guard sl(mutex_);
 
     // process all amendments we know of
@@ -510,8 +518,7 @@ AmendmentTableImpl::doVoting(
 
         if (enabledAmendments.count(entry.first) != 0)
         {
-                JLOG(j_.debug())
+            JLOG(j_.debug()) << entry.first << ": amendment already enabled";
-                    << entry.first << ": amendment already enabled";
         }
         else if (
             hasValMajority && (majorityTime == NetClock::time_point{}) &&
@@ -521,8 +528,7 @@ AmendmentTableImpl::doVoting(
             JLOG(j_.debug()) << entry.first << ": amendment got majority";
             actions[entry.first] = tfGotMajority;
         }
-            else if (
+        else if (!hasValMajority && (majorityTime != NetClock::time_point{}))
-                !hasValMajority && (majorityTime != NetClock::time_point{}))
         {
             // Ledger says majority, validators say no
             JLOG(j_.debug()) << entry.first << ": amendment lost majority";
@@ -541,13 +547,11 @@ AmendmentTableImpl::doVoting(
 
     // Stash for reporting
     lastVote_ = std::move(vote);
-    }
-
     return actions;
 }
 
 bool
-AmendmentTableImpl::needValidatedLedger(LedgerIndex ledgerSeq)
+AmendmentTableImpl::needValidatedLedger(LedgerIndex ledgerSeq) const
 {
     std::lock_guard sl(mutex_);
 
@@ -567,13 +571,17 @@ AmendmentTableImpl::doValidatedLedger(
         enable(e);
 
     std::lock_guard sl(mutex_);
-    // Since we have the whole list in `majority`, reset the time flag, even if
+
-    // it's currently set. If it's not set when the loop is done, then any
+    // Remember the ledger sequence of this update.
+    lastUpdateSeq_ = ledgerSeq;
+
+    // Since we have the whole list in `majority`, reset the time flag, even
+    // if it's currently set. If it's not set when the loop is done, then any
     // prior unknown amendments have lost majority.
     firstUnsupportedExpected_.reset();
     for (auto const& [hash, time] : majority)
     {
-        auto s = add(hash);
+        auto s = add(hash, sl);
 
         if (s->enabled)
             continue;
@@ -591,10 +599,11 @@ AmendmentTableImpl::doValidatedLedger(
 }
 
 void
-AmendmentTableImpl::setJson(
+AmendmentTableImpl::injectJson(
     Json::Value& v,
     const uint256& id,
-    const AmendmentState& fs)
+    const AmendmentState& fs,
+    std::lock_guard<std::mutex> const&) const
 {
     if (!fs.name.empty())
         v[jss::name] = fs.name;
@@ -621,30 +630,34 @@ AmendmentTableImpl::setJson(
 }
 
 Json::Value
-AmendmentTableImpl::getJson(int)
+AmendmentTableImpl::getJson() const
 {
     Json::Value ret(Json::objectValue);
     {
         std::lock_guard sl(mutex_);
         for (auto const& e : amendmentMap_)
         {
-            setJson(
+            injectJson(
-                ret[to_string(e.first)] = Json::objectValue, e.first, e.second);
+                ret[to_string(e.first)] = Json::objectValue,
+                e.first,
+                e.second,
+                sl);
         }
     }
     return ret;
 }
 
 Json::Value
-AmendmentTableImpl::getJson(uint256 const& amendmentID)
+AmendmentTableImpl::getJson(uint256 const& amendmentID) const
 {
     Json::Value ret = Json::objectValue;
     Json::Value& jAmendment = (ret[to_string(amendmentID)] = Json::objectValue);
 
     {
         std::lock_guard sl(mutex_);
-        auto a = add(amendmentID);
+        auto a = get(amendmentID, sl);
-        setJson(jAmendment, amendmentID, *a);
+        if (a)
+            injectJson(jAmendment, amendmentID, *a, sl);
     }
 
     return ret;

src/ripple/rpc/handlers/Feature1.cpp

@@ -45,7 +45,7 @@ doFeature(RPC::JsonContext& context)
 
     if (!context.params.isMember(jss::feature))
     {
-        auto features = table.getJson(0);
+        auto features = table.getJson();
 
         for (auto const& [h, t] : majorities)
         {
@@ -67,9 +67,9 @@ doFeature(RPC::JsonContext& context)
     if (context.params.isMember(jss::vetoed))
     {
         if (context.params[jss::vetoed].asBool())
-            context.app.getAmendmentTable().veto(feature);
+            table.veto(feature);
         else
-            context.app.getAmendmentTable().unVeto(feature);
+            table.unVeto(feature);
     }
 
     Json::Value jvReply = table.getJson(feature);

src/test/app/AmendmentTable_test.cpp

@@ -29,6 +29,8 @@
 #include <ripple/protocol/SecretKey.h>
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/digest.h>
+#include <ripple/protocol/jss.h>
+#include <test/jtx/Env.h>
 #include <test/unit_test/SuiteJournal.h>
 
 namespace ripple {
@@ -51,16 +53,6 @@ private:
         return result;
     }
 
-    static std::vector<std::string>
-    createSet(int group, int count)
-    {
-        std::vector<std::string> amendments;
-        for (int i = 0; i < count; i++)
-            amendments.push_back(
-                "Amendment" + std::to_string((1000000 * group) + i));
-        return amendments;
-    }
-
     static Section
     makeSection(std::vector<std::string> const& amendments)
     {
@@ -78,43 +70,52 @@ private:
         return section;
     }
 
-    std::vector<std::string> const m_set1;
+    // All useful amendments are supported amendments.
-    std::vector<std::string> const m_set2;
+    // Enabled amendments are typically a subset of supported amendments.
-    std::vector<std::string> const m_set3;
+    // Vetoed amendments should be supported but not enabled.
-    std::vector<std::string> const m_set4;
+    // Unsupported amendments may be added to the AmendmentTable.
-    std::vector<std::string> const m_set5;
+    std::vector<std::string> const supported_{
+        "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
+        enabled_{"b", "d", "f", "h", "j", "l", "n", "p"};
+    std::vector<std::string> const vetoed_{"a", "c", "e"};
+    std::vector<std::string> const unsupported_{"v", "w", "x"};
+    std::vector<std::string> const unsupportedMajority_{"y", "z"};
 
     Section const emptySection;
 
     test::SuiteJournal journal;
 
 public:
-    AmendmentTable_test()
+    AmendmentTable_test() : journal("AmendmentTable_test", *this)
-        : m_set1(createSet(1, 12))
-        , m_set2(createSet(2, 12))
-        , m_set3(createSet(3, 12))
-        , m_set4(createSet(4, 12))
-        , m_set5(createSet(5, 12))
-        , journal("AmendmentTable_test", *this)
     {
     }
 
     std::unique_ptr<AmendmentTable>
     makeTable(
-        int w,
+        std::chrono::seconds majorityTime,
         Section const supported,
         Section const enabled,
         Section const vetoed)
     {
         return make_AmendmentTable(
-            weeks(w), majorityFraction, supported, enabled, vetoed, journal);
+            majorityTime,
+            majorityFraction,
+            supported,
+            enabled,
+            vetoed,
+            journal);
     }
 
     std::unique_ptr<AmendmentTable>
-    makeTable(int w)
+    makeTable(std::chrono::seconds majorityTime)
     {
         return makeTable(
-            w, makeSection(m_set1), makeSection(m_set2), makeSection(m_set3));
+            majorityTime,
+            makeSection(supported_),
+            makeSection(enabled_),
+            makeSection(vetoed_));
     }
 
     void
@@ -122,23 +123,22 @@ public:
     {
         testcase("Construction");
 
-        auto table = makeTable(1);
+        auto table = makeTable(weeks(1));
 
-        for (auto const& a : m_set1)
+        for (auto const& a : supported_)
         {
             BEAST_EXPECT(table->isSupported(amendmentId(a)));
-            BEAST_EXPECT(!table->isEnabled(amendmentId(a)));
         }
 
-        for (auto const& a : m_set2)
+        for (auto const& a : enabled_)
         {
             BEAST_EXPECT(table->isSupported(amendmentId(a)));
             BEAST_EXPECT(table->isEnabled(amendmentId(a)));
         }
 
-        for (auto const& a : m_set3)
+        for (auto const& a : vetoed_)
         {
-            BEAST_EXPECT(!table->isSupported(amendmentId(a)));
+            BEAST_EXPECT(table->isSupported(amendmentId(a)));
             BEAST_EXPECT(!table->isEnabled(amendmentId(a)));
         }
     }
@@ -148,26 +148,43 @@ public:
     {
         testcase("Name to ID mapping");
 
-        auto table = makeTable(1);
+        auto table = makeTable(weeks(1));
 
-        for (auto const& a : m_set1)
+        for (auto const& a : supported_)
             BEAST_EXPECT(table->find(a) == amendmentId(a));
-        for (auto const& a : m_set2)
+        for (auto const& a : enabled_)
             BEAST_EXPECT(table->find(a) == amendmentId(a));
 
-        for (auto const& a : m_set3)
+        for (auto const& a : vetoed_)
+            BEAST_EXPECT(table->find(a) == amendmentId(a));
+        for (auto const& a : unsupported_)
             BEAST_EXPECT(!table->find(a));
-        for (auto const& a : m_set4)
+        for (auto const& a : unsupportedMajority_)
-            BEAST_EXPECT(!table->find(a));
-        for (auto const& a : m_set5)
             BEAST_EXPECT(!table->find(a));
+
+        // Vetoing an unsupported amendment should add the amendment to table.
+        // Verify that unsupportedID is not in table.
+        uint256 const unsupportedID = amendmentId(unsupported_[0]);
+        {
+            Json::Value const unsupp =
+                table->getJson(unsupportedID)[to_string(unsupportedID)];
+            BEAST_EXPECT(unsupp.size() == 0);
+        }
+
+        // After vetoing unsupportedID verify that it is in table.
+        table->veto(unsupportedID);
+        {
+            Json::Value const unsupp =
+                table->getJson(unsupportedID)[to_string(unsupportedID)];
+            BEAST_EXPECT(unsupp[jss::vetoed].asBool());
+        }
     }
 
     void
     testBadConfig()
     {
-        auto const section = makeSection(m_set1);
+        auto const section = makeSection(supported_);
-        auto const id = to_string(amendmentId(m_set2[0]));
+        auto const id = to_string(amendmentId(enabled_[0]));
 
         testcase("Bad Config");
 
@@ -177,7 +194,7 @@ public:
 
             try
             {
-                if (makeTable(2, test, emptySection, emptySection))
+                if (makeTable(weeks(2), test, emptySection, emptySection))
                     fail("Accepted only amendment ID");
             }
             catch (...)
@@ -192,7 +209,7 @@ public:
 
             try
             {
-                if (makeTable(2, test, emptySection, emptySection))
+                if (makeTable(weeks(2), test, emptySection, emptySection))
                     fail("Accepted extra arguments");
             }
             catch (...)
@@ -210,7 +227,7 @@ public:
 
             try
             {
-                if (makeTable(2, test, emptySection, emptySection))
+                if (makeTable(weeks(2), test, emptySection, emptySection))
                     fail("Accepted short amendment ID");
             }
             catch (...)
@@ -228,7 +245,7 @@ public:
 
             try
             {
-                if (makeTable(2, test, emptySection, emptySection))
+                if (makeTable(weeks(2), test, emptySection, emptySection))
                     fail("Accepted long amendment ID");
             }
             catch (...)
@@ -247,7 +264,7 @@ public:
 
             try
             {
-                if (makeTable(2, test, emptySection, emptySection))
+                if (makeTable(weeks(2), test, emptySection, emptySection))
                     fail("Accepted non-hex amendment ID");
             }
             catch (...)
@@ -257,77 +274,82 @@ public:
         }
     }
 
-    std::map<uint256, bool>
-    getState(AmendmentTable* table, std::set<uint256> const& exclude)
-    {
-        std::map<uint256, bool> state;
-
-        auto track = [&state, table](std::vector<std::string> const& v) {
-            for (auto const& a : v)
-            {
-                auto const id = amendmentId(a);
-                state[id] = table->isEnabled(id);
-            }
-        };
-
-        track(m_set1);
-        track(m_set2);
-        track(m_set3);
-        track(m_set4);
-        track(m_set5);
-
-        for (auto const& a : exclude)
-            state.erase(a);
-
-        return state;
-    }
-
     void
-    testEnableDisable()
+    testEnableVeto()
     {
-        testcase("enable & disable");
+        testcase("enable and veto");
 
-        auto const testAmendment = amendmentId("TestAmendment");
+        std::unique_ptr<AmendmentTable> table = makeTable(weeks(2));
-        auto table = makeTable(2);
 
-        // Subset of amendments to enable
+        // Note which entries are pre-enabled.
-        std::set<uint256> enabled;
+        std::set<uint256> allEnabled;
-        enabled.insert(testAmendment);
+        for (std::string const& a : enabled_)
-        enabled.insert(amendmentId(m_set1[0]));
+            allEnabled.insert(amendmentId(a));
-        enabled.insert(amendmentId(m_set2[0]));
-        enabled.insert(amendmentId(m_set3[0]));
-        enabled.insert(amendmentId(m_set4[0]));
-        enabled.insert(amendmentId(m_set5[0]));
 
-        // Get the state before, excluding the items we'll change:
+        // Subset of amendments to late-enable
-        auto const pre_state = getState(table.get(), enabled);
+        std::set<uint256> lateEnabled;
+        lateEnabled.insert(amendmentId(supported_[0]));
+        lateEnabled.insert(amendmentId(enabled_[0]));
+        lateEnabled.insert(amendmentId(vetoed_[0]));
 
-        // Enable the subset and verify
+        // Do the late enabling.
-        for (auto const& a : enabled)
+        for (uint256 const& a : lateEnabled)
             table->enable(a);
 
-        for (auto const& a : enabled)
+        // So far all enabled amendments are supported.
-            BEAST_EXPECT(table->isEnabled(a));
+        BEAST_EXPECT(!table->hasUnsupportedEnabled());
 
-        // Disable the subset and verify
+        // Verify all pre- and late-enables are enabled and nothing else.
-        for (auto const& a : enabled)
+        allEnabled.insert(lateEnabled.begin(), lateEnabled.end());
-            table->disable(a);
+        for (std::string const& a : supported_)
+        {
+            uint256 const supportedID = amendmentId(a);
+            BEAST_EXPECT(
+                table->isEnabled(supportedID) ==
+                (allEnabled.find(supportedID) != allEnabled.end()));
+        }
 
-        for (auto const& a : enabled)
+        // All supported and unVetoed amendments should be returned as desired.
-            BEAST_EXPECT(!table->isEnabled(a));
+        {
+            std::set<uint256> vetoed;
+            for (std::string const& a : vetoed_)
+                vetoed.insert(amendmentId(a));
 
-        // Get the state after, excluding the items we changed:
+            std::vector<uint256> const desired = table->getDesired();
-        auto const post_state = getState(table.get(), enabled);
+            for (uint256 const& a : desired)
+                BEAST_EXPECT(vetoed.count(a) == 0);
 
-        // Ensure the states are identical
+            // Unveto an amendment that is already not vetoed.  Shouldn't
-        auto ret = std::mismatch(
+            // hurt anything, but the values returned by getDesired()
-            pre_state.begin(),
+            // shouldn't change.
-            pre_state.end(),
+            table->unVeto(amendmentId(supported_[1]));
-            post_state.begin(),
+            BEAST_EXPECT(desired == table->getDesired());
-            post_state.end());
+        }
 
-        BEAST_EXPECT(ret.first == pre_state.end());
+        // UnVeto one of the vetoed amendments.  It should now be desired.
-        BEAST_EXPECT(ret.second == post_state.end());
+        {
+            uint256 const unvetoedID = amendmentId(vetoed_[0]);
+            table->unVeto(unvetoedID);
+
+            std::vector<uint256> const desired = table->getDesired();
+            BEAST_EXPECT(
+                std::find(desired.begin(), desired.end(), unvetoedID) !=
+                desired.end());
+        }
+
+        // Veto all supported amendments.  Now desired should be empty.
+        for (std::string const& a : supported_)
+        {
+            table->veto(amendmentId(a));
+        }
+        BEAST_EXPECT(table->getDesired().empty());
+
+        // Enable an unsupported amendment.
+        {
+            BEAST_EXPECT(!table->hasUnsupportedEnabled());
+            table->enable(amendmentId(unsupported_[0]));
+            BEAST_EXPECT(table->hasUnsupportedEnabled());
+        }
     }
 
     std::vector<std::pair<PublicKey, SecretKey>>
@@ -456,7 +478,8 @@ public:
         auto const testAmendment = amendmentId("TestAmendment");
         auto const validators = makeValidators(10);
 
-        auto table = makeTable(2, emptySection, emptySection, emptySection);
+        auto table =
+            makeTable(weeks(2), emptySection, emptySection, emptySection);
 
         std::vector<std::pair<uint256, int>> votes;
         std::vector<uint256> ourVotes;
@@ -495,7 +518,7 @@ public:
         auto const testAmendment = amendmentId("vetoedAmendment");
 
         auto table = makeTable(
-            2, emptySection, emptySection, makeSection(testAmendment));
+            weeks(2), emptySection, emptySection, makeSection(testAmendment));
 
         auto const validators = makeValidators(10);
 
@@ -531,8 +554,8 @@ public:
     {
         testcase("voteEnable");
 
-        auto table =
+        auto table = makeTable(
-            makeTable(2, makeSection(m_set1), emptySection, emptySection);
+            weeks(2), makeSection(supported_), emptySection, emptySection);
 
         auto const validators = makeValidators(10);
         std::vector<std::pair<uint256, int>> votes;
@@ -543,35 +566,35 @@ public:
         // Week 1: We should vote for all known amendments not enabled
         doRound(
             *table, weeks{1}, validators, votes, ourVotes, enabled, majority);
-        BEAST_EXPECT(ourVotes.size() == m_set1.size());
+        BEAST_EXPECT(ourVotes.size() == supported_.size());
         BEAST_EXPECT(enabled.empty());
-        for (auto const& i : m_set1)
+        for (auto const& i : supported_)
             BEAST_EXPECT(majority.find(amendmentId(i)) == majority.end());
 
         // Now, everyone votes for this feature
-        for (auto const& i : m_set1)
+        for (auto const& i : supported_)
             votes.emplace_back(amendmentId(i), 256);
 
         // Week 2: We should recognize a majority
         doRound(
             *table, weeks{2}, validators, votes, ourVotes, enabled, majority);
-        BEAST_EXPECT(ourVotes.size() == m_set1.size());
+        BEAST_EXPECT(ourVotes.size() == supported_.size());
         BEAST_EXPECT(enabled.empty());
 
-        for (auto const& i : m_set1)
+        for (auto const& i : supported_)
             BEAST_EXPECT(majority[amendmentId(i)] == weekTime(weeks{2}));
 
         // Week 5: We should enable the amendment
         doRound(
             *table, weeks{5}, validators, votes, ourVotes, enabled, majority);
-        BEAST_EXPECT(enabled.size() == m_set1.size());
+        BEAST_EXPECT(enabled.size() == supported_.size());
 
         // Week 6: We should remove it from our votes and from having a majority
         doRound(
             *table, weeks{6}, validators, votes, ourVotes, enabled, majority);
-        BEAST_EXPECT(enabled.size() == m_set1.size());
+        BEAST_EXPECT(enabled.size() == supported_.size());
         BEAST_EXPECT(ourVotes.empty());
-        for (auto const& i : m_set1)
+        for (auto const& i : supported_)
             BEAST_EXPECT(majority.find(amendmentId(i)) == majority.end());
     }
 
@@ -583,7 +606,7 @@ public:
 
         auto const testAmendment = amendmentId("detectMajority");
         auto table = makeTable(
-            2, makeSection(testAmendment), emptySection, emptySection);
+            weeks(2), makeSection(testAmendment), emptySection, emptySection);
 
         auto const validators = makeValidators(16);
 
@@ -648,7 +671,7 @@ public:
         auto const validators = makeValidators(16);
 
         auto table = makeTable(
-            8, makeSection(testAmendment), emptySection, emptySection);
+            weeks(8), makeSection(testAmendment), emptySection, emptySection);
 
         std::set<uint256> enabled;
         majorityAmendments_t majority;
@@ -712,32 +735,44 @@ public:
     {
         testcase("hasUnsupportedEnabled");
 
-        using namespace std::chrono_literals;
+        test::jtx::Env env(*this);  // Used only for its Rules
+        env.close();
 
-        int constexpr w = 1;
+        using namespace std::chrono_literals;
+        weeks constexpr w(1);
         auto table = makeTable(w);
         BEAST_EXPECT(!table->hasUnsupportedEnabled());
         BEAST_EXPECT(!table->firstUnsupportedExpected());
+        BEAST_EXPECT(table->needValidatedLedger(1));
 
         std::set<uint256> enabled;
+        std::for_each(
+            unsupported_.begin(),
+            unsupported_.end(),
+            [&enabled](auto const& s) { enabled.insert(amendmentId(s)); });
+
         majorityAmendments_t majority;
-        std::for_each(m_set4.begin(), m_set4.end(), [&enabled](auto const& s) {
-            enabled.insert(amendmentId(s));
-        });
         table->doValidatedLedger(1, enabled, majority);
         BEAST_EXPECT(table->hasUnsupportedEnabled());
         BEAST_EXPECT(!table->firstUnsupportedExpected());
+
         NetClock::duration t{1000s};
         std::for_each(
-            m_set5.begin(), m_set5.end(), [&majority, &t](auto const& s) {
+            unsupportedMajority_.begin(),
+            unsupportedMajority_.end(),
+            [&majority, &t](auto const& s) {
                 majority[amendmentId(s)] = NetClock::time_point{--t};
             });
+
         table->doValidatedLedger(1, enabled, majority);
         BEAST_EXPECT(table->hasUnsupportedEnabled());
         BEAST_EXPECT(
             table->firstUnsupportedExpected() &&
-            *table->firstUnsupportedExpected() ==
+            *table->firstUnsupportedExpected() == NetClock::time_point{t} + w);
-                NetClock::time_point{t} + weeks{w});
+
+        // Make sure the table knows when it needs an update.
+        BEAST_EXPECT(!table->needValidatedLedger(256));
+        BEAST_EXPECT(table->needValidatedLedger(257));
     }
 
     void
@@ -746,7 +781,7 @@ public:
         testConstruct();
         testGet();
         testBadConfig();
-        testEnableDisable();
+        testEnableVeto();
         testNoOnUnknown();
         testNoOnVetoed();
         testVoteEnable();