From 28ed2b9e69a7cb392a8e8978cddf7beef784da23 Mon Sep 17 00:00:00 2001 From: Howard Hinnant Date: Thu, 6 Aug 2020 10:32:33 -0400 Subject: [PATCH] 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 | 1 + src/ripple/app/misc/AmendmentTable.h | 1 + src/ripple/app/misc/impl/AmendmentTable.cpp | 155 +++++++++++++++++--- src/test/app/AmendmentTable_test.cpp | 124 +++++++++++----- 4 files changed, 225 insertions(+), 56 deletions(-) diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 5b4531af49..c03c84b601 100644 --- a/src/ripple/app/main/Application.cpp +++ b/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, diff --git a/src/ripple/app/misc/AmendmentTable.h b/src/ripple/app/misc/AmendmentTable.h index e0884fecbb..c811b6003a 100644 --- a/src/ripple/app/misc/AmendmentTable.h +++ b/src/ripple/app/misc/AmendmentTable.h @@ -164,6 +164,7 @@ public: std::unique_ptr make_AmendmentTable( + Application& app, std::chrono::seconds majorityTime, Section const& supported, Section const& enabled, diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 68ad92b204..ffb259729a 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/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 const& sl); @@ -239,8 +242,15 @@ private: AmendmentState const& state, std::lock_guard 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 featureVotesCount; + *db << sql, soci::into(featureVotesCount); + bool exists = static_cast(*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,31 +362,93 @@ AmendmentTableImpl::AmendmentTableImpl( } } + hash_set 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) + { + 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 amendment_hash; + boost::optional amendment_name; + boost::optional 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)) { - JLOG(j_.info()) << "Amendment " << a.first << " is vetoed."; - - if (!a.second.empty()) - s->name = a.second; - - s->vetoed = true; + Throw( + "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; + } } } } @@ -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 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( - majorityTime, supported, enabled, vetoed, journal); + app, majorityTime, supported, enabled, vetoed, journal); } } // namespace ripple diff --git a/src/test/app/AmendmentTable_test.cpp b/src/test/app/AmendmentTable_test.cpp index 3b4ec47d14..1f626c0a30 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -51,14 +51,22 @@ private: } static Section - makeSection(std::vector const& amendments) + makeSection( + std::string const& name, + std::vector 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 const& amendments) + { + return makeSection("Test", amendments); + } + static Section makeSection(uint256 const& amendment) { @@ -67,6 +75,17 @@ private: return section; } + std::unique_ptr + 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 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 - 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 + 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 table = makeTable(weeks(2)); + test::jtx::Env env{*this, makeConfig()}; + std::unique_ptr table = makeTable(env, weeks(2)); - // Note which entries are pre-enabled. + // Note which entries are enabled. std::set allEnabled; - for (std::string 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])); + // 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> votes; std::vector 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> 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 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));