Improve deterministic transaction sorting in TxQ

src/ripple/app/misc/TxQ.h

@@ -23,6 +23,7 @@
 #include <ripple/app/tx/applySteps.h>
 #include <ripple/ledger/ApplyView.h>
 #include <ripple/ledger/OpenView.h>
+#include <ripple/protocol/RippleLedgerHash.h>
 #include <ripple/protocol/STTx.h>
 #include <ripple/protocol/SeqProxy.h>
 #include <ripple/protocol/TER.h>
@@ -340,7 +341,7 @@ class TxQ
         in the queue.
     */
     std::vector<TxDetails>
-    getAccountTxs(AccountID const& account, ReadView const& view) const;
+    getAccountTxs(AccountID const& account) const;
 
     /** Returns information about all transactions currently
         in the queue.
@@ -349,7 +350,7 @@ class TxQ
         in the queue.
     */
     std::vector<TxDetails>
-    getTxs(ReadView const& view) const;
+    getTxs() const;
 
     /** Summarize current fee metrics for the `fee` RPC command.
 
@@ -575,6 +576,16 @@ class TxQ
         */
         static constexpr int retriesAllowed = 10;
 
+        /** The hash of the parent ledger.
+
+           This is used to pseudo-randomize the transaction order when
+           populating byFee_, by XORing it with the transaction hash (txID).
+           Using a single static and doing the XOR operation every time was
+           tested to be as fast or faster than storing the computed "sort key",
+           and obviously uses less memory.
+         */
+        static LedgerHash parentHashComp;
+
     public:
         /// Constructor
         MaybeTx(
@@ -621,22 +632,26 @@ class TxQ
         explicit OrderCandidates() = default;
 
         /** Sort @ref MaybeTx by `feeLevel` descending, then by
-         * transaction ID ascending
+         * pseudo-randomized transaction ID ascending
          *
          * The transaction queue is ordered such that transactions
          * paying a higher fee are in front of transactions paying
          * a lower fee, giving them an opportunity to be processed into
          * the open ledger first. Within transactions paying the same
-         * fee, order by the arbitrary but consistent transaction ID.
-         * This allows validators to build similar queues in the same
-         * order, and thus have more similar initial proposals.
+         * fee, order by the arbitrary but consistent pseudo-randomized
+         * transaction ID. The ID is pseudo-randomized by XORing it with
+         * the open ledger's parent hash, which is deterministic, but
+         * unpredictable. This allows validators to build similar queues
+         * in the same order, and thus have more similar initial
+         * proposals.
          *
          */
         bool
         operator()(const MaybeTx& lhs, const MaybeTx& rhs) const
         {
             if (lhs.feeLevel == rhs.feeLevel)
-                return lhs.txID < rhs.txID;
+                return (lhs.txID ^ MaybeTx::parentHashComp) <
+                    (rhs.txID ^ MaybeTx::parentHashComp);
             return lhs.feeLevel > rhs.feeLevel;
         }
     };
@@ -770,6 +785,14 @@ class TxQ
     */
     std::optional<size_t> maxSize_;
 
+#if !NDEBUG
+    /**
+        parentHash_ checks that no unexpected ledger transitions
+        happen, and is only checked via debug asserts.
+    */
+    LedgerHash parentHash_{beast::zero};
+#endif
+
     /** Most queue operations are done under the master lock,
         but use this mutex for the RPC "fee" command, which isn't.
     */

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

@@ -265,6 +265,8 @@ TxQ::FeeMetrics::escalatedSeriesFeeLevel(
     return totalFeeLevel;
 }
 
+LedgerHash TxQ::MaybeTx::parentHashComp{};
+
 TxQ::MaybeTx::MaybeTx(
     std::shared_ptr<STTx const> const& txn_,
     TxID const& txID_,
@@ -467,13 +469,12 @@ TxQ::eraseAndAdvance(TxQ::FeeMultiSet::const_iterator_type candidateIter)
 
     // Check if the next transaction for this account is earlier in the queue,
     // which means we skipped it earlier, and need to try it again.
-    OrderCandidates o;
     auto const feeNextIter = std::next(candidateIter);
     bool const useAccountNext =
         accountNextIter != txQAccount.transactions.end() &&
         accountNextIter->first > candidateIter->seqProxy &&
         (feeNextIter == byFee_.end() ||
-         o(accountNextIter->second, *feeNextIter));
+         byFee_.value_comp()(accountNextIter->second, *feeNextIter));
 
     auto const candidateNextIter = byFee_.erase(candidateIter);
     txQAccount.transactions.erase(accountIter);
@@ -1529,6 +1530,37 @@ TxQ::accept(Application& app, OpenView& view)
         }
     }
 
+    // All transactions that can be moved out of the queue into the open
+    // ledger have been. Rebuild the queue using the open ledger's
+    // parent hash, so that transactions paying the same fee are
+    // reordered.
+    LedgerHash const& parentHash = view.info().parentHash;
+#if !NDEBUG
+    auto const startingSize = byFee_.size();
+    assert(parentHash != parentHash_);
+    parentHash_ = parentHash;
+#endif
+    // byFee_ doesn't "own" the candidate objects inside it, so it's
+    // perfectly safe to wipe it and start over, repopulating from
+    // byAccount_.
+    //
+    // In the absence of a "re-sort the list in place" function, this
+    // was the fastest method tried to repopulate the list.
+    // Other methods included: create a new list and moving items over one at a
+    // time, create a new list and merge the old list into it.
+    byFee_.clear();
+
+    MaybeTx::parentHashComp = parentHash;
+
+    for (auto& [_, account] : byAccount_)
+    {
+        for (auto& [_, candidate] : account.transactions)
+        {
+            byFee_.insert(candidate);
+        }
+    }
+    assert(byFee_.size() == startingSize);
+
     return ledgerChanged;
 }
 
@@ -1740,7 +1772,7 @@ TxQ::getTxRequiredFeeAndSeq(
 }
 
 std::vector<TxQ::TxDetails>
-TxQ::getAccountTxs(AccountID const& account, ReadView const& view) const
+TxQ::getAccountTxs(AccountID const& account) const
 {
     std::vector<TxDetails> result;
 
@@ -1761,7 +1793,7 @@ TxQ::getAccountTxs(AccountID const& account, ReadView const& view) const
 }
 
 std::vector<TxQ::TxDetails>
-TxQ::getTxs(ReadView const& view) const
+TxQ::getTxs() const
 {
     std::vector<TxDetails> result;
 

src/ripple/rpc/handlers/AccountInfo.cpp

@@ -128,8 +128,7 @@ doAccountInfo(RPC::JsonContext& context)
         {
             Json::Value jvQueueData = Json::objectValue;
 
-            auto const txs =
-                context.app.getTxQ().getAccountTxs(accountID, *ledger);
+            auto const txs = context.app.getTxQ().getAccountTxs(accountID);
             if (!txs.empty())
             {
                 jvQueueData[jss::txn_count] =
@@ -298,7 +297,7 @@ doAccountInfoGrpc(
                 return {result, errorStatus};
             }
             std::vector<TxQ::TxDetails> const txs =
-                context.app.getTxQ().getAccountTxs(accountID, *ledger);
+                context.app.getTxQ().getAccountTxs(accountID);
             org::xrpl::rpc::v1::QueueData& queueData =
                 *result.mutable_queue_data();
             RPC::convert(queueData, txs);

src/ripple/rpc/handlers/LedgerHandler.cpp

@@ -94,7 +94,7 @@ LedgerHandler::check()
             return rpcINVALID_PARAMS;
         }
 
-        queueTxs_ = context_.app.getTxQ().getTxs(*ledger_);
+        queueTxs_ = context_.app.getTxQ().getTxs();
     }
 
     return Status::OK;