Rm some offers where the quality is reduced #3827

seelabs · 2021-04-20T01:58:27Z

High Level Overview of Change

Substantial reductions in an offer's effective quality from its initial quality may clog offer books. This patch removes such offers, much like unfunded offers are removed.

Type of Change

[ x] Bug fix (non-breaking change which fixes an issue)

donovanhide · 2021-04-20T05:58:32Z

src/ripple/app/tx/impl/OfferStream.cpp

+                return offer_.amount();
+            }();
+
+            XRPAmount const thresholdToRm{2};


A comment to explain why this is 2 rather than 1 would be helpful in understanding the issue.

@donovanhide I changed it back to 1. Anything with values larger than 1 should give a quality that's reasonably in line with the initial quality. The reason I made it larger is I was thinking "removing some more tiny offers is a good thing", but they should be removed with regular operation. There's no need for the larger threshold.

Fixed in the patch 7474063b72 [fold] Change the threshold and minor refactor

seelabs · 2021-04-21T22:13:07Z

I made some changes to support IOU/IOU offers and squashed (squashed because the implementation was different enough that it wasn't useful to look at the older patch).

Substantial reductions in an offer's effective quality from its initial quality may clog offer books.

donovanhide · 2021-04-22T18:16:50Z

I've just encountered this blocking offer, as seen through

rippled book_offers "XRP" "CNY/rKiCet8SdvWxPXnAgYarFUXMh1zCPz432Y"

   "result" : {
      "ledger_current_index" : 63075722,
      "offers" : [
         {
            "Account" : "rBf5SF3p3UNssTwVQKn7egKREG1xRB7XRP",
            "BookDirectory" : "49789A0B460DC77A2CED9349C432AEA97352345BA3C7313A5A03E53E344E70C2",
            "BookNode" : "0",
            "Flags" : 0,
            "LedgerEntryType" : "Offer",
            "OwnerNode" : "0",
            "PreviousTxnID" : "5BC8338002DC3F89873529470FE85C283D39091013E0EE65AF4A861343E4278E",
            "PreviousTxnLgrSeq" : 63075059,
            "Sequence" : 1247166,
            "TakerGets" : {
               "currency" : "CNY",
               "issuer" : "rKiCet8SdvWxPXnAgYarFUXMh1zCPz432Y",
               "value" : "0.001632496332"
            },
            "TakerPays" : "179",
            "index" : "58887D81BD62D50C1B57B06F32ADFCAAC514BC159F7CDE7F984731D98E533B8F",
            "owner_funds" : "0.000003594192",
            "quality" : "109648.025842093",
            "taker_gets_funded" : {
               "currency" : "CNY",
               "issuer" : "rKiCet8SdvWxPXnAgYarFUXMh1zCPz432Y",
               "value" : "0.000003594192"
            },
            "taker_pays_funded" : "0"
         },

Is the definite proof that this offer is a blocker the taker_pays_funded value being 0 or the tiny taker_gets_funded amount?

scottschurr · 2021-04-22T20:23:39Z

I haven't finished my review yet, but the results are very promising. I just finished replaying the transaction that started this hunt. With the new code in place both Taker and FlowCross have the same results for that transaction. I think that's great news!

seelabs · 2021-04-23T02:02:25Z

@donovanhide I don't think we can look at the offer alone and find definite proof that it's a blocker. We need to look at the original offer quality and see how owner funds effect it. So I could concoct an offer with tiny owner funds and a reported taker_pays_funded of zero that may still not change the quality to block in practice.

Having said that, a reported XRP amount of taker_pays_funded of zero is a very strong indication that the offer would block. It means the taker_pays can't get lower and the quality is going to become lower than the initial quality.

As an aside, that taker_pays_funded looks like a bug to me. It should actually be 1, not 0. It looks like an edge case that isn't handed correctly in the reporting code.

donovanhide · 2021-04-23T06:18:36Z

@seelabs So the correct way to calculate the "blockerness" is to do TakerPays/TakerGets compared with taker_pays_funded/taker_gets_funded. If different, compare to the next offer's quality in the directory of offers for that orderbook and side combination? Obviously this doesn't work if taker_pays_funded gives the wrong value, but I can calculate XRP balances and reserve requirements using my own code.

seelabs · 2021-04-23T11:14:49Z

@donovanhide A decent check would be: 1) only look at offers with Taker Pays of XRP. 2) Calculate taker_pays_funded. If this is over a drop, it's not blocking. 3) If this is under a drop, round up to a drop (you could just look at the reported taker_pays_funded and check if it's zero. If so, assume it should have been one). 4) Calculate the "effective quality" of taker_gets_funded/taker_pays_funded. If the "effective quality" is smaller than the quality it was initially placed at, it may be blocking. The larger the quality difference, the more blocking it is.

Having said all that, a good "quick and dirty check" is to look for funded offers with a reported taker_pays_funded of zero drops. It's not foolproof, but it should work well in practice.

One more quick note: A small payment that sends XRP and delivers CNY should clear that blocking offer. I'll also be AFK most of the day today, but will respond to further messages later tonight to tomorrow.

donovanhide · 2021-04-23T11:34:43Z

@seelabs That's great, thanks for the details!

yxxyun · 2021-04-24T02:11:17Z

XRP/XLM.Ripplefox is blocked by dust offer, path payment seems can't remove it?

scottschurr · 2021-04-27T01:57:10Z

I'm still working on the code review, but I'm almost done. Nice work!

I noted the concern that we're not removing underfunded offers when the payment/offer crossing fails. So I decided to see how important that concern might be. I kept a record of Offer Crossing and Payments for an hour to see the ratio of successes to tec failures. A failure with something other than tesSUCCESS or tec* won't have any effect on the ledger.

               Count  Percent
Offer Create:  44541   100.0%
  Success:     44272    99.4%
  Claim:         269     0.6%

Payment:        6208   100.0%
  Success:      5563    89.6%
  Claim:         645    10.4%

Total:         50749   100.0%
  Success:     49835    98.2%
  Claim:         914     1.8%

I was surprised that the percentage of failing payments was that high. But even at that, if we look a the sum of offer crossing and payments success vs tec the failure rate is under 2%, at least for that one hour sampling window. I take that as a suggestion that we don't need to worry too much about removing very small offers on transactions that fail with a tec.

scottschurr

Nice changes and great unit tests. I left a few thoughts for you to consider, but none of them are show stoppers for me.

scottschurr · 2021-04-26T20:16:53Z

src/test/app/Offer_test.cpp

+            env.close();
+            env.trust(USD(1000), alice, bob, carol);
+            // underfund carol's offer
+            auto initialCarolUSD = USD(0.1);


FWIW, this value can be raised to USD(0.499) and still have the test behave as expected. At USD(0.5) the case that enables all amendments other than rmSmallIncreasedQOffers fails (which is what we'd hope for).

My inclination would be to use USD(0.499) for the test. It's nice to set the test conditions near an inflection point so we are more sensitive to conditions changing in the future. But that's just a suggestion.

fixed in d77ad06020 [fold] Change test params so they're closer to behavior change thresholds

scottschurr · 2021-04-26T20:35:39Z

src/test/app/Offer_test.cpp

+            env.close();
+            env.trust(USD(1000), alice, bob, carol);
+            env.close();
+            auto const initialCarolUSD = USD(0.1);


Oh! Interesting! Here we can run this value all the way up to USD(0.999) and have the test pass as expected under the various enabled amendments. If I take the value to USD(1.0) then the test fails the case that enables all amendments other than rmSmallIncreasedQOffers.

So my inclination would be to use USD(0.999) here, since it's close to the cusp where the test would start to fail. That way if conditions change in the future we're more likely to see it. That's just a suggestion, however.

It's interesting that the critical point is different between offer crossing and payments. I suspect it's a difference in rounding behaviors, but I didn't chase it down. I think the difference is interesting, but not critical.

fixed in d77ad06020 [fold] Change test params so they're closer to behavior change thresholds

scottschurr · 2021-04-27T17:22:37Z

src/ripple/app/tx/impl/OfferStream.cpp

+    static_assert(
+        std::is_same_v<TTakerPays, IOUAmount> ||
+            std::is_same_v<TTakerPays, XRPAmount>,
+        "");


Rather than leave the quotes empty, I might be inclined to say "STAmount is not supported", since that's the most likely alternative. But your call.

fixed in 9b8c050cb3 [fold] Minor cleanups

scottschurr · 2021-04-27T17:23:00Z

src/ripple/app/tx/impl/OfferStream.cpp

+    static_assert(
+        std::is_same_v<TTakerGets, IOUAmount> ||
+            std::is_same_v<TTakerGets, XRPAmount>,
+        "");


Same comment as above.

fixed in 9b8c050cb3 [fold] Minor cleanups

scottschurr · 2021-04-27T17:28:51Z

src/ripple/app/tx/impl/OfferStream.cpp

+        return false;
+
+    // Consider removing the offer if `TakerPays` is XRP or `TakerPays` and
+    // `TakerGets` are both IOU and `TakerPays` < `TakerGets`


It took quite a while for me to make sense out the comment regarding IOUs. Everything here is correct, but the consequences did not jump out at me. Consider rewriting the comment something like this:

// Consider removing the offer... // o If `TakerPays` is XRP (because of XRP drops granularity) or // o If... // - `TakerPays` and `TakerGets` are both IOU and // - `TakerPays` < `TakerGets` (numerator < denominator)

fixed in 9b8c050cb3 [fold] Minor cleanups

scottschurr · 2021-04-27T17:37:32Z

src/ripple/basics/IOUAmount.h

@@ -129,6 +129,9 @@ class IOUAmount : private boost::totally_ordered<IOUAmount>,
    {
        return mantissa_;
    }
+
+    static IOUAmount
+    minPositiveAmount();


Long term, this could be made constexpr. But that will require making IOUAmount::normalize() constexpr, which is too big a change to make right now.

If it were easy I'd do it, but this is going to be a rarely called function; making this constexpr isn't going to to make much difference one way or the other.

scottschurr · 2021-04-27T17:50:10Z

src/test/app/Offer_test.cpp

+            }
+        }
+    }
+


I'll mention that there's a test case in this file (around line 5700) that displays "false assert". Taken out of context that name is a little scary. I chased it briefly until I figured out where it was coming from. As a drive-by change consider changing

testcase("false assert");

to

testcase("incorrect assert fixed");

Just a thought.

fixed in 9b8c050cb3 [fold] Minor cleanups

scottschurr · 2021-04-27T17:59:37Z

src/ripple/app/tx/impl/OfferStream.cpp

@@ -222,6 +291,66 @@ TOfferStreamBase<TIn, TOut>::step()
                    << "Removing became unfunded offer " << entry->key();
            }
            offer_ = TOffer<TIn, TOut>{};
+            // See comment at top of loop for why the offer is removed


I think the comments in the block describe why we're removing the offer, what's not quite clear at this point is how. The comment at the top of the loop makes that clear. Consider changing this comment to "... for how the ...".

fixed in 9b8c050cb3 [fold] Minor cleanups

scottschurr · 2021-04-27T18:00:12Z

src/ripple/app/tx/impl/OfferStream.cpp

+                // Without the `if constexpr`, the
+                // `shouldRmSmallIncreasedQOffer` template will be instantiated
+                // even if it is never used. This can cause compiler errors in
+                // some cases, hense the `if constexpr` guard.


Great comment! Thanks! BTW, perhaps change "hense" to "hence'?

fixed in 9b8c050cb3 [fold] Minor cleanups

scottschurr · 2021-04-27T18:03:14Z

src/ripple/app/tx/impl/OfferStream.cpp

+                                 << entry->key();
+            }
+            offer_ = TOffer<TIn, TOut>{};
+            // See comment at top of loop for why the offer is removed


Similar to earlier comment. Consider "... for how the ...".

fixed in 9b8c050cb3 [fold] Minor cleanups

…olds

seelabs · 2021-04-27T20:13:07Z

Thanks for the great view @scottschurr! I think I addressed all the comments.

scottschurr

👍

nbougalis

Looks reasonable. Left some comments that I'd like to see answered/addressed before signing off.

nbougalis · 2021-05-06T06:37:07Z

src/ripple/protocol/AmountConversions.h

+template <class T>
+T
+toAmount(IOUAmount const& amt)
+{
+    static_assert(sizeof(T) == -1, "Must use specialized function");
+    return T(0);
+}


Why not just = delete this instead of using the curious sizeof(T) == -1 construct?

I never think of using delete for anything but constructors and assignment operators. But you're right, this is a perfect use for them. Thanks!

Fixed in a06ca4c951 Misc cleanups (Nik's review)

nbougalis · 2021-05-06T06:38:41Z

src/test/app/Offer_test.cpp


        // Carol doesn't quite have enough funds for this offer
        // The amount left after this offer is taken will cause
        // STAmount to incorrectly round to zero when the next offer
        // (at a good quality) is considered. (when the
        // stAmountCalcSwitchover2 patch is inactive)
-        env(offer(carol, drops(1), USD(1)));
+        env(offer(carol, drops(1), USD(0.99999)));


I think stAmountCalcSwitchover2 was removed, so while here we should update the comment.

I didn't want to lose the reference to stAmountCalcSwitchover2. I modified the comment to say that is "now removed". Keeping that reference is a reminder of the motivation for this test case.

Fixed in a06ca4c951 Misc cleanups (Nik's review)

nbougalis · 2021-05-06T06:42:33Z

src/ripple/app/tx/impl/OfferStream.cpp

+
+    TTakerPays const thresh = TTakerPays::minPositiveAmount();
+
+    if (effectiveAmounts.in > thresh)


At your option, micronit: thresh is only ever used here and could just be inlined: if (effectiveAmounts.in > TTakerPays::minPositiveAmount())

Fixed in a06ca4c951 Misc cleanups (Nik's review)

nbougalis · 2021-05-06T06:57:53Z

src/ripple/app/tx/impl/OfferStream.cpp

+                if constexpr (!(std::is_same_v<TIn, IOUAmount> ||
+                                std::is_same_v<TOut, XRPAmount>))
+                    return shouldRmSmallIncreasedQOffer<XRPAmount, IOUAmount>();


The logic here just confuses me. First, is there a reason we don't use (!std::is_same_v<TIn, IOUAmount> && !std::is_same_v<TOut, XRPAmount>)? For that matter, why not just say what we, apparently, mean, which I think is: (std::is_same_v<TIn, XRPAmount> && std::is_same_v<TOut, IOUAmount>)?

Second, and maybe it's the fact that it's almost midnight, but I'm confused why the conditional is even necessary? Under what circumstances do we enter this codepath where inIsXRP == true but std::is_same_v<TIn, XRPAmount> == false?

There are three questions:

Why not (!std::is_same_v<TIn, IOUAmount> && !std::is_same_v<TOut, XRPAmount>)?
Sometimes I write !(error_condition1 || error_condition2) instead of (!error_condition1 && !error_conditions2). Think of the huge savings by saving one fewer !. More seriously, I don't have a strong preference; I find both about equally readable.

Why not (std::is_same_v<TIn, XRPAmount> && std::is_same_v<TOut, IOUAmount>)?
Because TIn can be XRPAmount or STAmount, and TOut can be IOUAmount or STAmount.

Why is the conditional even needed? Under what circumstances do we enter this codepath...?
While the codepath would never be executed, the template would still be instantiated, and we'd get compiler errors. In particular, we'd end up calling things like: toAmount<IOUAmount>(xrpAmount); Even though we know this will never be executed, the compiler will still try to compile that and we'd get a compiler error.The if constexpr prevents the template from being instantiated at all.

I added some minor clarifying comments, but kept the code essentially the same. I can revisit if you still think this is confusing.

nbougalis · 2021-05-06T07:00:38Z

src/ripple/app/tx/impl/OfferStream.cpp

+            }
+            if (!inIsXRP && outIsXRP)
+            {
+                // See comment above for `if constexpr` rational


Typo: rational should be rationale.

Fixed in a06ca4c951 Misc cleanups (Nik's review)

nbougalis · 2021-05-06T07:01:15Z

src/ripple/app/tx/impl/OfferStream.cpp

+            }
+            else
+            {
+                JLOG(j_.trace()) << "Removing became tiny offer due to "


"Removing became" is gramatically incorrect.

It mirrors the "became unfunded" terminology we use for offers. Having said that, I don't mind rewording it. Changed to "removing tiny offer that became tiny due to..."

Fixed in a06ca4c951 Misc cleanups (Nik's review)

* Inline minPositiveAmount * Add some clarifying comments * Fix mispelling * Use deleted function instead of static_assert

scottschurr

👍 Looks good to me!

donovanhide · 2021-12-07T12:31:31Z

Unfortunately, looks like this is still happening:

rippled book_offers XRP CNY/rKiCet8SdvWxPXnAgYarFUXMh1zCPz432Y | more
Loading: "/etc/opt/ripple/rippled.cfg"
2021-Dec-07 12:28:03.605525281 UTC HTTPClient:NFO Connecting to 127.0.0.1:5005

{
   "result" : {
      "ledger_current_index" : 68183403,
      "offers" : [
         {
            "Account" : "rKcLoZAqpYTgiu47kWnV1XxzRNeXKkTcau",
            "BookDirectory" : "49789A0B460DC77A2CED9349C432AEA97352345BA3C7313A5A06C7AD4A172715",
            "BookNode" : "0",
            "Flags" : 0,
            "LedgerEntryType" : "Offer",
            "OwnerNode" : "0",
            "PreviousTxnID" : "A6CFD591B2FBD41CC19E8F693F4C45212DFBB18AB5FF3E9CFD0279F617826527",
            "PreviousTxnLgrSeq" : 68183064,
            "Sequence" : 333,
            "TakerGets" : {
               "currency" : "CNY",
               "issuer" : "rKiCet8SdvWxPXnAgYarFUXMh1zCPz432Y",
               "value" : "1.2000061817857"
            },
            "TakerPays" : "229008",
            "index" : "BEC40A8F5FD0A87F86EAA91496613EE41B6C9C8358222E8F2C31703D25FD4AE6",
            "owner_funds" : "0.0000061817857",
            "quality" : "190839.6946564885",
            "taker_gets_funded" : {
               "currency" : "CNY",
               "issuer" : "rKiCet8SdvWxPXnAgYarFUXMh1zCPz432Y",
               "value" : "0.0000061817857"
            },
            "taker_pays_funded" : "1"
         },

donovanhide · 2021-12-19T08:04:24Z

Might not be related, but this offer is uncrossable.

$ rippled book_offers ADA/rnNDjH8YTJLVQMccqLebzC77KLkWxrdEZi XRP
Loading: "/etc/opt/ripple/rippled.cfg"
2021-Dec-19 08:03:07.132081913 UTC HTTPClient:NFO Connecting to 127.0.0.1:5005

{
   "result" : {
      "ledger_current_index" : 68434259,
      "offers" : [
         {
            "Account" : "rLZLMZ922JCRHHMvzAtttw9gCdFfnTyZ34",
            "BookDirectory" : "2818605EC090C5150A69A8B4D3632B4271AC8257A6991685510AA87BEE538000",
            "BookNode" : "0",
            "Flags" : 131072,
            "LedgerEntryType" : "Offer",
            "OwnerNode" : "1",
            "PreviousTxnID" : "BCF09BA40BE31A8BDC2AC6C7651A96140E864A3CF83FDEB49170BD0149535E80",
            "PreviousTxnLgrSeq" : 68375269,
            "Sequence" : 66481802,
            "TakerGets" : "10000",
            "TakerPays" : {
               "currency" : "ADA",
               "issuer" : "rnNDjH8YTJLVQMccqLebzC77KLkWxrdEZi",
               "value" : "3"
            },
            "index" : "7CF342B72DC283536B4242BEB76C63C44574C552DED857C439881B792FE2C4FC",
            "owner_funds" : "17688155210",
            "quality" : "0.0003"
         }
      ],
      "status" : "success",
      "validated" : false
   }
}

intelliot · 2022-01-24T16:29:42Z

If relevant, please feel free to copy the above examples to #4081 (new issue where this problem is being worked on)

seelabs requested review from nbougalis and scottschurr April 20, 2021 01:58

seelabs assigned nbougalis and scottschurr Apr 20, 2021

donovanhide reviewed Apr 20, 2021

View reviewed changes

seelabs force-pushed the rm-bad-q-offer branch from 441776a to 4a0e8f6 Compare April 21, 2021 22:11

Rm some offers where the quality is reduced:

e1bf2c9

Substantial reductions in an offer's effective quality from its initial quality may clog offer books.

seelabs force-pushed the rm-bad-q-offer branch from 4a0e8f6 to e1bf2c9 Compare April 21, 2021 22:19

[fold] Add IOU/IOU payment test & some comments

8ad2fd5

scottschurr approved these changes Apr 27, 2021

View reviewed changes

seelabs added 2 commits April 27, 2021 16:02

[fold] Minor cleanups

9b8c050

[fold] Change test params so they're closer to behavior change thresh…

d77ad06

…olds

scottschurr approved these changes Apr 27, 2021

View reviewed changes

nbougalis reviewed May 6, 2021

View reviewed changes

Misc cleanups (Nik's review):

a06ca4c

* Inline minPositiveAmount * Add some clarifying comments * Fix mispelling * Use deleted function instead of static_assert

scottschurr approved these changes May 7, 2021

View reviewed changes

nbougalis approved these changes May 7, 2021

View reviewed changes

manojsdoshi mentioned this pull request May 7, 2021

Proposed 1.7.2 #3843

Merged

This was referenced Jun 2, 2021

Proposed 1.8.0-b3 #3857

Closed

Proposed 1.8.0-b3 #3858

Merged

manojsdoshi closed this in #3858 Jun 4, 2021


		TTakerPays const thresh = TTakerPays::minPositiveAmount();

		if (effectiveAmounts.in > thresh)

Rm some offers where the quality is reduced #3827

Rm some offers where the quality is reduced #3827

Conversation

seelabs commented Apr 20, 2021

High Level Overview of Change

Type of Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs commented Apr 21, 2021

donovanhide commented Apr 22, 2021

scottschurr commented Apr 22, 2021

seelabs commented Apr 23, 2021

donovanhide commented Apr 23, 2021 • edited Loading

seelabs commented Apr 23, 2021 • edited Loading

donovanhide commented Apr 23, 2021

yxxyun commented Apr 24, 2021

scottschurr commented Apr 27, 2021

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs commented Apr 27, 2021

scottschurr left a comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

donovanhide commented Dec 7, 2021 • edited Loading

donovanhide commented Dec 19, 2021 • edited Loading

intelliot commented Jan 24, 2022

donovanhide commented Apr 23, 2021 •

edited

Loading

seelabs commented Apr 23, 2021 •

edited

Loading

seelabs May 6, 2021 •

edited

Loading

donovanhide commented Dec 7, 2021 •

edited

Loading

donovanhide commented Dec 19, 2021 •

edited

Loading