Improve transaction relay logic

src/test/app/HashRouter_test.cpp

@@ -18,6 +18,7 @@
 //==============================================================================
 
 #include <xrpld/app/misc/HashRouter.h>
+#include <xrpld/core/Config.h>
 #include <xrpl/basics/chrono.h>
 #include <xrpl/beast/unit_test.h>
 
@@ -26,12 +27,22 @@ namespace test {
 
 class HashRouter_test : public beast::unit_test::suite
 {
+    HashRouter::Setup
+    getSetup(std::chrono::seconds hold, std::chrono::seconds relay)
+    {
+        HashRouter::Setup setup;
+        setup.holdTime = hold;
+        setup.relayTime = relay;
+        return setup;
+    }
+
     void
     testNonExpiration()
     {
+        testcase("Non-expiration");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
-        HashRouter router(stopwatch, 2s);
+        HashRouter router(getSetup(2s, 1s), stopwatch);
 
         uint256 const key1(1);
         uint256 const key2(2);
@@ -66,9 +77,10 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testExpiration()
     {
+        testcase("Expiration");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
-        HashRouter router(stopwatch, 2s);
+        HashRouter router(getSetup(2s, 1s), stopwatch);
 
         uint256 const key1(1);
         uint256 const key2(2);
@@ -143,10 +155,11 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testSuppression()
     {
+        testcase("Suppression");
         // Normal HashRouter
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
-        HashRouter router(stopwatch, 2s);
+        HashRouter router(getSetup(2s, 1s), stopwatch);
 
         uint256 const key1(1);
         uint256 const key2(2);
@@ -172,9 +185,10 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testSetFlags()
     {
+        testcase("Set Flags");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
-        HashRouter router(stopwatch, 2s);
+        HashRouter router(getSetup(2s, 1s), stopwatch);
 
         uint256 const key1(1);
         BEAST_EXPECT(router.setFlags(key1, 10));
@@ -185,9 +199,10 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testRelay()
     {
+        testcase("Relay");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
-        HashRouter router(stopwatch, 1s);
+        HashRouter router(getSetup(50s, 1s), stopwatch);
 
         uint256 const key1(1);
 
@@ -228,9 +243,10 @@ class HashRouter_test : public beast::unit_test::suite
     void
     testProcess()
     {
+        testcase("Process");
         using namespace std::chrono_literals;
         TestStopwatch stopwatch;
-        HashRouter router(stopwatch, 5s);
+        HashRouter router(getSetup(5s, 1s), stopwatch);
         uint256 const key(1);
         HashRouter::PeerShortID peer = 1;
         int flags;
@@ -242,6 +258,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
+            {
+                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
+            {
+                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
+            {
+                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
@@ -252,6 +373,7 @@ class HashRouter_test : public beast::unit_test::suite
         testSetFlags();
         testRelay();
         testProcess();
+        testSetup();
     }
 };
 

src/xrpld/app/ledger/LocalTxs.h

@@ -34,6 +34,11 @@ namespace ripple {
 class LocalTxs
 {
 public:
+    // The number of ledgers to hold a transaction is essentially
+    // arbitrary. It should be sufficient to allow the transaction to
+    // get into a fully-validated ledger.
+    static constexpr int holdLedgers = 5;
+
     virtual ~LocalTxs() = default;
 
     // Add a new local transaction

src/xrpld/app/ledger/detail/LedgerMaster.cpp

@@ -458,25 +458,17 @@ LedgerMaster::storeLedger(std::shared_ptr<Ledger const> ledger)
 void
 LedgerMaster::applyHeldTransactions()
 {
-    std::lock_guard sl(m_mutex);
+    CanonicalTXSet const set = [this]() {
+        std::lock_guard sl(m_mutex);
+        // VFALCO NOTE The hash for an open ledger is undefined so we use
+        // something that is a reasonable substitute.
+        CanonicalTXSet set(app_.openLedger().current()->info().parentHash);
+        std::swap(mHeldTransactions, set);
+        return set;
+    }();
 
-    app_.openLedger().modify([&](OpenView& view, beast::Journal j) {
-        bool any = false;
-        for (auto const& it : mHeldTransactions)
-        {
-            ApplyFlags flags = tapNONE;
-            auto const result =
-                app_.getTxQ().apply(app_, view, it.second, flags, j);
-            any |= result.applied;
-        }
-        return any;
-    });
-
-    // VFALCO TODO recreate the CanonicalTxSet object instead of resetting
-    // it.
-    // VFALCO NOTE The hash for an open ledger is undefined so we use
-    // something that is a reasonable substitute.
-    mHeldTransactions.reset(app_.openLedger().current()->info().parentHash);
+    if (!set.empty())
+        app_.getOPs().processTransactionSet(set);
 }
 
 std::shared_ptr<STTx const>

src/xrpld/app/ledger/detail/LocalTxs.cpp

@@ -52,14 +52,9 @@ namespace ripple {
 class LocalTx
 {
 public:
-    // The number of ledgers to hold a transaction is essentially
-    // arbitrary. It should be sufficient to allow the transaction to
-    // get into a fully-validated ledger.
-    static int const holdLedgers = 5;
-
     LocalTx(LedgerIndex index, std::shared_ptr<STTx const> const& txn)
         : m_txn(txn)
-        , m_expire(index + holdLedgers)
+        , m_expire(index + LocalTxs::holdLedgers)
         , m_id(txn->getTransactionID())
         , m_account(txn->getAccountID(sfAccount))
         , m_seqProxy(txn->getSeqProxy())

src/xrpld/app/main/Application.cpp

@@ -440,8 +440,8 @@ class ApplicationImp : public Application, public BasicApp
               std::make_unique<LoadFeeTrack>(logs_->journal("LoadManager")))
 
         , hashRouter_(std::make_unique<HashRouter>(
-              stopwatch(),
-              HashRouter::getDefaultHoldTime()))
+              setup_HashRouter(*config_),
+              stopwatch()))
 
         , mValidations(
               ValidationParms(),

src/xrpld/app/main/Tuning.h

@@ -20,11 +20,15 @@
 #ifndef RIPPLE_APP_MAIN_TUNING_H_INCLUDED
 #define RIPPLE_APP_MAIN_TUNING_H_INCLUDED
 
+#include <chrono>
+
 namespace ripple {
 
 constexpr std::size_t fullBelowTargetSize = 524288;
 constexpr std::chrono::seconds fullBelowExpiration = std::chrono::minutes{10};
 
+constexpr std::size_t maxPoppedTransactions = 10;
+
 }  // namespace ripple
 
 #endif

src/xrpld/app/misc/CanonicalTXSet.cpp

@@ -66,18 +66,22 @@ CanonicalTXSet::popAcctTransaction(std::shared_ptr<STTx const> const& tx)
     //  1. Prioritize transactions with Sequences over transactions with
     //     Tickets.
     //
-    //  2. Don't worry about consecutive Sequence numbers.  Creating Tickets
-    //     can introduce a discontinuity in Sequence numbers.
+    //  2. For transactions not using Tickets, look for consecutive Sequence
+    //     numbers. For transactions using Tickets, don't worry about
+    //     consecutive Sequence numbers. Tickets can process out of order.
     //
     //  3. After handling all transactions with Sequences, return Tickets
     //     with the lowest Ticket ID first.
     std::shared_ptr<STTx const> result;
     uint256 const effectiveAccount{accountKey(tx->getAccountID(sfAccount))};
 
-    Key const after(effectiveAccount, tx->getSeqProxy(), beast::zero);
+    auto const seqProxy = tx->getSeqProxy();
+    Key const after(effectiveAccount, seqProxy, beast::zero);
     auto const itrNext{map_.lower_bound(after)};
     if (itrNext != map_.end() &&
-        itrNext->first.getAccount() == effectiveAccount)
+        itrNext->first.getAccount() == effectiveAccount &&
+        (!itrNext->second->getSeqProxy().isSeq() ||
+         itrNext->second->getSeqProxy().value() == seqProxy.value() + 1))
     {
         result = std::move(itrNext->second);
         map_.erase(itrNext);

src/xrpld/app/misc/HashRouter.cpp

@@ -19,6 +19,8 @@
 
 #include <xrpld/app/misc/HashRouter.h>
 
+#include <xrpld/core/Config.h>
+
 namespace ripple {
 
 auto
@@ -33,7 +35,7 @@ HashRouter::emplace(uint256 const& key) -> std::pair<Entry&, bool>
     }
 
     // See if any supressions need to be expired
-    expire(suppressionMap_, holdTime_);
+    expire(suppressionMap_, setup_.holdTime);
 
     return std::make_pair(
         std::ref(suppressionMap_.emplace(key, Entry()).first->second), true);
@@ -122,10 +124,45 @@ HashRouter::shouldRelay(uint256 const& key)
 
     auto& s = emplace(key).first;
 
-    if (!s.shouldRelay(suppressionMap_.clock().now(), holdTime_))
+    if (!s.shouldRelay(suppressionMap_.clock().now(), setup_.relayTime))
         return {};
 
     return s.releasePeerSet();
 }
 
+HashRouter::Setup
+setup_HashRouter(Config const& config)
+{
+    using namespace std::chrono;
+
+    HashRouter::Setup setup;
+    auto const& section = config.section("hashrouter");
+
+    std::int32_t tmp;
+
+    if (set(tmp, "hold_time", section))
+    {
+        if (tmp < 12)
+            Throw<std::runtime_error>(
+                "HashRouter hold time must be at least 12 seconds (the "
+                "approximate validation time for three ledgers).");
+        setup.holdTime = seconds(tmp);
+    }
+    if (set(tmp, "relay_time", section))
+    {
+        if (tmp < 8)
+            Throw<std::runtime_error>(
+                "HashRouter relay time must be at least 8 seconds (the "
+                "approximate validation time for two ledgers).");
+        setup.relayTime = seconds(tmp);
+    }
+    if (setup.relayTime > setup.holdTime)
+    {
+        Throw<std::runtime_error>(
+            "HashRouter relay time must be less than or equal to hold time");
+    }
+
+    return setup;
+}
+
 }  // namespace ripple