Amendment fixReducedOffersV2 fixes issue #4937

scottschurr commented 4 weeks ago

High Level Overview of Change

In issue https://github.com/XRPLF/rippled/issues/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

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

API Impact

None.

codecov[bot] commented 4 weeks ago

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

[![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/5032/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) ```diff @@ 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](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) | Coverage Δ | | |---|---|---| | [src/ripple/app/paths/AMMOffer.h](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fpaths%2FAMMOffer.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvcGF0aHMvQU1NT2ZmZXIuaA==) | `83.3% <ø> (ø)` | | | [src/ripple/app/paths/impl/AMMOffer.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fpaths%2Fimpl%2FAMMOffer.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvcGF0aHMvaW1wbC9BTU1PZmZlci5jcHA=) | `83.1% <100.0%> (+1.2%)` | :arrow_up: | | [src/ripple/app/paths/impl/BookStep.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fpaths%2Fimpl%2FBookStep.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvcGF0aHMvaW1wbC9Cb29rU3RlcC5jcHA=) | `93.1% <100.0%> (-0.1%)` | :arrow_down: | | [src/ripple/app/tx/impl/Offer.h](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Ftx%2Fimpl%2FOffer.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvdHgvaW1wbC9PZmZlci5o) | `97.6% <100.0%> (+0.1%)` | :arrow_up: | | [src/ripple/protocol/Feature.h](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2FFeature.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9GZWF0dXJlLmg=) | `100.0% <ø> (ø)` | | | [src/ripple/protocol/impl/Feature.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2Fimpl%2FFeature.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9pbXBsL0ZlYXR1cmUuY3Bw) | `93.9% <ø> (ø)` | | | [src/ripple/protocol/impl/Quality.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2Fimpl%2FQuality.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9pbXBsL1F1YWxpdHkuY3Bw) | `91.8% <100.0%> (+0.6%)` | :arrow_up: | | [src/ripple/protocol/Quality.h](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2FQuality.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9RdWFsaXR5Lmg=) | `98.5% <94.1%> (+2.7%)` | :arrow_up: | ... and [7 files with indirect coverage changes](https://app.codecov.io/gh/XRPLF/rippled/pull/5032/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/5032/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/5032?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)

scottschurr commented 2 weeks ago

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.

XRPLF / rippled