Address review feedback from @Bronek:

- Add a HashRouter test case for setup_HashRouter config processing.
- Correct a typo in setup_HashRouter.
- Add a missing include.
- Update the documentation for HashRouter::Setup.
- Update the parameter name of HashRouter::Entry::shouldRelay.

src/test/app/HashRouter_test.cpp

@@ -38,6 +38,7 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testNonExpiration()
     {
+        testcase("Non-expiration");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
         HashRouter router(getSetup(2s, 1s), stopwatch);
@@ -75,6 +76,7 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testExpiration()
     {
+        testcase("Expiration");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
         HashRouter router(getSetup(2s, 1s), stopwatch);
@@ -152,6 +154,7 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testSuppression()
     {
+        testcase("Suppression");
         // Normal HashRouter
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
@@ -181,6 +184,7 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testSetFlags()
     {
+        testcase("Set Flags");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
         HashRouter router(getSetup(2s, 1s), stopwatch);
@@ -194,6 +198,7 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testRelay()
     {
+        testcase("Relay");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
         HashRouter router(getSetup(50s, 1s), stopwatch);
@@ -237,6 +242,7 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testProcess()
     {
+        testcase("Process");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
         HashRouter router(getSetup(5s, 1s), stopwatch);
@@ -251,6 +257,111 @@ class HashRouter_test : public beast::unit_test::suite
         BEAST_EXPECT(router.shouldProcess(key, peer, flags, 1s));
     }
 
+    void
+    testSetup()
+    {
+        testcase("setup_HashRouter");
+
+        using namespace std::chrono_literals;
+        {
+            Config cfg;
+            // default
+            auto const setup = setup_HashRouter(cfg);
+            BEAST_EXPECT(setup.holdTime == 300s);
+            BEAST_EXPECT(setup.relayTime == 30s);
+        }
+        {
+            Config cfg;
+            // non-default
+            auto& h = cfg.section("hashrouter");
+            h.set("hold_time", "600");
+            h.set("relay_time", "15");
+            auto const setup = setup_HashRouter(cfg);
+            BEAST_EXPECT(setup.holdTime == 600s);
+            BEAST_EXPECT(setup.relayTime == 15s);
+        }
+        {
+            Config cfg;
+            // equal
+            auto& h = cfg.section("hashrouter");
+            h.set("hold_time", "400");
+            h.set("relay_time", "400");
+            auto const setup = setup_HashRouter(cfg);
+            BEAST_EXPECT(setup.holdTime == 400s);
+            BEAST_EXPECT(setup.relayTime == 400s);
+        }
+        {
+            Config cfg;
+            // wrong order
+            auto& h = cfg.section("hashrouter");
+            h.set("hold_time", "60");
+            h.set("relay_time", "120");
+            try
+            {
+                auto const setup = setup_HashRouter(cfg);
+                fail();
+            }
+            catch (std::exception const& e)
+            {
+                std::string expected =
+                    "HashRouter relay time must be less than or equal to hold "
+                    "time";
+                BEAST_EXPECT(e.what() == expected);
+            }
+        }
+        {
+            Config cfg;
+            // too small hold
+            auto& h = cfg.section("hashrouter");
+            h.set("hold_time", "10");
+            h.set("relay_time", "120");
+            try
+            {
+                auto const setup = setup_HashRouter(cfg);
+                fail();
+            }
+            catch (std::exception const& e)
+            {
+                std::string expected =
+                    "HashRouter hold time must be at least 12 seconds (the "
+                    "approximate validation time for three "
+                    "ledgers).";
+                BEAST_EXPECT(e.what() == expected);
+            }
+        }
+        {
+            Config cfg;
+            // too small relay
+            auto& h = cfg.section("hashrouter");
+            h.set("hold_time", "500");
+            h.set("relay_time", "6");
+            try
+            {
+                auto const setup = setup_HashRouter(cfg);
+                fail();
+            }
+            catch (std::exception const& e)
+            {
+                std::string expected =
+                    "HashRouter relay time must be at least 8 seconds (the "
+                    "approximate validation time for two ledgers).";
+                BEAST_EXPECT(e.what() == expected);
+            }
+        }
+        {
+            Config cfg;
+            // garbage
+            auto& h = cfg.section("hashrouter");
+            h.set("hold_time", "alice");
+            h.set("relay_time", "bob");
+            auto const setup = setup_HashRouter(cfg);
+            // The set function ignores values that don't covert, so the
+            // defaults are left unchanged
+            BEAST_EXPECT(setup.holdTime == 300s);
+            BEAST_EXPECT(setup.relayTime == 30s);
+        }
+    }
+
 public:
     void
     run() override
@@ -261,6 +372,7 @@ class HashRouter_test : public beast::unit_test::suite
         testSetFlags();
         testRelay();
         testProcess();
+        testSetup();
     }
 };
 

src/xrpld/app/main/Tuning.h

@@ -20,6 +20,8 @@
 #ifndef RIPPLE_APP_MAIN_TUNING_H_INCLUDED
 #define RIPPLE_APP_MAIN_TUNING_H_INCLUDED
 
+#include <chrono>
+
 namespace ripple {
 
 constexpr std::size_t fullBelowTargetSize = 524288;

src/xrpld/app/misc/HashRouter.cpp

@@ -142,7 +142,7 @@ setup_HashRouter(Config const& config)
 
     if (set(tmp, "hold_time", section))
     {
-        if (tmp < 8)
+        if (tmp < 12)
             Throw<std::runtime_error>(
                 "HashRouter hold time must be at least 12 seconds (the "
                 "approximate validation time for three ledgers).");

src/xrpld/app/misc/HashRouter.h

@@ -61,8 +61,11 @@ class HashRouter
 
     /** Structure used to customize @ref HashRouter behavior.
      *
-     * Even though these items are configurable, don't change them unless there
-     * is a good reason, and network-wide coordination to do it.
+     * Even though these items are configurable, they are undocumented. Don't
+     * change them unless there is a good reason, and network-wide coordination
+     * to do it.
+     *
+     * Configuration is processed in setup_HashRouter.
      */
     struct Setup
     {
@@ -132,9 +135,9 @@ class HashRouter
         bool
         shouldRelay(
             Stopwatch::time_point const& now,
-            std::chrono::seconds holdTime)
+            std::chrono::seconds relayTime)
         {
-            if (relayed_ && *relayed_ + holdTime > now)
+            if (relayed_ && *relayed_ + relayTime > now)
                 return false;
             relayed_.emplace(now);
             return true;

src/xrpld/app/misc/NetworkOPs.cpp

@@ -1418,7 +1418,9 @@ NetworkOPsImp::processTransactionSet(CanonicalTXSet const& set)
     }
 
     doTransactionSyncBatch(lock, [&](std::unique_lock<std::mutex> const&) {
-        assert(lock.owns_lock());
+        XRPL_ASSERT(
+            lock.owns_lock(),
+            "ripple::NetworkOPsImp::processTransactionSet has lock");
         return std::any_of(
             mTransactions.begin(), mTransactions.end(), [](auto const& t) {
                 return t.transaction->getApplying();