`fixReducedOffersV1`: curtail the occurrence of order books that are blocked by reduced offers #4512

scottschurr · 2023-04-26T23:04:21Z

High Level Overview of Change

The goal of this amendment is to curtail the occurrence of order books that are blocked by reduced offers.

This commit identifies three ways in which offers can be reduced:

A new offer can be partially crossed by existing offers, so the new offer is reduced when placed in the ledger.
An in-ledger offer can be partially crossed by a new offer in a transaction. So the in-ledger offer is reduced by the new offer.
An in-ledger offer may be under-funded. In this case the in-ledger offer is scaled down to match the available funds.

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.

This commit adjusts some rounding modes so that the quality of a reduced offer is always at least as good (from the taker's perspective) as the original offer.

These rounding changes are on an amendment because they will affect transaction outcomes. The amendment is titled fixReducedOffersV1 because additional ways of producing reduced offers may come to light. So there may be a future need for a V2 amendment.

Context of Change

Occasional blocked order books have been a long-standing low-level irritant with the DEX. There has been at least one patch that helped to manage the problem. But the patch did not really make the problem go away. This pull request goes to the root of these blockages and just makes them go away.

First, it's important to recognize that both XRP and IOUs have finite representations in the ledger. For example, it is not possible to represent fractions of drops. IOUs also have a finite representation that becomes apparent with very small or very large numbers. So when a number cannot be precisely represented in XRP or by an IOU it must be rounded.

This pull request changes the rounding mode that is used when calculating a reduced offer. The rounding is changed so that the quality of the reduced offer is always the same or better than the quality of the original offer (viewing the offer's quality from the taker's perspective).

Note that this change affects the values that offers in the ledger contain. However the change in representation is at most either...

One drop of XRP or
1e-81 of an IOU.

As the values in offers get tiny, rounding effects can have an outsize influence on the quality of an offer. But the offers are only being adjusted by minute amounts of actual funds.

Type of Change

Breaking change (fix or feature that would cause existing functionality to not work as expected)
Tests (tests for the changes are included in this pull request)
Documentation Updates

I'm calling this a breaking change because outcomes on the DEX are affected. People who care deeply about the specifics of DEX corner cases would do well to understand the consequences of this pull request. The pull request also includes an amendment, which can cause amendment blockage.

I'm unsure whether the Doc team will want to include discussion of the details of this change. But they should be aware of the change and decide for themselves.

Interactions with `Number`

It turns out that this pull request interacts with the new Number integration with STAmount. There are three specific things to note:

STAmount::canonicalize rounding The rounding characteristics of STAmount::canonicalize() changed when Number was integrated. The new default canonicalize rounding interacted badly with the rounding that fixReducedOffersV1 requires. Fortunately, Number provides a way to remotely control its rounding mode. This pull request uses that capability.

Number Rounding Modes The rounding modes that Number provides do not align precisely with the rounding that STAmount operations use. STAmount uses a bool named roundUp. But, what that bool really means is roundAwayFromZero. So the rounding direction reverses for negative numbers. If the bool is true STAmount rounds away from zero, and false rounds toward zero.

Number provides a towards_zero rounding mode. So that's covered. But Number does not provide an away_from_zero rounding mode. So I faked this with Number's upward rounding mode. This should usually work. And it works in all of my tests because STAmount values are usually positive.

But it would be nice if Number provided an away_from_zero rounding mode. That would remove the compromise of faking it with upward.

Number::RoundModeGuard. As I was surgically controlling the Number rounding mode to get the behavior I wanted out of STAmount::canonicalize() I introduced a bug. I wanted a way to change the Number rounding mode only while I was in a scope and have that mode revert when I left scope. I thought that Number provided that precise tool with saveNumberRoundMode. But I was wrong. I did not read the code carefully enough and made assumptions about the behavior. So I misused saveNumberRoundMode and @HowardHinnant set me right.

@HowardHinnant ended up implementing NumberRoundModeGuard to give me exactly the behavior that I wanted. You can see that (almost trivial) implementation in STAmount.cpp of this pull request.

In general I personally think NumberRoundModeGuard is exactly the tool that folks should reach for when surgically adjusting the rounding mode. I'd like to suggest that it should be a permanent fixture in Number.

Number Request Summary Here are the changes I'm hoping for:

Add an away_from_zero rounding mode to Number.
Make Number::RoundModeGuard a part of the Number implementation similar to saveNumberRoundMode.

I'll be curious what @gregtatcam and @HowardHinnant think regarding these requests.

I'm requesting @gregtatcam and @HowardHinnant as reviewers since they have the most experience with Number. I'm hoping that @seelabs will also take a gander since these changes are integrated into the payment engine.

The goal of this amendment is to curtail the occurrence of order books that are blocked by reduced offers. This commit identifies three ways in which offers can be reduced: 1. A new offer can be partially crossed by existing offers, so the new offer is reduced when placed in the ledger. 2. An in-ledger offer can be partially crossed by a new offer in a transaction. So the in-ledger offer is reduced by the new offer. 3. An in-ledger offer may be under-funded. In this case the in-ledger offer is scaled down to match the available funds. 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. This commit adjusts some rounding modes so that the quality of a reduced offer is always at least as good (from the taker's perspective) as the original offer. The amendment is titled fixReducedOffersV1 because additional ways of producing reduced offers may come to light. So there may be a future need for a V2 amendment.

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

gregtatcam · 2023-05-04T18:02:51Z

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

@@ -577,6 +586,7 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
        sb, afView, book_, sb.parentCloseTime(), counter, j_);

    bool const flowCross = afView.rules().enabled(featureFlowCross);
+    bool const fixReduced = afView.rules().enabled(fixReducedOffersV1);
    bool offerAttempted = false;
    std::optional<Quality> ofrQ;
    while (offers.step())


This is a comment for line 590 else if (*ofrQ != offer.quality()) below (doesn't look like github allows commenting on collapsed lines). I'm not sure if this is a problem but judging from the PR comment

This commit adjusts some rounding modes so that the quality of a reduced offer is always at least as good (from the taker's perspective) as the original offer.

I assume it's possible that the reduced offer written back to the ledger has a better quality than the original offer, in which case this offer might block the order book since the check on line 590 would fail for this offer.

Good for you to point out.

The way I understand this code (and @seelabs may correct me on this), forEachOffer() is intended to run through all offers that have the same Quality in an order book. So we get the Quality of the offer at the tip of the order book -- that's how the optional is filled in. If any subsequent offers in the order book have a different Quality, then we return from forEachOffer().

But that doesn't mean we're done. It just means that we've done as much as we can with this Strand for now. If the Quality from this go-round is good enough, then the payment engine will try this stand again and get some more offers out of this path.

One more source of confusion is that the code works with two different Qualitys here. There is the Quality that is encoded in the offer book directory page -- that should be the Quality of the offer as initially placed (not necessarily reflecting the actual Quality of a reduced offer). I'm pretty sure that's the Quality being used in the place you pointed out. There are other places that deal in the actual Quality of an offer.

At any rate, the determination that a particular order book is blocked happens further up the call stack here: https://github.com/XRPLF/rippled/blob/develop/src/ripple/app/paths/impl/StrandFlow.h#L638-L644

Thanks. I missed that this better quality offer or the next one after it are going to be picked on the next payment engine iteration if there is more output left.

src/test/app/ReducedOffer_test.cpp

scottschurr

Thanks for the review, @gregtatcam. I believe the top-most commit addresses your comments.

scottschurr · 2023-05-05T00:04:31Z

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

@@ -577,6 +586,7 @@ BookStep<TIn, TOut, TDerived>::forEachOffer(
        sb, afView, book_, sb.parentCloseTime(), counter, j_);

    bool const flowCross = afView.rules().enabled(featureFlowCross);
+    bool const fixReduced = afView.rules().enabled(fixReducedOffersV1);
    bool offerAttempted = false;
    std::optional<Quality> ofrQ;
    while (offers.step())


Good for you to point out.

The way I understand this code (and @seelabs may correct me on this), forEachOffer() is intended to run through all offers that have the same Quality in an order book. So we get the Quality of the offer at the tip of the order book -- that's how the optional is filled in. If any subsequent offers in the order book have a different Quality, then we return from forEachOffer().

But that doesn't mean we're done. It just means that we've done as much as we can with this Strand for now. If the Quality from this go-round is good enough, then the payment engine will try this stand again and get some more offers out of this path.

One more source of confusion is that the code works with two different Qualitys here. There is the Quality that is encoded in the offer book directory page -- that should be the Quality of the offer as initially placed (not necessarily reflecting the actual Quality of a reduced offer). I'm pretty sure that's the Quality being used in the place you pointed out. There are other places that deal in the actual Quality of an offer.

At any rate, the determination that a particular order book is blocked happens further up the call stack here: https://github.com/XRPLF/rippled/blob/develop/src/ripple/app/paths/impl/StrandFlow.h#L638-L644

gregtatcam · 2023-05-05T17:50:23Z

There are compiler warnings on OSX M1 Ventura 13.3.1(a), clang 14.03:

src/ripple/beast/container/detail/aged_unordered_container.h:197:23: warning: 'binary_function<ripple::base_uint<256>, beast::detail::aged_unordered_container<false, true, ripple::base_uint<256>, ripple::HashRouter::Entry, std::chrono::steady_clock, ripple::hardened_hash<>, std::equal_to<ripple::base_uint<256>>, std::allocator<std::pair<const ripple::base_uint<256>, ripple::HashRouter::Entry>>>::element, bool>' is deprecated [-Wdeprecated-declarations]
          public std::binary_function<Key, element, bool>

I don't see these warnings on develop. I assume it just needs to be merged with the latest develop?

scottschurr

@gregtatcam, sorry that I wasn't paying close enough attention last time. The most recent commit adds the method that you suggested.

src/test/app/ReducedOffer_test.cpp

scottschurr · 2023-05-05T21:20:40Z

I just looked at the latest develop. I see this commit which I think fixes the compiler warning you are seeing: 8d482d3
This pull request hasn't picked up that particular commit yet.

I can rebase if you'd like, as long as it wouldn't mess up @HowardHinnant's review.

gregtatcam · 2023-05-05T21:57:14Z

I just looked at the latest develop. I see this commit which I think fixes the compiler warning you are seeing: 8d482d3 This pull request hasn't picked up that particular commit yet.

I can rebase if you'd like, as long as it wouldn't mess up @HowardHinnant's review.

I checked that the rebase fixes this warning. No need to rebase now.

gregtatcam · 2023-05-05T22:04:57Z

Should we consider including in this PR a fix for the related issue: https://ripplelabs.atlassian.net/browse/RXL-226? The fix is to pass the limitQuality to the limitStepOut() and if the quality of the adjusted offer is less than the limitQuality then re-calculate the offer with the roundUp set to false.

scottschurr · 2023-05-05T22:23:24Z

We could consider including that fix. My current inclination is to put that fix on a separate amendment. These three fixes are useful and have thorough unit tests. At the least we'd need to add unit tests for the new fix. With those tests I'd hope to learn whether we can set roundUp to false in that location unconditionally. There may be additional things we learn while wringing out that additional change.

Beyond that, I heard a report recently that the the tfFillOrKill flag on OfferCreate transactions is not working as expected. That may (or may not) be yet another problem with ledger rounding. That problem report could be investigated (and possibly fixed) in the separate amendment.

gregtatcam

👍 LGTM

HowardHinnant

Added one minor suggestion.

I'm fine with moving NumberRoundModeGuard to Number.h. And I think that is a good name for this functionality.

On adding Number::away_from_zero. I'm fine with doing this if there is agreement it would be useful. And it looks like a fairly minor addition in terms of effort. It wasn't included initially because it wasn't asked for and it isn't part of the C API. However C20 did add "round away from zero on tie", which is somewhat similar.

We should add it if it would simplify client code, and not if it would just add confusion.

src/ripple/protocol/impl/STAmount.cpp

scottschurr · 2023-05-31T21:23:17Z

The topmost commit addresses comments by @HowardHinnant.

In terms of adding a new Number::away_from_zero rounding mode, I'm requesting it in an effort to make the most recent change more correct. It's probably not critical because it will only affect a divRound call that has a negative result. I think those are very uncommon today.

This is the situation:

    STAmount result = [&]() {
        // If appropriate, tell Number the rounding mode we are using.
        MightSaveRound const savedRound(
            roundUp ? Number::upward : Number::towards_zero);
        return STAmount(issue, amount, offset, resultNegative);
    }();

The variable roundUp is a bool. Even though it's called roundUp, the implementation seems to treat it as...

true: round away from zero
false: round toward zero.

Here I'm using Number::upward as a stand-in. And it's the right choice as long as resultNegative == false. But if resultNegative == true I'll get the wrong behavior.

Since I'm looking at this harder, I happen to have enough information to synthesize the behavior that I want:

    STAmount result = [&]() {
        // If appropriate, tell Number the rounding mode we are using.
        // Note that "roundUp == true" actually means "round away from zero".
        // Otherwise round toward zero.
        MightSaveRound const savedRound(
            roundUp ^ resultNegative ? Number::upward : Number::downward);
        return STAmount(issue, amount, offset, resultNegative);
    }();

I guess I'm inclined to use this work around, since it's the only place in the code that would use the new Number::away_from_zero rounding mode, at least for now.

Thoughts?

scottschurr · 2023-06-01T18:30:22Z

The top-most commit fixes the rounding problem in divRound without requiring a new rounding mode in Number. @HowardHinnant, it would be great if you would review that commit and make sure it makes sense to you. Thanks.

HowardHinnant

Looks good to me.

scottschurr · 2023-06-02T20:26:44Z

@gregtatcam, do you want a last opportunity to look over this pull request before I mark it Passed? Thanks.

gregtatcam · 2023-06-02T20:51:36Z

@gregtatcam, do you want a last opportunity to look over this pull request before I mark it Passed? Thanks.

I'll review tonight.

gregtatcam · 2023-06-02T21:23:06Z

Thanks, looks good!

…F#4512) Curtail the occurrence of order books that are blocked by reduced offers with the implementation of the fixReducedOffersV1 amendment. This commit identifies three ways in which offers can be reduced: 1. A new offer can be partially crossed by existing offers, so the new offer is reduced when placed in the ledger. 2. An in-ledger offer can be partially crossed by a new offer in a transaction. So the in-ledger offer is reduced by the new offer. 3. An in-ledger offer may be under-funded. In this case the in-ledger offer is scaled down to match the available funds. 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. This commit adjusts some rounding modes so that the quality of a reduced offer is always at least as good (from the taker's perspective) as the original offer. The amendment is titled fixReducedOffersV1 because additional ways of producing reduced offers may come to light. Therefore, there may be a future need for a V2 amendment.

shortthefomo · 2023-07-18T02:32:18Z

ignore me missed that this was merged!

Curtail the occurrence of order books that are blocked by reduced offers with the implementation of the fixReducedOffersV1 amendment. This commit identifies three ways in which offers can be reduced: 1. A new offer can be partially crossed by existing offers, so the new offer is reduced when placed in the ledger. 2. An in-ledger offer can be partially crossed by a new offer in a transaction. So the in-ledger offer is reduced by the new offer. 3. An in-ledger offer may be under-funded. In this case the in-ledger offer is scaled down to match the available funds. 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. This commit adjusts some rounding modes so that the quality of a reduced offer is always at least as good (from the taker's perspective) as the original offer. The amendment is titled fixReducedOffersV1 because additional ways of producing reduced offers may come to light. Therefore, there may be a future need for a V2 amendment.

…F#4512) Curtail the occurrence of order books that are blocked by reduced offers with the implementation of the fixReducedOffersV1 amendment. This commit identifies three ways in which offers can be reduced: 1. A new offer can be partially crossed by existing offers, so the new offer is reduced when placed in the ledger. 2. An in-ledger offer can be partially crossed by a new offer in a transaction. So the in-ledger offer is reduced by the new offer. 3. An in-ledger offer may be under-funded. In this case the in-ledger offer is scaled down to match the available funds. 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. This commit adjusts some rounding modes so that the quality of a reduced offer is always at least as good (from the taker's perspective) as the original offer. The amendment is titled fixReducedOffersV1 because additional ways of producing reduced offers may come to light. Therefore, there may be a future need for a V2 amendment.

scottschurr added 3 commits April 20, 2023 12:22

[FOLD] Address review comments

a061338

[FOLD] Fixes to accommodate Number rounding changes to STAmount

17e11a6

scottschurr added the Amendment label Apr 26, 2023

scottschurr requested review from gregtatcam and HowardHinnant April 26, 2023 23:04

scottschurr assigned gregtatcam and HowardHinnant Apr 26, 2023

gregtatcam reviewed May 4, 2023

View reviewed changes

[FOLD] Address one round of review comments

49aaa49

scottschurr commented May 5, 2023

View reviewed changes

[FOLD] Add offerInLedger() method to ReducedOffer unit test

166d238

scottschurr commented May 5, 2023

View reviewed changes

src/test/app/ReducedOffer_test.cpp Show resolved Hide resolved

gregtatcam approved these changes May 6, 2023

View reviewed changes

intelliot mentioned this pull request May 9, 2023

fixReducedOffersV1: curtail the occurrence of order books that are blocked by reduced offers #4264

Closed

3 tasks

HowardHinnant approved these changes May 31, 2023

View reviewed changes

src/ripple/protocol/impl/STAmount.cpp Show resolved Hide resolved

[FOLD] Delete special copy members of Number rounding guards

1a645bc

[FOLD] Fix rounding in divRound without a new Number rounding mode

e68ddf8

HowardHinnant self-requested a review June 2, 2023 12:54

HowardHinnant approved these changes Jun 2, 2023

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 2, 2023

intelliot added this to the 1.12 milestone Jun 9, 2023

intelliot unassigned gregtatcam and HowardHinnant Jun 20, 2023

intelliot merged commit 724a301 into XRPLF:develop Jun 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fixReducedOffersV1`: curtail the occurrence of order books that are blocked by reduced offers #4512

`fixReducedOffersV1`: curtail the occurrence of order books that are blocked by reduced offers #4512

scottschurr commented Apr 26, 2023

gregtatcam May 4, 2023

scottschurr May 5, 2023

gregtatcam May 5, 2023

scottschurr left a comment

scottschurr May 5, 2023

gregtatcam commented May 5, 2023

scottschurr left a comment

scottschurr commented May 5, 2023

gregtatcam commented May 5, 2023

gregtatcam commented May 5, 2023

scottschurr commented May 5, 2023

gregtatcam left a comment

HowardHinnant left a comment

scottschurr commented May 31, 2023

scottschurr commented Jun 1, 2023

HowardHinnant left a comment

scottschurr commented Jun 2, 2023

gregtatcam commented Jun 2, 2023

gregtatcam commented Jun 2, 2023

shortthefomo commented Jul 18, 2023 •

edited

Loading

fixReducedOffersV1: curtail the occurrence of order books that are blocked by reduced offers #4512

fixReducedOffersV1: curtail the occurrence of order books that are blocked by reduced offers #4512

Conversation

scottschurr commented Apr 26, 2023

High Level Overview of Change

Context of Change

Type of Change

Interactions with Number

gregtatcam May 4, 2023

Choose a reason for hiding this comment

scottschurr May 5, 2023

Choose a reason for hiding this comment

gregtatcam May 5, 2023

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr May 5, 2023

Choose a reason for hiding this comment

gregtatcam commented May 5, 2023

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr commented May 5, 2023

gregtatcam commented May 5, 2023

gregtatcam commented May 5, 2023

scottschurr commented May 5, 2023

gregtatcam left a comment

Choose a reason for hiding this comment

HowardHinnant left a comment

Choose a reason for hiding this comment

scottschurr commented May 31, 2023

scottschurr commented Jun 1, 2023

HowardHinnant left a comment

Choose a reason for hiding this comment

scottschurr commented Jun 2, 2023

gregtatcam commented Jun 2, 2023

gregtatcam commented Jun 2, 2023

shortthefomo commented Jul 18, 2023 • edited Loading

`fixReducedOffersV1`: curtail the occurrence of order books that are blocked by reduced offers #4512

`fixReducedOffersV1`: curtail the occurrence of order books that are blocked by reduced offers #4512

Interactions with `Number`

shortthefomo commented Jul 18, 2023 •

edited

Loading