Amendment fixReducedOffersV2 fixes issue #4937 #5032

scottschurr · 2024-05-31T19:06:48Z

High Level Overview of Change

In issue #4937 @shortthefomo reported additional instances of order book blocking since the fixReducedOffersV1 amendment went live. Thanks to that report I was able to identify an additional case where blocking offers could be generated. This pull request fixes the newly identified case.

Context of Change

Having order books occasionally blocked by offers that were modified so that their quality was worse than the originally placed quality has been a problem for a while. Three different ways that this could occur were identified and addressed by amendment fixReducedOffersV1. All three of those case were addressed by carefully looking at and fixing the numeric rounding applied during calculations.

This new amendment, fixReducedOffersV2, identifies and fixes an additional rounding case that was missed during the research for fixReducedOffersV1 .

Beyond the fix itself, there are two kinds of unit test additions in this pull request.

There's a new test that directly exercises the failing case both with and without the amendment. It demonstrates that the old rounding behavior could lead to blocked order books. It also shows that with the new rounding behavior the order books are no longer blocked for this range of cases.
A few pre-existing test cases failed due to the changed rounding. Those test cases were examined and the tests patched to handle the new rounding behavior.

Type of Change

Bug fix (non-breaking change which fixes an issue)
Tests (you added tests for code that already exists, or your new feature included in this PR)

API Impact

None.

codecov · 2024-05-31T19:30:17Z

Codecov Report

Attention: Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.3%. Comparing base (263e984) to head (e5dbc1d).

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5032   +/-   ##
=======================================
  Coverage     71.3%   71.3%           
=======================================
  Files          796     796           
  Lines        66883   66887    +4     
  Branches     10889   10871   -18     
=======================================
+ Hits         47658   47682   +24     
+ Misses       19225   19205   -20

Files	Coverage Δ
src/ripple/app/paths/AMMOffer.h	`83.3% <ø> (ø)`
src/ripple/app/paths/impl/AMMOffer.cpp	`83.1% <100.0%> (+1.2%)`	⬆️
src/ripple/app/paths/impl/BookStep.cpp	`93.1% <100.0%> (-0.1%)`	⬇️
src/ripple/app/tx/impl/Offer.h	`97.6% <100.0%> (+0.1%)`	⬆️
src/ripple/protocol/Feature.h	`100.0% <ø> (ø)`
src/ripple/protocol/impl/Feature.cpp	`93.9% <ø> (ø)`
src/ripple/protocol/impl/Quality.cpp	`91.8% <100.0%> (+0.6%)`	⬆️
src/ripple/protocol/Quality.h	`98.5% <94.1%> (+2.7%)`	⬆️

... and 7 files with indirect coverage changes

gregtatcam · 2024-06-03T19:34:48Z

src/test/app/Flow_test.cpp


        testLineQuality(features);
        testFalseDry(features);
+        testDirectStep(features - reducedOffersV2);


Why need to run this test with reducedOffersV2 enabled? There is no change to testDirectStep with/without the amendment.

I agree, it's not needed. I was just following a pre-existing pattern where testDirectStep and testBookStep were having their amendments adjusted as a pair. I'd be fine with removing this test iteration if you'd like.

I think it should be removed if it doesn't accomplish anything. It just increases the tests running time without any benefit.

gregtatcam · 2024-06-03T23:01:35Z

src/test/app/Flow_test.cpp

+            {
+                env.require(balance(carol, EUR(1)));
+                env.require(balance(bob, USD(0.4)));
+                env.require(balance(bob, EUR(999)));


Does it make sense to extend this test to show that the order book is actually blocked without the amendment?

gregtatcam · 2024-06-03T23:07:04Z

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

 {
    if (limit < stpAmt.in)
    {
        stpAmt.in = limit;
        auto const inLmt =
            mulRatio(stpAmt.in, QUALITY_ONE, transferRateIn, /*roundUp*/ false);
-        ofrAmt = offer.limitIn(ofrAmt, inLmt);
+        // It turns out we can prevent order book blocking by (strictly)
+        // rounding down the ceil_in() result.  This adjustment changes


Would it be beneficial to add a comment here and in limitStepOut explaining why the choice of the rounding in each case prevents order book blocking?

Sure. Maybe something like...

// It turns out we can prevent order book blocking by (strictly) // rounding down the ceil_in() result. By rounding down we // guarantee that the quality of an offer left in the ledger is // as good or better than the quality of the containing order // book page.

gregtatcam · 2024-06-04T11:49:05Z

src/test/app/ReducedOffer_test.cpp

+            }
+            else
+            {
+                BEAST_EXPECT(blockedCount > 80);


Shouldn't we check for the exact count of the blocking rates since the same tests are executed on every run? If this count changes in the next release then doesn't it indicate some change that needs to be investigated?

I could. But the other, similar, tests in this file also just check for a threshold in the failing case. When I was initially developing the similar tests I found that small changes to the test would result in these numbers bouncing a bit. So just checking a threshold made development easier. The goal here is to verify that there was indeed a problem and that this test exercises the problem. Even one blocking offer would still be a problem, but we're verifying that there's at least a certain number.

The check where the number really matters in the one on line 782. We really want that value to be zero. That's how we have confidence that the problem is fully addressed, at least for this case.

So, yeah, I could put in an exact number. But my preference is not to. You okay with that?

Yeah, it's fine.

gregtatcam

👍 Looks good. I left a few minor comments for you to consider.

scottschurr

Thanks for the review @gregtatcam. I've addressed a couple of your comments. There were also two that I didn't do anything with, but I left notes about why I they are the way they are. I'm open to continuing the conversation on those if you'd like.

scottschurr · 2024-06-04T00:56:00Z

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

 {
    if (limit < stpAmt.in)
    {
        stpAmt.in = limit;
        auto const inLmt =
            mulRatio(stpAmt.in, QUALITY_ONE, transferRateIn, /*roundUp*/ false);
-        ofrAmt = offer.limitIn(ofrAmt, inLmt);
+        // It turns out we can prevent order book blocking by (strictly)
+        // rounding down the ceil_in() result.  This adjustment changes


Sure. Maybe something like...

// It turns out we can prevent order book blocking by (strictly) // rounding down the ceil_in() result. By rounding down we // guarantee that the quality of an offer left in the ledger is // as good or better than the quality of the containing order // book page.

scottschurr · 2024-06-04T18:36:48Z

src/test/app/Flow_test.cpp


        testLineQuality(features);
        testFalseDry(features);
+        testDirectStep(features - reducedOffersV2);


I agree, it's not needed. I was just following a pre-existing pattern where testDirectStep and testBookStep were having their amendments adjusted as a pair. I'd be fine with removing this test iteration if you'd like.

scottschurr · 2024-06-04T21:17:54Z

src/test/app/ReducedOffer_test.cpp

+            }
+            else
+            {
+                BEAST_EXPECT(blockedCount > 80);


I could. But the other, similar, tests in this file also just check for a threshold in the failing case. When I was initially developing the similar tests I found that small changes to the test would result in these numbers bouncing a bit. So just checking a threshold made development easier. The goal here is to verify that there was indeed a problem and that this test exercises the problem. Even one blocking offer would still be a problem, but we're verifying that there's at least a certain number.

The check where the number really matters in the one on line 782. We really want that value to be zero. That's how we have confidence that the problem is fully addressed, at least for this case.

So, yeah, I could put in an exact number. But my preference is not to. You okay with that?

gregtatcam

👍

seelabs

I think this is ready to go, but I left a couple of nits for you to consider. I'll approve it after you take a look. I'm fine with however you decide to resolve the nits (including keeping them as they are).

seelabs · 2024-06-11T21:07:04Z

src/ripple/app/paths/AMMOffer.h

@@ -103,15 +104,19 @@ class AMMOffer
    limitOut(
        TAmounts<TIn, TOut> const& offrAmt,
        TOut const& limit,
-        bool fixReducedOffers,
+        Rules const& rules,


Consider using the globally available rules rather than passing this as a parameter. (also fine as-is, but I think I'd like to get away from passing rules around as much as we do and just using the new global).

seelabs · 2024-06-11T21:07:28Z

src/ripple/app/paths/AMMOffer.h

+    limitIn(
+        TAmounts<TIn, TOut> const& offrAmt,
+        TIn const& limit,
+        Rules const& rules,


See comment above about passing rules as a param.

seelabs · 2024-06-11T21:08:17Z

src/ripple/protocol/Quality.h

@@ -200,6 +200,29 @@ class Quality
            toAmount<In>(stRes.in), toAmount<Out>(stRes.out));
    }

+    Amounts
+    ceil_in_strict(Amounts const& amount, STAmount const& limit, bool roundUp)


Could use a comment for how "strict" differs from the non-strict function.

seelabs · 2024-06-11T21:09:57Z

src/ripple/protocol/Quality.h

+
+    template <class In, class Out>
+    TAmounts<In, Out>
+    ceil_in_strict(


We usually don't define functions inline in the class. I know this is a member template and that gets a little tedious outside of the class. I'd prefer it was defined out of the class, but it's a minor nit.

seelabs · 2024-06-11T21:14:30Z

src/ripple/protocol/Quality.h

+        TAmounts<In, Out> const& amount,
+        In const& limit,
+        bool roundUp) const
+    {


This looks like a copy of ceil_in with one function called changed.

If ceil_in is being deprecated and will be replaced with ceil_in_strict then I actually prefer the code duplication. And I think this is the case. If this is not the case, then let's look at merging the two.

On the same topic, if the ceil_in is being deprecated, consider renaming ceil_in to ceil_in_deprecated or somesuch and renaming ceil_in_strict to ceil_in.

I'm not sure about ceil_in being deprecated. But it certainly has an odd behavior regarding rounding. So we might eventually want to replace all uses of it with ceil_in_strict. But I'd like to hold off on renaming either of them until we know that we're actually going to make that change (which would have to go under an amendment). There are a few things in this code base that have been "deprecated" for quite a few years without ever being removed. I'd like to not add to that confusion.

seelabs · 2024-06-11T21:16:38Z

src/ripple/protocol/impl/Quality.cpp

-Amounts
-Quality::ceil_in(Amounts const& amount, STAmount const& limit) const
+template <STAmount (
+    *DivRoundFunc)(STAmount const&, STAmount const&, Issue const&, bool)>


I usually like passing the callback as a parameter rather than as a template. But I'm also fine with this as-is.

seelabs

👍

scottschurr · 2024-06-13T16:07:28Z

Proposed commit message:

fixReducedOffersV2: prevent offers from blocking order books:

Fixes issue #4937.

The fixReducedOffersV1 amendment fixed certain forms of offer
modification that could lead to blocked order books.  Reduced
offers can block order books if the effective quality of the
reduced offer is worse than the quality of the original offer
(from the perspective of the taker). It turns out that, for
small values, the quality of the reduced offer can be
significantly affected by the rounding mode used during
scaling computations.

Issue #4937 identified an additional code path that modified
offers in a way that could lead to blocked order books.  This
commit changes the rounding in that newly located code path so
the quality of the modified offer is never worse than the
quality of the offer as it was originally placed.

It is possible that additional ways of producing blocking
offers will come to light.  Therefore there may be a future
need for a V3 amendment.

* upstream/develop: (32 commits) fixInnerObjTemplate2 amendment (XRPLF#5047) Set version to 2.3.0-b1 Ignore restructuring commits (XRPLF#4997) Recompute loops (XRPLF#4997) Rewrite includes (XRPLF#4997) Rearrange sources (XRPLF#4997) Move CMake directory (XRPLF#4997) Add bin/physical.sh (XRPLF#4997) Prepare to rearrange sources: (XRPLF#4997) Change order of checks in amm_info: (XRPLF#4924) Add the fixEnforceNFTokenTrustline amendment: (XRPLF#4946) Replaces the usage of boost::string_view with std::string_view (XRPLF#4509) docs: explain how to find a clang-format patch generated by CI (XRPLF#4521) XLS-52d: NFTokenMintOffer (XRPLF#4845) chore: remove repeat words (XRPLF#5041) Expose all amendments known by libxrpl (XRPLF#5026) fixReducedOffersV2: prevent offers from blocking order books: (XRPLF#5032) Additional unit tests for testing deletion of trust lines (XRPLF#4886) Fix conan typo: (XRPLF#5044) Add new command line option to make replaying transactions easier: (XRPLF#5027) ...

…5032) Fixes issue XRPLF#4937. The fixReducedOffersV1 amendment fixed certain forms of offer modification that could lead to blocked order books. Reduced offers can block order books if the effective quality of the reduced offer is worse than the quality of the original offer (from the perspective of the taker). It turns out that, for small values, the quality of the reduced offer can be significantly affected by the rounding mode used during scaling computations. Issue XRPLF#4937 identified an additional code path that modified offers in a way that could lead to blocked order books. This commit changes the rounding in that newly located code path so the quality of the modified offer is never worse than the quality of the offer as it was originally placed. It is possible that additional ways of producing blocking offers will come to light. Therefore there may be a future need for a V3 amendment.

Amendment fixReducedOffersV2 fixes issue XRPLF#4937

9ddf547

scottschurr added Bug Amendment labels May 31, 2024

scottschurr requested review from gregtatcam and seelabs May 31, 2024 19:06

scottschurr assigned gregtatcam and seelabs May 31, 2024

gregtatcam reviewed Jun 3, 2024

View reviewed changes

gregtatcam reviewed Jun 4, 2024

View reviewed changes

gregtatcam approved these changes Jun 4, 2024

View reviewed changes

[FOLD] Address review comments

186eda8

scottschurr commented Jun 4, 2024

View reviewed changes

[FOLD] Remove unneeded test

af7849c

gregtatcam approved these changes Jun 5, 2024

View reviewed changes

seelabs reviewed Jun 11, 2024

View reviewed changes

scottschurr added 2 commits June 11, 2024 18:03

[FOLD] Prefer getCurrentTransactionRules() over passing Rules

aecda42

[FOLD] Address comments on the Quality implementation

52a04e6

seelabs approved these changes Jun 13, 2024

View reviewed changes

scottschurr added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 13, 2024

Merge branch 'develop' into blocked-book

e5dbc1d

seelabs merged commit ae7ea33 into XRPLF:develop Jun 13, 2024
19 checks passed

seelabs mentioned this pull request Jun 20, 2024

Proposed 2.3.0-b1 #5050

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amendment fixReducedOffersV2 fixes issue #4937 #5032

Amendment fixReducedOffersV2 fixes issue #4937 #5032

scottschurr commented May 31, 2024

codecov bot commented May 31, 2024 •

edited

Loading

gregtatcam Jun 3, 2024

scottschurr Jun 4, 2024

gregtatcam Jun 4, 2024

gregtatcam Jun 3, 2024

gregtatcam Jun 3, 2024

scottschurr Jun 4, 2024

gregtatcam Jun 4, 2024

scottschurr Jun 4, 2024

gregtatcam Jun 4, 2024

gregtatcam left a comment

scottschurr left a comment

scottschurr Jun 4, 2024

scottschurr Jun 4, 2024

scottschurr Jun 4, 2024

gregtatcam left a comment

seelabs left a comment

seelabs Jun 11, 2024

seelabs Jun 11, 2024

seelabs Jun 11, 2024

seelabs Jun 11, 2024

seelabs Jun 11, 2024

scottschurr Jun 13, 2024

seelabs Jun 11, 2024

seelabs left a comment

scottschurr commented Jun 13, 2024 •

edited

Loading

Amendment fixReducedOffersV2 fixes issue #4937 #5032

Amendment fixReducedOffersV2 fixes issue #4937 #5032

Conversation

scottschurr commented May 31, 2024

High Level Overview of Change

Context of Change

Type of Change

API Impact

codecov bot commented May 31, 2024 • edited Loading

Codecov Report

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

gregtatcam left a comment

Choose a reason for hiding this comment

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

gregtatcam left a comment

Choose a reason for hiding this comment

seelabs 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 left a comment

Choose a reason for hiding this comment

scottschurr commented Jun 13, 2024 • edited Loading

codecov bot commented May 31, 2024 •

edited

Loading

scottschurr commented Jun 13, 2024 •

edited

Loading