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 @@ 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;