diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index 54b56524d1..06baed9d7a 100644 --- a/cfg/xrpld-example.cfg +++ b/cfg/xrpld-example.cfg @@ -953,6 +953,21 @@ # # Optional keys for NuDB and RocksDB: # +# cache_size Size of cache for database records. Default is 16384. +# Setting this value to 0 will use the default value. +# +# cache_age Length of time in minutes to keep database records +# cached. Default is 5 minutes. Setting this value to +# 0 will use the default value. +# +# Note: if cache_size or cache_age is not specified, +# default values will be used for the unspecified +# parameter. +# +# Note: the cache will not be created if online_delete +# is specified, because the rotating NodeStore does +# not use this cache). +# # fast_load Boolean. If set, load the last persisted ledger # from disk upon process start before syncing to # the network. This is likely to improve performance diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 3aeb4b5bcd..93bef82a8c 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -2,12 +2,14 @@ #include +#include #include #include #include #include #include #include +#include #include #include @@ -40,6 +42,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 kInt64Digits = 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::kInt64Digits - 1); + /** MantissaRange defines a range for the mantissa of a normalized Number. * * The mantissa is in the range [min, max], where @@ -76,6 +119,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 +133,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,23 +150,35 @@ 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; + // 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 } } + // Keep this function for future use with different ways to compute + // the ranges. + static constexpr rep + getMin(MantissaScale scale, int exponent) + { + if (exponent < 0 || exponent >= kPowerOfTen.size()) + throw std::runtime_error("Invalid exponent"); // LCOV_EXCL_LINE + return kPowerOfTen[exponent]; + } + static constexpr CuspRoundingFix isCuspFixEnabled(MantissaScale scale) { @@ -477,8 +529,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; @@ -519,7 +570,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; @@ -725,16 +777,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); diff --git a/include/xrpl/ledger/helpers/LendingHelpers.h b/include/xrpl/ledger/helpers/LendingHelpers.h index b7a1ed6bf2..32f94ee277 100644 --- a/include/xrpl/ledger/helpers/LendingHelpers.h +++ b/include/xrpl/ledger/helpers/LendingHelpers.h @@ -461,6 +461,7 @@ loanAccruedInterest( ExtendedPaymentComponents computeOverpaymentComponents( + Rules const& rules, Asset const& asset, int32_t const loanScale, Number const& overpayment, diff --git a/include/xrpl/nodestore/Database.h b/include/xrpl/nodestore/Database.h index ca2dde560c..438a3cc7fc 100644 --- a/include/xrpl/nodestore/Database.h +++ b/include/xrpl/nodestore/Database.h @@ -131,6 +131,10 @@ public: std::uint32_t ledgerSeq, std::function const&)>&& callback); + /** Remove expired entries from the positive and negative caches. */ + virtual void + sweep() = 0; + /** Gather statistics pertaining to read and write activities. * * @param obj Json object reference into which to place counters. diff --git a/include/xrpl/nodestore/detail/DatabaseNodeImp.h b/include/xrpl/nodestore/detail/DatabaseNodeImp.h index dd94b27075..951c60c8c7 100644 --- a/include/xrpl/nodestore/detail/DatabaseNodeImp.h +++ b/include/xrpl/nodestore/detail/DatabaseNodeImp.h @@ -22,6 +22,32 @@ public: beast::Journal j) : Database(scheduler, readThreads, config, j), backend_(std::move(backend)) { + std::optional cacheSize, cacheAge; + + if (config.exists("cache_size")) + { + cacheSize = get(config, "cache_size"); + if (cacheSize.value() < 0) + Throw("Specified negative value for cache_size"); + } + + if (config.exists("cache_age")) + { + cacheAge = get(config, "cache_age"); + if (cacheAge.value() < 0) + Throw("Specified negative value for cache_age"); + } + + if (cacheSize.has_value() || cacheAge.has_value()) + { + cache_ = std::make_shared>( + "DatabaseNodeImp", + cacheSize.value_or(0), + std::chrono::minutes(cacheAge.value_or(0)), + stopwatch(), + j); + } + XRPL_ASSERT( backend_, "xrpl::NodeStore::DatabaseNodeImp::DatabaseNodeImp : non-null " @@ -73,7 +99,13 @@ public: std::uint32_t ledgerSeq, std::function const&)>&& callback) override; + void + sweep() override; + private: + // Cache for database objects. This cache is not always initialized. Check + // for null before using. + std::shared_ptr> cache_; // Persistent key/value storage std::shared_ptr backend_; diff --git a/include/xrpl/nodestore/detail/DatabaseRotatingImp.h b/include/xrpl/nodestore/detail/DatabaseRotatingImp.h index 39441ef4d8..1ba9435a5f 100644 --- a/include/xrpl/nodestore/detail/DatabaseRotatingImp.h +++ b/include/xrpl/nodestore/detail/DatabaseRotatingImp.h @@ -55,6 +55,9 @@ public: void sync() override; + void + sweep() override; + private: std::shared_ptr writableBackend_; std::shared_ptr archiveBackend_; diff --git a/package/debian/rules b/package/debian/rules index cd94da7e5b..612fe1b1a9 100644 --- a/package/debian/rules +++ b/package/debian/rules @@ -10,7 +10,7 @@ override_dh_auto_configure override_dh_auto_build override_dh_auto_test: override_dh_installsystemd: dh_installsystemd --no-stop-on-upgrade xrpld.service - dh_installsystemd --name=update-xrpld --no-start update-xrpld.service update-xrpld.timer + dh_installsystemd --name=update-xrpld --no-enable --no-start update-xrpld.service update-xrpld.timer execute_before_dh_installtmpfiles: dh_installsysusers diff --git a/package/shared/update-xrpld.timer b/package/shared/update-xrpld.timer index 21dabf1400..9fba09d30a 100644 --- a/package/shared/update-xrpld.timer +++ b/package/shared/update-xrpld.timer @@ -3,7 +3,7 @@ Description=Daily xrpld update check [Timer] OnCalendar=*-*-* 00:00:00 -RandomizedDelaySec=24h +RandomizedDelaySec=4h Persistent=true [Install] diff --git a/package/shared/xrpld.service b/package/shared/xrpld.service index 72b6cc9938..8e10ed2eee 100644 --- a/package/shared/xrpld.service +++ b/package/shared/xrpld.service @@ -2,14 +2,15 @@ Description=XRP Ledger Daemon After=network-online.target Wants=network-online.target -StartLimitIntervalSec=300 +StartLimitIntervalSec=5min StartLimitBurst=5 [Service] Type=simple ExecStart=/usr/bin/xrpld --net --silent --conf /etc/xrpld/xrpld.cfg -Restart=always +Restart=on-failure RestartSec=5s +TimeoutStopSec=5min NoNewPrivileges=true ProtectSystem=full ProtectHome=true diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 11f5934b04..275d82d8c9 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 { @@ -398,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), @@ -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,12 @@ Number::normalize( internalrep const& maxMantissa, MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, 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 <> @@ -624,7 +640,12 @@ Number::normalize( internalrep const& maxMantissa, MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, 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 <> @@ -637,7 +658,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 @@ -838,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 @@ -850,71 +874,155 @@ 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; auto const& maxMantissa = range.max; auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; - // 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 - 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; - XRPL_ASSERT_PARTS(f >= minMantissa * 10, "Number::operator/=", "factor expected size"); + // 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 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. + // + // 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, or 0.34 + // + // The most extreme results are 10/99 and 99/10 + // * 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. + // + // 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 + // * largeRange.log = 18, fits in 10^19 + // * The expanded numerator must fit in 10^38 + // * 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. - // 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; + // Stage 1: Do the initial division with a factor of 10^17. + auto constexpr factorExponent = 17; + + uint128_t constexpr f = kPowerOfTen[factorExponent]; auto const numerator = uint128_t(nm) * f; - auto zm = numerator / dmu; - auto ze = ne - de - (small ? 17 : 19); - bool zn = (ns * ds) < 0; - if (!small) + auto zm = numerator / dm; + auto ze = ne - de - factorExponent; + 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 + // 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; + + if (range.scale != MantissaRange::MantissaScale::Small) { - // Virtually multiply numerator by correctionFactor. Since that would - // overflow in the existing uint128_t, we'll do that part separately. + // Stage 2 + // + // 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 existing behavior. + // 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) / dm) / 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. - auto const remainder = (numerator % dmu); + // 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 % dm); 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 - // integer value we just computed - ze -= 3; + 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; + // 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 % dm != 0; + } } } - normalize(zn, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled); - 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"); diff --git a/src/libxrpl/ledger/helpers/LendingHelpers.cpp b/src/libxrpl/ledger/helpers/LendingHelpers.cpp index 9cda5905c9..15ebf33e46 100644 --- a/src/libxrpl/ledger/helpers/LendingHelpers.cpp +++ b/src/libxrpl/ledger/helpers/LendingHelpers.cpp @@ -559,15 +559,34 @@ tryOverpayment( << ", new total value: " << newLoanProperties.loanState.valueOutstanding << ", first payment principal: " << newLoanProperties.firstPaymentPrincipal; - // Calculate what the new loan state should be with the new periodic payment - // including rounding errors - auto const newTheoreticalState = computeTheoreticalLoanState( - rules, - newLoanProperties.periodicPayment, - periodicRate, - paymentRemaining, - managementFeeRate) + - errors; + // Calculate what the new loan state should be with the new periodic payment, + // including the preserved rounding errors. + + auto const newTheoreticalState = [&]() { + auto const state = computeTheoreticalLoanState( + rules, + newLoanProperties.periodicPayment, + periodicRate, + paymentRemaining, + managementFeeRate) + + errors; + + if (!rules.enabled(fixCleanup3_2_0)) + return state; + + // The new principal is known exactly: it is reduced by the overpayment's + // principal portion. computeTheoreticalLoanState instead derives the + // principal -- and, from it, the management fee and interest -- via a + // lossy (P * factor) / factor round-trip. Pin the principal to the exact + // value and re-derive the management fee from the exact interest gross + // (value - principal), so the intermediate state is fully consistent with + // the exact principal rather than the one-scale-unit-high round-trip. + Number const principal = + roundedOldState.principalOutstanding - overpaymentComponents.trackedPrincipalDelta; + Number const managementFee = + tenthBipsOfValue(state.valueOutstanding - principal, managementFeeRate); + return constructLoanState(state.valueOutstanding, principal, managementFee); + }(); JLOG(j.debug()) << "new theoretical value: " << newTheoreticalState.valueOutstanding << ", principal: " << newTheoreticalState.principalOutstanding @@ -762,13 +781,6 @@ doOverpayment( // The proxies still hold the original (pre-overpayment) values, which // allows us to compute deltas and verify they match what we expect // from the overpaymentComponents and loanPaymentParts. - - XRPL_ASSERT_PARTS( - overpaymentComponents.trackedPrincipalDelta == - principalOutstandingProxy - newRoundedLoanState.principalOutstanding, - "xrpl::detail::doOverpayment", - "principal change agrees"); - JLOG(j.debug()) << "valueChange: " << loanPaymentParts.valueChange << ", totalValue before: " << *totalValueOutstandingProxy << ", totalValue after: " << newRoundedLoanState.valueOutstanding @@ -780,35 +792,50 @@ doOverpayment( << overpaymentComponents.trackedPrincipalDelta - (totalValueOutstandingProxy - newRoundedLoanState.valueOutstanding); - // The valueChange returned by tryOverpayment satisfies - // valueChange = (newInterestDue - oldInterestDue) + untrackedInterest. - // Using the loan-state identity v = p + i + m and the adjacent - // `principal change agrees` assertion (dp = oldP - newP), this - // rearranges into three independently-computable terms: - // - // 1. TVO change beyond what principal repayment alone explains: - // newTVO - (oldTVO - dp) - // 2. Management fee released by re-amortization (positive when - // mfee decreased; zero when managementFeeRate == 0): - // oldMfee - newMfee - // 3. The overpayment's penalty interest part (= untrackedInterest - // for the overpayment path; see computeOverpaymentComponents): - // trackedInterestPart() - [[maybe_unused]] Number const tvoChange = newRoundedLoanState.valueOutstanding - - (totalValueOutstandingProxy - overpaymentComponents.trackedPrincipalDelta); - [[maybe_unused]] Number const managementFeeReleased = - managementFeeOutstandingProxy - newRoundedLoanState.managementFeeDue; - [[maybe_unused]] Number const interestPart = overpaymentComponents.trackedInterestPart(); + // The three assertions below are invariants that only hold once + // fixCleanup3_2_0 pins the new principal to the exact reduction + // (oldPrincipal - trackedPrincipalDelta). Before the amendment, the lossy + // (P * factor) / factor round-trip can leave the new principal one + // scale-unit high, so these equalities do not hold on the pre-amendment + // code path and must be gated to match the fix they verify. + if (rules.enabled(fixCleanup3_2_0)) + { + // The valueChange returned by tryOverpayment satisfies + // valueChange = (newInterestDue - oldInterestDue) + untrackedInterest. + // Using the loan-state identity v = p + i + m and the adjacent + // `principal change agrees` assertion (dp = oldP - newP), this + // rearranges into three independently-computable terms: + // + // 1. TVO change beyond what principal repayment alone explains: + // newTVO - (oldTVO - dp) + // 2. Management fee released by re-amortization (positive when + // mfee decreased; zero when managementFeeRate == 0): + // oldMfee - newMfee + // 3. The overpayment's penalty interest part (= untrackedInterest + // for the overpayment path; see computeOverpaymentComponents): + // trackedInterestPart() + [[maybe_unused]] Number const tvoChange = newRoundedLoanState.valueOutstanding - + (totalValueOutstandingProxy - overpaymentComponents.trackedPrincipalDelta); + [[maybe_unused]] Number const managementFeeReleased = + managementFeeOutstandingProxy - newRoundedLoanState.managementFeeDue; + [[maybe_unused]] Number const interestPart = overpaymentComponents.trackedInterestPart(); - XRPL_ASSERT_PARTS( - loanPaymentParts.valueChange == tvoChange + managementFeeReleased + interestPart, - "xrpl::detail::doOverpayment", - "interest paid agrees"); + XRPL_ASSERT_PARTS( + overpaymentComponents.trackedPrincipalDelta == + principalOutstandingProxy - newRoundedLoanState.principalOutstanding, + "xrpl::detail::doOverpayment", + "principal change agrees"); - XRPL_ASSERT_PARTS( - overpaymentComponents.trackedPrincipalDelta == loanPaymentParts.principalPaid, - "xrpl::detail::doOverpayment", - "principal payment matches"); + XRPL_ASSERT_PARTS( + loanPaymentParts.valueChange == tvoChange + managementFeeReleased + interestPart, + "xrpl::detail::doOverpayment", + "interest paid agrees"); + + XRPL_ASSERT_PARTS( + overpaymentComponents.trackedPrincipalDelta == loanPaymentParts.principalPaid, + "xrpl::detail::doOverpayment", + "principal payment matches"); + } // All validations passed, so update the proxy objects (which will // modify the actual Loan ledger object) @@ -1144,11 +1171,13 @@ computePaymentComponents( // Cap each component to never exceed what's actually outstanding deltas.principal = std::min(deltas.principal, currentLedgerState.principalOutstanding); - XRPL_ASSERT_PARTS( - deltas.interest <= currentLedgerState.interestDue, - "xrpl::detail::computePaymentComponents", - "interest due delta not greater than outstanding"); - + if (fixCleanup320Enabled) + { + XRPL_ASSERT_PARTS( + deltas.interest <= currentLedgerState.interestDue, + "xrpl::detail::computePaymentComponents", + "interest due delta not greater than outstanding"); + } // Cap interest to both the outstanding amount AND what's left of the // periodic payment after principal is paid deltas.interest = std::min( @@ -1289,6 +1318,7 @@ computePaymentComponents( */ ExtendedPaymentComponents computeOverpaymentComponents( + Rules const& rules, Asset const& asset, int32_t const loanScale, Number const& overpayment, @@ -1296,10 +1326,13 @@ computeOverpaymentComponents( TenthBips32 const overpaymentFeeRate, TenthBips16 const managementFeeRate) { - XRPL_ASSERT( - overpayment > 0 && isRounded(asset, overpayment, loanScale), - "xrpl::detail::computeOverpaymentComponents : valid overpayment " - "amount"); + if (rules.enabled(fixCleanup3_2_0)) + { + XRPL_ASSERT( + overpayment > 0 && isRounded(asset, overpayment, loanScale), + "xrpl::detail::computeOverpaymentComponents : valid overpayment " + "amount"); + } // First, deduct the fixed overpayment fee from the total amount. // This reduces the effective payment that will be applied to the loan. @@ -2055,6 +2088,7 @@ loanMakePayment( { detail::ExtendedPaymentComponents const overpaymentComponents = detail::computeOverpaymentComponents( + view.rules(), asset, loanScale, overpayment, diff --git a/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp b/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp index 0151c5b2df..f8653f7111 100644 --- a/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp +++ b/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp @@ -3,6 +3,8 @@ #include #include #include +#include +#include #include #include #include @@ -19,6 +21,16 @@ namespace xrpl::permissioned_dex { bool accountInDomain(ReadView const& view, AccountID const& account, Domain const& domainID) { + // Avoid constructing a zero-key PermissionedDomain keylet. + // keylet::permissionedDomain(uint256) uses the DomainID as the ledger key. + if (view.rules().enabled(fixCleanup3_2_0) && domainID == beast::kZero) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::permissioned_dex::accountInDomain : domainID is zero"); + return false; + // LCOV_EXCL_STOP + } + auto const sleDomain = view.read(keylet::permissionedDomain(domainID)); if (!sleDomain) return false; diff --git a/src/libxrpl/nodestore/DatabaseNodeImp.cpp b/src/libxrpl/nodestore/DatabaseNodeImp.cpp index a51c34079b..9323d69131 100644 --- a/src/libxrpl/nodestore/DatabaseNodeImp.cpp +++ b/src/libxrpl/nodestore/DatabaseNodeImp.cpp @@ -24,6 +24,13 @@ DatabaseNodeImp::store(NodeObjectType type, Blob&& data, uint256 const& hash, st auto obj = NodeObject::createObject(type, std::move(data), hash); backend_->store(obj); + if (cache_) + { + // After the store, replace a negative cache entry if there is one + cache_->canonicalize(hash, obj, [](std::shared_ptr const& n) { + return n->getType() == NodeObjectType::Dummy; + }); + } } void @@ -32,9 +39,25 @@ DatabaseNodeImp::asyncFetch( std::uint32_t ledgerSeq, std::function const&)>&& callback) { + if (cache_) + { + std::shared_ptr const obj = cache_->fetch(hash); + if (obj) + { + callback(obj->getType() == NodeObjectType::Dummy ? nullptr : obj); + return; + } + } Database::asyncFetch(hash, ledgerSeq, std::move(callback)); } +void +DatabaseNodeImp::sweep() +{ + if (cache_) + cache_->sweep(); +} + std::shared_ptr DatabaseNodeImp::fetchNodeObject( uint256 const& hash, @@ -42,32 +65,58 @@ DatabaseNodeImp::fetchNodeObject( FetchReport& fetchReport, bool duplicate) { - std::shared_ptr nodeObject = nullptr; - Status status = Status::Ok; + std::shared_ptr nodeObject = cache_ ? cache_->fetch(hash) : nullptr; + if (!nodeObject) + { + JLOG(j_.trace()) << "fetchNodeObject " << hash << ": record not " + << (cache_ ? "cached" : "found"); - try - { - status = backend_->fetch(hash, &nodeObject); - } - catch (std::exception const& e) - { - JLOG(j_.fatal()) << "fetchNodeObject " << hash - << ": Exception fetching from backend: " << e.what(); - rethrow(); - } + Status status = Status::Ok; + try + { + status = backend_->fetch(hash, &nodeObject); + } + catch (std::exception const& e) + { + JLOG(j_.fatal()) << "fetchNodeObject " << hash + << ": Exception fetching from backend: " << e.what(); + rethrow(); + } - switch (status) + switch (status) + { + case Status::Ok: + if (cache_) + { + if (nodeObject) + { + cache_->canonicalizeReplaceClient(hash, nodeObject); + } + else + { + auto notFound = NodeObject::createObject(NodeObjectType::Dummy, {}, hash); + cache_->canonicalizeReplaceClient(hash, notFound); + if (notFound->getType() != NodeObjectType::Dummy) + nodeObject = notFound; + } + } + break; + case Status::NotFound: + break; + case Status::DataCorrupt: + JLOG(j_.fatal()) << "fetchNodeObject " << hash << ": nodestore data is corrupted"; + break; + default: + JLOG(j_.warn()) << "fetchNodeObject " << hash << ": backend returns unknown result " + << static_cast(status); + break; + } + } + else { - case Status::Ok: - case Status::NotFound: - break; - case Status::DataCorrupt: - JLOG(j_.fatal()) << "fetchNodeObject " << hash << ": nodestore data is corrupted"; - break; - default: - JLOG(j_.warn()) << "fetchNodeObject " << hash << ": backend returns unknown result " - << static_cast(status); - break; + JLOG(j_.trace()) << "fetchNodeObject " << hash << ": record found in cache"; + if (nodeObject->getType() == NodeObjectType::Dummy) + nodeObject.reset(); } if (nodeObject) diff --git a/src/libxrpl/nodestore/DatabaseRotatingImp.cpp b/src/libxrpl/nodestore/DatabaseRotatingImp.cpp index 24b0e2de2e..1de6a83b4c 100644 --- a/src/libxrpl/nodestore/DatabaseRotatingImp.cpp +++ b/src/libxrpl/nodestore/DatabaseRotatingImp.cpp @@ -113,6 +113,12 @@ DatabaseRotatingImp::store(NodeObjectType type, Blob&& data, uint256 const& hash storeStats(1, nObj->getData().size()); } +void +DatabaseRotatingImp::sweep() +{ + // Nothing to do. +} + std::shared_ptr DatabaseRotatingImp::fetchNodeObject( uint256 const& hash, diff --git a/src/libxrpl/protocol/BuildInfo.cpp b/src/libxrpl/protocol/BuildInfo.cpp index 2e33d03088..7de1862dfc 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -23,7 +23,7 @@ namespace { //------------------------------------------------------------------------------ // clang-format off // NOLINTNEXTLINE(readability-identifier-naming) -char const* const versionString = "3.2.0-rc2" +char const* const versionString = "3.2.0-rc3" // clang-format on ; diff --git a/src/libxrpl/telemetry/NullTelemetry.cpp b/src/libxrpl/telemetry/NullTelemetry.cpp index 812220acdb..41d7aed635 100644 --- a/src/libxrpl/telemetry/NullTelemetry.cpp +++ b/src/libxrpl/telemetry/NullTelemetry.cpp @@ -10,14 +10,18 @@ its own factory that can return the real TelemetryImpl. */ -#include #include #include #include #ifdef XRPL_ENABLE_TELEMETRY +#include +#include #include +#include +#include +#include #include #endif diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index c189c0ec99..4fd4c3e4a3 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -34,7 +34,6 @@ #include #include #include -#include #include #include diff --git a/src/libxrpl/tx/transactors/dex/OfferCreate.cpp b/src/libxrpl/tx/transactors/dex/OfferCreate.cpp index 8bf69d25c0..f982837c5e 100644 --- a/src/libxrpl/tx/transactors/dex/OfferCreate.cpp +++ b/src/libxrpl/tx/transactors/dex/OfferCreate.cpp @@ -94,6 +94,12 @@ OfferCreate::preflight(PreflightContext const& ctx) if (tx.isFlag(tfHybrid) && !tx.isFieldPresent(sfDomainID)) return temINVALID_FLAG; + // A zero DomainID is invalid for a PermissionedDomain ledger entry because + // keylet::permissionedDomain(uint256) uses the DomainID as the ledger key. + if (auto const domainID = tx[~sfDomainID]; + ctx.rules.enabled(fixCleanup3_2_0) && domainID && *domainID == beast::kZero) + return temMALFORMED; + bool const bImmediateOrCancel(tx.isFlag(tfImmediateOrCancel)); bool const bFillOrKill(tx.isFlag(tfFillOrKill)); diff --git a/src/libxrpl/tx/transactors/payment/Payment.cpp b/src/libxrpl/tx/transactors/payment/Payment.cpp index 1848d34786..d7ad5d3b3c 100644 --- a/src/libxrpl/tx/transactors/payment/Payment.cpp +++ b/src/libxrpl/tx/transactors/payment/Payment.cpp @@ -125,6 +125,12 @@ Payment::preflight(PreflightContext const& ctx) if (!mpTokensV2 && isDstMPT && ctx.tx.isFieldPresent(sfPaths)) return temMALFORMED; + // A zero DomainID is invalid for a PermissionedDomain ledger entry because + // keylet::permissionedDomain(uint256) uses the DomainID as the ledger key. + if (auto const domainID = tx[~sfDomainID]; + ctx.rules.enabled(fixCleanup3_2_0) && domainID && *domainID == beast::kZero) + return temMALFORMED; + bool const partialPaymentAllowed = tx.isFlag(tfPartialPayment); bool const limitQuality = tx.isFlag(tfLimitQuality); bool const defaultPathsAllowed = !tx.isFlag(tfNoRippleDirect); diff --git a/src/test/app/LendingHelpers_test.cpp b/src/test/app/LendingHelpers_test.cpp index af46dd2e0f..ac8e0764fc 100644 --- a/src/test/app/LendingHelpers_test.cpp +++ b/src/test/app/LendingHelpers_test.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -513,7 +514,9 @@ class LendingHelpers_test : public beast::unit_test::Suite auto const expectedOverpaymentManagementFee = Number{10}; // 10% of 100 auto const expectedPrincipalPortion = Number{400}; // 1,000 - 100 - 500 + Env const env{*this}; auto const components = xrpl::detail::computeOverpaymentComponents( + env.current()->rules(), iou, loanScale, overpayment, @@ -854,7 +857,13 @@ class LendingHelpers_test : public beast::unit_test::Suite Number const overpaymentAmount{50}; auto const overpaymentComponents = computeOverpaymentComponents( - asset, loanScale, overpaymentAmount, TenthBips32(0), TenthBips32(0), managementFeeRate); + env.current()->rules(), + asset, + loanScale, + overpaymentAmount, + TenthBips32(0), + TenthBips32(0), + managementFeeRate); auto const loanProperties = computeLoanProperties( env.current()->rules(), @@ -942,6 +951,7 @@ class LendingHelpers_test : public beast::unit_test::Suite auto const periodicRate = loanPeriodicRate(loanInterestRate, paymentInterval); auto const overpaymentComponents = computeOverpaymentComponents( + env.current()->rules(), asset, loanScale, Number{50, 0}, @@ -1037,6 +1047,7 @@ class LendingHelpers_test : public beast::unit_test::Suite auto const periodicRate = loanPeriodicRate(loanInterestRate, paymentInterval); auto const overpaymentComponents = computeOverpaymentComponents( + env.current()->rules(), asset, loanScale, Number{50, 0}, @@ -1138,6 +1149,7 @@ class LendingHelpers_test : public beast::unit_test::Suite auto const periodicRate = loanPeriodicRate(loanInterestRate, paymentInterval); auto const overpaymentComponents = computeOverpaymentComponents( + env.current()->rules(), asset, loanScale, Number{50, 0}, @@ -1247,6 +1259,7 @@ class LendingHelpers_test : public beast::unit_test::Suite auto const periodicRate = loanPeriodicRate(loanInterestRate, paymentInterval); auto const overpaymentComponents = computeOverpaymentComponents( + env.current()->rules(), asset, loanScale, Number{50, 0}, @@ -1344,7 +1357,6 @@ class LendingHelpers_test : public beast::unit_test::Suite using namespace jtx; using namespace xrpl::detail; - Env const env{*this}; Account const issuer{"issuer"}; PrettyAsset const asset = issuer["USD"]; std::int32_t const loanScale = -5; @@ -1355,7 +1367,9 @@ class LendingHelpers_test : public beast::unit_test::Suite std::uint32_t const paymentsRemaining = 10; auto const periodicRate = loanPeriodicRate(loanInterestRate, paymentInterval); + Env const env{*this}; auto const overpaymentComponents = computeOverpaymentComponents( + env.current()->rules(), asset, loanScale, Number{50, 0}, @@ -1363,87 +1377,97 @@ class LendingHelpers_test : public beast::unit_test::Suite TenthBips32(10'000), // 10% overpayment fee managementFeeRate); - auto const loanProperties = computeLoanProperties( - env.current()->rules(), - asset, - loanPrincipal, - loanInterestRate, - paymentInterval, - paymentsRemaining, - managementFeeRate, - loanScale); + struct Outcome + { + LoanPaymentParts parts; + LoanState oldState; + LoanState newState; + }; - auto const ret = tryOverpayment( - env.current()->rules(), - asset, - loanScale, - overpaymentComponents, - loanProperties.loanState, - loanProperties.periodicPayment, - periodicRate, - paymentsRemaining, - managementFeeRate, - env.journal); + // Run tryOverpayment under a given amendment set. At this (non-near-zero) + // rate computeLoanProperties is amendment-independent, so the loan state + // is identical across the amendment; only tryOverpayment's fixCleanup3_2_0 + // behaviour (the exact-principal pin and the management-fee re-derivation + // from that principal) differs. + auto run = [&](FeatureBitset features) -> std::optional { + Env const env{*this, features}; + auto const loanProperties = computeLoanProperties( + env.current()->rules(), + asset, + loanPrincipal, + loanInterestRate, + paymentInterval, + paymentsRemaining, + managementFeeRate, + loanScale); + auto const ret = tryOverpayment( + env.current()->rules(), + asset, + loanScale, + overpaymentComponents, + loanProperties.loanState, + loanProperties.periodicPayment, + periodicRate, + paymentsRemaining, + managementFeeRate, + env.journal); + if (!BEAST_EXPECT(ret)) + return std::nullopt; + return Outcome{ + .parts = ret->first, + .oldState = loanProperties.loanState, + .newState = ret->second.loanState}; + }; - BEAST_EXPECT(ret); + auto const fixedOpt = run(testableAmendments()); + auto const legacyOpt = run(testableAmendments() - fixCleanup3_2_0); + if (!fixedOpt || !legacyOpt) + { + BEAST_EXPECT(fixedOpt.has_value()); + BEAST_EXPECT(legacyOpt.has_value()); + return; + } + Outcome const& fixed = *fixedOpt; + Outcome const& legacy = *legacyOpt; - auto const& [actualPaymentParts, newLoanProperties] = *ret; - auto const& newState = newLoanProperties.loanState; + // Components that the amendment does not change. The management fee is + // charged against the overpayment interest portion first, so interest + // paid stays 4.5 and fee paid 5.5; the principal repaid is 40 in both. + auto checkCommon = [&](Outcome const& o, char const* tag) { + BEAST_EXPECTS( + (o.parts.interestPaid == Number{45, -1}), + std::string(tag) + " interestPaid " + to_string(o.parts.interestPaid)); + BEAST_EXPECTS( + (o.parts.feePaid == Number{55, -1}), + std::string(tag) + " feePaid " + to_string(o.parts.feePaid)); + BEAST_EXPECTS( + o.parts.principalPaid == 40, + std::string(tag) + " principalPaid " + to_string(o.parts.principalPaid)); + BEAST_EXPECT( + o.parts.principalPaid == + o.oldState.principalOutstanding - o.newState.principalOutstanding); + // v = p + i + m identity: the non-interest part of valueChange equals + // the interest-due change. + BEAST_EXPECT( + o.parts.valueChange - o.parts.interestPaid == + o.newState.interestDue - o.oldState.interestDue); + }; + checkCommon(fixed, "fixed"); + checkCommon(legacy, "legacy"); - // =========== VALIDATE PAYMENT PARTS =========== - - // Since there is loan management fee, the fee is charged against - // overpayment interest portion first, so interest paid remains 4.5 - BEAST_EXPECTS( - (actualPaymentParts.interestPaid == Number{45, -1}), - " interestPaid mismatch: expected 4.5, got " + - to_string(actualPaymentParts.interestPaid)); - - // With overpayment interest portion, value change should equal the - // interest decrease plus overpayment interest portion - BEAST_EXPECTS( - (actualPaymentParts.valueChange == - Number{-164737, -5} + actualPaymentParts.interestPaid), - " valueChange mismatch: expected " + - to_string(Number{-164737, -5} + actualPaymentParts.interestPaid) + ", got " + - to_string(actualPaymentParts.valueChange)); - - // While there is no overpayment fee, fee paid should equal the - // management fee charged against the overpayment interest portion - BEAST_EXPECTS( - (actualPaymentParts.feePaid == Number{55, -1}), - " feePaid mismatch: expected 5.5, got " + to_string(actualPaymentParts.feePaid)); - - BEAST_EXPECTS( - actualPaymentParts.principalPaid == 40, - " principalPaid mismatch: expected 40, got `" + - to_string(actualPaymentParts.principalPaid)); - - // =========== VALIDATE STATE CHANGES =========== - - BEAST_EXPECTS( - actualPaymentParts.principalPaid == - loanProperties.loanState.principalOutstanding - newState.principalOutstanding, - " principalPaid mismatch: expected " + - to_string( - loanProperties.loanState.principalOutstanding - newState.principalOutstanding) + - ", got " + to_string(actualPaymentParts.principalPaid)); - - // Note that the management fee value change is not captured, as this - // value is not needed to correctly update the Vault state. - BEAST_EXPECTS( - (newState.managementFeeDue - loanProperties.loanState.managementFeeDue == - Number{-18304, -5}), - " management fee change mismatch: expected " + to_string(Number{-18304, -5}) + - ", got " + - to_string(newState.managementFeeDue - loanProperties.loanState.managementFeeDue)); - - BEAST_EXPECTS( - actualPaymentParts.valueChange - actualPaymentParts.interestPaid == - newState.interestDue - loanProperties.loanState.interestDue, - " valueChange mismatch: expected " + - to_string(newState.interestDue - loanProperties.loanState.interestDue) + ", got " + - to_string(actualPaymentParts.valueChange - actualPaymentParts.interestPaid)); + // With fixCleanup3_2_0 the management fee is re-derived from the exact + // principal; without it, from the one-scale-unit-high round-trip + // principal. So the management fee outstanding (and hence the value + // change, via v = p + i + m) differ by exactly one scale-unit (1e-5 at + // loanScale -5) between the two paths. + BEAST_EXPECT((fixed.parts.valueChange == Number{-164738, -5} + fixed.parts.interestPaid)); + BEAST_EXPECT( + (fixed.newState.managementFeeDue - fixed.oldState.managementFeeDue == + Number{-18303, -5})); + BEAST_EXPECT((legacy.parts.valueChange == Number{-164737, -5} + legacy.parts.interestPaid)); + BEAST_EXPECT( + (legacy.newState.managementFeeDue - legacy.oldState.managementFeeDue == + Number{-18304, -5})); } public: diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index c380655563..c3b5850231 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -7580,6 +7580,366 @@ protected: attemptWithdrawShares(depositorB, sharesLpB, tesSUCCESS); } + // A residual overpayment can reduce the stored principal by one scale-unit + // *less* than computeOverpaymentComponents predicts, firing the + // "principal change agrees" XRPL_ASSERT_PARTS in doOverpayment: + // + // trackedPrincipalDelta == principalOutstanding - newPrincipalOutstanding + // + // tryOverpayment re-amortizes the loan at the reduced principal, then + // re-derives the theoretical principal from the new periodic payment via + // (P * paymentFactor) / paymentFactor. That round-trip is not exact in + // Number's 19-digit arithmetic; a positive residual pushes the recomputed + // principal a hair above the exact grid point `oldPrincipal - delta`, and + // the Upward rounding in tryOverpayment then bumps it a full scale-unit + // higher. The principal therefore drops by `delta - 1 unit`, not `delta`. + // + // Concrete case (isolated, at the tryOverpayment level): + // A 100 USD loan at the minimum non-zero rate, 3 payments, loanScale -10. + // After one regular payment (principalOutstanding 66.6666666674) a residual overpayment of + // 0.049999998 yields trackedPrincipalDelta 0.048999998 but only reduces the principal by + // 0.0489999979 (newPrincipal 66.6176666695) — short by 1e-10. + // + // With fixCleanup3_2_0, tryOverpayment pins the new principal to the exact, + // on-grid reduction (oldPrincipal - trackedPrincipalDelta) instead of the + // lossy (P*factor)/factor round-trip, so the assertion holds and the + // overpayment applies cleanly. The three "principal change agrees" / + // "interest paid agrees" / "principal payment matches" assertions are + // gated behind the same amendment, so without it they are disabled (the + // server does not abort) and the loan keeps the pre-amendment computation. + // + // The test runs the same scenario under both amendment settings and checks + // the stored principal against a ground-truth value derived independently of + // the loan-state computation under test. + void + testBugOverpaymentPrincipalChange() + { + testcase("bug: doOverpayment asserts 'principal change agrees'"); + + using namespace jtx; + using namespace loan; + using namespace xrpl::detail; + + struct Params + { + TenthBips32 interestRate; + TenthBips16 managementFeeRate; + std::uint32_t paymentTotal; + std::uint32_t paymentInterval; + std::int64_t principal; + Number overpayment; + TenthBips32 overpaymentInterestRate; + TenthBips32 overpaymentFeeRate; + std::optional vaultScale; + }; + + struct Result + { + Number principalOutstanding; // stored principal after the LoanPay + Number expectedNewPrincipal; // ground truth, independent of the fix + Number managementFeeChange; // managementFeeOutstanding after - before + Number unit; // one scale-unit at the loan scale + }; + + auto runScenario = [this](FeatureBitset features, Params const& p) -> Result { + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const lender{"vaultOwner"}; + Account const borrower{"borrower"}; + + env.fund(XRP(1'000'000), issuer, lender, borrower); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const iouAsset = issuer["USD"]; + Asset const asset = iouAsset.raw(); + STAmount const iouLimit{asset, Number{9'999'999'999'999'999LL}}; + env(trust(lender, iouLimit)); + env(trust(borrower, iouLimit)); + env(pay(issuer, lender, iouAsset(1'000'000))); + env(pay(issuer, borrower, iouAsset(1'000'000))); + env.close(); + + auto const broker = createVaultAndBroker( + env, + iouAsset, + lender, + {.vaultDeposit = 900'000, + .debtMax = 0, + .managementFeeRate = p.managementFeeRate, + .vaultScale = p.vaultScale}); + + auto const brokerSle = env.le(broker.brokerKeylet()); + BEAST_EXPECT(brokerSle); + auto const loanSequence = brokerSle ? brokerSle->at(sfLoanSequence) : 0; + auto const loanKeylet = keylet::loan(broker.brokerID, loanSequence); + + env(set(borrower, broker.brokerID, Number{p.principal}, tfLoanOverpayment), + Sig(sfCounterpartySignature, lender), + kInterestRate(p.interestRate), + kPaymentTotal(p.paymentTotal), + kPaymentInterval(p.paymentInterval), + kGracePeriod(p.paymentInterval), + kOverpaymentFee(p.overpaymentFeeRate), + kOverpaymentInterestRate(p.overpaymentInterestRate), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + // The single LoanPay below makes one regular payment (the overpayment + // is smaller than one period) and leaves the residual as an + // overpayment. + auto const s = getCurrentState(env, broker, loanKeylet); + auto const periodicRate = loanPeriodicRate(s.interestRate, s.paymentInterval); + auto const onePeriod = computePaymentComponents( + env.current()->rules(), + asset, + s.loanScale, + s.totalValue, + s.principalOutstanding, + s.managementFeeOutstanding, + s.periodicPayment, + periodicRate, + s.paymentRemaining, + p.managementFeeRate); + + // Ground truth: the stored principal must drop by exactly the regular + // payment's principal portion plus the overpayment's principal + // portion. computeOverpaymentComponents depends only on the + // overpayment amount and rates (not on the loan-state computation + // under test), so it is an independent oracle. Both components are + // computed under the same rules as the env so the payment factor + // matches. + auto const overpaymentComponents = computeOverpaymentComponents( + env.current()->rules(), + asset, + s.loanScale, + p.overpayment, + p.overpaymentInterestRate, + p.overpaymentFeeRate, + p.managementFeeRate); + Number const expectedNewPrincipal = s.principalOutstanding - + onePeriod.trackedPrincipalDelta - overpaymentComponents.trackedPrincipalDelta; + + Number const managementFeeBefore = s.managementFeeOutstanding; + + STAmount const payAmount{asset, onePeriod.trackedValueDelta + p.overpayment}; + env(pay(borrower, loanKeylet.key, payAmount), + Txflags(tfLoanOverpayment), + Ter(tesSUCCESS)); + env.close(); + + auto const loanSle = env.le(loanKeylet); + BEAST_EXPECT(loanSle); + + return Result{ + .principalOutstanding = loanSle ? Number{loanSle->at(sfPrincipalOutstanding)} : 0, + .expectedNewPrincipal = expectedNewPrincipal, + .managementFeeChange = + (loanSle ? Number{loanSle->at(sfManagementFeeOutstanding)} : Number{0}) - + managementFeeBefore, + .unit = Number{1, s.loanScale}}; + }; + + // Scenario 1: the original near-zero-rate principal reproduction + // (loanScale -10, no management fee). 0.049999998 is smaller than one + // period, so it stays a residual overpayment. + Params const principalCase{ + .interestRate = TenthBips32{1}, + .managementFeeRate = TenthBips16{0}, + .paymentTotal = 3, + .paymentInterval = 60, + .principal = 100, + .overpayment = Number{49999998, -9}, + .overpaymentInterestRate = TenthBips32{1000}, + .overpaymentFeeRate = TenthBips32{1000}, + .vaultScale = 1}; + + // With fixCleanup3_2_0 the stored principal lands exactly on the + // ground-truth grid point: it is reduced by exactly the overpayment's + // principal portion. This is the key correctness check: if the principal + // pin were removed (even with the assertions still gated off), the lossy + // (P * factor) / factor round-trip would leave the principal one + // scale-unit high and this would fail. + Result const fixed = runScenario(all_, principalCase); + BEAST_EXPECTS( + fixed.principalOutstanding == fixed.expectedNewPrincipal, + "fixed principal " + to_string(fixed.principalOutstanding) + " != expected " + + to_string(fixed.expectedNewPrincipal)); + + // Without the amendment the loan amortizes with the catastrophically + // cancelling near-zero payment factor, so its schedule (and ground truth) + // differ from the fixed case; the gated assertions keep the server from + // aborting and the overpayment still lands exactly on that schedule. + Result const legacy = runScenario(all_ - fixCleanup3_2_0, principalCase); + BEAST_EXPECTS( + legacy.principalOutstanding == legacy.expectedNewPrincipal, + "legacy principal " + to_string(legacy.principalOutstanding) + " != expected " + + to_string(legacy.expectedNewPrincipal)); + + // Scenario 2: a normal-rate loan with a 10% management fee. At a normal + // rate the payment factor is identical across the amendment, so toggling + // fixCleanup3_2_0 isolates the fix. This overpayment (found by search) + // lands on a state where both the principal and the management fee differ + // by one scale-unit between the fixed and legacy paths. + Params const feeCase{ + .interestRate = TenthBips32{10000}, + .managementFeeRate = TenthBips16{10000}, + .paymentTotal = 6, + .paymentInterval = 30u * 24 * 60 * 60, + .principal = 1000, + .overpayment = Number{214367363, -10}, + .overpaymentInterestRate = TenthBips32{0}, + .overpaymentFeeRate = TenthBips32{0}, + .vaultScale = std::nullopt}; + + Result const feeFixed = runScenario(all_, feeCase); + Result const feeLegacy = runScenario(all_ - fixCleanup3_2_0, feeCase); + + // With the fix the principal is the exact reduction; without it the lossy + // (P * factor) / factor round-trip leaves it one scale-unit high. + BEAST_EXPECTS( + feeFixed.principalOutstanding == feeFixed.expectedNewPrincipal, + "fee-case fixed principal " + to_string(feeFixed.principalOutstanding) + + " != expected " + to_string(feeFixed.expectedNewPrincipal)); + BEAST_EXPECTS( + feeLegacy.principalOutstanding == feeLegacy.expectedNewPrincipal + feeLegacy.unit, + "fee-case legacy principal " + to_string(feeLegacy.principalOutstanding) + + " != expected " + to_string(feeLegacy.expectedNewPrincipal + feeLegacy.unit)); + + // Management fee: the overpayment re-amortizes a fee-bearing loan, so the management fee + // outstanding drops. + // + // Unlike the principal that is already at the correct precision, the re-amortized + // management fee is tenthBipsOfValue of the new schedule's gross interest, which depends + // on the recomputed periodic payment. So the expected change below is a pinned constant + // captured from a passing run a magic value only because there is nothing simpler to + // compare against. + // + // At the integration level, toggling the amendment also changes the regular payment's + // rounding so a fixed-vs-legacy comparison cannot isolate the overpayment management-fee + // fix. + BEAST_EXPECT(feeFixed.managementFeeChange == feeLegacy.managementFeeChange); + BEAST_EXPECTS( + (feeFixed.managementFeeChange == Number{-8219709543, -10}), + "fee-case mgmt fee change " + to_string(feeFixed.managementFeeChange)); + } + + // A LoanSet with InterestRate = 1 (0.001% annualized, the minimum non-zero + // rate). At such a near-zero rate the closed-form payment factor + // (1 + r)^n - 1 cancels catastrophically. + // + // Without fixCleanup3_2_0 the resulting amortization is degenerate and the + // LoanSet is rejected with tecPRECISION_LOSS (no loan created). With the + // amendment, computePowerMinusOneHybrid uses a numerically-stable series + // expansion, so the loan is created and the scheduled payments + // (2 * periodicPayment) cover the principal — no economic underpayment + // (yield theft). + // + // The test runs the same LoanSet under both amendment settings and pins the + // exact outcome for each. + void + testLoanSetNearZeroInterestRateSucceeds() + { + testcase("LoanSet near-zero interest rate covers principal"); + + using namespace jtx; + using namespace loan; + + Number const principalRequested{1000}; + + struct Result + { + TER ter = tesSUCCESS; + bool created = false; + std::int32_t loanScale = 0; + Number principal; + Number totalValue; + Number managementFee; + Number periodicPayment; + }; + + auto runScenario = [&](FeatureBitset features, TER expectedTer) -> Result { + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const lender{"vaultOwner"}; + Account const borrower{"borrower"}; + + env.fund(XRP(1'000'000), issuer, lender, borrower); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const iouAsset = issuer["USD"]; + STAmount const iouLimit{iouAsset.raw(), Number{9'999'999'999'999'999LL}}; + env(trust(lender, iouLimit)); + env(trust(borrower, iouLimit)); + env(pay(issuer, lender, iouAsset(1'000'000))); + env(pay(issuer, borrower, iouAsset(1'000'000))); + env.close(); + + auto const broker = createVaultAndBroker( + env, + iouAsset, + lender, + {.vaultDeposit = 100'000, .debtMax = 0, .managementFeeRate = TenthBips16{0}}); + + auto const brokerSle = env.le(broker.brokerKeylet()); + BEAST_EXPECT(brokerSle); + auto const loanSequence = brokerSle ? brokerSle->at(sfLoanSequence) : 0; + auto const loanKeylet = keylet::loan(broker.brokerID, loanSequence); + + env(set(borrower, broker.brokerID, principalRequested), + Sig(sfCounterpartySignature, lender), + kInterestRate(TenthBips32{1}), + kPaymentTotal(2), + kPaymentInterval(400), + Fee(env.current()->fees().base * 2), + Ter(expectedTer)); + env.close(); + + Result r; + r.ter = env.ter(); + if (auto const loanSle = env.le(loanKeylet)) + { + r.created = true; + r.loanScale = loanSle->at(sfLoanScale); + r.principal = loanSle->at(sfPrincipalOutstanding); + r.totalValue = loanSle->at(sfTotalValueOutstanding); + r.managementFee = loanSle->at(sfManagementFeeOutstanding); + r.periodicPayment = loanSle->at(sfPeriodicPayment); + } + return r; + }; + + Result const fixed = runScenario(all_, tesSUCCESS); + Result const legacy = runScenario(all_ - fixCleanup3_2_0, tecPRECISION_LOSS); + + // Without the amendment, the catastrophically-cancelling closed-form + // payment factor produces a degenerate amortization that fails + // checkLoanGuards: the LoanSet is rejected with tecPRECISION_LOSS and no + // loan is created. + BEAST_EXPECT(legacy.ter == tecPRECISION_LOSS); + BEAST_EXPECT(!legacy.created); + + // With the amendment the stable series expansion produces a valid loan + // at loanScale -10. + BEAST_EXPECT(fixed.ter == tesSUCCESS); + BEAST_EXPECT(fixed.created); + BEAST_EXPECT(fixed.loanScale == -10); + BEAST_EXPECT(fixed.principal == principalRequested); + BEAST_EXPECT((fixed.totalValue == Number{10000000001903, -10})); + BEAST_EXPECT(fixed.managementFee == beast::kZero); + + // Periodic payment from the numerically-stable series expansion, and the + // scheduled total (2 * periodicPayment) which exceeds the 1000 principal + // — no economic underpayment / yield theft. + BEAST_EXPECT((fixed.periodicPayment == Number{5000000000951293762, -16})); + BEAST_EXPECT((fixed.periodicPayment * 2 == Number{1000000000190258752, -15})); + BEAST_EXPECT(fixed.periodicPayment * 2 > principalRequested); + } + // An overpayment whose residual amount has more precision than loanScale // fires the isRounded(asset, overpayment, loanScale) assertion in // computeOverpaymentComponents (and a downstream "interest paid agrees" @@ -8358,12 +8718,14 @@ protected: testLimitExceeded(); testLoanSetBlockedLoanPayAllowedWhenCanTransferCleared(); testLendingCanTradeClearedNoImpact(); + testBugOverpaymentPrincipalChange(); testBugOverpayUnroundedAmount(); for (auto const flags : {0u, tfLoanOverpayment}) testYieldTheftRounding(flags); testBugInterestDueDeltaCrash(); testFullLifecycleVaultPnLNearZeroRate(); + testLoanSetNearZeroInterestRateSucceeds(); } // Tests run under each entry in amendmentCombinations(). diff --git a/src/test/app/PermissionedDEX_test.cpp b/src/test/app/PermissionedDEX_test.cpp index c6e94d7994..a88cbaa868 100644 --- a/src/test/app/PermissionedDEX_test.cpp +++ b/src/test/app/PermissionedDEX_test.cpp @@ -197,6 +197,20 @@ class PermissionedDEX_test : public beast::unit_test::Suite env.close(); } + // test preflight - malformed DomainID being zero + // Only test this with fixCleanup3_2_0 enabled. Without the fix, + // an assert-enabled build can crash when Ledger::read() receives + // a zero-key PermissionedDomain keylet. + if (features[fixCleanup3_2_0]) + { + Env env(*this, features); + auto const& [gw_, domainOwner, alice_, bob_, carol_, USD, domainID, credType] = + PermissionedDEX(env); + + env(offer(bob_, XRP(10), USD(10)), Domain(uint256{}), Ter(temMALFORMED)); + env.close(); + } + // preclaim - someone outside of the domain cannot create domain offer { Env env(*this, features); @@ -396,6 +410,24 @@ class PermissionedDEX_test : public beast::unit_test::Suite env.close(); } + // test preflight - malformed DomainID being zero + // Only test this with fixCleanup3_2_0 enabled. Without the fix, + // an assert-enabled build can crash when Ledger::read() receives + // a zero-key PermissionedDomain keylet. + if (features[fixCleanup3_2_0]) + { + Env env(*this, features); + auto const& [gw_, domainOwner, alice_, bob_, carol_, USD, domainID, credType] = + PermissionedDEX(env); + + env(pay(bob_, alice_, USD(10)), + Path(~USD), + Sendmax(XRP(10)), + Domain(uint256{}), + Ter(temMALFORMED)); + env.close(); + } + // preclaim - cannot send payment with non existent domain { Env env(*this, features); @@ -1772,7 +1804,9 @@ public: // Test domain offer (w/o hybrid) testOfferCreate(all); + testOfferCreate(all - fixCleanup3_2_0); testPayment(all); + testPayment(all - fixCleanup3_2_0); testBookStep(all); testRippling(all); testOfferTokenIssuerInDomain(all); diff --git a/src/test/app/SHAMapStore_test.cpp b/src/test/app/SHAMapStore_test.cpp index fc5ef02465..6e279eadb2 100644 --- a/src/test/app/SHAMapStore_test.cpp +++ b/src/test/app/SHAMapStore_test.cpp @@ -520,6 +520,25 @@ public: ///////////////////////////////////////////////////////////// // Create NodeStore with two backends to allow online deletion of data. // Normally, SHAMapStoreImp handles all these details. + auto nscfg = env.app().config().section(ConfigSection::nodeDatabase()); + + // Provide default values. + if (!nscfg.exists("cache_size")) + { + nscfg.set( + "cache_size", + std::to_string( + env.app().config().getValueFor(SizedItem::TreeCacheSize, std::nullopt))); + } + + if (!nscfg.exists("cache_age")) + { + nscfg.set( + "cache_age", + std::to_string( + env.app().config().getValueFor(SizedItem::TreeCacheAge, std::nullopt))); + } + NodeStoreScheduler scheduler(env.app().getJobQueue()); std::string const writableDb = "write"; @@ -528,7 +547,6 @@ public: auto archiveBackend = makeBackendRotating(env, scheduler, archiveDb); static constexpr int kReadThreads = 4; - auto nscfg = env.app().config().section(ConfigSection::nodeDatabase()); auto dbr = std::make_unique( scheduler, kReadThreads, diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 26060c70e9..81019970ad 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -6,11 +6,14 @@ #include #include +// NOLINTNEXTLINE(misc-include-cleaner) +#include #include #include #include #include +#include #include #include #include @@ -40,6 +43,40 @@ class Number_test : public beast::unit_test::Suite return out; } + using dec = boost::multiprecision::cpp_dec_float_50; + + template + 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; + p /= pow10(-n); + return p; + } + + static std::string + fmt(dec const& v) + { + std::ostringstream os; + os << std::setprecision(40) << v; + return os.str(); + } + public: void testZero() @@ -1589,39 +1626,249 @@ public: void testUpwardRoundsDown() { - testcase << "upward rounding produces a value below exact at kMaxRep cusp"; + auto const scale = Number::getMantissaScale(); + { + 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}; + 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; - Number const a = kAValue; - Number const b = kBValue; - Number const product = a * b; + 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" + << " stored.mantissa = " << product.mantissa() << "\n" + << " stored.exponent = " << product.exponent() << "\n"; + log.flush(); - BEAST_EXPECT(signedDifference >= 0); - BEAST_EXPECT(product.mantissa() == (std::numeric_limits::max() / 10) + 1); - BEAST_EXPECT(product.exponent() == 19); + 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; + } + } + + { + /* Companion regression for the kMaxRep cusp behavior, but for + * `operator/=` on the cusp-fix-ENABLED `Large` scale. + * + * 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): + * 1. `operator/=` computes `numerator = nm * 10^17` and + * `zm = numerator / dm` (integer division, truncates remainder). + * 2. If `remainder != 0`, the correction block runs: + * zm *= 100000 + * correction = (remainder * 100000) / dm // also truncates + * zm += correction + * 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 + * the algorithm returns the truncated zm + */ + testcase << "operator/= Upward on Large returns value < truth " << to_string(scale); + + 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"; + log.flush(); + + // Upward 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/=: 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 @@ -1651,9 +1898,9 @@ public: testTruncate(); testRounding(); testInt64(); + + testUpwardRoundsDown(); } - // This test sets its own number range - testUpwardRoundsDown(); } }; diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index ac90fe6c96..b641f1bc75 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -1017,6 +1017,10 @@ public: JLOG(journal_.debug()) << "MasterTransaction sweep. Size before: " << oldMasterTxSize << "; size after: " << masterTxCache.size(); } + { + // Sweep NodeStore database cache(s), if enabled. + getNodeStore().sweep(); + } { std::size_t const oldLedgerMasterCacheSize = getLedgerMaster().getFetchPackCacheSize(); diff --git a/src/xrpld/app/misc/SHAMapStoreImp.cpp b/src/xrpld/app/misc/SHAMapStoreImp.cpp index fc3f71c0cc..3f4bd6280f 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.cpp +++ b/src/xrpld/app/misc/SHAMapStoreImp.cpp @@ -165,6 +165,22 @@ std::unique_ptr SHAMapStoreImp::makeNodeStore(int readThreads) { auto nscfg = app_.config().section(ConfigSection::nodeDatabase()); + + // Provide default values. + if (!nscfg.exists("cache_size")) + { + nscfg.set( + "cache_size", + std::to_string(app_.config().getValueFor(SizedItem::TreeCacheSize, std::nullopt))); + } + + if (!nscfg.exists("cache_age")) + { + nscfg.set( + "cache_age", + std::to_string(app_.config().getValueFor(SizedItem::TreeCacheAge, std::nullopt))); + } + std::unique_ptr db; if (deleteInterval_ != 0u) @@ -254,6 +270,8 @@ SHAMapStoreImp::run() LedgerIndex lastRotated = stateDb_.getState().lastRotated; netOPs_ = &app_.getOPs(); ledgerMaster_ = &app_.getLedgerMaster(); + fullBelowCache_ = &(*app_.getNodeFamily().getFullBelowCache()); + treeNodeCache_ = &(*app_.getNodeFamily().getTreeNodeCache()); if (advisoryDelete_) canDelete_ = stateDb_.getCanDelete(); @@ -542,16 +560,16 @@ SHAMapStoreImp::clearCaches(LedgerIndex validatedSeq) // Also clear the FullBelowCache so its generation counter is bumped. // This prevents stale "full below" markers from persisting across // backend rotation/online deletion and interfering with SHAMap sync. - app_.getNodeFamily().getFullBelowCache()->clear(); + fullBelowCache_->clear(); } void SHAMapStoreImp::freshenCaches() { - if (freshenCache(*app_.getNodeFamily().getTreeNodeCache())) + if (freshenCache(*treeNodeCache_)) + return; + if (freshenCache(app_.getMasterTransaction().getCache())) return; - - freshenCache(app_.getMasterTransaction().getCache()); } void diff --git a/src/xrpld/app/misc/SHAMapStoreImp.h b/src/xrpld/app/misc/SHAMapStoreImp.h index 4f0ce04e0d..1803c15e71 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.h +++ b/src/xrpld/app/misc/SHAMapStoreImp.h @@ -7,6 +7,8 @@ #include #include #include +#include +#include #include #include @@ -93,6 +95,8 @@ private: // as of run() or before NetworkOPs* netOPs_ = nullptr; LedgerMaster* ledgerMaster_ = nullptr; + FullBelowCache* fullBelowCache_ = nullptr; + TreeNodeCache* treeNodeCache_ = nullptr; static constexpr auto kNodeStoreName = "NodeStore";