Fix bug in qualityUpperBound:

* In and Out parameters were swapped when calculating the rate
* In and out qualities were not calculated correctly; use existing functions
  to get the qualities
* Added tests to check that theoretical quality matches actual computed quality
* Remove in/out parameter from qualityUpperBound
* Rename an overload of qualityUpperBound to adjustQualityWithFees
* Add fix amendment

Builds/CMake/RippledCore.cmake

@@ -755,6 +755,7 @@ else ()
     src/test/app/SetRegularKey_test.cpp
     src/test/app/SetTrust_test.cpp
     src/test/app/Taker_test.cpp
+    src/test/app/TheoreticalQuality_test.cpp
     src/test/app/Ticket_test.cpp
     src/test/app/Transaction_ordering_test.cpp
     src/test/app/TrustAndBalance_test.cpp

src/ripple/app/paths/impl/BookStep.cpp

@@ -123,8 +123,8 @@ class BookStep : public StepImp<TIn, TOut, BookStep<TIn, TOut, TDerived>>
         return book_;
     }
 
-    boost::optional<Quality>
-    qualityUpperBound(ReadView const& v, DebtDirection& dir) const override;
+    std::pair<boost::optional<Quality>, DebtDirection>
+    qualityUpperBound(ReadView const& v, DebtDirection prevStepDir) const override;
 
     std::pair<TIn, TOut>
     revImp (
@@ -247,7 +247,7 @@ class BookPaymentStep
     }
 
     Quality
-    qualityUpperBound(ReadView const& v,
+    adjustQualityWithFees(ReadView const& v,
         Quality const& ofrQ,
         DebtDirection prevStepDir) const
     {
@@ -392,7 +392,7 @@ class BookOfferCrossingStep
     }
 
     Quality
-    qualityUpperBound(ReadView const& v,
+    adjustQualityWithFees(ReadView const& v,
         Quality const& ofrQ,
         DebtDirection prevStepDir) const
     {
@@ -426,21 +426,22 @@ bool BookStep<TIn, TOut, TDerived>::equal (Step const& rhs) const
 }
 
 template <class TIn, class TOut, class TDerived>
-boost::optional<Quality>
+std::pair<boost::optional<Quality>, DebtDirection>
 BookStep<TIn, TOut, TDerived>::qualityUpperBound(
-    ReadView const& v, DebtDirection& dir) const
+    ReadView const& v,
+    DebtDirection prevStepDir) const
 {
-    auto const prevStepDir = dir;
-    dir = this->debtDirection(v, StrandDirection::forward);
+    auto const dir = this->debtDirection(v, StrandDirection::forward);
 
     // This can be simplified (and sped up) if directories are never empty.
     Sandbox sb(&v, tapNONE);
     BookTip bt(sb, book_);
     if (!bt.step(j_))
-        return boost::none;
+        return {boost::none, dir};
 
-    return static_cast<TDerived const*>(this)->qualityUpperBound(
+    Quality const q = static_cast<TDerived const*>(this)->adjustQualityWithFees(
         v, bt.quality(), prevStepDir);
+    return {q, dir};
 }
 
 // Adjust the offer amount and step amount subject to the given input limit

src/ripple/app/paths/impl/DirectStep.cpp

@@ -23,6 +23,7 @@
 #include <ripple/basics/IOUAmount.h>
 #include <ripple/basics/Log.h>
 #include <ripple/ledger/PaymentSandbox.h>
+#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/Quality.h>
 
 #include <boost/container/flat_set.hpp>
@@ -153,8 +154,8 @@ class DirectStepI : public StepImp<IOUAmount, IOUAmount, DirectStepI<TDerived>>
     std::uint32_t
     lineQualityIn (ReadView const& v) const override;
 
-    boost::optional<Quality>
-    qualityUpperBound(ReadView const& v, DebtDirection& dir) const override;
+    std::pair<boost::optional<Quality>, DebtDirection>
+    qualityUpperBound(ReadView const& v, DebtDirection dir) const override;
 
     std::pair<IOUAmount, IOUAmount>
     revImp (
@@ -820,24 +821,42 @@ DirectStepI<TDerived>::lineQualityIn (ReadView const& v) const
 }
 
 template <class TDerived>
-boost::optional<Quality>
-DirectStepI<TDerived>::qualityUpperBound(ReadView const& v, DebtDirection& dir)
+std::pair<boost::optional<Quality>, DebtDirection>
+DirectStepI<TDerived>::qualityUpperBound(ReadView const& v, DebtDirection prevStepDir)
     const
 {
-    auto const prevStepDebtDir = dir;
-    dir = this->debtDirection(v, StrandDirection::forward);
-    std::uint32_t const srcQOut = [&]() -> std::uint32_t {
-        if (redeems(prevStepDebtDir) && issues(dir))
-            return transferRate(v, src_).value;
-        return QUALITY_ONE;
-    }();
-    auto dstQIn =
-        static_cast<TDerived const*>(this)->quality(v, QualityDirection::in);
+    auto const dir = this->debtDirection(v, StrandDirection::forward);
+
+    if (!v.rules().enabled(fixQualityUpperBound))
+    {
+        std::uint32_t const srcQOut = [&]() -> std::uint32_t {
+            if (redeems(prevStepDir) && issues(dir))
+                return transferRate(v, src_).value;
+            return QUALITY_ONE;
+        }();
+        auto dstQIn = static_cast<TDerived const*>(this)->quality(
+            v, QualityDirection::in);
+
+        if (isLast_ && dstQIn > QUALITY_ONE)
+            dstQIn = QUALITY_ONE;
+        Issue const iss{currency_, src_};
+        return {Quality(getRate(STAmount(iss, srcQOut), STAmount(iss, dstQIn))),
+                dir};
+    }
+
+    auto const [srcQOut, dstQIn] = redeems(dir)
+        ? qualitiesSrcRedeems(v)
+        : qualitiesSrcIssues(v, prevStepDir);
 
-    if (isLast_ && dstQIn > QUALITY_ONE)
-        dstQIn = QUALITY_ONE;
     Issue const iss{currency_, src_};
-    return Quality(getRate(STAmount(iss, srcQOut), STAmount(iss, dstQIn)));
+    // Be careful not to switch the parameters to `getRate`. The
+    // `getRate(offerOut, offerIn)` function is usually used for offers. It
+    // returns offerIn/offerOut. For a direct step, the rate is srcQOut/dstQIn
+    // (Input*dstQIn/srcQOut = Output; So rate = srcQOut/dstQIn). Although the
+    // first parameter is called `offerOut`, it should take the `dstQIn`
+    // variable.
+    return {Quality(getRate(STAmount(iss, dstQIn), STAmount(iss, srcQOut))),
+            dir};
 }
 
 template <class TDerived>

src/ripple/app/paths/impl/Steps.h

@@ -176,18 +176,24 @@ class Step
         return QUALITY_ONE;
     }
 
+    // clang-format off
     /**
        Find an upper bound of quality for the step
 
        @param v view to query the ledger state from
-       @param dir in/out param. Set to DebtDirection::redeems if the previous step redeems.
-       Will be set to DebtDirection::redeems if this step redeems; Will be set to DebtDirection::issues if this
-       step does not redeem
-       @return The upper bound of quality for the step, or boost::none if the
-       step is dry.
+       @param prevStepDir Set to DebtDirection::redeems if the previous step redeems.
+       @return A pair. The first element is the upper bound of quality for the step, or boost::none if the
+       step is dry. The second element will be set to DebtDirection::redeems if this steps redeems,
+       DebtDirection:issues if this step issues.
+       @note it is an upper bound because offers on the books may be unfunded.
+       If there is always a funded offer at the tip of the book, then we could
+       rename this `theoreticalQuality` rather than `qualityUpperBound`. It
+       could still differ from the actual quality, but except for "dust" amounts,
+       it should be a good estimate for the actual quality.
      */
-    virtual boost::optional<Quality>
-    qualityUpperBound(ReadView const& v, DebtDirection& dir) const = 0;
+    // clang-format on
+    virtual std::pair<boost::optional<Quality>, DebtDirection>
+    qualityUpperBound(ReadView const& v, DebtDirection prevStepDir) const = 0;
 
     /**
        If this step is a BookStep, return the book.

src/ripple/app/paths/impl/StrandFlow.h

@@ -320,6 +320,24 @@ struct FlowResult
 };
 /// @endcond
 
+/// @cond INTERNAL
+inline boost::optional<Quality>
+qualityUpperBound(ReadView const& v, Strand const& strand)
+{
+    Quality q{STAmount::uRateOne};
+    boost::optional<Quality> stepQ;
+    DebtDirection dir = DebtDirection::issues;
+    for (auto const& step : strand)
+    {
+        if (std::tie(stepQ, dir) = step->qualityUpperBound(v, dir); stepQ)
+            q = composed_quality(q, *stepQ);
+        else
+            return boost::none;
+    }
+    return q;
+};
+/// @endcond
+
 /// @cond INTERNAL
 /* Track the non-dry strands
 
@@ -396,23 +414,6 @@ class ActiveStrands
 };
 /// @endcond
 
-/// @cond INTERNAL
-boost::optional<Quality>
-qualityUpperBound(ReadView const& v, Strand const& strand)
-{
-    Quality q{STAmount::uRateOne};
-    DebtDirection dir = DebtDirection::issues;
-    for (auto const& step : strand)
-    {
-        if (auto const stepQ = step->qualityUpperBound(v, dir))
-            q = composed_quality(q, *stepQ);
-        else
-            return boost::none;
-    }
-    return q;
-};
-/// @endcond
-
 /**
    Request `out` amount from a collection of strands
 

src/ripple/app/paths/impl/XRPEndpointStep.cpp

@@ -95,8 +95,8 @@ class XRPEndpointStep : public StepImp<
         return DebtDirection::issues;
     }
 
-    boost::optional<Quality>
-    qualityUpperBound(ReadView const& v, DebtDirection& dir) const override;
+    std::pair<boost::optional<Quality>, DebtDirection>
+    qualityUpperBound(ReadView const& v, DebtDirection prevStepDir) const override;
 
     std::pair<XRPAmount, XRPAmount>
     revImp (
@@ -241,15 +241,14 @@ inline bool operator==(XRPEndpointStep<TDerived> const& lhs,
 }
 
 template <class TDerived>
-boost::optional<Quality>
+std::pair<boost::optional<Quality>, DebtDirection>
 XRPEndpointStep<TDerived>::qualityUpperBound(
-    ReadView const& v, DebtDirection& dir) const
+    ReadView const& v, DebtDirection prevStepDir) const
 {
-    dir = this->debtDirection(v, StrandDirection::forward);
-    return Quality{STAmount::uRateOne};
+    return {Quality{STAmount::uRateOne},
+            this->debtDirection(v, StrandDirection::forward)};
 }
 
-
 template <class TDerived>
 std::pair<XRPAmount, XRPAmount>
 XRPEndpointStep<TDerived>::revImp (

src/ripple/protocol/Feature.h

@@ -108,6 +108,8 @@ class FeatureCollections
         "fixCheckThreading",
         "fixPayChanRecipientOwnerDir",
         "DeletableAccounts",
+        // fixQualityUpperBound should be activated before FlowCross
+        "fixQualityUpperBound",
     };
 
     std::vector<uint256> features;
@@ -394,6 +396,7 @@ extern uint256 const fixMasterKeyAsRegularKey;
 extern uint256 const fixCheckThreading;
 extern uint256 const fixPayChanRecipientOwnerDir;
 extern uint256 const featureDeletableAccounts;
+extern uint256 const fixQualityUpperBound;
 
 } // ripple
 

src/ripple/protocol/Quality.h

@@ -25,6 +25,7 @@
 #include <ripple/protocol/AmountConversions.h>
 #include <ripple/protocol/STAmount.h>
 
+#include <algorithm>
 #include <cstdint>
 #include <ostream>
 
@@ -122,6 +123,9 @@ class Quality
     static const int maxTickSize = 16;
 
 private:
+    // This has the same representation as STAmount, see the comment on the STAmount.
+    // However, this class does not alway use the canonical representation. In particular,
+    // the increment and decrement operators may cause a non-canonical representation.
     value_type m_value;
 
 public:
@@ -270,6 +274,39 @@ class Quality
         os << quality.m_value;
         return os;
     }
+
+    // return the relative distance (relative error) between two qualities. This is used for testing only.
+    // relative distance is abs(a-b)/min(a,b)
+    friend double
+    relativeDistance(Quality const& q1, Quality const& q2)
+    {
+        assert(q1.m_value > 0 && q2.m_value > 0);
+
+        if (q1.m_value == q2.m_value)  // make expected common case fast
+            return 0;
+
+        auto const [minV, maxV] = std::minmax(q1.m_value, q2.m_value);
+
+        auto mantissa = [](std::uint64_t rate) {
+            return rate & ~(255ull << (64 - 8));
+        };
+        auto exponent = [](std::uint64_t rate) {
+            return static_cast<int>(rate >> (64 - 8)) - 100;
+        };
+
+        auto const minVMantissa = mantissa(minV);
+        auto const maxVMantissa = mantissa(maxV);
+        auto const expDiff = exponent(maxV) - exponent(minV);
+
+        double const minVD = static_cast<double>(minVMantissa);
+        double const maxVD = expDiff ? maxVMantissa * pow(10, expDiff)
+                                     : static_cast<double>(maxVMantissa);
+
+        // maxVD and minVD are scaled so they have the same exponents. Dividing
+        // cancels out the exponents, so we only need to deal with the (scaled)
+        // mantissas
+        return (maxVD - minVD) / minVD;
+    }
 };
 
 /** Calculate the quality of a two-hop path given the two hops.

src/ripple/protocol/impl/Feature.cpp

@@ -128,6 +128,7 @@ detail::supportedAmendments ()
         "fixCheckThreading",
         "fixPayChanRecipientOwnerDir",
         "DeletableAccounts",
+        "fixQualityUpperBound",
     };
     return supported;
 }
@@ -185,5 +186,6 @@ uint256 const fixMasterKeyAsRegularKey = *getRegisteredFeature("fixMasterKeyAsRe
 uint256 const fixCheckThreading = *getRegisteredFeature("fixCheckThreading");
 uint256 const fixPayChanRecipientOwnerDir = *getRegisteredFeature("fixPayChanRecipientOwnerDir");
 uint256 const featureDeletableAccounts = *getRegisteredFeature("DeletableAccounts");
+uint256 const fixQualityUpperBound = *getRegisteredFeature("fixQualityUpperBound");
 
 } // ripple