Improve deterministic transaction sorting in TxQ:

* Txs with the same fee level will sort by TxID XORed with the parent
  ledger hash.
* The TxQ is re-sorted after every ledger.
* Attempt to future-proof the TxQ tie breaking test

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 @@ public:
         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 @@ public:
         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 @@ private:
         */
         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 @@ private:
         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.
+         * fee, order by the arbitrary but consistent pseudo-randomized
-         * This allows validators to build similar queues in the same
+         * transaction ID. The ID is pseudo-randomized by XORing it with
-         * order, and thus have more similar initial proposals.
+         * 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 @@ private:
     */
     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 =
+            auto const txs = context.app.getTxQ().getAccountTxs(accountID);
-                context.app.getTxQ().getAccountTxs(accountID, *ledger);
             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;

src/test/rpc/LedgerRPC_test.cpp

@@ -1539,10 +1539,11 @@ class LedgerRPC_test : public beast::unit_test::suite
 
         jrr = env.rpc("json", "ledger", to_string(jv))[jss::result];
         const std::string txid1 = [&]() {
+            auto const& parentHash = env.current()->info().parentHash;
             if (BEAST_EXPECT(jrr[jss::queue_data].size() == 2))
             {
-                const std::string txid0 = [&]() {
+                const std::string txid1 = [&]() {
-                    auto const& txj = jrr[jss::queue_data][0u];
+                    auto const& txj = jrr[jss::queue_data][1u];
                     BEAST_EXPECT(txj[jss::account] == alice.human());
                     BEAST_EXPECT(txj[jss::fee_level] == "256");
                     BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS");
@@ -1554,7 +1555,7 @@ class LedgerRPC_test : public beast::unit_test::suite
                     return tx[jss::hash].asString();
                 }();
 
-                auto const& txj = jrr[jss::queue_data][1u];
+                auto const& txj = jrr[jss::queue_data][0u];
                 BEAST_EXPECT(txj[jss::account] == alice.human());
                 BEAST_EXPECT(txj[jss::fee_level] == "256");
                 BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS");
@@ -1563,9 +1564,12 @@ class LedgerRPC_test : public beast::unit_test::suite
                 auto const& tx = txj[jss::tx];
                 BEAST_EXPECT(tx[jss::Account] == alice.human());
                 BEAST_EXPECT(tx[jss::TransactionType] == jss::OfferCreate);
-                const auto txid1 = tx[jss::hash].asString();
+                const auto txid0 = tx[jss::hash].asString();
-                BEAST_EXPECT(txid0 < txid1);
+                uint256 tx0, tx1;
-                return txid1;
+                BEAST_EXPECT(tx0.parseHex(txid0));
+                BEAST_EXPECT(tx1.parseHex(txid1));
+                BEAST_EXPECT((tx0 ^ parentHash) < (tx1 ^ parentHash));
+                return txid0;
             }
             return std::string{};
         }();
@@ -1577,6 +1581,7 @@ class LedgerRPC_test : public beast::unit_test::suite
         jrr = env.rpc("json", "ledger", to_string(jv))[jss::result];
         if (BEAST_EXPECT(jrr[jss::queue_data].size() == 2))
         {
+            auto const& parentHash = env.current()->info().parentHash;
             auto const txid0 = [&]() {
                 auto const& txj = jrr[jss::queue_data][0u];
                 BEAST_EXPECT(txj[jss::account] == alice.human());
@@ -1593,7 +1598,10 @@ class LedgerRPC_test : public beast::unit_test::suite
             BEAST_EXPECT(txj["last_result"] == "terPRE_SEQ");
             BEAST_EXPECT(txj.isMember(jss::tx));
             BEAST_EXPECT(txj[jss::tx] == txid1);
-            BEAST_EXPECT(txid0 < txid1);
+            uint256 tx0, tx1;
+            BEAST_EXPECT(tx0.parseHex(txid0));
+            BEAST_EXPECT(tx1.parseHex(txid1));
+            BEAST_EXPECT((tx0 ^ parentHash) < (tx1 ^ parentHash));
         }
 
         env.close();