From 48b1716e6f5a2bcb80f40d9bddebaadb45ccb2f0 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sat, 23 May 2026 18:45:36 +0100 Subject: [PATCH 01/15] Make Number::operator/= significantly more accurate - Prevents extreme dust rounding from getting lost, especially when rounding away from zero. (Upward for positive, downward for negative.) --- src/libxrpl/basics/Number.cpp | 55 +++++++++---- src/test/basics/Number_test.cpp | 137 ++++++++++++++++++++++++++------ 2 files changed, 149 insertions(+), 43 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 11f5934b04..0a22d439ea 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -858,31 +858,51 @@ Number::operator/=(Number const& y) auto const& maxMantissa = range.max; auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; + // Cache power of 10 computations to not waste time on subsequent calls + auto getPower10 = [](int exponent) { + static std::unordered_map powersOf10; + if (!powersOf10.contains(exponent)) + { + uint128_t result = 1; + for (int i = 0; i < exponent; ++i) + result *= 10; + powersOf10[exponent] = result; + } + return powersOf10.at(exponent); + }; + // Shift by 10^17 gives greatest precision while not overflowing - // uint128_t or the cast back to int64_t - // TODO: Can/should this be made bigger for largeRange? - // log(2^128,10) ~ 38.5 - // largeRange.log = 18, fits in 10^19 - // f can be up to 10^(38-19) = 10^19 safely + // uint128_t or the cast back to int64_t for small mantissas. + // + // For large mantissas: + // * log(2^128,10) ~ 38.5 + // * largeRange.log = 18, fits in 10^19 + // * The expanded numerator must fit in 10^38 + // * f can be up to 10^(38-19) = 10^19 safely bool const small = Number::getMantissaScale() == MantissaRange::MantissaScale::Small; - uint128_t const f = small ? 100'000'000'000'000'000 : 10'000'000'000'000'000'000ULL; + auto const factorExponent = small ? 17 : 19; + + uint128_t const f = getPower10(factorExponent); XRPL_ASSERT_PARTS(f >= minMantissa * 10, "Number::operator/=", "factor expected size"); // unsigned denominator auto const dmu = static_cast(dm); - // correctionFactor can be anything between 10 and f, depending on how much - // extra precision we want to only use for rounding with the - // largeRange. Three digits seems like plenty, and is more than - // the smallRange uses. - uint128_t const correctionFactor = 1'000; - auto const numerator = uint128_t(nm) * f; auto zm = numerator / dmu; - auto ze = ne - de - (small ? 17 : 19); + auto ze = ne - de - factorExponent; bool zn = (ns * ds) < 0; if (!small) { + // zm may now hold up to 20 digits. The correctionFactor must be small enough to not cause + // overflow, but as large as possible otherwise + // * zm fits in 10^21 + // * correctionFactor can be up to 10^(38-21) = 10^17 safely + bool const useBigCorrection = + cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled; + auto const correctionExponent = useBigCorrection ? 17 : 3; + uint128_t const correctionFactor = getPower10(correctionExponent); + // Virtually multiply numerator by correctionFactor. Since that would // overflow in the existing uint128_t, we'll do that part separately. // The math for this would work for small mantissas, but we need to @@ -906,11 +926,12 @@ Number::operator/=(Number const& y) if (remainder != 0) { zm *= correctionFactor; - auto const correction = remainder * correctionFactor / dmu; - zm += correction; - // divide by 1000 by moving the exponent, so we don't lose the + // divide by the correctionFactor by moving the exponent, so we don't lose the // integer value we just computed - ze -= 3; + ze -= correctionExponent; + + auto const correction = (remainder * correctionFactor) / dmu; + zm += correction; } } normalize(zn, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled); diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 52af19ed36..9a80caa8e4 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -6,10 +6,12 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -39,6 +41,30 @@ class Number_test : public beast::unit_test::Suite return out; } + using dec = boost::multiprecision::cpp_dec_float_50; + + template + static T + pow10(int n) + { + T p = 1; + if (n >= 0) + for (int i = 0; i < n; ++i) + p *= 10; + else + for (int i = 0; i < -n; ++i) + p /= 10; + return p; + } + + static std::string + fmt(dec const& v) + { + std::ostringstream os; + os << std::setprecision(40) << v; + return os.str(); + } + public: void testZero() @@ -1588,40 +1614,99 @@ public: void testUpwardRoundsDown() { - testcase << "upward rounding produces a value below exact at kMaxRep cusp"; + { + testcase << "upward rounding produces a value below exact at kMaxRep cusp"; - NumberMantissaScaleGuard const mg{MantissaRange::MantissaScale::Large}; - NumberRoundModeGuard const rg{Number::RoundingMode::Upward}; + NumberMantissaScaleGuard const mg{MantissaRange::MantissaScale::Large}; + NumberRoundModeGuard const rg{Number::RoundingMode::Upward}; - constexpr std::int64_t kAValue = 1'000'000'000'000'049'863LL; - constexpr std::int64_t kBValue = 9'223'372'036'854'315'903LL; + constexpr std::int64_t kAValue = 1'000'000'000'000'049'863LL; + constexpr std::int64_t kBValue = 9'223'372'036'854'315'903LL; - // Public conversion operator: STAmount::operator Number() const. - Number const a = kAValue; - Number const b = kBValue; - Number const product = a * b; + // Public conversion operator: STAmount::operator Number() const. + Number const a = kAValue; + Number const b = kBValue; + Number const product = a * b; - // Exact reference in BigInt. - BigInt const exactProduct = BigInt(kAValue) * BigInt(kBValue); + // Exact reference in BigInt. + BigInt const exactProduct = BigInt(kAValue) * BigInt(kBValue); - // What Number actually stored. - BigInt storedValue = BigInt(product.mantissa()); - for (int i = 0; i < product.exponent(); ++i) - storedValue *= 10; + // What Number actually stored. + BigInt storedValue = BigInt(product.mantissa()); + for (int i = 0; i < product.exponent(); ++i) + storedValue *= 10; - BigInt const signedDifference = storedValue - exactProduct; + BigInt const signedDifference = storedValue - exactProduct; - log << "\n" - << " a = " << fmt(BigInt(kAValue)) << "\n" - << " b = " << fmt(BigInt(kBValue)) << "\n" - << " exact a*b = " << fmt(exactProduct) << "\n" - << " stored = " << fmt(storedValue) << "\n" - << " stored - exact = " << fmt(signedDifference) << "\n" - << " upward = " << (signedDifference >= 0 ? "held" : "VIOLATED") << "\n"; + log << "\n" + << " a = " << fmt(BigInt(kAValue)) << "\n" + << " b = " << fmt(BigInt(kBValue)) << "\n" + << " exact a*b = " << fmt(exactProduct) << "\n" + << " stored = " << fmt(storedValue) << "\n" + << " stored - exact = " << fmt(signedDifference) << "\n" + << " upward = " << (signedDifference >= 0 ? "held" : "VIOLATED") << "\n"; - BEAST_EXPECT(signedDifference >= 0); - BEAST_EXPECT(product.mantissa() == (std::numeric_limits::max() / 10) + 1); - BEAST_EXPECT(product.exponent() == 19); + BEAST_EXPECT(signedDifference >= 0); + BEAST_EXPECT(signedDifference < pow10(product.exponent())); + BEAST_EXPECT(product.mantissa() == (std::numeric_limits::max() / 10) + 1); + BEAST_EXPECT(product.exponent() == 19); + log.flush(); + } + + { + /* Companion to NumberUpwardWrongDirection_test (which targets + * `operator*=` Upward at the kMaxRep cusp on LargeLegacy), but for + * `operator/=` on the cusp-fix-ENABLED `Large` scale. + * + * Under `Large` (`CuspRoundingFix::Enabled`), `operator/=` with Upward + * rounding can return a value STRICTLY LESS than the exact quotient, + * violating Upward's directional invariant. + * + * Mechanism (fix-enabled path): + * 1. `operator/=` computes `numerator = nm * 10^19` and + * `zm = numerator / dm` (integer division, truncates remainder). + * 2. If `remainder != 0`, the correction block runs: + * zm *= 1000 + * correction = (remainder * 1000) / dm // also truncates + * zm += correction + * ze -= 3 + * The truncation in `correction` discards a sub-1/1000 residual. + * 3. `normalize`'s shift loop reduces zm to fit, but the discarded + * residual is BELOW the Guard's visibility, so the Guard sees fraction = 0. + * 4. Under Upward + positive, `round()` returns -1 (no round-up), and + * the algorithm returns the truncated zm + */ + testcase << "operator/= Upward on Large returns value < truth "; + + NumberMantissaScaleGuard const scaleGuard{MantissaRange::MantissaScale::Large}; + NumberRoundModeGuard const roundGuard{Number::RoundingMode::Upward}; + + constexpr std::int64_t aValue = 2LL; + constexpr std::int64_t bValue = 1'000'000'000'000'000'007LL; + // bValue = 10^18 + 7 (prime, in [minMantissa, kMaxRep]). + + Number const a{aValue, 0}; + Number const b{bValue, 0}; + Number const quotient = a / b; + + dec const exact = dec(aValue) / dec(bValue); + dec const stored = dec(quotient.mantissa()) * pow10(quotient.exponent()); + dec const diff = stored - exact; + + log << "\n" + << " a = " << aValue << "\n" + << " b = " << bValue << "\n" + << " exact a/b = " << fmt(exact) << "\n" + << " stored a/b = " << fmt(stored) << "\n" + << " stored - exact = " << fmt(diff) + << " (negative => Upward gave value BELOW truth)\n" + << " quotient.mantissa = " << quotient.mantissa() << "\n" + << " quotient.exponent = " << quotient.exponent() << "\n"; + + // Upward invariant: stored >= exact. Bug: stored < exact. + BEAST_EXPECT(stored >= exact); + BEAST_EXPECT(diff < pow10(quotient.exponent())); + } } void From 27456fa439ef283bfa2f2304640f725a804f69c3 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 26 May 2026 15:50:31 -0400 Subject: [PATCH 02/15] Use the local range instead of calling a function --- src/libxrpl/basics/Number.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 0a22d439ea..cd89e163d3 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -879,7 +879,7 @@ Number::operator/=(Number const& y) // * largeRange.log = 18, fits in 10^19 // * The expanded numerator must fit in 10^38 // * f can be up to 10^(38-19) = 10^19 safely - bool const small = Number::getMantissaScale() == MantissaRange::MantissaScale::Small; + bool const small = range.scale == MantissaRange::MantissaScale::Small; auto const factorExponent = small ? 17 : 19; uint128_t const f = getPower10(factorExponent); From 5abecb9fcb8f91aff466b58b95af09d2c0aa7742 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 27 May 2026 00:07:48 -0400 Subject: [PATCH 03/15] Significant rewrite - Simplify Number::operator/= to use more constexpr values, and fewer variations. - Most significantly, rounding up doesn't need more precision, it only needs to know if there's a remainder after the current precision work is done. Tracked similarly Guard::xbit_. - Build a constexpr lookup array for powers of 10. Only a handful of values are used, but since it's built at compile time, and constexpr, unused values do not affect memory or performance. --- include/xrpl/basics/Number.h | 87 +++++++++++++--- src/libxrpl/basics/Number.cpp | 169 ++++++++++++++++++++++---------- src/test/basics/Number_test.cpp | 4 + 3 files changed, 194 insertions(+), 66 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 3aeb4b5bcd..37899a1cf4 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -40,6 +40,47 @@ isPowerOfTen(T value) return logTen(value).has_value(); } +namespace detail { + +/** Builds a table of the powers of 10 + * + * This function is marked consteval, so it can only be run in + * a constexpr context. This assures that it is and can only be run at + * compile time. Doing it at runtime would be pretty wasteful and + * inefficient. + */ +constexpr std::size_t int64digits = 20; +consteval std::array +buildPowersOfTen() +{ + std::array result; + + std::uint64_t power = 1; + std::size_t exponent = 0; + // end the loop early so it doesn't overflow; + for (; exponent < result.size() - 1; ++exponent, power *= 10) + { + result[exponent] = power; + if (power > std::numeric_limits::max() / 10) + throw std::logic_error("Power of 10 table is too big"); + } + result[exponent] = power; + if (power < std::numeric_limits::max() / 10) + throw std::logic_error("Power of 10 table is not big enough for the uint64_t type"); + + return result; +} + +} // namespace detail + +constexpr std::array kPowerOfTen = detail::buildPowersOfTen(); + +static_assert(kPowerOfTen[0] == 1); +static_assert(kPowerOfTen[1] == 10); +static_assert(kPowerOfTen[10] == 10'000'000'000); +static_assert( + isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::int64digits - 1); + /** MantissaRange defines a range for the mantissa of a normalized Number. * * The mantissa is in the range [min, max], where @@ -76,6 +117,7 @@ isPowerOfTen(T value) struct MantissaRange final { using rep = std::uint64_t; + enum class MantissaScale { Small, // LargeLegacy can be removed when fixCleanup3_2_0 is retired @@ -89,19 +131,15 @@ struct MantissaRange final Enabled = true, }; - explicit constexpr MantissaRange(MantissaScale scale) - : min(getMin(scale)) - , cuspRoundingFixEnabled(isCuspFixEnabled(scale)) - , log(logTen(min).value_or(-1)) - , scale(scale) + explicit constexpr MantissaRange(MantissaScale sc) : scale(sc) { } - rep min; - rep max{(min * 10) - 1}; - CuspRoundingFix cuspRoundingFixEnabled; - int log; - MantissaScale scale; + MantissaScale const scale; + int const log{getExponent(scale)}; + rep const min{getMin(scale, log)}; + rep const max{(min * 10) - 1}; + CuspRoundingFix const cuspRoundingFixEnabled{isCuspFixEnabled(scale)}; static MantissaRange const& getMantissaRange(MantissaScale scale); @@ -110,16 +148,34 @@ struct MantissaRange final getAllScales(); private: - static constexpr rep - getMin(MantissaScale scale) + static constexpr int + getExponent(MantissaScale scale) { switch (scale) { case MantissaScale::Small: - return 1'000'000'000'000'000ULL; + return 15; case MantissaScale::LargeLegacy: case MantissaScale::Large: - return 1'000'000'000'000'000'000ULL; + return 18; + default: + // If called in a constexpr context, this throw assures that the build fails if an + // invalid scale is used. + throw std::runtime_error("Unknown mantissa scale"); // LCOV_EXCL_LINE + } + } + + // Keep this function for future use with different ways to compute + // the ranges. + static constexpr rep + getMin(MantissaScale scale, int exponent) + { + switch (scale) + { + case MantissaScale::Small: + case MantissaScale::LargeLegacy: + case MantissaScale::Large: + return kPowerOfTen[exponent]; default: // If called in a constexpr context, this throw assures that the build fails if an // invalid scale is used. @@ -519,7 +575,8 @@ private: int& exponent, MantissaRange::rep const& minMantissa, MantissaRange::rep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled); + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, + bool dropped); [[nodiscard]] bool isnormal() const noexcept; diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index cd89e163d3..cd2d213e29 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -178,6 +178,10 @@ public: setPositive() noexcept; void setNegative() noexcept; + // Should only be called by doNormalize, and then only for division + // operations with remainders. + void + setDropped() noexcept; [[nodiscard]] bool isNegative() const noexcept; @@ -250,6 +254,12 @@ Number::Guard::setNegative() noexcept sbit_ = 1; } +inline void +Number::Guard::setDropped() noexcept +{ + xbit_ = 1; +} + inline bool Number::Guard::isNegative() const noexcept { @@ -512,8 +522,6 @@ Number::one() return Number{false, range.min, -range.log, Number::Unchecked{}}; } -// Use the member names in this static function for now so the diff is cleaner -// TODO: Rename the function parameters to get rid of the "_" suffix template void doNormalize( @@ -522,7 +530,8 @@ doNormalize( int& exponent, MantissaRange::rep const& minMantissa, MantissaRange::rep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, + bool dropped) { static constexpr auto kMinExponent = Number::kMinExponent; static constexpr auto kMaxExponent = Number::kMaxExponent; @@ -547,6 +556,8 @@ doNormalize( Guard g; if (negative) g.setNegative(); + if (dropped) + g.setDropped(); while (m > maxMantissa) { if (exponent >= kMaxExponent) @@ -611,7 +622,8 @@ Number::normalize( internalrep const& maxMantissa, MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled); + doNormalize( + negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); } template <> @@ -624,7 +636,8 @@ Number::normalize( internalrep const& maxMantissa, MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled); + doNormalize( + negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); } template <> @@ -637,7 +650,8 @@ Number::normalize( internalrep const& maxMantissa, MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled); + doNormalize( + negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); } void @@ -858,32 +872,62 @@ Number::operator/=(Number const& y) auto const& maxMantissa = range.max; auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; - // Cache power of 10 computations to not waste time on subsequent calls - auto getPower10 = [](int exponent) { - static std::unordered_map powersOf10; - if (!powersOf10.contains(exponent)) - { - uint128_t result = 1; - for (int i = 0; i < exponent; ++i) - result *= 10; - powersOf10[exponent] = result; - } - return powersOf10.at(exponent); - }; - - // Shift by 10^17 gives greatest precision while not overflowing - // uint128_t or the cast back to int64_t for small mantissas. + // Division operates on two large integers (16-digit for small + // mantissas, 19-digit for large) using integer math. If the values + // were just divided directly, the result would be at most 9. + // Introduce a power-of-ten multiplication factor for the numerator + // which will ensure the result has a meaningful number of digits. + // + // Consider numbers with a 2-digit mantissa: + // * Assume both numbers have an exponent of 0, using "ToNearest" rounding + // * 23 / 67 = 0 + // * Use a factor of 10^4 + // * 230'000 / 67 = 3432 with an exponent of -4 + // * The normalized result will be 34, exponent -2 + // + // The most extreme results are 10/99 and 99/10 + // * 100'000 / 99 = 1'010e-4 = 10e-2 + // * 990'000 / 10 = 99'000e-4 = 99e-1 + // + // Note that the computations give 2 or 3 digits after the + // decimal point to determine which way to round for most scenarios. + // + // For small mantissas (where the MantissaRange.log == 15), shifting by 10^17 gives sufficient + // precision while not overflowing uint128_t or the cast back to int64_t. (This is legacy + // behavior, which must not be changed.) + // + // For large mantissas (where the MantissaRange.log == 18), a shift by 10^20 would be optimal + // for most scenarios. However, larger mantissa values would overflow 2^128 (log(2^128,10) + // ~ 38.5). // - // For large mantissas: // * log(2^128,10) ~ 38.5 // * largeRange.log = 18, fits in 10^19 // * The expanded numerator must fit in 10^38 - // * f can be up to 10^(38-19) = 10^19 safely - bool const small = range.scale == MantissaRange::MantissaScale::Small; - auto const factorExponent = small ? 17 : 19; + // * f not be more than 10^(38-19) = 10^19 safely + // + // So, we do the division into stages: + // + // Stage 1: Use the same factor of 10^17, for the initial division. This + // will frequently not result in a whole number quotient. + // + // Stage 2: If there is a remainder from the first step, repeat the + // process with a "correction" factor of 10^5. Shift the + // result of Stage 1 over by 5 places, and add the second result to it. + // This is equivalent to if we had used an initial factor of 10^22, + // a couple digits more than we actually need. + // + // Stage 3: If there is still a remainder, and the CuspRoundingFix + // is enabled, pass a flag indicating such to doNormalize. The Guard + // in doNormalize will treat that flag as if non-zero digits had + // been dropped from the mantissa when shrinking it into range. + // This is only relevant when rounding away from zero (Upward for + // positive numbers, Downward for negative), or if the "regular" + // remainder is exactly 0.5 for "ToNearest". This will give the + // rounding the most accurate result possible, as if infinite + // precision was used in the initial calculation. + auto constexpr factorExponent = 17; - uint128_t const f = getPower10(factorExponent); - XRPL_ASSERT_PARTS(f >= minMantissa * 10, "Number::operator/=", "factor expected size"); + uint128_t constexpr f = kPowerOfTen[factorExponent]; // unsigned denominator auto const dmu = static_cast(dm); @@ -892,21 +936,21 @@ Number::operator/=(Number const& y) auto zm = numerator / dmu; auto ze = ne - de - factorExponent; bool zn = (ns * ds) < 0; - if (!small) - { - // zm may now hold up to 20 digits. The correctionFactor must be small enough to not cause - // overflow, but as large as possible otherwise - // * zm fits in 10^21 - // * correctionFactor can be up to 10^(38-21) = 10^17 safely - bool const useBigCorrection = - cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled; - auto const correctionExponent = useBigCorrection ? 17 : 3; - uint128_t const correctionFactor = getPower10(correctionExponent); + // dropped is used in the same way as Guard::xbit_. In the case of + // division, it indicates if there's any remainder left over after + // we have been as precise as reasonable. If there is, it would be as + // if we were using infinite precision math, and a non-zero digit + // had been shifted off the end of the result when normalizing. + bool dropped = false; - // Virtually multiply numerator by correctionFactor. Since that would - // overflow in the existing uint128_t, we'll do that part separately. + if (range.scale != MantissaRange::MantissaScale::Small) + { + // Stage 2 + // + // If there is a remainder, treat is as a secondary numerator. + // Multiply by correctionFactor separately from stage 1. // The math for this would work for small mantissas, but we need to - // preserve existing behavior. + // preserve legacy behavior. // // Consider: // ((numerator * correctionFactor) / dmu) / correctionFactor @@ -917,24 +961,47 @@ Number::operator/=(Number const& y) // // = ((numerator / dmu * correctionFactor) // + ((numerator % dmu) * correctionFactor) / dmu) / correctionFactor + // = ((zm * correctionFactor) + // + (remainder * correctionFactor) / dmu) / correctionFactor // - // We have already set `mantissa_ = numerator / dmu`. Now we - // compute `remainder = numerator % dmu`, and if it is - // nonzero, we do the rest of the arithmetic. If it's zero, we can skip - // it. + // The trick is that multiplication by correctionFactor is done on the mantissa, but + // division by correctionFactor is done by modifying the exponent, so no precision is lost + // until we normalize. + // + // If remainder is zero, we can skip this stage entirely because + // the first stage gave an exact answer. + auto constexpr correctionExponent = 5; + uint128_t constexpr correctionFactor = kPowerOfTen[correctionExponent]; + static_assert(factorExponent + correctionExponent == 22); + auto const remainder = (numerator % dmu); if (remainder != 0) { - zm *= correctionFactor; - // divide by the correctionFactor by moving the exponent, so we don't lose the - // integer value we just computed - ze -= correctionExponent; + auto const partialNumerator = remainder * correctionFactor; + auto const correction = partialNumerator / dmu; - auto const correction = (remainder * correctionFactor) / dmu; - zm += correction; + if (correction != 0) + { + zm *= correctionFactor; + // divide by the correctionFactor by moving the exponent, so we don't lose the + // integer value we just computed + ze -= correctionExponent; + + zm += correction; + } + + // Stage 3: If there's still anything left, and the cusp + // rounding fix is enabled, flag if there is still + // a remainder from stage 2. + bool const useTrailingRemainder = + cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled; + if (useTrailingRemainder) + { + dropped = partialNumerator % dmu != 0; + } } } - normalize(zn, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled); + doNormalize(zn, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled, dropped); negative_ = zn; mantissa_ = static_cast(zm); exponent_ = ze; diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index a50038363d..ddf3e35ce3 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -50,11 +50,15 @@ class Number_test : public beast::unit_test::Suite { T p = 1; if (n >= 0) + { for (int i = 0; i < n; ++i) p *= 10; + } else + { for (int i = 0; i < -n; ++i) p /= 10; + } return p; } From ae9c72bb7c7ec9e59c3502ae42f3700cfb06ef08 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 27 May 2026 00:36:38 -0400 Subject: [PATCH 04/15] Test optimization: Number_test::pow10 --- src/test/basics/Number_test.cpp | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index ddf3e35ce3..230752d37c 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -48,17 +48,23 @@ class Number_test : public beast::unit_test::Suite static T pow10(int n) { + if (n == 0) + return 1; + if (n == 1) + return 10; + + if (n > 1) + { + auto r = pow10(n / 2); + r *= r; + if (n % 2 != 0) + r *= 10; + return r; + } + + // n < 0 T p = 1; - if (n >= 0) - { - for (int i = 0; i < n; ++i) - p *= 10; - } - else - { - for (int i = 0; i < -n; ++i) - p /= 10; - } + p /= pow10(-n); return p; } From 4ec049e727b5f5d19afd6334decb7ceb5a6df265 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 27 May 2026 12:09:13 -0400 Subject: [PATCH 05/15] Minor fixes: missing include, variable init, typo --- include/xrpl/basics/Number.h | 3 ++- src/libxrpl/basics/Number.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 37899a1cf4..2dfe3a408f 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -2,6 +2,7 @@ #include +#include #include #include #include @@ -53,7 +54,7 @@ constexpr std::size_t int64digits = 20; consteval std::array buildPowersOfTen() { - std::array result; + std::array result{}; std::uint64_t power = 1; std::size_t exponent = 0; diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index cd2d213e29..4dba1e8474 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -408,7 +408,7 @@ Number::Guard::doRoundUp( // _don't_ increment the mantissa. Instead, divide and round recursively. It should // be impossible to recurse more than once, because once the mantissa is divided by // 10, it will be _well_ under maxMantissa and kMaxRep, so adding 1 will have no - // change of bringing it back over. + // chance of bringing it back over. doDropDigit(mantissa, exponent); XRPL_ASSERT_PARTS( safeToIncrement(mantissa), From 8dcd88e83cf63b7e74db6af79685fa3ab130027f Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 27 May 2026 12:34:49 -0400 Subject: [PATCH 06/15] Tweak how the denominator is handled in division - Removes one int64 to 128 conversion --- src/libxrpl/basics/Number.cpp | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 4dba1e8474..28ae3ad12b 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -864,8 +864,10 @@ Number::operator/=(Number const& y) bool const dp = y.negative_; int const ds = (dp ? -1 : 1); - auto dm = y.mantissa_; - auto de = y.exponent_; + // Create the denominator as 128-bit unsigned, since that's what we + // need to work with. + uint128_t const dm = static_cast(y.mantissa_); + auto const de = y.exponent_; auto const& range = kRange.get(); auto const& minMantissa = range.min; @@ -929,11 +931,9 @@ Number::operator/=(Number const& y) uint128_t constexpr f = kPowerOfTen[factorExponent]; - // unsigned denominator - auto const dmu = static_cast(dm); auto const numerator = uint128_t(nm) * f; - auto zm = numerator / dmu; + auto zm = numerator / dm; auto ze = ne - de - factorExponent; bool zn = (ns * ds) < 0; // dropped is used in the same way as Guard::xbit_. In the case of @@ -953,16 +953,16 @@ Number::operator/=(Number const& y) // preserve legacy behavior. // // Consider: - // ((numerator * correctionFactor) / dmu) / correctionFactor - // = ((numerator / dmu) * correctionFactor) / correctionFactor) + // ((numerator * correctionFactor) / dm) / correctionFactor + // = ((numerator / dm) * correctionFactor) / correctionFactor) // // But that assumes infinite precision. With integer math, this is // equivalent to // - // = ((numerator / dmu * correctionFactor) - // + ((numerator % dmu) * correctionFactor) / dmu) / correctionFactor + // = ((numerator / dm * correctionFactor) + // + ((numerator % dm) * correctionFactor) / dm) / correctionFactor // = ((zm * correctionFactor) - // + (remainder * correctionFactor) / dmu) / correctionFactor + // + (remainder * correctionFactor) / dm) / correctionFactor // // The trick is that multiplication by correctionFactor is done on the mantissa, but // division by correctionFactor is done by modifying the exponent, so no precision is lost @@ -974,11 +974,11 @@ Number::operator/=(Number const& y) uint128_t constexpr correctionFactor = kPowerOfTen[correctionExponent]; static_assert(factorExponent + correctionExponent == 22); - auto const remainder = (numerator % dmu); + auto const remainder = (numerator % dm); if (remainder != 0) { auto const partialNumerator = remainder * correctionFactor; - auto const correction = partialNumerator / dmu; + auto const correction = partialNumerator / dm; if (correction != 0) { @@ -997,7 +997,7 @@ Number::operator/=(Number const& y) cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled; if (useTrailingRemainder) { - dropped = partialNumerator % dmu != 0; + dropped = partialNumerator % dm != 0; } } } From de2efa5cb9f1c445d9ed345ef11fa7b59bf0aec1 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 27 May 2026 13:06:52 -0400 Subject: [PATCH 07/15] Remove TMax entirely from normalizeToRange; check type matching directly --- include/xrpl/basics/Number.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 2dfe3a408f..f7ef23e605 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -534,8 +534,7 @@ public: template < auto MinMantissa, auto MaxMantissa, - Integral64 T = std::decay_t, - Integral64 TMax = std::decay_t> + Integral64 T = std::decay_t> [[nodiscard]] std::pair normalizeToRange() const; @@ -783,16 +782,18 @@ Number::isnormal() const noexcept kMinExponent <= exponent_ && exponent_ <= kMaxExponent); } -template +template std::pair Number::normalizeToRange() const { static_assert(std::is_same_v || std::is_same_v); - static_assert(std::is_same_v); + static_assert(std::is_same_v>); + static_assert(std::is_same_v>); auto constexpr kMIN = static_cast(MinMantissa); auto constexpr kMAX = static_cast(MaxMantissa); static_assert(kMIN > 0); static_assert(kMIN % 10 == 0); + static_assert(isPowerOfTen(kMIN)); static_assert(kMAX % 10 == 9); static_assert((kMAX + 1) / 10 == kMIN); From f9551ac5ca08bb851b2a17d2f5259de13c105876 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Wed, 27 May 2026 20:24:18 +0100 Subject: [PATCH 08/15] style: Run shfmt on workflows, actions and markdown bash code (#7333) --- .github/actions/build-deps/action.yml | 18 +- .github/actions/generate-version/action.yml | 10 +- .github/scripts/format-inline-bash.py | 403 ++++++++++++++++++ .github/workflows/build-nix-image.yml | 4 +- .github/workflows/check-pr-description.yml | 10 +- .github/workflows/check-pr-title.yml | 2 +- .github/workflows/on-pr.yml | 8 +- .github/workflows/pre-commit.yml | 2 +- .../workflows/reusable-build-docker-image.yml | 2 +- .../workflows/reusable-build-test-config.yml | 76 ++-- .../workflows/reusable-check-levelization.yml | 10 +- .github/workflows/reusable-check-rename.yml | 10 +- .github/workflows/reusable-clang-tidy.yml | 48 +-- .github/workflows/reusable-package.yml | 2 +- .../workflows/reusable-strategy-matrix.yml | 2 +- .pre-commit-config.yaml | 13 + BUILD.md | 6 +- cspell.config.yaml | 1 + include/xrpl/protocol_autogen/README.md | 4 +- package/README.md | 18 +- 20 files changed, 533 insertions(+), 116 deletions(-) create mode 100755 .github/scripts/format-inline-bash.py diff --git a/.github/actions/build-deps/action.yml b/.github/actions/build-deps/action.yml index 9d52be1998..0891d56dfa 100644 --- a/.github/actions/build-deps/action.yml +++ b/.github/actions/build-deps/action.yml @@ -37,12 +37,12 @@ runs: run: | echo 'Installing dependencies.' conan install \ - --profile ci \ - --build="${BUILD_OPTION}" \ - --options:host='&:tests=True' \ - --options:host='&:xrpld=True' \ - --settings:all build_type="${BUILD_TYPE}" \ - --conf:all tools.build:jobs=${BUILD_NPROC} \ - --conf:all tools.build:verbosity="${LOG_VERBOSITY}" \ - --conf:all tools.compilation:verbosity="${LOG_VERBOSITY}" \ - . + --profile ci \ + --build="${BUILD_OPTION}" \ + --options:host='&:tests=True' \ + --options:host='&:xrpld=True' \ + --settings:all build_type="${BUILD_TYPE}" \ + --conf:all tools.build:jobs=${BUILD_NPROC} \ + --conf:all tools.build:verbosity="${LOG_VERBOSITY}" \ + --conf:all tools.compilation:verbosity="${LOG_VERBOSITY}" \ + . diff --git a/.github/actions/generate-version/action.yml b/.github/actions/generate-version/action.yml index 8edb7920c6..50b3166596 100644 --- a/.github/actions/generate-version/action.yml +++ b/.github/actions/generate-version/action.yml @@ -15,7 +15,7 @@ runs: shell: bash env: VERSION: ${{ github.ref_name }} - run: echo "VERSION=${VERSION}" >> "${GITHUB_ENV}" + run: echo "VERSION=${VERSION}" >>"${GITHUB_ENV}" # When a tag is not pushed, then the version (e.g. 1.2.3-b0) is extracted # from the BuildInfo.cpp file and the shortened commit hash appended to it. @@ -28,17 +28,17 @@ runs: echo 'Extracting version from BuildInfo.cpp.' VERSION="$(cat src/libxrpl/protocol/BuildInfo.cpp | grep "versionString =" | awk -F '"' '{print $2}')" if [[ -z "${VERSION}" ]]; then - echo 'Unable to extract version from BuildInfo.cpp.' - exit 1 + echo 'Unable to extract version from BuildInfo.cpp.' + exit 1 fi echo 'Appending shortened commit hash to version.' SHA='${{ github.sha }}' VERSION="${VERSION}+${SHA:0:7}" - echo "VERSION=${VERSION}" >> "${GITHUB_ENV}" + echo "VERSION=${VERSION}" >>"${GITHUB_ENV}" - name: Output version id: version shell: bash - run: echo "version=${VERSION}" >> "${GITHUB_OUTPUT}" + run: echo "version=${VERSION}" >>"${GITHUB_OUTPUT}" diff --git a/.github/scripts/format-inline-bash.py b/.github/scripts/format-inline-bash.py new file mode 100755 index 0000000000..423c78109c --- /dev/null +++ b/.github/scripts/format-inline-bash.py @@ -0,0 +1,403 @@ +#!/usr/bin/env python3 + +""" +Format embedded shell snippets using the shfmt hook configured in +.pre-commit-config.yaml. + +Two shapes are recognised: + +* YAML workflow/action files: literal block-scalar runs (`run: |`) and + single-line runs (`run: some command`). A single-line run is upgraded to + a `run: |` block scalar if shfmt's output spans multiple lines. + +* Markdown files: ``` ```bash ``` fenced code blocks. + +Any block that shfmt cannot parse is skipped with a warning on stderr, so +the file is left untouched and surrounding blocks still get formatted. + +For each occurrence the body is dedented, written to a temp .sh file, +formatted via `pre-commit run shfmt --files ` (falling back to +`prek`), then re-indented and written back in place. + +When invoked without arguments, every .yml/.yaml under .github/ plus every +.md file in the repo is scanned. When invoked with file arguments (the +pre-commit case), only those files are processed. +""" + +from __future__ import annotations + +import re +import shutil +import subprocess +import sys +import tempfile +from dataclasses import dataclass +from pathlib import Path +from typing import Union + +REPO = Path(__file__).resolve().parents[2] + +_HOOK_RUNNER = next((cmd for cmd in ("pre-commit", "prek") if shutil.which(cmd)), None) +if _HOOK_RUNNER is None: + sys.exit("error: neither `pre-commit` nor `prek` found on PATH") + +RUN_BLOCK_RE = re.compile(r"^(?P[ \t]*(?:- )?)run:[ \t]*\|[+-]?[ \t]*$") +RUN_INLINE_RE = re.compile( + r"^(?P[ \t]*(?:- )?)run:[ \t]+" r"(?P(?!\|[+-]?[ \t]*$)\S.*?)[ \t]*$" +) +MD_BASH_OPEN_RE = re.compile(r"^(?P[ ]{0,3})`{3}bash[ \t]*$") +MD_FENCE_CLOSE_RE = re.compile(r"^[ ]{0,3}`{3,}[ \t]*$") + + +@dataclass(frozen=True) +class BlockRun: + """A `run: |` block scalar; `body_start:body_end` slices into `lines`.""" + + body_start: int + body_end: int + body_indent: int + + +@dataclass(frozen=True) +class InlineRun: + """A single-line `run: value` at `line_idx`.""" + + line_idx: int + prefix: str + value: str + + +@dataclass(frozen=True) +class MdBashBlock: + """A markdown ``` ```bash ``` fenced code block. + + `body_start:body_end` slices into the file's lines; `open_line_idx` + points at the opening fence line. + """ + + open_line_idx: int + body_start: int + body_end: int + body_indent: int + + +RunItem = Union[BlockRun, InlineRun] + + +def _scan_block_body( + lines: list[str], body_start: int, run_col: int +) -> tuple[int | None, int]: + """Locate the body of a `run: |` block scalar starting at `body_start`. + + Returns `(body_indent, scan_end)`. `scan_end` is the line index where the + outer scanner should resume. `body_indent` is `None` when no body is + present (the scalar is empty, or the next non-blank line has indent + `<= run_col`). + """ + body_indent: int | None = None + scan_end = len(lines) + for idx in range(body_start, len(lines)): + line = lines[idx] + if line.strip() == "": + continue + indent = len(line) - len(line.lstrip(" ")) + if body_indent is None: + if indent > run_col: + body_indent = indent + else: + scan_end = idx + break + elif indent < body_indent: + scan_end = idx + break + if body_indent is not None: + while scan_end > body_start and lines[scan_end - 1].strip() == "": + scan_end -= 1 + if scan_end <= body_start: + body_indent = None + return body_indent, scan_end + + +def find_run_blocks(lines: list[str]) -> list[RunItem]: + """Return run items in document order.""" + items: list[RunItem] = [] + line_idx = 0 + while line_idx < len(lines): + line = lines[line_idx] + if block_match := RUN_BLOCK_RE.match(line): + run_col = len(block_match.group("prefix")) + body_start = line_idx + 1 + body_indent, scan_end = _scan_block_body(lines, body_start, run_col) + if body_indent is not None: + items.append( + BlockRun( + body_start=body_start, + body_end=scan_end, + body_indent=body_indent, + ) + ) + line_idx = scan_end + continue + if inline_match := RUN_INLINE_RE.match(line): + items.append( + InlineRun( + line_idx=line_idx, + prefix=inline_match.group("prefix"), + value=inline_match.group("value"), + ) + ) + line_idx += 1 + return items + + +def find_md_bash_blocks(lines: list[str]) -> list[MdBashBlock]: + """Return ``` ```bash ``` fenced code blocks in document order.""" + blocks: list[MdBashBlock] = [] + line_idx = 0 + while line_idx < len(lines): + open_match = MD_BASH_OPEN_RE.match(lines[line_idx]) + if not open_match: + line_idx += 1 + continue + body_start = line_idx + 1 + close_idx = next( + ( + j + for j in range(body_start, len(lines)) + if MD_FENCE_CLOSE_RE.match(lines[j]) + ), + None, + ) + if close_idx is None: + line_idx = body_start + continue + body = lines[body_start:close_idx] + non_blank = [b for b in body if b.strip()] + body_indent = ( + min(len(b) - len(b.lstrip(" ")) for b in non_blank) + if non_blank + else len(open_match.group("indent")) + ) + blocks.append( + MdBashBlock( + open_line_idx=line_idx, + body_start=body_start, + body_end=close_idx, + body_indent=body_indent, + ) + ) + line_idx = close_idx + 1 + return blocks + + +def dedent(lines: list[str], n: int) -> list[str]: + pad = " " * n + return [ + ( + "" + if line.strip() == "" + else (line[n:] if line.startswith(pad) else line.lstrip(" ")) + ) + for line in lines + ] + + +def reindent(lines: list[str], n: int) -> list[str]: + pad = " " * n + return [pad + line if line else "" for line in lines] + + +_SHFMT_ERR_RE = re.compile(r"\.sh:\d+:\d+:\s") +_GHA_EXPR_RE = re.compile(r"\$\{\{.*?\}\}", re.DOTALL) +_GHA_PLACEHOLDER_RE = re.compile(r"__GHA_EXPR_(\d+)__") + + +def _encode_gha_exprs(text: str) -> tuple[str, list[str]]: + """Replace `${{ ... }}` expressions with bash-safe placeholder identifiers.""" + exprs: list[str] = [] + + def repl(match: re.Match[str]) -> str: + exprs.append(match.group(0)) + return f"__GHA_EXPR_{len(exprs) - 1}__" + + return _GHA_EXPR_RE.sub(repl, text), exprs + + +def _decode_gha_exprs(text: str, exprs: list[str]) -> str: + """Restore `${{ ... }}` expressions from placeholder identifiers.""" + return _GHA_PLACEHOLDER_RE.sub(lambda m: exprs[int(m.group(1))], text) + + +def shfmt_via_hook(tmp_path: Path) -> tuple[bool, str]: + # `${{ ... }}` is not valid shell, so swap it for a placeholder identifier + # that shfmt can parse, then restore it after formatting. + encoded, exprs = _encode_gha_exprs(tmp_path.read_text()) + if exprs: + tmp_path.write_text(encoded) + res = subprocess.run( + [_HOOK_RUNNER, "run", "shfmt", "--files", str(tmp_path)], + cwd=REPO, + capture_output=True, + text=True, + ) + output = res.stdout + res.stderr + # shfmt emits parse errors as "::: ". + parse_err = bool(_SHFMT_ERR_RE.search(output)) + # A non-zero exit that is neither a parse error nor pre-commit's "I had + # to modify files" signal means the hook itself failed to run (missing + # binary, install failure, bad config, ...). Surface that loudly rather + # than silently treating it as a no-op. + if ( + res.returncode != 0 + and not parse_err + and "files were modified by this hook" not in output + ): + sys.exit( + f"error: `{_HOOK_RUNNER} run shfmt` failed with exit {res.returncode}:\n{output}" + ) + if exprs and not parse_err: + tmp_path.write_text(_decode_gha_exprs(tmp_path.read_text(), exprs)) + return not parse_err, output + + +def _skip(path: Path, where: int, kind: str, output: str) -> None: + print( + f" shfmt could not parse {kind} at {path}:{where + 1} — skipped", + file=sys.stderr, + ) + print(f" {output.strip()}", file=sys.stderr) + + +def process_yaml_file(path: Path, tmp_path: Path) -> int: + text = path.read_text() + had_nl = text.endswith("\n") + lines = text.split("\n") + if had_nl: + lines = lines[:-1] + items = find_run_blocks(lines) + if not items: + return 0 + changed = 0 + # Process in reverse so earlier indices remain valid as we splice. + for item in reversed(items): + if isinstance(item, BlockRun): + body = lines[item.body_start : item.body_end] + tmp_path.write_text("\n".join(dedent(body, item.body_indent)) + "\n") + ok, output = shfmt_via_hook(tmp_path) + if not ok: + _skip(path, item.body_start, "block", output) + continue + formatted = tmp_path.read_text().rstrip("\n") + new_body = reindent(formatted.split("\n"), item.body_indent) + if new_body != body: + lines[item.body_start : item.body_end] = new_body + changed += 1 + else: + tmp_path.write_text(item.value + "\n") + ok, output = shfmt_via_hook(tmp_path) + if not ok: + _skip(path, item.line_idx, "inline run", output) + continue + formatted = tmp_path.read_text().rstrip("\n") + if formatted == item.value: + continue + formatted_lines = formatted.split("\n") + if len(formatted_lines) == 1: + lines[item.line_idx] = f"{item.prefix}run: {formatted}" + else: + body_indent = len(item.prefix) + 2 + lines[item.line_idx : item.line_idx + 1] = [ + f"{item.prefix}run: |", + *reindent(formatted_lines, body_indent), + ] + changed += 1 + new_text = "\n".join(lines) + ("\n" if had_nl else "") + if new_text != text: + path.write_text(new_text) + return changed + + +def process_md_file(path: Path, tmp_path: Path) -> int: + text = path.read_text() + had_nl = text.endswith("\n") + lines = text.split("\n") + if had_nl: + lines = lines[:-1] + blocks = find_md_bash_blocks(lines) + if not blocks: + return 0 + changed = 0 + for block in reversed(blocks): + body = lines[block.body_start : block.body_end] + tmp_path.write_text("\n".join(dedent(body, block.body_indent)) + "\n") + ok, output = shfmt_via_hook(tmp_path) + if not ok: + _skip(path, block.open_line_idx, "```bash block", output) + continue + formatted = tmp_path.read_text().rstrip("\n") + formatted_lines = formatted.split("\n") if formatted else [] + new_body = reindent(formatted_lines, block.body_indent) + if new_body != body: + lines[block.body_start : block.body_end] = new_body + changed += 1 + new_text = "\n".join(lines) + ("\n" if had_nl else "") + if new_text != text: + path.write_text(new_text) + return changed + + +def process_file(path: Path, tmp_path: Path) -> int: + if path.suffix in (".yml", ".yaml"): + return process_yaml_file(path, tmp_path) + if path.suffix == ".md": + return process_md_file(path, tmp_path) + return 0 + + +def gather_files(argv: list[str]) -> list[Path]: + """Return YAML workflow/action files and markdown files that we should + process — either the paths in `argv` or, when `argv` is empty, every + such file in the repo (skipping `external/`).""" + if argv: + candidates: list[Path] = [ + (REPO / a).resolve() if not Path(a).is_absolute() else Path(a) for a in argv + ] + else: + gh = REPO / ".github" + candidates = [ + *gh.rglob("*.yml"), + *gh.rglob("*.yaml"), + *( + p + for p in REPO.rglob("*.md") + if "external" not in p.relative_to(REPO).parts + ), + ] + return sorted( + p + for p in candidates + if p.exists() + and ( + (p.suffix in (".yml", ".yaml") and ".github" in p.parts) + or p.suffix == ".md" + ) + ) + + +def main(argv: list[str]) -> int: + files = gather_files(argv) + if not files: + return 0 + with tempfile.TemporaryDirectory(prefix="format-inline-bash-") as tmpdir: + tmp_path = Path(tmpdir) / "shfmt.sh" + total = 0 + for f in files: + n = process_file(f, tmp_path) + if n: + print(f"{f.relative_to(REPO)}: reformatted {n} block(s)") + total += n + return 1 if total else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/.github/workflows/build-nix-image.yml b/.github/workflows/build-nix-image.yml index edd35132fa..bae4cfd437 100644 --- a/.github/workflows/build-nix-image.yml +++ b/.github/workflows/build-nix-image.yml @@ -100,8 +100,8 @@ jobs: - name: Create multi-arch manifests run: | - for tag in $(jq -cr '.tags[]' <<< "$DOCKER_METADATA_OUTPUT_JSON"); do - docker buildx imagetools create -t "$tag" "${tag}-amd64" "${tag}-arm64" + for tag in $(jq -cr '.tags[]' <<<"$DOCKER_METADATA_OUTPUT_JSON"); do + docker buildx imagetools create -t "$tag" "${tag}-amd64" "${tag}-arm64" done - name: Inspect image diff --git a/.github/workflows/check-pr-description.yml b/.github/workflows/check-pr-description.yml index f6eee50291..3f345a9dcb 100644 --- a/.github/workflows/check-pr-description.yml +++ b/.github/workflows/check-pr-description.yml @@ -20,11 +20,11 @@ jobs: env: PR_BODY: ${{ github.event.pull_request.body }} if: ${{ github.event_name == 'pull_request' }} - run: printenv PR_BODY > pr_body.md + run: printenv PR_BODY >pr_body.md - name: Check PR description differs from template if: ${{ github.event_name == 'pull_request' }} - run: > - python .github/scripts/check-pr-description.py - --template-file .github/pull_request_template.md - --pr-body-file pr_body.md + run: | + python .github/scripts/check-pr-description.py \ + --template-file .github/pull_request_template.md \ + --pr-body-file pr_body.md diff --git a/.github/workflows/check-pr-title.yml b/.github/workflows/check-pr-title.yml index 5631950df6..79962e0620 100644 --- a/.github/workflows/check-pr-title.yml +++ b/.github/workflows/check-pr-title.yml @@ -11,4 +11,4 @@ on: jobs: check_title: if: ${{ github.event.pull_request.draft != true }} - uses: XRPLF/actions/.github/workflows/check-pr-title.yml@291206777251b4d493641b5afbdf7c23009d2988 + uses: XRPLF/actions/.github/workflows/check-pr-title.yml@cba1f0891650baf1a9c88624dc2d72573be2eb81 diff --git a/.github/workflows/on-pr.yml b/.github/workflows/on-pr.yml index ca715e0376..db3c8667e5 100644 --- a/.github/workflows/on-pr.yml +++ b/.github/workflows/on-pr.yml @@ -98,7 +98,7 @@ jobs: READY: ${{ contains(github.event.pull_request.labels.*.name, 'Ready to merge') }} MERGE: ${{ github.event_name == 'merge_group' }} run: | - echo "go=${{ (env.DRAFT != 'true' && env.READY == 'true') || env.FILES == 'true' || env.MERGE == 'true' }}" >> "${GITHUB_OUTPUT}" + echo "go=${{ (env.DRAFT != 'true' && env.READY == 'true') || env.FILES == 'true' || env.MERGE == 'true' }}" >>"${GITHUB_OUTPUT}" cat "${GITHUB_OUTPUT}" outputs: go: ${{ steps.go.outputs.go == 'true' }} @@ -168,9 +168,9 @@ jobs: PR_URL: ${{ github.event.pull_request.html_url }} run: | gh api --method POST -H "Accept: application/vnd.github+json" -H "X-GitHub-Api-Version: 2022-11-28" \ - /repos/xrplf/clio/dispatches -f "event_type=check_libxrpl" \ - -F "client_payload[ref]=${{ needs.upload-recipe.outputs.recipe_ref }}" \ - -F "client_payload[pr_url]=${PR_URL}" + /repos/xrplf/clio/dispatches -f "event_type=check_libxrpl" \ + -F "client_payload[ref]=${{ needs.upload-recipe.outputs.recipe_ref }}" \ + -F "client_payload[pr_url]=${PR_URL}" passed: if: failure() || cancelled() diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index 2f15b82266..de6a4f40b4 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -14,7 +14,7 @@ on: jobs: # Call the workflow in the XRPLF/actions repo that runs the pre-commit hooks. run-hooks: - uses: XRPLF/actions/.github/workflows/pre-commit.yml@5e942d61bf32f7557a7c159cfac4712a687b3e3a + uses: XRPLF/actions/.github/workflows/pre-commit.yml@cba1f0891650baf1a9c88624dc2d72573be2eb81 with: runs_on: ubuntu-latest container: '{ "image": "ghcr.io/xrplf/ci/tools-rippled-pre-commit:sha-41ec7c1" }' diff --git a/.github/workflows/reusable-build-docker-image.yml b/.github/workflows/reusable-build-docker-image.yml index 5b555b713f..c3795e56fa 100644 --- a/.github/workflows/reusable-build-docker-image.yml +++ b/.github/workflows/reusable-build-docker-image.yml @@ -53,7 +53,7 @@ jobs: env: PLATFORM: ${{ inputs.platform }} run: | - echo "arch=${PLATFORM##*/}" >> $GITHUB_OUTPUT + echo "arch=${PLATFORM##*/}" >>$GITHUB_OUTPUT - name: Set up Docker Buildx uses: docker/setup-buildx-action@d7f5e7f509e45cec5c76c4d5afdd7de93d0b3df5 # v4.1.0 diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 4f9b926e98..31457bb892 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -113,7 +113,7 @@ jobs: - name: Set ccache log file if: ${{ inputs.ccache_enabled && runner.debug == '1' }} - run: echo "CCACHE_LOGFILE=${{ runner.temp }}/ccache.log" >> "${GITHUB_ENV}" + run: echo "CCACHE_LOGFILE=${{ runner.temp }}/ccache.log" >>"${GITHUB_ENV}" - name: Print build environment uses: XRPLF/actions/print-build-env@59dec886e4afb05a1724443af08baccbc045b574 @@ -146,11 +146,11 @@ jobs: CMAKE_ARGS: ${{ inputs.cmake_args }} run: | cmake \ - -G '${{ runner.os == 'Windows' && 'Visual Studio 17 2022' || 'Ninja' }}' \ - -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \ - -DCMAKE_BUILD_TYPE="${BUILD_TYPE}" \ - ${CMAKE_ARGS} \ - .. + -G '${{ runner.os == 'Windows' && 'Visual Studio 17 2022' || 'Ninja' }}' \ + -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \ + -DCMAKE_BUILD_TYPE="${BUILD_TYPE}" \ + ${CMAKE_ARGS} \ + .. - name: Check protocol autogen files are up-to-date working-directory: ${{ env.BUILD_DIR }} @@ -172,10 +172,10 @@ jobs: cmake --build . --target code_gen DIFF=$(git -C .. status --porcelain -- include/xrpl/protocol_autogen src/tests/libxrpl/protocol_autogen) if [ -n "${DIFF}" ]; then - echo "::error::Generated protocol files are out of date" - git -C .. diff -- include/xrpl/protocol_autogen src/tests/libxrpl/protocol_autogen - echo "${MESSAGE}" - exit 1 + echo "::error::Generated protocol files are out of date" + git -C .. diff -- include/xrpl/protocol_autogen src/tests/libxrpl/protocol_autogen + echo "${MESSAGE}" + exit 1 fi - name: Build the binary @@ -186,18 +186,18 @@ jobs: CMAKE_TARGET: ${{ inputs.cmake_target }} run: | cmake \ - --build . \ - --config "${BUILD_TYPE}" \ - --parallel "${BUILD_NPROC}" \ - --target "${CMAKE_TARGET}" + --build . \ + --config "${BUILD_TYPE}" \ + --parallel "${BUILD_NPROC}" \ + --target "${CMAKE_TARGET}" - name: Show ccache statistics if: ${{ inputs.ccache_enabled }} run: | ccache --show-stats -vv if [ '${{ runner.debug }}' = '1' ]; then - cat "${CCACHE_LOGFILE}" - curl ${CCACHE_REMOTE_STORAGE%|*}/status || true + cat "${CCACHE_LOGFILE}" + curl ${CCACHE_REMOTE_STORAGE%|*}/status || true fi - name: Upload the binary (Linux) @@ -214,7 +214,7 @@ jobs: working-directory: ${{ env.BUILD_DIR }} run: | set -o pipefail - ./xrpld --definitions | python3 -m json.tool > server_definitions.json + ./xrpld --definitions | python3 -m json.tool >server_definitions.json - name: Upload server definitions if: ${{ github.event.repository.visibility == 'public' && inputs.config_name == 'debian-bookworm-gcc-13-amd64-release' }} @@ -231,10 +231,10 @@ jobs: run: | ldd ./xrpld if [ "$(ldd ./xrpld | grep -E '(libstdc\+\+|libgcc)' | wc -l)" -eq 0 ]; then - echo 'The binary is statically linked.' + echo 'The binary is statically linked.' else - echo 'The binary is dynamically linked.' - exit 1 + echo 'The binary is dynamically linked.' + exit 1 fi - name: Verify presence of instrumentation (Linux) @@ -250,12 +250,12 @@ jobs: run: | ASAN_OPTS="include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-asan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/asan.supp" if [[ "${CONFIG_NAME}" == *gcc* ]]; then - ASAN_OPTS="${ASAN_OPTS}:alloc_dealloc_mismatch=0" + ASAN_OPTS="${ASAN_OPTS}:alloc_dealloc_mismatch=0" fi - echo "ASAN_OPTIONS=${ASAN_OPTS}" >> ${GITHUB_ENV} - echo "TSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-tsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/tsan.supp" >> ${GITHUB_ENV} - echo "UBSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-ubsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/ubsan.supp" >> ${GITHUB_ENV} - echo "LSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-lsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/lsan.supp" >> ${GITHUB_ENV} + echo "ASAN_OPTIONS=${ASAN_OPTS}" >>${GITHUB_ENV} + echo "TSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-tsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/tsan.supp" >>${GITHUB_ENV} + echo "UBSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-ubsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/ubsan.supp" >>${GITHUB_ENV} + echo "LSAN_OPTIONS=include=${GITHUB_WORKSPACE}/sanitizers/suppressions/runtime-lsan-options.txt:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/lsan.supp" >>${GITHUB_ENV} - name: Run the separate tests if: ${{ !inputs.build_only }} @@ -266,9 +266,9 @@ jobs: PARALLELISM: ${{ runner.os == 'Windows' && '1' || steps.nproc.outputs.nproc }} run: | ctest \ - --output-on-failure \ - -C "${BUILD_TYPE}" \ - -j "${PARALLELISM}" + --output-on-failure \ + -C "${BUILD_TYPE}" \ + -j "${PARALLELISM}" - name: Run the embedded tests if: ${{ !inputs.build_only }} @@ -278,7 +278,7 @@ jobs: run: | set -o pipefail # Coverage builds are slower due to instrumentation; use fewer parallel jobs to avoid flakiness - [ "$COVERAGE_ENABLED" = "true" ] && BUILD_NPROC=$(( BUILD_NPROC - 2 )) + [ "$COVERAGE_ENABLED" = "true" ] && BUILD_NPROC=$((BUILD_NPROC - 2)) ./xrpld --unittest --unittest-jobs "${BUILD_NPROC}" 2>&1 | tee unittest.log - name: Show test failure summary @@ -287,19 +287,19 @@ jobs: WORKING_DIR: ${{ runner.os == 'Windows' && format('{0}\{1}', env.BUILD_DIR, inputs.build_type) || env.BUILD_DIR }} run: | if [ ! -d "${WORKING_DIR}" ]; then - echo "Working directory '${WORKING_DIR}' does not exist." - exit 0 + echo "Working directory '${WORKING_DIR}' does not exist." + exit 0 fi cd "${WORKING_DIR}" if [ ! -f unittest.log ]; then - echo "unittest.log not found; embedded tests may not have run." - exit 0 + echo "unittest.log not found; embedded tests may not have run." + exit 0 fi if ! grep -E "failed" unittest.log; then - echo "Log present but no failure lines found in unittest.log." + echo "Log present but no failure lines found in unittest.log." fi - name: Debug failure (Linux) if: ${{ failure() && runner.os == 'Linux' && !inputs.build_only }} @@ -317,10 +317,10 @@ jobs: BUILD_TYPE: ${{ inputs.build_type }} run: | cmake \ - --build . \ - --config "${BUILD_TYPE}" \ - --parallel "${BUILD_NPROC}" \ - --target coverage + --build . \ + --config "${BUILD_TYPE}" \ + --parallel "${BUILD_NPROC}" \ + --target coverage - name: Upload coverage report if: ${{ github.repository == 'XRPLF/rippled' && !inputs.build_only && env.COVERAGE_ENABLED == 'true' }} diff --git a/.github/workflows/reusable-check-levelization.yml b/.github/workflows/reusable-check-levelization.yml index 4efe3e1138..b5d57a177a 100644 --- a/.github/workflows/reusable-check-levelization.yml +++ b/.github/workflows/reusable-check-levelization.yml @@ -38,9 +38,9 @@ jobs: run: | DIFF=$(git status --porcelain) if [ -n "${DIFF}" ]; then - # Print the differences to give the contributor a hint about what to - # expect when running levelization on their own machine. - git diff - echo "${MESSAGE}" - exit 1 + # Print the differences to give the contributor a hint about what to + # expect when running levelization on their own machine. + git diff + echo "${MESSAGE}" + exit 1 fi diff --git a/.github/workflows/reusable-check-rename.yml b/.github/workflows/reusable-check-rename.yml index 56a1a3e637..7aa5b80594 100644 --- a/.github/workflows/reusable-check-rename.yml +++ b/.github/workflows/reusable-check-rename.yml @@ -48,9 +48,9 @@ jobs: run: | DIFF=$(git status --porcelain) if [ -n "${DIFF}" ]; then - # Print the differences to give the contributor a hint about what to - # expect when running the renaming scripts on their own machine. - git diff - echo "${MESSAGE}" - exit 1 + # Print the differences to give the contributor a hint about what to + # expect when running the renaming scripts on their own machine. + git diff + echo "${MESSAGE}" + exit 1 fi diff --git a/.github/workflows/reusable-clang-tidy.yml b/.github/workflows/reusable-clang-tidy.yml index e01a50cf6d..8be1db5fb2 100644 --- a/.github/workflows/reusable-clang-tidy.yml +++ b/.github/workflows/reusable-clang-tidy.yml @@ -70,13 +70,13 @@ jobs: working-directory: ${{ env.BUILD_DIR }} run: | cmake \ - -G 'Ninja' \ - -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \ - -DCMAKE_BUILD_TYPE="${BUILD_TYPE}" \ - -Dtests=ON \ - -Dwerr=ON \ - -Dxrpld=ON \ - .. + -G 'Ninja' \ + -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \ + -DCMAKE_BUILD_TYPE="${BUILD_TYPE}" \ + -Dtests=ON \ + -Dwerr=ON \ + -Dxrpld=ON \ + .. # clang-tidy needs headers generated from proto files - name: Build libxrpl.libpb @@ -133,7 +133,7 @@ jobs: - name: Write issue header if: ${{ steps.run_clang_tidy.outcome != 'success' }} run: | - cat > "${ISSUE_FILE}" <"${ISSUE_FILE}" < filtered-output.txt || true + # Extract lines containing 'error:', 'warning:', or 'note:' + grep -E '(error:|warning:|note:)' "${OUTPUT_FILE}" >filtered-output.txt || true - # If filtered output is empty, use original (might be a different error format) - if [ ! -s filtered-output.txt ]; then - cp "${OUTPUT_FILE}" filtered-output.txt - fi + # If filtered output is empty, use original (might be a different error format) + if [ ! -s filtered-output.txt ]; then + cp "${OUTPUT_FILE}" filtered-output.txt + fi - # Truncate if too large - head -c 60000 filtered-output.txt >> "${ISSUE_FILE}" - if [ "$(wc -c < filtered-output.txt)" -gt 60000 ]; then - echo "" >> "${ISSUE_FILE}" - echo "... (output truncated, see artifacts for full output)" >> "${ISSUE_FILE}" - fi + # Truncate if too large + head -c 60000 filtered-output.txt >>"${ISSUE_FILE}" + if [ "$(wc -c >"${ISSUE_FILE}" + echo "... (output truncated, see artifacts for full output)" >>"${ISSUE_FILE}" + fi - rm filtered-output.txt + rm filtered-output.txt else - echo "No output file found" >> "${ISSUE_FILE}" + echo "No output file found" >>"${ISSUE_FILE}" fi - name: Append issue footer if: ${{ steps.run_clang_tidy.outcome != 'success' }} run: | - cat >> "${ISSUE_FILE}" <>"${ISSUE_FILE}" <> "${GITHUB_OUTPUT}" + ./generate.py --packaging --config=linux.json >>"${GITHUB_OUTPUT}" generate-version: runs-on: ubuntu-latest diff --git a/.github/workflows/reusable-strategy-matrix.yml b/.github/workflows/reusable-strategy-matrix.yml index b1232a138f..62d65ad3fa 100644 --- a/.github/workflows/reusable-strategy-matrix.yml +++ b/.github/workflows/reusable-strategy-matrix.yml @@ -42,4 +42,4 @@ jobs: env: GENERATE_CONFIG: ${{ inputs.os != '' && format('--config={0}.json', inputs.os) || '' }} GENERATE_OPTION: ${{ inputs.strategy_matrix == 'all' && '--all' || '' }} - run: ./generate.py ${GENERATE_OPTION} ${GENERATE_CONFIG} >> "${GITHUB_OUTPUT}" + run: ./generate.py ${GENERATE_OPTION} ${GENERATE_CONFIG} >>"${GITHUB_OUTPUT}" diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 23441c9dde..c9dec89435 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -66,6 +66,19 @@ repos: - id: shfmt args: [--write, --indent=4, --case-indent=true] + - repo: local + hooks: + - id: format-inline-bash-workflows + name: "format `run:` blocks in workflows/actions" + entry: ./.github/scripts/format-inline-bash.py + language: python + files: ^\.github/(workflows|actions)/.*\.ya?ml$ + - id: format-inline-bash-markdown + name: "format ```bash blocks in markdown" + entry: ./.github/scripts/format-inline-bash.py + language: python + files: \.md$ + - repo: https://github.com/streetsidesoftware/cspell-cli rev: 4643f154907327ee0a2c7038f0296e0dd77d9776 # frozen: v10.0.0 hooks: diff --git a/BUILD.md b/BUILD.md index a1163dbfcc..1d3fc8f774 100644 --- a/BUILD.md +++ b/BUILD.md @@ -151,8 +151,8 @@ git init git remote add origin git@github.com:XRPLF/conan-center-index.git git sparse-checkout init for recipe in "${recipes[@]}"; do - echo "Checking out recipe '${recipe}'..." - git sparse-checkout add recipes/${recipe} + echo "Checking out recipe '${recipe}'..." + git sparse-checkout add recipes/${recipe} done git fetch origin master git checkout master @@ -180,7 +180,7 @@ the new recipe will be automatically pulled from the official Conan Center. If you see an error similar to the following after running `conan profile show`: -```bash +```text ERROR: Invalid setting '17' is not a valid 'settings.compiler.version' value. Possible values are ['5.0', '5.1', '6.0', '6.1', '7.0', '7.3', '8.0', '8.1', '9.0', '9.1', '10.0', '11.0', '12.0', '13', '13.0', '13.1', '14', '14.0', '15', diff --git a/cspell.config.yaml b/cspell.config.yaml index f26268c804..ed936941e4 100644 --- a/cspell.config.yaml +++ b/cspell.config.yaml @@ -93,6 +93,7 @@ words: - daria - dcmake - dearmor + - dedented - deleteme - demultiplexer - deserializaton diff --git a/include/xrpl/protocol_autogen/README.md b/include/xrpl/protocol_autogen/README.md index 860ed1a242..608ffed085 100644 --- a/include/xrpl/protocol_autogen/README.md +++ b/include/xrpl/protocol_autogen/README.md @@ -15,8 +15,8 @@ Generation requires a one-time setup step to create a virtual environment and install Python dependencies, followed by running the generation target: ```bash -cmake --build . --target setup_code_gen # create venv and install dependencies (once) -cmake --build . --target code_gen # generate code +cmake --build . --target setup_code_gen # create venv and install dependencies (once) +cmake --build . --target code_gen # generate code ``` By default, `CODEGEN_VENV_DIR` points to `.venv` in the project root. The diff --git a/package/README.md b/package/README.md index 2089e32e64..867ca273b4 100644 --- a/package/README.md +++ b/package/README.md @@ -74,10 +74,10 @@ VERSION=2.4.0-local PKG_RELEASE=1 docker run --rm \ - -v "$(pwd):/src" \ - -w /src \ - "$IMAGE" \ - ./package/build_pkg.sh --pkg-version "$VERSION" --pkg-release "$PKG_RELEASE" + -v "$(pwd):/src" \ + -w /src \ + "$IMAGE" \ + ./package/build_pkg.sh --pkg-version "$VERSION" --pkg-release "$PKG_RELEASE" # Output: # build/debbuild/*.deb (DEB + dbgsym .ddeb) @@ -92,12 +92,12 @@ needed, but the host toolchain replaces the pinned CI image: ```bash cmake \ - -Dxrpld=ON \ - -Dxrpld_version=2.4.0-local \ - -Dtests=OFF \ - .. + -Dxrpld=ON \ + -Dxrpld_version=2.4.0-local \ + -Dtests=OFF \ + .. -cmake --build . --target package # deb on Debian/Ubuntu, rpm on RHEL +cmake --build . --target package # deb on Debian/Ubuntu, rpm on RHEL ``` The `cmake/XrplPackaging.cmake` module defines the target only if at least one From 2f3558c610ee3d446b1c68a003442e63d4abbdf5 Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 28 May 2026 10:57:29 -0400 Subject: [PATCH 09/15] ci: Run PR title and description checks on staging and release branches (#7331) Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com> --- .github/workflows/check-pr-description.yml | 13 +++++++++++-- .github/workflows/check-pr-title.yml | 13 +++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/.github/workflows/check-pr-description.yml b/.github/workflows/check-pr-description.yml index 3f345a9dcb..ff28220171 100644 --- a/.github/workflows/check-pr-description.yml +++ b/.github/workflows/check-pr-description.yml @@ -5,8 +5,17 @@ on: types: - checks_requested pull_request: - types: [opened, edited, reopened, synchronize, ready_for_review] - branches: [develop] + types: + - opened + - edited + - reopened + - synchronize + - ready_for_review + branches: + - develop + - "release-*" + - "release/*" + - "staging/*" jobs: check_description: diff --git a/.github/workflows/check-pr-title.yml b/.github/workflows/check-pr-title.yml index 79962e0620..4b5f679df1 100644 --- a/.github/workflows/check-pr-title.yml +++ b/.github/workflows/check-pr-title.yml @@ -5,8 +5,17 @@ on: types: - checks_requested pull_request: - types: [opened, edited, reopened, synchronize, ready_for_review] - branches: [develop] + types: + - opened + - edited + - reopened + - synchronize + - ready_for_review + branches: + - develop + - "release-*" + - "release/*" + - "staging/*" jobs: check_title: From 06a3f76ccdf8edda8cf1c31f1fa5c2c07c3bdd64 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 28 May 2026 17:45:53 -0400 Subject: [PATCH 10/15] Skip clang-tidy false positive: misc-include-cleaner --- src/test/basics/Number_test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 230752d37c..a4caed39f6 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -6,6 +6,7 @@ #include #include +// NOLINTNEXTLINE(misc-include-cleaner) #include #include From 2fdfd2b42015ac25e68c8804b1101540089b5fc4 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 28 May 2026 18:37:15 -0400 Subject: [PATCH 11/15] Review feedback from Tapanito and AI - Add missing headers. - Improve code coverage exclusions. - Clean up several variable names. - Improve explanatory comments. - Remove the switch statement from MantissaRange::getMin. Change it to a straight power of ten lookup. --- include/xrpl/basics/Number.h | 29 ++++++++++-------------- src/libxrpl/basics/Number.cpp | 42 ++++++++++++++++++++++++++--------- 2 files changed, 43 insertions(+), 28 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index f7ef23e605..93bef82a8c 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -50,11 +51,11 @@ namespace detail { * compile time. Doing it at runtime would be pretty wasteful and * inefficient. */ -constexpr std::size_t int64digits = 20; -consteval std::array +constexpr std::size_t kInt64Digits = 20; +consteval std::array buildPowersOfTen() { - std::array result{}; + std::array result{}; std::uint64_t power = 1; std::size_t exponent = 0; @@ -74,13 +75,13 @@ buildPowersOfTen() } // namespace detail -constexpr std::array kPowerOfTen = detail::buildPowersOfTen(); +constexpr std::array kPowerOfTen = detail::buildPowersOfTen(); static_assert(kPowerOfTen[0] == 1); static_assert(kPowerOfTen[1] == 10); static_assert(kPowerOfTen[10] == 10'000'000'000); static_assert( - isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::int64digits - 1); + isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::kInt64Digits - 1); /** MantissaRange defines a range for the mantissa of a normalized Number. * @@ -159,10 +160,12 @@ private: case MantissaScale::LargeLegacy: case MantissaScale::Large: return 18; + // LCOV_EXCL_START default: // If called in a constexpr context, this throw assures that the build fails if an // invalid scale is used. - throw std::runtime_error("Unknown mantissa scale"); // LCOV_EXCL_LINE + throw std::runtime_error("Unknown mantissa scale"); + // LCOV_EXCL_STOP } } @@ -171,17 +174,9 @@ private: static constexpr rep getMin(MantissaScale scale, int exponent) { - switch (scale) - { - case MantissaScale::Small: - case MantissaScale::LargeLegacy: - case MantissaScale::Large: - return kPowerOfTen[exponent]; - default: - // If called in a constexpr context, this throw assures that the build fails if an - // invalid scale is used. - throw std::runtime_error("Unknown mantissa scale"); // LCOV_EXCL_LINE - } + if (exponent < 0 || exponent >= kPowerOfTen.size()) + throw std::runtime_error("Invalid exponent"); // LCOV_EXCL_LINE + return kPowerOfTen[exponent]; } static constexpr CuspRoundingFix diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 28ae3ad12b..275d82d8c9 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -622,8 +622,12 @@ Number::normalize( internalrep const& maxMantissa, MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { + // Not used by every compiler version, and thus not necessarily + // counted by coverage build + // LCOV_EXCL_START doNormalize( negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); + // LCOV_EXCL_STOP } template <> @@ -636,8 +640,12 @@ Number::normalize( internalrep const& maxMantissa, MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { + // Not used by every compiler version, and thus not necessarily + // counted by coverage build + // LCOV_EXCL_START doNormalize( negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); + // LCOV_EXCL_STOP } template <> @@ -852,7 +860,9 @@ Number::operator/=(Number const& y) return *this; // n* = numerator // d* = denominator - // *p = negative (positive?) + // z* = result (quotient) + // *p = negative (p for positive, even though the value means not + // positive?) // *s = sign // *m = mantissa // *e = exponent @@ -876,7 +886,10 @@ Number::operator/=(Number const& y) // Division operates on two large integers (16-digit for small // mantissas, 19-digit for large) using integer math. If the values - // were just divided directly, the result would be at most 9. + // were just divided directly, the result would be only ever be one + // digit or zero - not very useful. + // e.g. 9'876'543'210'987'654 / 1'234'567'890'123'456 = 8 + // 1'234'567'890'123'456 / 9'876'543'210'987'654 = 0 // Introduce a power-of-ten multiplication factor for the numerator // which will ensure the result has a meaningful number of digits. // @@ -885,11 +898,11 @@ Number::operator/=(Number const& y) // * 23 / 67 = 0 // * Use a factor of 10^4 // * 230'000 / 67 = 3432 with an exponent of -4 - // * The normalized result will be 34, exponent -2 + // * The normalized result will be 34, exponent -2, or 0.34 // // The most extreme results are 10/99 and 99/10 - // * 100'000 / 99 = 1'010e-4 = 10e-2 - // * 990'000 / 10 = 99'000e-4 = 99e-1 + // * 100'000 / 99 = 1'010e-4 = 10e-2 or 0.10 + // * 990'000 / 10 = 99'000e-4 = 99e-1 or 9.9 // // Note that the computations give 2 or 3 digits after the // decimal point to determine which way to round for most scenarios. @@ -899,8 +912,7 @@ Number::operator/=(Number const& y) // behavior, which must not be changed.) // // For large mantissas (where the MantissaRange.log == 18), a shift by 10^20 would be optimal - // for most scenarios. However, larger mantissa values would overflow 2^128 (log(2^128,10) - // ~ 38.5). + // for most scenarios. However, larger mantissa values would overflow 2^128. // // * log(2^128,10) ~ 38.5 // * largeRange.log = 18, fits in 10^19 @@ -927,6 +939,8 @@ Number::operator/=(Number const& y) // remainder is exactly 0.5 for "ToNearest". This will give the // rounding the most accurate result possible, as if infinite // precision was used in the initial calculation. + + // Stage 1: Do the initial division with a factor of 10^17. auto constexpr factorExponent = 17; uint128_t constexpr f = kPowerOfTen[factorExponent]; @@ -935,7 +949,7 @@ Number::operator/=(Number const& y) auto zm = numerator / dm; auto ze = ne - de - factorExponent; - bool zn = (ns * ds) < 0; + bool zp = (ns * ds) < 0; // dropped is used in the same way as Guard::xbit_. In the case of // division, it indicates if there's any remainder left over after // we have been as precise as reasonable. If there is, it would be as @@ -947,7 +961,7 @@ Number::operator/=(Number const& y) { // Stage 2 // - // If there is a remainder, treat is as a secondary numerator. + // If there is a remainder, treat it as a secondary numerator. // Multiply by correctionFactor separately from stage 1. // The math for this would work for small mantissas, but we need to // preserve legacy behavior. @@ -980,6 +994,12 @@ Number::operator/=(Number const& y) auto const partialNumerator = remainder * correctionFactor; auto const correction = partialNumerator / dm; + // If the correction is zero, we do not have to make any + // modifications to z*, because it will not have any + // effect on the final result. (We'd be adding a bunch of + // zeros to the end of zm that would just be removed in + // normalize.) However, if that is the case, then Stage 3 is + // even more important for accuracy. if (correction != 0) { zm *= correctionFactor; @@ -1001,8 +1021,8 @@ Number::operator/=(Number const& y) } } } - doNormalize(zn, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled, dropped); - negative_ = zn; + doNormalize(zp, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled, dropped); + negative_ = zp; mantissa_ = static_cast(zm); exponent_ = ze; XRPL_ASSERT_PARTS(isnormal(), "xrpl::Number::operator/=", "result is normalized"); From cd21d745384487fc9bcf7abf68159eddbd261364 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 28 May 2026 19:58:52 -0400 Subject: [PATCH 12/15] Update the testUpwardRoundsDown test to run under all scales - Demonstrates the incorrect "before" behavior --- src/test/basics/Number_test.cpp | 73 ++++++++++++++++++++++++++------- 1 file changed, 59 insertions(+), 14 deletions(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index a4caed39f6..f635747153 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1626,10 +1626,11 @@ public: void testUpwardRoundsDown() { + auto const scale = Number::getMantissaScale(); { - testcase << "upward rounding produces a value below exact at kMaxRep cusp"; + testcase << "upward rounding produces a value below exact at kMaxRep cusp " + << to_string(scale); - NumberMantissaScaleGuard const mg{MantissaRange::MantissaScale::Large}; NumberRoundModeGuard const rg{Number::RoundingMode::Upward}; constexpr std::int64_t kAValue = 1'000'000'000'000'049'863LL; @@ -1655,13 +1656,41 @@ public: << " exact a*b = " << fmt(exactProduct) << "\n" << " stored = " << fmt(storedValue) << "\n" << " stored - exact = " << fmt(signedDifference) << "\n" - << " upward = " << (signedDifference >= 0 ? "held" : "VIOLATED") << "\n"; - - BEAST_EXPECT(signedDifference >= 0); - BEAST_EXPECT(signedDifference < pow10(product.exponent())); - BEAST_EXPECT(product.mantissa() == (std::numeric_limits::max() / 10) + 1); - BEAST_EXPECT(product.exponent() == 19); + << " upward = " << (signedDifference >= 0 ? "held" : "VIOLATED") << "\n" + << " stored.mantissa = " << product.mantissa() << "\n" + << " stored.exponent = " << product.exponent() << "\n"; log.flush(); + + switch (scale) + { + case MantissaRange::MantissaScale::Large: + BEAST_EXPECT(signedDifference >= 0); + BEAST_EXPECT(signedDifference < pow10(product.exponent())); + BEAST_EXPECT( + product.mantissa() == (std::numeric_limits::max() / 10) + 1); + BEAST_EXPECT(product.exponent() == 19); + break; + + case MantissaRange::MantissaScale::LargeLegacy: + BEAST_EXPECT(signedDifference < 0); + BEAST_EXPECT( + product.mantissa() == + (std::numeric_limits::max() / 100) * 100); + BEAST_EXPECT(product.exponent() == 18); + break; + + case MantissaRange::MantissaScale::Small: + // The seemingly weird rounding here is because + // a & b are both normalized, and both round up when + // being converted to Number, so you're really + // getting 1_000_000_000_000_050 * 9_223_372_036_854_316 + BEAST_EXPECT(signedDifference >= 0); + BEAST_EXPECT( + product.mantissa() == + (std::numeric_limits::max() / 1000) + 3); + BEAST_EXPECT(product.exponent() == 21); + break; + } } { @@ -1687,9 +1716,8 @@ public: * 4. Under Upward + positive, `round()` returns -1 (no round-up), and * the algorithm returns the truncated zm */ - testcase << "operator/= Upward on Large returns value < truth "; + testcase << "operator/= Upward on Large returns value < truth " << to_string(scale); - NumberMantissaScaleGuard const scaleGuard{MantissaRange::MantissaScale::Large}; NumberRoundModeGuard const roundGuard{Number::RoundingMode::Upward}; constexpr std::int64_t aValue = 2LL; @@ -1713,10 +1741,27 @@ public: << " (negative => Upward gave value BELOW truth)\n" << " quotient.mantissa = " << quotient.mantissa() << "\n" << " quotient.exponent = " << quotient.exponent() << "\n"; + log.flush(); // Upward invariant: stored >= exact. Bug: stored < exact. - BEAST_EXPECT(stored >= exact); - BEAST_EXPECT(diff < pow10(quotient.exponent())); + switch (scale) + { + case MantissaRange::MantissaScale::Large: + BEAST_EXPECT(stored >= exact); + BEAST_EXPECT(diff < pow10(quotient.exponent())); + break; + + case MantissaRange::MantissaScale::LargeLegacy: + BEAST_EXPECT(stored < exact); + BEAST_EXPECT(diff >= -pow10(quotient.exponent())); + break; + + case MantissaRange::MantissaScale::Small: + // Small mantissa doesn't have the correction for + // dropped remainders + BEAST_EXPECT(stored < exact); + break; + } } } @@ -1747,9 +1792,9 @@ public: testTruncate(); testRounding(); testInt64(); + + testUpwardRoundsDown(); } - // This test sets its own number range - testUpwardRoundsDown(); } }; From be9ae88d48a1037b66ab859e9cd80c9d817e5e48 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 28 May 2026 23:38:54 -0400 Subject: [PATCH 13/15] Accept AI suggestion Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/test/basics/Number_test.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index f635747153..f407072df7 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1703,14 +1703,14 @@ public: * violating Upward's directional invariant. * * Mechanism (fix-enabled path): - * 1. `operator/=` computes `numerator = nm * 10^19` and + * 1. `operator/=` computes `numerator = nm * 10^17` and * `zm = numerator / dm` (integer division, truncates remainder). * 2. If `remainder != 0`, the correction block runs: - * zm *= 1000 - * correction = (remainder * 1000) / dm // also truncates + * zm *= 100000 + * correction = (remainder * 100000) / dm // also truncates * zm += correction - * ze -= 3 - * The truncation in `correction` discards a sub-1/1000 residual. + * ze -= 5 + * The truncation in `correction` discards a sub-1/100000 residual. * 3. `normalize`'s shift loop reduces zm to fit, but the discarded * residual is BELOW the Guard's visibility, so the Guard sees fraction = 0. * 4. Under Upward + positive, `round()` returns -1 (no round-up), and From c0569037f8cad7d1c157a96133cd8aae6da5c812 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 29 May 2026 16:34:51 -0400 Subject: [PATCH 14/15] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/test/basics/Number_test.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index f407072df7..c1b122637d 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1694,12 +1694,11 @@ public: } { - /* Companion to NumberUpwardWrongDirection_test (which targets - * `operator*=` Upward at the kMaxRep cusp on LargeLegacy), but for + /* Companion regression for the kMaxRep cusp behavior, but for * `operator/=` on the cusp-fix-ENABLED `Large` scale. * - * Under `Large` (`CuspRoundingFix::Enabled`), `operator/=` with Upward - * rounding can return a value STRICTLY LESS than the exact quotient, + * Before the dropped-remainder fix, `operator/=` with Upward + * rounding could return a value STRICTLY LESS than the exact quotient, * violating Upward's directional invariant. * * Mechanism (fix-enabled path): From f6a26ca34f1739fb8f51bd9079f0c03a9c4bff00 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 29 May 2026 18:54:08 -0400 Subject: [PATCH 15/15] CI feedback: Add test cases covering other rounding modes - Downward with a negative result, and ToNearest with a remainder slightly larger than 0.5. --- src/test/basics/Number_test.cpp | 107 ++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index c1b122637d..81019970ad 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1762,6 +1762,113 @@ public: break; } } + { + /* Companion test case for Upward positive operator/=: Downward negative + */ + testcase << "operator/= Downward on Large returns value < truth " << to_string(scale); + + NumberRoundModeGuard const roundGuard{Number::RoundingMode::Downward}; + + constexpr std::int64_t aValue = -2LL; + constexpr std::int64_t bValue = 1'000'000'000'000'000'007LL; + // bValue = 10^18 + 7 (prime, in [minMantissa, kMaxRep]). + + Number const a{aValue, 0}; + Number const b{bValue, 0}; + Number const quotient = a / b; + + dec const exact = dec(aValue) / dec(bValue); + dec const stored = dec(quotient.mantissa()) * pow10(quotient.exponent()); + dec const diff = stored - exact; + + log << "\n" + << " a = " << aValue << "\n" + << " b = " << bValue << "\n" + << " exact a/b = " << fmt(exact) << "\n" + << " stored a/b = " << fmt(stored) << "\n" + << " stored - exact = " << fmt(diff) + << " (positive => Downward gave value ABOVE truth)\n" + << " quotient.mantissa = " << quotient.mantissa() << "\n" + << " quotient.exponent = " << quotient.exponent() << "\n"; + log.flush(); + + // invariant: stored <= exact. Bug: stored > exact. + switch (scale) + { + case MantissaRange::MantissaScale::Large: + BEAST_EXPECT(stored <= exact); + BEAST_EXPECT(diff > -pow10(quotient.exponent())); + break; + + case MantissaRange::MantissaScale::LargeLegacy: + BEAST_EXPECT(stored > exact); + BEAST_EXPECT(diff <= pow10(quotient.exponent())); + break; + + case MantissaRange::MantissaScale::Small: + // Small mantissa doesn't have the correction for + // dropped remainders + BEAST_EXPECT(stored < exact); + break; + } + } + { + /* Companion test case for Upward positive operator/=: ToNearest + * + * With ToNearest, if the dropped digits are exactly "5", then the mantissa will be + * rounded to even. The numbers below result in a value where the unrounded mantissa + * ends in an even digit, and "infinite precision" would drop + * "500000000000000000145...", but doNormalize only sees "5". Without the rounding fix, + * doNormalize rounds down to the even value. With the rounding fix, doNormalize knows + * there are more digits beyond "5", and so rounds _up_ to the odd value. + */ + testcase << "operator/= ToNearest on Large returns value < truth " << to_string(scale); + + NumberRoundModeGuard const roundGuard{Number::RoundingMode::ToNearest}; + + constexpr std::int64_t aValue = 1'269'917'268'816'087'809LL; + constexpr std::int64_t bValue = 3'458'525'013'821'685'511LL; + // bValue = 10^18 + 7 (prime, in [minMantissa, kMaxRep]). + + Number const a{aValue, 0}; + Number const b{bValue, 0}; + Number const quotient = a / b; + + dec const exact = dec(aValue) / dec(bValue); + dec const stored = dec(quotient.mantissa()) * pow10(quotient.exponent()); + dec const diff = stored - exact; + + log << "\n" + << " a = " << aValue << "\n" + << " b = " << bValue << "\n" + << " exact a/b = " << fmt(exact) << "\n" + << " stored a/b = " << fmt(stored) << "\n" + << " stored - exact = " << fmt(diff) + << " (negative => ToNearest gave value BELOW truth)\n" + << " quotient.mantissa = " << quotient.mantissa() << "\n" + << " quotient.exponent = " << quotient.exponent() << "\n"; + log.flush(); + + // invariant: stored >= exact. Bug: stored < exact. + switch (scale) + { + case MantissaRange::MantissaScale::Large: + BEAST_EXPECT(stored >= exact); + BEAST_EXPECT(diff < pow10(quotient.exponent())); + break; + + case MantissaRange::MantissaScale::LargeLegacy: + BEAST_EXPECT(stored < exact); + BEAST_EXPECT(diff >= -pow10(quotient.exponent())); + break; + + case MantissaRange::MantissaScale::Small: + // Small mantissa doesn't have the correction for + // dropped remainders + BEAST_EXPECT(stored < exact); + break; + } + } } void