Use correct manifest cache when loading ValidatorList

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

@@ -107,7 +107,7 @@ ValidatorList::load (
 
         auto id = PublicKey(makeSlice(ret.first));
 
-        if (validatorManifests_.revoked (id))
+        if (publisherManifests_.revoked (id))
         {
             JLOG (j_.warn()) <<
                 "Configured validator list publisher key is revoked: " << key;

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

@@ -383,11 +383,18 @@ ValidatorSite::onSiteFetch(
     detail::response_type&& res,
     std::size_t siteIdx)
 {
-    bool shouldRetry = false;
     {
         std::lock_guard <std::mutex> lock_sites{sites_mutex_};
-        try
+        auto onError = [&](std::string const& errMsg, bool retry)
         {
+            sites_[siteIdx].lastRefreshStatus.emplace(
+                Site::Status{clock_type::now(),
+                            ListDisposition::invalid,
+                            errMsg});
+            if (retry)
+                sites_[siteIdx].nextRefresh =
+                        clock_type::now() + ERROR_RETRY_INTERVAL;
+        };
         if (ec)
         {
             JLOG (j_.warn()) <<
@@ -397,10 +404,12 @@ ValidatorSite::onSiteFetch(
                     ec.value() <<
                     ":" <<
                     ec.message();
-                shouldRetry = true;
+            onError("fetch error", true);
-                throw std::runtime_error{"fetch error"};
         }
-
+        else
+        {
+            try
+            {
                 using namespace boost::beast::http;
                 switch (res.result())
                 {
@@ -412,7 +421,8 @@ ValidatorSite::onSiteFetch(
                 case status::found :
                 case status::temporary_redirect :
                 {
-                auto newLocation = processRedirect (res, siteIdx, lock_sites);
+                    auto newLocation =
+                        processRedirect (res, siteIdx, lock_sites);
                     assert(newLocation);
                     // for perm redirects, also update our starting URI
                     if (res.result() == status::moved_permanently ||
@@ -431,20 +441,14 @@ ValidatorSite::onSiteFetch(
                         sites_[siteIdx].activeResource->uri <<
                         " returned bad status: " <<
                         res.result_int();
-                shouldRetry = true;
+                    onError("bad result code", true);
-                throw std::runtime_error{"bad result code"};
                 }
                 }
             }
             catch (std::exception& ex)
             {
-            sites_[siteIdx].lastRefreshStatus.emplace(
+                onError(ex.what(), false);
-                Site::Status{clock_type::now(),
+            }
-                ListDisposition::invalid,
-                ex.what()});
-            if (shouldRetry)
-                sites_[siteIdx].nextRefresh =
-                    clock_type::now() + ERROR_RETRY_INTERVAL;
         }
         sites_[siteIdx].activeResource.reset();
     }

src/test/app/ValidatorList_test.cpp

@@ -355,6 +355,40 @@ private:
             for (auto const& key : keys)
                 BEAST_EXPECT(trustedKeys->trustedPublisher (key));
         }
+        {
+            // Attempt to load a publisher key that has been revoked.
+            // Should fail
+            ManifestCache valManifests;
+            ManifestCache pubManifests;
+            auto trustedKeys = std::make_unique <ValidatorList> (
+                valManifests, pubManifests, env.timeKeeper(), env.journal);
+
+            auto const pubRevokedSecret = randomSecretKey();
+            auto const pubRevokedPublic =
+                derivePublicKey(KeyType::ed25519, pubRevokedSecret);
+            auto const pubRevokedSigning = randomKeyPair(KeyType::secp256k1);
+            // make this manifest revoked (seq num = max)
+            //  -- thus should not be loaded
+            pubManifests.applyManifest (*Manifest::make_Manifest (
+                makeManifestString (
+                    pubRevokedPublic,
+                    pubRevokedSecret,
+                    pubRevokedSigning.first,
+                    pubRevokedSigning.second,
+                    std::numeric_limits<std::uint32_t>::max ())));
+
+            // this one is not revoked (and not in manifest cache at all.)
+            auto legitKey = randomMasterKey();
+
+            std::vector<std::string> cfgPublishers = {
+                strHex(pubRevokedPublic),
+                strHex(legitKey) };
+            BEAST_EXPECT(trustedKeys->load (
+                emptyLocalKey, emptyCfgKeys, cfgPublishers));
+
+            BEAST_EXPECT(!trustedKeys->trustedPublisher (pubRevokedPublic));
+            BEAST_EXPECT(trustedKeys->trustedPublisher (legitKey));
+        }
     }
 
     void

src/test/app/ValidatorSite_test.cpp

@@ -31,6 +31,7 @@
 #include <test/jtx/TrustedPublisherServer.h>
 #include <boost/algorithm/string/predicate.hpp>
 #include <boost/asio.hpp>
+#include <chrono>
 
 namespace ripple {
 namespace test {
@@ -165,6 +166,7 @@ private:
             std::string uri;
             std::string expectMsg;
             bool shouldFail;
+            bool isRetry;
         };
         std::vector<publisher> servers;
 
@@ -191,6 +193,7 @@ private:
             servers.push_back({});
             auto& item = servers.back();
             item.shouldFail = ! cfg.second.empty();
+            item.isRetry = cfg.first == "/bad-resource";
             item.expectMsg = cfg.second;
             item.list.reserve (listSize);
             while (item.list.size () < listSize)
@@ -243,11 +246,21 @@ private:
                     != u.shouldFail, to_string(myStatus));
             if (u.shouldFail)
             {
+                using namespace std::chrono;
                 BEAST_EXPECTS(
                     sink.strm_.str().find(u.expectMsg) != std::string::npos,
                     sink.strm_.str());
                 log << " -- Msg: " <<
                     myStatus[jss::last_refresh_message].asString() << std::endl;
+                std::stringstream nextRefreshStr
+                    {myStatus[jss::next_refresh_time].asString()};
+                system_clock::time_point nextRefresh;
+                date::from_stream (nextRefreshStr, "%Y-%b-%d %T", nextRefresh);
+                BEAST_EXPECT(!nextRefreshStr.fail());
+                auto now = system_clock::now();
+                BEAST_EXPECTS(
+                    nextRefresh <= now + (u.isRetry ? seconds{30} : minutes{5}),
+                    "Now: " + to_string(now) + ", NR: " + nextRefreshStr.str());
             }
         }
     }