Fix V2 line-quality bugs

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

@@ -39,6 +39,7 @@ class DirectStepI : public StepImp<IOUAmount, IOUAmount, DirectStepI>
     AccountID src_;
     AccountID dst_;
     Currency currency_;
+    bool isLast_ = false;
 
     // Charge transfer fees when the prev step redeems
     Step const* const prevStep_ = nullptr;
@@ -72,18 +73,22 @@ class DirectStepI : public StepImp<IOUAmount, IOUAmount, DirectStepI>
         PaymentSandbox& sb,
         bool srcRedeems,
         bool fwd) const;
+
   public:
     DirectStepI (
         AccountID const& src,
         AccountID const& dst,
         Currency const& c,
         Step const* prevStep,
+        bool isLast,
         beast::Journal j)
             :src_(src)
             , dst_(dst)
             , currency_ (c)
+            , isLast_ (isLast)
             , prevStep_ (prevStep)
-            , j_ (j) {}
+            , j_ (j)
+    {}
 
     AccountID const& src () const
     {
@@ -122,6 +127,9 @@ class DirectStepI : public StepImp<IOUAmount, IOUAmount, DirectStepI>
     bool
     redeems (ReadView const& sb, bool fwd) const override;
 
+    std::uint32_t
+    lineQualityIn (ReadView const& v) const override;
+
     std::pair<IOUAmount, IOUAmount>
     revImp (
         PaymentSandbox& sb,
@@ -484,11 +492,11 @@ DirectStepI::validFwd (
 static
 std::uint32_t
 quality (
-    PaymentSandbox const& sb,
+    ReadView const& sb,
     AccountID const& src,
     AccountID const& dst,
     Currency const& currency,
-    // set true for quality in, false for quality out
+    // set true for dst quality in, false for src quality out
     bool qin)
 {
     if (src == dst)
@@ -501,18 +509,20 @@ quality (
         return QUALITY_ONE;
 
     auto const& field = [&]() -> SF_U32 const&
-    {
-        if (dst < src)
     {
         if (qin)
+        {
+            // compute dst quality in
+            if (dst < src)
                 return sfLowQualityIn;
             else
-                return sfLowQualityOut;
+                return sfHighQualityIn;
         }
         else
         {
-            if (qin)
+            // compute src quality out
-                return sfHighQualityIn;
+            if (src < dst)
+                return sfLowQualityOut;
             else
                 return sfHighQualityOut;
         }
@@ -536,27 +546,35 @@ DirectStepI::qualities (
 {
     if (srcRedeems)
     {
-        return std::make_pair(
+        if (!prevStep_)
-            quality (
+            return {QUALITY_ONE, QUALITY_ONE};
-                sb, src_, dst_, currency_,
+
-                false),
+        auto const prevStepQIn = prevStep_->lineQualityIn (sb);
-            QUALITY_ONE);
+        auto srcQOut = quality (sb, src_, dst_, currency_, false);
+        if (prevStepQIn > srcQOut)
+            srcQOut = prevStepQIn;
+        return {srcQOut, QUALITY_ONE};
     }
     else
     {
         // Charge a transfer rate when issuing and previous step redeems
         auto const prevStepRedeems = prevStep_ && prevStep_->redeems (sb, fwd);
-        std::uint32_t const srcQOut = prevStepRedeems
+        std::uint32_t const srcQOut =
-            ? transferRate (sb, src_).value
+            prevStepRedeems ? transferRate (sb, src_).value : QUALITY_ONE;
-            : QUALITY_ONE;
+        auto dstQIn = quality (sb, src_, dst_, currency_, true);
-        return std::make_pair(
+        if (isLast_ && dstQIn > QUALITY_ONE)
-            srcQOut,
+            dstQIn = QUALITY_ONE;
-            quality ( // dst quality in
+        return {srcQOut, dstQIn};
-                sb, src_, dst_, currency_,
-                true));
     }
 }
 
+std::uint32_t
+DirectStepI::lineQualityIn (ReadView const& v) const
+{
+    // dst quality in
+    return quality (v, src_, dst_, currency_, true);
+}
+
 TER DirectStepI::check (StrandContext const& ctx) const
 {
     if (!src_ || !dst_)
@@ -565,6 +583,12 @@ TER DirectStepI::check (StrandContext const& ctx) const
         return temBAD_PATH;
     }
 
+    if (src_ == dst_)
+    {
+        JLOG (j_.debug()) << "DirectStepI: same src and dst.";
+        return temBAD_PATH;
+    }
+
     {
         auto sleSrc = ctx.view.read (keylet::account (src_));
         if (!sleSrc)
@@ -692,7 +716,7 @@ make_DirectStepI (
 {
     // Only charge a transfer fee if the previous step redeems
     auto r = std::make_unique<DirectStepI> (
-        src, dst, c, ctx.prevStep, ctx.j);
+        src, dst, c, ctx.prevStep, ctx.isLast, ctx.j);
     auto ter = r->check (ctx);
     if (ter != tesSUCCESS)
         return {ter, nullptr};

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

@@ -22,6 +22,7 @@
 
 #include <ripple/app/paths/impl/AmountSpec.h>
 #include <ripple/basics/Log.h>
+#include <ripple/protocol/Quality.h>
 #include <ripple/protocol/STLedgerEntry.h>
 #include <ripple/protocol/TER.h>
 
@@ -132,6 +133,14 @@ public:
         return false;
     }
 
+    /** If this step is a DirectStepI, return the quality in of the dst account.
+     */
+    virtual std::uint32_t
+    lineQualityIn (ReadView const&) const
+    {
+        return QUALITY_ONE;
+    }
+
     /**
        If this step is a BookStep, return the book.
     */

src/ripple/app/tests/Flow_test.cpp

@@ -512,6 +512,82 @@ struct Flow_test : public beast::unit_test::suite
         }
     }
 
+    void testLineQuality ()
+    {
+        testcase ("Line Quality");
+
+        using namespace jtx;
+        auto const alice = Account ("alice");
+        auto const bob = Account ("bob");
+        auto const carol = Account ("carol");
+        auto const dan = Account ("dan");
+        auto const USDA = alice["USD"];
+        auto const USDB = bob["USD"];
+        auto const USDC = carol["USD"];
+        auto const USDD = dan["USD"];
+
+        //   Dan -> Bob -> Alice -> Carol; vary bobDanQIn and bobAliceQOut
+        for (auto bobDanQIn : {80, 100, 120})
+            for (auto bobAliceQOut : {80, 100, 120})
+                for (auto const& f : {feature ("nullFeature"), featureFlowV2})
+                {
+                    if (f != featureFlowV2 && bobDanQIn < 100 && bobAliceQOut < 100)
+                        continue;  // Bug in flow v1
+                    Env env (*this, features (f));
+                    env.fund (XRP (10000), alice, bob, carol, dan);
+                    env (trust (bob, USDD (100)), qualityInPercent (bobDanQIn));
+                    env (trust (bob, USDA (100)),
+                        qualityOutPercent (bobAliceQOut));
+                    env (trust (carol, USDA (100)));
+
+                    env (pay (alice, bob, USDA (100)));
+                    env.require (balance (bob, USDA (100)));
+                    env (pay (dan, carol, USDA (10)), path (bob),
+                        sendmax (USDD (100)), txflags (tfNoRippleDirect));
+                    env.require (balance (bob, USDA (90)));
+                    if (bobAliceQOut > bobDanQIn)
+                        env.require (
+                            balance (bob, USDD (10.0 * double(bobAliceQOut) /
+                                              double(bobDanQIn))));
+                    else
+                        env.require (balance (bob, USDD (10)));
+                    env.require (balance (carol, USDA (10)));
+                }
+
+        // bob -> alice -> carol; vary carolAliceQIn
+        for (auto carolAliceQIn : {80, 100, 120})
+            for (auto const& f : {feature ("nullFeature"), featureFlowV2})
+            {
+                Env env (*this, features (f));
+                env.fund (XRP (10000), alice, bob, carol);
+                env (trust (bob, USDA (10)));
+                env (trust (carol, USDA (10)), qualityInPercent (carolAliceQIn));
+
+                env (pay (alice, bob, USDA (10)));
+                env.require (balance (bob, USDA (10)));
+                env (pay (bob, carol, USDA (5)), sendmax (USDA (10)));
+                auto const effectiveQ =
+                    carolAliceQIn > 100 ? 1.0 : carolAliceQIn / 100.0;
+                env.require (balance (bob, USDA (10.0 - 5.0 / effectiveQ)));
+            }
+
+        // bob -> alice -> carol; bobAliceQOut varies.
+        for (auto bobAliceQOut : {80, 100, 120})
+            for (auto const& f : {feature ("nullFeature"), featureFlowV2})
+            {
+                Env env (*this, features (f));
+                env.fund (XRP (10000), alice, bob, carol);
+                env (trust (bob, USDA (10)), qualityOutPercent (bobAliceQOut));
+                env (trust (carol, USDA (10)));
+
+                env (pay (alice, bob, USDA (10)));
+                env.require (balance (bob, USDA (10)));
+                env (pay (bob, carol, USDA (5)), sendmax (USDA (5)));
+                env.require (balance (carol, USDA (5)));
+                env.require (balance (bob, USDA (10-5)));
+            }
+    }
+
     void testBookStep ()
     {
         testcase ("Book Step");
@@ -1185,6 +1261,7 @@ struct Flow_test : public beast::unit_test::suite
     void run() override
     {
         testDirectStep ();
+        testLineQuality();
         testBookStep ();
         testTransferRate ();
         testToStrand ();