From f1290bfa4b04b0703d3a0fcd000336623a43eb76 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Wed, 8 Jul 2026 09:27:22 -0400 Subject: [PATCH] Update the coverage --- src/libxrpl/protocol/QualityFunction.cpp | 11 ++++- src/libxrpl/protocol/STAmount.cpp | 13 +++++- src/libxrpl/tx/paths/BookStep.cpp | 14 +++++- src/libxrpl/tx/paths/OfferStream.cpp | 6 ++- .../tx/transactors/dex/AMMWithdraw.cpp | 19 +++++++- src/test/app/OfferMPT_test.cpp | 44 +++++++++++++++++++ src/test/protocol/STAmount_test.cpp | 15 +++++++ 7 files changed, 113 insertions(+), 9 deletions(-) diff --git a/src/libxrpl/protocol/QualityFunction.cpp b/src/libxrpl/protocol/QualityFunction.cpp index 3c800be497..ffe583b7e1 100644 --- a/src/libxrpl/protocol/QualityFunction.cpp +++ b/src/libxrpl/protocol/QualityFunction.cpp @@ -38,14 +38,21 @@ QualityFunction::outFromAvgQ(Quality const& quality) return std::nullopt; return out; } - return std::nullopt; + // The sole caller (StrandFlow::limitOut) only invokes this on a non-const + // quality function, so m_ != 0 here, and a real payment/offer never yields + // a zero-rate limit quality (it would divide by zero above). This fallback + // is therefore unreachable in practice. + return std::nullopt; // LCOV_EXCL_LINE } bool QualityFunction::satisfiesAvgQ(Quality const& quality, Number const& out) const { + // satisfiesAvgQ is only reached from StrandFlow::limitOut *after* + // outFromAvgQ returned a value, which requires a non-zero rate. So a + // zero-rate quality never reaches here; this guard is defensive. if (quality.rate() == beast::kZero) - return false; + return false; // LCOV_EXCL_LINE return m_ * out + b_ >= 1 / quality.rate(); } diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 0d222b66a6..136a152479 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -1474,11 +1474,20 @@ roundNumberResult( if (roundUp && !resultNegative && !result) { - // Preserve existing mulRound/divRound behavior for a positive result - // too small to represent in the target asset. + // Intended to preserve existing mulRound/divRound behavior for a + // positive result too small to represent in the target asset. + // + // Unreachable in practice: when roundUp is set, roundMode() above + // selects Upward, and materializing a Number into an STAmount honors + // that mode (Number::operator rep()), so any positive value rounds up + // to at least the smallest representable unit. Hence, a positive result + // is never !result here; the only zero case is a zero operand, which + // the mulRound/divRound callers handle before reaching this function. + // LCOV_EXCL_START if (asset.integral()) return STAmount{asset, 1}; return STAmount{asset, STAmount::kMinValue, STAmount::kMinOffset, false}; + // LCOV_EXCL_STOP } return result; diff --git a/src/libxrpl/tx/paths/BookStep.cpp b/src/libxrpl/tx/paths/BookStep.cpp index c53ec723f5..20b9e5e187 100644 --- a/src/libxrpl/tx/paths/BookStep.cpp +++ b/src/libxrpl/tx/paths/BookStep.cpp @@ -849,7 +849,11 @@ BookStep::forEachOffer( removeOffer("Removing offer with overflowing amount calculation"); return true; } - throw; + // An overflow can only be produced by a crafted MPT offer, and MPT + // offers require featureMPTokensV2 (enforced at OfferCreate + // preflight). So the amendment is always enabled when we get here + // and this legacy re-throw is unreachable in practice. + throw; // LCOV_EXCL_LINE } }; @@ -922,7 +926,13 @@ BookStep::consumeOffer( // deleted by BookTip::step(). if (auto const err = checkCreateMPT(sb, book_.in.get(), offer.owner(), j_); !isTesSuccess(err)) - Throw(err); + { + // checkCreateMPT only fails on tecDIR_FULL (its source line is + // itself LCOV-excluded) or a missing offer-owner account, which + // cannot happen since that account owns the offer being + // consumed. Defensive and unreachable in practice. + Throw(err); // LCOV_EXCL_LINE + } } auto const dr = offer.send( diff --git a/src/libxrpl/tx/paths/OfferStream.cpp b/src/libxrpl/tx/paths/OfferStream.cpp index 426aa113df..445a5ba984 100644 --- a/src/libxrpl/tx/paths/OfferStream.cpp +++ b/src/libxrpl/tx/paths/OfferStream.cpp @@ -330,7 +330,11 @@ TOfferStreamBase::step() offer_ = TOffer{}; continue; } - throw; + // The strict reduction only overflows for a crafted MPT offer, and + // MPT offers require featureMPTokensV2 (enforced at OfferCreate + // preflight). So the amendment is always enabled here and this + // legacy re-throw is unreachable in practice. + throw; // LCOV_EXCL_LINE } if (shouldRemoveSmallIncreasedQOffer) diff --git a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp index 44972aa4fb..1ee6b12de2 100644 --- a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp @@ -687,7 +687,18 @@ AMMWithdraw::withdraw( !isTesSuccess(err)) { if (authHandling != AuthHandling::IgnoreAuth || err != tecNO_AUTH) - return err; + { + // Unreachable in practice. Normal withdraws (authHandling + // != IgnoreAuth) are rejected for unauthorized holders in + // preclaim, so they never get here. Under clawback + // (IgnoreAuth) requireAuth returns a non-tecNO_AUTH error + // (e.g. tecEXPIRED) only for a domain-authorized MPT, but no + // such MPT can be in an AMM pool: a directly domain-gated + // RequireAuth MPT fails AMMCreate/deposit with tecNO_AUTH, + // and vault shares (whose recursive auth could yield + // tecEXPIRED) are rejected by AMMCreate with tecWRONG_ASSET. + return err; // LCOV_EXCL_LINE + } // AMMClawback ignores authorization so the issuer can recover // MPT locked in the pool even if the holder deleted their @@ -708,7 +719,11 @@ AMMWithdraw::withdraw( if (auto const err = checkCreateMPT(view, mptIssue, account, createFlags, journal); !isTesSuccess(err)) { - return err; + // checkCreateMPT only fails on tecDIR_FULL (its source line is + // itself LCOV-excluded) or a missing account, which cannot + // happen since `account` is the withdrawing LP. Defensive and + // unreachable in practice. + return err; // LCOV_EXCL_LINE } } return tesSUCCESS; diff --git a/src/test/app/OfferMPT_test.cpp b/src/test/app/OfferMPT_test.cpp index 225843d967..326659e21c 100644 --- a/src/test/app/OfferMPT_test.cpp +++ b/src/test/app/OfferMPT_test.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -6,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -23,6 +25,7 @@ #include #include +#include #include #include #include @@ -47,6 +50,7 @@ #include #include #include +#include #include #include #include @@ -3656,6 +3660,46 @@ public: BEAST_EXPECT(env.balance(poisonMaker, token) == token(funded)); BEAST_EXPECT(env.balance(taker, token) == token(0)); } + + { + // Same overflow scenario as the ownerGives case above, but run with + // trace-level logging so BookStep::forEachOffer's removeOffer() + // emits its "Removing offer with overflowing amount calculation" + // trace line. This exercises the JLOG body inside removeOffer, + // which is skipped when logging is above trace severity. + std::string logs; + { + Env env{ + *this, + envconfig(), + features, + std::make_unique(&logs), + beast::Severity::Trace}; + env.fund(XRP(10'000), issuer, taker); + env.close(); + + MPTTester const token{ + {.env = env, .issuer = issuer, .holders = {taker}, .transferFee = 10'000}}; + + std::int64_t const poisonAmount = 8'500'000'000'000'000'000LL; + auto const poisonSeq = env.seq(issuer); + env(offer(issuer, XRP(1), token(poisonAmount))); + env.close(); + + auto const poisonKeylet = keylet::offer(issuer.id(), poisonSeq); + BEAST_EXPECT(env.le(poisonKeylet) != nullptr); + + auto const takerSeq = env.seq(taker); + env(offer(taker, token(100), XRP(100))); + env.close(); + + BEAST_EXPECT(env.le(poisonKeylet) == nullptr); + BEAST_EXPECT(env.le(keylet::offer(taker.id(), takerSeq)) != nullptr); + } + BEAST_EXPECT( + logs.find("Removing offer with overflowing amount calculation") != + std::string::npos); + } } void diff --git a/src/test/protocol/STAmount_test.cpp b/src/test/protocol/STAmount_test.cpp index ee3a7bbd02..893b1bdb8e 100644 --- a/src/test/protocol/STAmount_test.cpp +++ b/src/test/protocol/STAmount_test.cpp @@ -1102,6 +1102,21 @@ public: BEAST_EXPECT(multiplyRound(largeAmount, transferRate, asset, true) == scaledAmount); BEAST_EXPECT(divideRound(scaledAmount, transferRate, asset, true) == largeAmount); } + + { + // mulRound with an integral (XRP) operand whose mantissa is below + // kMinValue exercises the legacy value-scaling loop that normalizes + // the mantissa before multiply. The MPTokensV2 Number path is + // not taken here because the target asset is an IOU. + Issue const usd{Currency(0x5553440000000000), AccountID(0x4985601)}; + STAmount const iouVal{usd, 5}; + STAmount const xrpVal{XRPAmount{7}}; // integral, mantissa < kMinValue + + auto const up = mulRound(iouVal, xrpVal, usd, /*roundUp*/ true); + auto const down = mulRound(iouVal, xrpVal, usd, /*roundUp*/ false); + BEAST_EXPECT(down.signum() > 0); + BEAST_EXPECT(up >= down); + } } void