Persist API-configured voting settings:

Prior to this commit, the amendments that a server would vote in support
of or against could be configured both via the configuration file and
via the command line "feature" command. Changes made in the configuration
file would only be loaded once at server startup and changes made via the
command line take effect immediately but are not persisted across
restarts.

This commit deprecates management of amendments via the configuration
file and stores the relevant information in the `wallet.db` database
file.

1. On startup, the new code parses the configuration file.
2. If the `[veto_amendments]` or `[amendments]` sections are present,
   we check if the `FeatureVotes` table is present in `wallet.db`.
3. If it is not, we create the `FeatureVotes` table and transfer the
   settings from the config file.
4. Proceed normally but only reference the `FeatureVotes` table instead
   of the config file.
5. Warns if the voting table already exists in `wallet.db` and there
   exists voting sections in the config file. The config file is ignored
   in this case.

This change addresses & closes #3366

src/ripple/app/main/Application.cpp

@@ -1360,6 +1360,7 @@ ApplicationImp::setup()
         Section enabledAmendments = config_->section(SECTION_AMENDMENTS);
 
         m_amendmentTable = make_AmendmentTable(
+            *this,
             config().AMENDMENT_MAJORITY_TIME,
             supportedAmendments,
             enabledAmendments,

src/ripple/app/misc/AmendmentTable.h

@@ -164,6 +164,7 @@ public:
 
 std::unique_ptr<AmendmentTable>
 make_AmendmentTable(
+    Application& app,
     std::chrono::seconds majorityTime,
     Section const& supported,
     Section const& enabled,

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

@@ -220,6 +220,9 @@ private:
 
     beast::Journal const j_;
 
+    // Database which persists veto/unveto vote
+    DatabaseCon& db_;
+
     // Finds or creates state.  Must be called with mutex_ locked.
     AmendmentState*
     add(uint256 const& amendment, std::lock_guard<std::mutex> const& sl);
@@ -239,8 +242,15 @@ private:
         AmendmentState const& state,
         std::lock_guard<std::mutex> const& sl) const;
 
+    void
+    persistVote(
+        uint256 const& amendment,
+        std::string const& name,
+        bool vote_to_veto) const;
+
 public:
     AmendmentTableImpl(
+        Application& app,
         std::chrono::seconds majorityTime,
         Section const& supported,
         Section const& enabled,
@@ -301,6 +311,7 @@ public:
 //------------------------------------------------------------------------------
 
 AmendmentTableImpl::AmendmentTableImpl(
+    Application& app,
     std::chrono::seconds majorityTime,
     Section const& supported,
     Section const& enabled,
@@ -310,9 +321,34 @@ AmendmentTableImpl::AmendmentTableImpl(
     , majorityTime_(majorityTime)
     , unsupportedEnabled_(false)
     , j_(journal)
+    , db_(app.getWalletDB())
 {
     std::lock_guard sl(mutex_);
 
+    // Find out if the FeatureVotes table exists in WalletDB
+    bool const featureVotesExist = [this]() {
+        auto db = db_.checkoutDb();
+        soci::transaction tr(*db);
+        std::string sql =
+            "SELECT count(*) FROM sqlite_master "
+            "WHERE type='table' AND name='FeatureVotes'";
+        boost::optional<int> featureVotesCount;
+        *db << sql, soci::into(featureVotesCount);
+        bool exists = static_cast<bool>(*featureVotesCount);
+
+        // Create FeatureVotes table in WalletDB if it doesn't exist
+        if (!exists)
+        {
+            *db << "CREATE TABLE  FeatureVotes ( "
+                   "AmendmentHash      CHARACTER(64) NOT NULL, "
+                   "AmendmentName      TEXT, "
+                   "Veto               INTEGER NOT NULL );";
+            tr.commit();
+        }
+        return exists;
+    }();
+
+    // Parse supported amendments
     for (auto const& a : parseSection(supported))
     {
         if (auto s = add(a.first, sl))
@@ -326,33 +362,95 @@ AmendmentTableImpl::AmendmentTableImpl(
         }
     }
 
+    hash_set<uint256> detect_conflict;
+    // Parse enabled amendments from config
     for (auto const& a : parseSection(enabled))
     {
-        if (auto s = add(a.first, sl))
-        {
-            JLOG(j_.debug()) << "Amendment " << a.first << " is enabled.";
-
-            if (!a.second.empty())
-                s->name = a.second;
-
-            s->supported = true;
-            s->enabled = true;
+        if (featureVotesExist)
+        {  // If the table existed, warn about duplicate config info
+            JLOG(j_.warn()) << "[amendments] section in config file ignored"
+                               " in favor of data in db/wallet.db.";
+            break;
+        }
+        else
+        {  // Otherwise transfer config data into the table
+            detect_conflict.insert(a.first);
+            persistVote(a.first, a.second, false);  // un-veto
         }
     }
 
+    // Parse vetoed amendments from config
     for (auto const& a : parseSection(vetoed))
     {
-        // Unknown amendments are effectively vetoed already
-        if (auto s = get(a.first, sl))
+        if (featureVotesExist)
+        {  // If the table existed, warn about duplicate config info
+            JLOG(j_.warn())
+                << "[veto_amendments] section in config file ignored"
+                   " in favor of data in db/wallet.db.";
+            break;
+        }
+        else
+        {  // Otherwise transfer config data into the table
+            if (detect_conflict.count(a.first) == 0)
             {
-            JLOG(j_.info()) << "Amendment " << a.first << " is vetoed.";
-
-            if (!a.second.empty())
-                s->name = a.second;
+                persistVote(a.first, a.second, true);  // veto
+            }
+            else
+            {
+                JLOG(j_.warn())
+                    << "[veto_amendments] section in config has amendment "
+                    << '(' << a.first << ", " << a.second
+                    << ") both [veto_amendments] and [amendments].";
+            }
+        }
+    }
 
+    // Read amendment votes from wallet.db
+    auto db = db_.checkoutDb();
+    soci::transaction tr(*db);
+    std::string sql =
+        "SELECT AmendmentHash, AmendmentName, Veto FROM FeatureVotes";
+    boost::optional<std::string> amendment_hash;
+    boost::optional<std::string> amendment_name;
+    boost::optional<int> vote_to_veto;
+    soci::statement st =
+        (db->prepare << sql,
+         soci::into(amendment_hash),
+         soci::into(amendment_name),
+         soci::into(vote_to_veto));
+    st.execute();
+    while (st.fetch())
+    {
+        uint256 amend_hash;
+        if (!amend_hash.parseHex(*amendment_hash))
+        {
+            Throw<std::runtime_error>(
+                "Invalid amendment ID '" + *amendment_hash + " in wallet.db");
+        }
+        if (*vote_to_veto)
+        {
+            // Unknown amendments are effectively vetoed already
+            if (auto s = get(amend_hash, sl))
+            {
+                JLOG(j_.info()) << "Amendment {" << *amendment_name << ", "
+                                << amend_hash << "} is vetoed.";
+                if (!amendment_name->empty())
+                    s->name = *amendment_name;
                 s->vetoed = true;
             }
         }
+        else  // un-veto
+        {
+            if (auto s = add(amend_hash, sl))
+            {
+                JLOG(j_.debug()) << "Amendment {" << *amendment_name << ", "
+                                 << amend_hash << "} is un-vetoed.";
+                if (!amendment_name->empty())
+                    s->name = *amendment_name;
+                s->vetoed = false;
+            }
+        }
+    }
 }
 
 AmendmentState*
@@ -402,6 +500,24 @@ AmendmentTableImpl::find(std::string const& name) const
     return {};
 }
 
+void
+AmendmentTableImpl::persistVote(
+    uint256 const& amendment,
+    std::string const& name,
+    bool vote_to_veto) const
+{
+    auto db = db_.checkoutDb();
+    soci::transaction tr(*db);
+    std::string sql =
+        "INSERT INTO FeatureVotes (AmendmentHash, AmendmentName, Veto) VALUES "
+        "('";
+    sql += to_string(amendment);
+    sql += "', '" + name;
+    sql += "', '" + std::to_string(int{vote_to_veto}) + "');";
+    *db << sql;
+    tr.commit();
+}
+
 bool
 AmendmentTableImpl::veto(uint256 const& amendment)
 {
@@ -411,6 +527,7 @@ AmendmentTableImpl::veto(uint256 const& amendment)
     if (s->vetoed)
         return false;
     s->vetoed = true;
+    persistVote(amendment, s->name, s->vetoed);
     return true;
 }
 
@@ -423,6 +540,7 @@ AmendmentTableImpl::unVeto(uint256 const& amendment)
     if (!s || !s->vetoed)
         return false;
     s->vetoed = false;
+    persistVote(amendment, s->name, s->vetoed);
     return true;
 }
 
@@ -693,6 +811,7 @@ AmendmentTableImpl::getJson(uint256 const& amendmentID) const
 
 std::unique_ptr<AmendmentTable>
 make_AmendmentTable(
+    Application& app,
     std::chrono::seconds majorityTime,
     Section const& supported,
     Section const& enabled,
@@ -700,7 +819,7 @@ make_AmendmentTable(
     beast::Journal journal)
 {
     return std::make_unique<AmendmentTableImpl>(
-        majorityTime, supported, enabled, vetoed, journal);
+        app, majorityTime, supported, enabled, vetoed, journal);
 }
 
 }  // namespace ripple

src/test/app/AmendmentTable_test.cpp

@@ -51,14 +51,22 @@ private:
     }
 
     static Section
-    makeSection(std::vector<std::string> const& amendments)
+    makeSection(
+        std::string const& name,
+        std::vector<std::string> const& amendments)
     {
-        Section section("Test");
+        Section section(name);
         for (auto const& a : amendments)
             section.append(to_string(amendmentId(a)) + " " + a);
         return section;
     }
 
+    static Section
+    makeSection(std::vector<std::string> const& amendments)
+    {
+        return makeSection("Test", amendments);
+    }
+
     static Section
     makeSection(uint256 const& amendment)
     {
@@ -67,6 +75,17 @@ private:
         return section;
     }
 
+    std::unique_ptr<Config>
+    makeConfig()
+    {
+        auto cfg = test::jtx::envconfig();
+        cfg->section(SECTION_AMENDMENTS) =
+            makeSection(SECTION_AMENDMENTS, enabled_);
+        cfg->section(SECTION_VETO_AMENDMENTS) =
+            makeSection(SECTION_VETO_AMENDMENTS, vetoed_);
+        return cfg;
+    }
+
     // All useful amendments are supported amendments.
     // Enabled amendments are typically a subset of supported amendments.
     // Vetoed amendments should be supported but not enabled.
@@ -91,19 +110,32 @@ public:
 
     std::unique_ptr<AmendmentTable>
     makeTable(
+        Application& app,
         std::chrono::seconds majorityTime,
-        Section const supported,
-        Section const enabled,
-        Section const vetoed)
+        Section const& supported,
+        Section const& enabled,
+        Section const& vetoed)
     {
         return make_AmendmentTable(
-            majorityTime, supported, enabled, vetoed, journal);
+            app, majorityTime, supported, enabled, vetoed, journal);
     }
 
     std::unique_ptr<AmendmentTable>
-    makeTable(std::chrono::seconds majorityTime)
+    makeTable(
+        test::jtx::Env& env,
+        std::chrono::seconds majorityTime,
+        Section const& supported,
+        Section const& enabled,
+        Section const& vetoed)
+    {
+        return makeTable(env.app(), majorityTime, supported, enabled, vetoed);
+    }
+
+    std::unique_ptr<AmendmentTable>
+    makeTable(test::jtx::Env& env, std::chrono::seconds majorityTime)
     {
         return makeTable(
+            env.app(),
             majorityTime,
             makeSection(supported_),
             makeSection(enabled_),
@@ -114,8 +146,8 @@ public:
     testConstruct()
     {
         testcase("Construction");
-
-        auto table = makeTable(weeks(1));
+        test::jtx::Env env{*this, makeConfig()};
+        auto table = makeTable(env, weeks(1));
 
         for (auto const& a : supported_)
         {
@@ -125,7 +157,6 @@ public:
         for (auto const& a : enabled_)
         {
             BEAST_EXPECT(table->isSupported(amendmentId(a)));
-            BEAST_EXPECT(table->isEnabled(amendmentId(a)));
         }
 
         for (auto const& a : vetoed_)
@@ -140,7 +171,8 @@ public:
     {
         testcase("Name to ID mapping");
 
-        auto table = makeTable(weeks(1));
+        test::jtx::Env env{*this, makeConfig()};
+        auto table = makeTable(env, weeks(1));
 
         for (auto const& a : supported_)
             BEAST_EXPECT(table->find(a) == amendmentId(a));
@@ -186,7 +218,8 @@ public:
 
             try
             {
-                if (makeTable(weeks(2), test, emptySection, emptySection))
+                test::jtx::Env env{*this, makeConfig()};
+                if (makeTable(env, weeks(2), test, emptySection, emptySection))
                     fail("Accepted only amendment ID");
             }
             catch (...)
@@ -201,7 +234,8 @@ public:
 
             try
             {
-                if (makeTable(weeks(2), test, emptySection, emptySection))
+                test::jtx::Env env{*this, makeConfig()};
+                if (makeTable(env, weeks(2), test, emptySection, emptySection))
                     fail("Accepted extra arguments");
             }
             catch (...)
@@ -219,7 +253,8 @@ public:
 
             try
             {
-                if (makeTable(weeks(2), test, emptySection, emptySection))
+                test::jtx::Env env{*this, makeConfig()};
+                if (makeTable(env, weeks(2), test, emptySection, emptySection))
                     fail("Accepted short amendment ID");
             }
             catch (...)
@@ -237,7 +272,8 @@ public:
 
             try
             {
-                if (makeTable(weeks(2), test, emptySection, emptySection))
+                test::jtx::Env env{*this, makeConfig()};
+                if (makeTable(env, weeks(2), test, emptySection, emptySection))
                     fail("Accepted long amendment ID");
             }
             catch (...)
@@ -256,7 +292,8 @@ public:
 
             try
             {
-                if (makeTable(weeks(2), test, emptySection, emptySection))
+                test::jtx::Env env{*this, makeConfig()};
+                if (makeTable(env, weeks(2), test, emptySection, emptySection))
                     fail("Accepted non-hex amendment ID");
             }
             catch (...)
@@ -271,28 +308,24 @@ public:
     {
         testcase("enable and veto");
 
-        std::unique_ptr<AmendmentTable> table = makeTable(weeks(2));
+        test::jtx::Env env{*this, makeConfig()};
+        std::unique_ptr<AmendmentTable> table = makeTable(env, weeks(2));
 
-        // Note which entries are pre-enabled.
+        // Note which entries are enabled.
         std::set<uint256> allEnabled;
-        for (std::string const& a : enabled_)
-            allEnabled.insert(amendmentId(a));
 
-        // Subset of amendments to late-enable
-        std::set<uint256> lateEnabled;
-        lateEnabled.insert(amendmentId(supported_[0]));
-        lateEnabled.insert(amendmentId(enabled_[0]));
-        lateEnabled.insert(amendmentId(vetoed_[0]));
+        // Subset of amendments to enable
+        allEnabled.insert(amendmentId(supported_[0]));
+        allEnabled.insert(amendmentId(enabled_[0]));
+        allEnabled.insert(amendmentId(vetoed_[0]));
 
-        // Do the late enabling.
-        for (uint256 const& a : lateEnabled)
+        for (uint256 const& a : allEnabled)
             table->enable(a);
 
         // So far all enabled amendments are supported.
         BEAST_EXPECT(!table->hasUnsupportedEnabled());
 
-        // Verify all pre- and late-enables are enabled and nothing else.
-        allEnabled.insert(lateEnabled.begin(), lateEnabled.end());
+        // Verify all enables are enabled and nothing else.
         for (std::string const& a : supported_)
         {
             uint256 const supportedID = amendmentId(a);
@@ -470,8 +503,9 @@ public:
         auto const testAmendment = amendmentId("TestAmendment");
         auto const validators = makeValidators(10);
 
+        test::jtx::Env env{*this};
         auto table =
-            makeTable(weeks(2), emptySection, emptySection, emptySection);
+            makeTable(env, weeks(2), emptySection, emptySection, emptySection);
 
         std::vector<std::pair<uint256, int>> votes;
         std::vector<uint256> ourVotes;
@@ -530,8 +564,13 @@ public:
 
         auto const testAmendment = amendmentId("vetoedAmendment");
 
+        test::jtx::Env env{*this};
         auto table = makeTable(
-            weeks(2), emptySection, emptySection, makeSection(testAmendment));
+            env,
+            weeks(2),
+            emptySection,
+            emptySection,
+            makeSection(testAmendment));
 
         auto const validators = makeValidators(10);
 
@@ -588,8 +627,9 @@ public:
     {
         testcase("voteEnable");
 
+        test::jtx::Env env{*this};
         auto table = makeTable(
-            weeks(2), makeSection(supported_), emptySection, emptySection);
+            env, weeks(2), makeSection(supported_), emptySection, emptySection);
 
         auto const validators = makeValidators(10);
         std::vector<std::pair<uint256, int>> votes;
@@ -667,8 +707,13 @@ public:
         testcase("detectMajority");
 
         auto const testAmendment = amendmentId("detectMajority");
+        test::jtx::Env env{*this};
         auto table = makeTable(
-            weeks(2), makeSection(testAmendment), emptySection, emptySection);
+            env,
+            weeks(2),
+            makeSection(testAmendment),
+            emptySection,
+            emptySection);
 
         auto const validators = makeValidators(16);
 
@@ -733,8 +778,13 @@ public:
         auto const testAmendment = amendmentId("lostMajority");
         auto const validators = makeValidators(16);
 
+        test::jtx::Env env{*this};
         auto table = makeTable(
-            weeks(8), makeSection(testAmendment), emptySection, emptySection);
+            env,
+            weeks(8),
+            makeSection(testAmendment),
+            emptySection,
+            emptySection);
 
         std::set<uint256> enabled;
         majorityAmendments_t majority;
@@ -800,12 +850,10 @@ public:
     {
         testcase("hasUnsupportedEnabled");
 
-        test::jtx::Env env(*this);  // Used only for its Rules
-        env.close();
-
         using namespace std::chrono_literals;
         weeks constexpr w(1);
-        auto table = makeTable(w);
+        test::jtx::Env env{*this, makeConfig()};
+        auto table = makeTable(env, w);
         BEAST_EXPECT(!table->hasUnsupportedEnabled());
         BEAST_EXPECT(!table->firstUnsupportedExpected());
         BEAST_EXPECT(table->needValidatedLedger(1));