From 040bf342570127e1f74142d7329df3bdee8199a2 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 21 Jan 2026 14:21:58 -0500 Subject: [PATCH] ValidVault tracks scale of original operands alongside deltas --- src/test/app/Invariants_test.cpp | 51 +++--- src/xrpld/app/tx/detail/InvariantCheck.cpp | 178 ++++++++++++++------- src/xrpld/app/tx/detail/InvariantCheck.h | 12 +- 3 files changed, 161 insertions(+), 80 deletions(-) diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index b8a8e5d55e..c7327542b4 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -3904,11 +3904,16 @@ class Invariants_test : public beast::unit_test::suite { std::string name; std::int32_t expectedMinScale; - std::initializer_list values; + std::initializer_list values; }; NumberMantissaScaleGuard g{MantissaRange::large}; + auto makeDelta = + [&vaultAsset](Number const& n) -> ValidVault::DeltaInfo { + return {n, n.scale(vaultAsset.raw())}; + }; + auto const testCases = std::vector{ { .name = "No values", @@ -3918,26 +3923,33 @@ class Invariants_test : public beast::unit_test::suite { .name = "Mixed integer and Number values", .expectedMinScale = -15, - .values = {1, -1, Number{10, -1}}, + .values = + {makeDelta(1), makeDelta(-1), makeDelta(Number{10, -1})}, }, { .name = "Mixed scales", .expectedMinScale = -17, - .values = {Number{1, -2}, Number{5, -3}, Number{3, -2}}, + .values = + {makeDelta(Number{1, -2}), + makeDelta(Number{5, -3}), + makeDelta(Number{3, -2})}, }, { .name = "Equal scales", - .expectedMinScale = -14, - .values = {Number{1, -1}, Number{5, -1}, Number{1, -1}}, + .expectedMinScale = -16, + .values = + {makeDelta(Number{1, -1}), + makeDelta(Number{5, -1}), + makeDelta(Number{1, -1})}, }, { .name = "Mixed mantissa sizes", .expectedMinScale = -12, .values = - {Number{1}, - Number{1234, -3}, - Number{12345, -6}, - Number{123, 1}}, + {makeDelta(Number{1}), + makeDelta(Number{1234, -3}), + makeDelta(Number{12345, -6}), + makeDelta(Number{123, 1})}, }, }; @@ -3958,10 +3970,10 @@ class Invariants_test : public beast::unit_test::suite // values would lose information, so check that the rounded // value matches the original. auto const actualRounded = - roundToAsset(vaultAsset, num, actualScale); + roundToAsset(vaultAsset, num.delta, actualScale); BEAST_EXPECTS( - actualRounded == num, - "number " + to_string(num) + " rounded to scale " + + actualRounded == num.delta, + "number " + to_string(num.delta) + " rounded to scale " + std::to_string(actualScale) + " is " + to_string(actualRounded)); } @@ -3973,9 +3985,9 @@ class Invariants_test : public beast::unit_test::suite .expectedMinScale = -15, .values = { - Number{1234567890123456789, -18}, - Number{12345, -4}, - Number{1}, + makeDelta(Number{1234567890123456789, -18}), + makeDelta(Number{12345, -4}), + makeDelta(Number{1}), }, }, }; @@ -3999,16 +4011,17 @@ class Invariants_test : public beast::unit_test::suite { if (!first) { - first = num; - firstRounded = roundToAsset(vaultAsset, num, actualScale); + first = num.delta; + firstRounded = + roundToAsset(vaultAsset, num.delta, actualScale); continue; } auto const numRounded = - roundToAsset(vaultAsset, num, actualScale); + roundToAsset(vaultAsset, num.delta, actualScale); BEAST_EXPECTS( numRounded != firstRounded, "at a scale of " + std::to_string(actualScale) + " " + - to_string(num) + " == " + to_string(*first)); + to_string(num.delta) + " == " + to_string(*first)); } } } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 84a40df4d0..88b75c7513 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -2669,7 +2669,7 @@ ValidVault::visitEntry( // state (zero if created) and "after" state (zero if destroyed), so the // invariants can validate that the change in account balances matches the // change in vault balances, stored to deltas_ at the end of this function. - Number balanceDelta{}; + DeltaInfo balanceDelta{numZero, STAmount::cMinOffset - 1}; std::int8_t sign = 0; if (before) @@ -2683,20 +2683,35 @@ ValidVault::visitEntry( // At this moment we have no way of telling if this object holds // vault shares or something else. Save it for finalize. beforeMPTs_.push_back(Shares::make(*before)); - balanceDelta = static_cast( + balanceDelta.delta = static_cast( before->getFieldU64(sfOutstandingAmount)); + // MPTs are ints, so the scale is always 0. + balanceDelta.scale = 0; sign = 1; break; case ltMPTOKEN: - balanceDelta = + balanceDelta.delta = static_cast(before->getFieldU64(sfMPTAmount)); + // MPTs are ints, so the scale is always 0. + balanceDelta.scale = 0; sign = -1; break; case ltACCOUNT_ROOT: - case ltRIPPLE_STATE: - balanceDelta = before->getFieldAmount(sfBalance); + balanceDelta.delta = before->getFieldAmount(sfBalance); + // Account balance is XRP, which is an int, so the scale is + // always 0. + balanceDelta.scale = 0; sign = -1; break; + case ltRIPPLE_STATE: { + auto const amount = before->getFieldAmount(sfBalance); + balanceDelta.delta = amount; + // Trust Line balances are STAmounts, so we can use the exponent + // directly to get the scale. + balanceDelta.scale = amount.exponent(); + sign = -1; + break; + } default:; } } @@ -2712,20 +2727,36 @@ ValidVault::visitEntry( // At this moment we have no way of telling if this object holds // vault shares or something else. Save it for finalize. afterMPTs_.push_back(Shares::make(*after)); - balanceDelta -= Number(static_cast( + balanceDelta.delta -= Number(static_cast( after->getFieldU64(sfOutstandingAmount))); + // MPTs are ints, so the scale is always 0. + balanceDelta.scale = 0; sign = 1; break; case ltMPTOKEN: - balanceDelta -= Number( + balanceDelta.delta -= Number( static_cast(after->getFieldU64(sfMPTAmount))); + // MPTs are ints, so the scale is always 0. + balanceDelta.scale = 0; sign = -1; break; case ltACCOUNT_ROOT: - case ltRIPPLE_STATE: - balanceDelta -= Number(after->getFieldAmount(sfBalance)); + balanceDelta.delta -= Number(after->getFieldAmount(sfBalance)); + // Account balance is XRP, which is an int, so the scale is + // always 0. + balanceDelta.scale = 0; sign = -1; break; + case ltRIPPLE_STATE: { + auto const amount = after->getFieldAmount(sfBalance); + balanceDelta.delta -= Number(amount); + // Trust Line balances are STAmounts, so we can use the exponent + // directly to get the scale. + balanceDelta.scale = + std::max(balanceDelta.scale, amount.exponent()); + sign = -1; + break; + } default:; } } @@ -2737,7 +2768,10 @@ ValidVault::visitEntry( // transferred to the account. We intentionally do not compare balanceDelta // against zero, to avoid missing such updates. if (sign != 0) - deltas_[key] = balanceDelta * sign; + { + balanceDelta.delta *= sign; + deltas_[key] = balanceDelta; + } } bool @@ -3017,13 +3051,15 @@ ValidVault::finalize( } auto const& vaultAsset = afterVault.asset; - auto const deltaAssets = [&](AccountID const& id) -> std::optional { + auto const deltaAssets = + [&](AccountID const& id) -> std::optional { auto const get = // - [&](auto const& it, std::int8_t sign = 1) -> std::optional { + [&](auto const& it, + std::int8_t sign = 1) -> std::optional { if (it == deltas_.end()) return std::nullopt; - return it->second * sign; + return DeltaInfo{it->second.delta * sign, it->second.scale}; }; return std::visit( @@ -3044,7 +3080,7 @@ ValidVault::finalize( }, vaultAsset.value()); }; - auto const deltaAssetsTxAccount = [&]() -> std::optional { + auto const deltaAssetsTxAccount = [&]() -> std::optional { auto ret = deltaAssets(tx[sfAccount]); // Nothing returned or not XRP transaction if (!ret.has_value() || !vaultAsset.native()) @@ -3055,13 +3091,14 @@ ValidVault::finalize( delegate.has_value() && *delegate != tx[sfAccount]) return ret; - *ret += fee.drops(); - if (*ret == zero) + ret->delta += fee.drops(); + if (ret->delta == zero) return std::nullopt; return ret; }; - auto const deltaShares = [&](AccountID const& id) -> std::optional { + auto const deltaShares = + [&](AccountID const& id) -> std::optional { auto const it = [&]() { if (id == afterVault.pseudoId) return deltas_.find( @@ -3069,7 +3106,7 @@ ValidVault::finalize( return deltas_.find(keylet::mptoken(afterVault.shareMPTID, id).key); }(); - return it != deltas_.end() ? std::optional(it->second) + return it != deltas_.end() ? std::optional(it->second) : std::nullopt; }; @@ -3211,19 +3248,31 @@ ValidVault::finalize( } // Get the coarsest scale to round calculations to + DeltaInfo totalDelta{ + afterVault.assetsTotal - beforeVault.assetsTotal, + std::max( + afterVault.assetsTotal.scale(vaultAsset), + beforeVault.assetsTotal.scale(vaultAsset))}; + DeltaInfo availableDelta{ + afterVault.assetsAvailable - beforeVault.assetsAvailable, + std::max( + afterVault.assetsAvailable.scale(vaultAsset), + beforeVault.assetsAvailable.scale( + vaultAsset))}; auto const minScale = computeMinScale( vaultAsset, { *maybeVaultDeltaAssets, - afterVault.assetsTotal - beforeVault.assetsTotal, - afterVault.assetsAvailable - - beforeVault.assetsAvailable, + totalDelta, + availableDelta, }); - auto const vaultDeltaAssets = - roundToAsset(vaultAsset, *maybeVaultDeltaAssets, minScale); + auto const vaultDeltaAssets = roundToAsset( + vaultAsset, maybeVaultDeltaAssets->delta, minScale); + auto const txAmount = + roundToAsset(vaultAsset, tx[sfAmount], minScale); - if (vaultDeltaAssets > tx[sfAmount]) + if (vaultDeltaAssets > txAmount) { JLOG(j.fatal()) << // "Invariant failed: deposit must not change vault " @@ -3261,7 +3310,7 @@ ValidVault::finalize( computeMinScale(vaultAsset, {*maybeAccDeltaAssets})); auto const accountDeltaAssets = roundToAsset( - vaultAsset, *maybeAccDeltaAssets, localMinScale); + vaultAsset, maybeAccDeltaAssets->delta, localMinScale); auto const localVaultDeltaAssets = roundToAsset( vaultAsset, vaultDeltaAssets, localMinScale); @@ -3301,7 +3350,7 @@ ValidVault::finalize( } // We don't need to round shares, they are integral MPT auto const& accountDeltaShares = *maybeAccDeltaShares; - if (accountDeltaShares <= zero) + if (accountDeltaShares.delta <= zero) { JLOG(j.fatal()) << // "Invariant failed: deposit must increase depositor " @@ -3311,7 +3360,8 @@ ValidVault::finalize( auto const maybeVaultDeltaShares = deltaShares(afterVault.pseudoId); - if (!maybeVaultDeltaShares || *maybeVaultDeltaShares == zero) + if (!maybeVaultDeltaShares || + maybeVaultDeltaShares->delta == zero) { JLOG(j.fatal()) << // "Invariant failed: deposit must change vault shares"; @@ -3320,7 +3370,7 @@ ValidVault::finalize( // We don't need to round shares, they are integral MPT auto const& vaultDeltaShares = *maybeVaultDeltaShares; - if (vaultDeltaShares * -1 != accountDeltaShares) + if (vaultDeltaShares.delta * -1 != accountDeltaShares.delta) { JLOG(j.fatal()) << // "Invariant failed: deposit must change depositor and " @@ -3372,14 +3422,23 @@ ValidVault::finalize( } // Get the most coarse scale to round calculations to + auto const totalDelta = DeltaInfo{ + afterVault.assetsTotal - beforeVault.assetsTotal, + std::max( + afterVault.assetsTotal.scale(vaultAsset), + beforeVault.assetsTotal.scale(vaultAsset))}; + auto const availableDelta = DeltaInfo{ + afterVault.assetsAvailable - beforeVault.assetsAvailable, + std::max( + afterVault.assetsAvailable.scale(vaultAsset), + beforeVault.assetsAvailable.scale( + vaultAsset))}; auto const minScale = computeMinScale( vaultAsset, - {*maybeVaultDeltaAssets, - afterVault.assetsTotal - beforeVault.assetsTotal, - afterVault.assetsAvailable - beforeVault.assetsAvailable}); + {*maybeVaultDeltaAssets, totalDelta, availableDelta}); - auto const vaultPseudoDeltaAssets = - roundToAsset(vaultAsset, *maybeVaultDeltaAssets, minScale); + auto const vaultPseudoDeltaAssets = roundToAsset( + vaultAsset, maybeVaultDeltaAssets->delta, minScale); if (vaultPseudoDeltaAssets >= zero) { @@ -3401,7 +3460,7 @@ ValidVault::finalize( { auto const maybeAccDelta = deltaAssetsTxAccount(); auto const maybeOtherAccDelta = - [&]() -> std::optional { + [&]() -> std::optional { if (auto const destination = tx[~sfDestination]; destination && *destination != tx[sfAccount]) return deltaAssets(*destination); @@ -3427,7 +3486,7 @@ ValidVault::finalize( computeMinScale(vaultAsset, {destinationDelta})); auto const roundedDestinationDelta = roundToAsset( - vaultAsset, destinationDelta, localMinScale); + vaultAsset, destinationDelta.delta, localMinScale); if (roundedDestinationDelta <= zero) { @@ -3457,7 +3516,7 @@ ValidVault::finalize( return false; } - if (*accountDeltaShares >= zero) + if (accountDeltaShares->delta >= zero) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must decrease depositor " @@ -3466,14 +3525,14 @@ ValidVault::finalize( } // We don't need to round shares, they are integral MPT auto const vaultDeltaShares = deltaShares(afterVault.pseudoId); - if (!vaultDeltaShares || *vaultDeltaShares == zero) + if (!vaultDeltaShares || vaultDeltaShares->delta == zero) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must change vault shares"; return false; // That's all we can do } - if (*vaultDeltaShares * -1 != *accountDeltaShares) + if (vaultDeltaShares->delta * -1 != accountDeltaShares->delta) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must change depositor " @@ -3536,14 +3595,25 @@ ValidVault::finalize( deltaAssets(afterVault.pseudoId); if (maybeVaultDeltaAssets) { + auto const totalDelta = DeltaInfo{ + afterVault.assetsTotal - beforeVault.assetsTotal, + std::max( + afterVault.assetsTotal.scale(vaultAsset), + beforeVault.assetsTotal.scale( + vaultAsset))}; + auto const availableDelta = DeltaInfo{ + afterVault.assetsAvailable - + beforeVault.assetsAvailable, + std::max( + afterVault.assetsAvailable.scale( + vaultAsset), + beforeVault.assetsAvailable.scale( + vaultAsset))}; auto const minScale = computeMinScale( vaultAsset, - {*maybeVaultDeltaAssets, - afterVault.assetsTotal - beforeVault.assetsTotal, - afterVault.assetsAvailable - - beforeVault.assetsAvailable}); + {*maybeVaultDeltaAssets, totalDelta, availableDelta}); auto const vaultDeltaAssets = roundToAsset( - vaultAsset, *maybeVaultDeltaAssets, minScale); + vaultAsset, maybeVaultDeltaAssets->delta, minScale); if (vaultDeltaAssets >= zero) { JLOG(j.fatal()) << // @@ -3592,7 +3662,7 @@ ValidVault::finalize( "Invariant failed: clawback must change holder shares"; return false; // That's all we can do } - if (*maybeAccountDeltaShares >= zero) + if (maybeAccountDeltaShares->delta >= zero) { JLOG(j.fatal()) << // "Invariant failed: clawback must decrease holder " @@ -3602,14 +3672,15 @@ ValidVault::finalize( // We don't need to round shares, they are integral MPT auto const vaultDeltaShares = deltaShares(afterVault.pseudoId); - if (!vaultDeltaShares || *vaultDeltaShares == zero) + if (!vaultDeltaShares || vaultDeltaShares->delta == zero) { JLOG(j.fatal()) << // "Invariant failed: clawback must change vault shares"; return false; // That's all we can do } - if (*vaultDeltaShares * -1 != *maybeAccountDeltaShares) + if (vaultDeltaShares->delta * -1 != + maybeAccountDeltaShares->delta) { JLOG(j.fatal()) << // "Invariant failed: clawback must change holder and " @@ -3650,28 +3721,17 @@ ValidVault::finalize( [[nodiscard]] std::int32_t ValidVault::computeMinScale( Asset const& asset, - std::initializer_list numbers) + std::initializer_list numbers) { if (numbers.size() == 0) return 0; - // Helper to get the minimal scale of an STAmount by removing trailing - // zeros from its mantissa - auto const getNatScale = [](Asset const& asset, - Number const& value) -> std::int32_t { - if (asset.integral()) - return 0; - if (value == beast::zero) - return value.exponent(); - - return value.scale(asset); - }; std::vector natScales; std::transform( numbers.begin(), numbers.end(), std::back_inserter(natScales), - [&](Number const& n) { return getNatScale(asset, n); }); + [&](DeltaInfo const& n) { return n.scale; }); return *std::max_element(natScales.begin(), natScales.end()); } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index 3f61588cac..2d1dbc0990 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/InvariantCheck.h @@ -880,11 +880,19 @@ class ValidVault Shares static make(SLE const&); }; +public: + struct DeltaInfo final + { + Number delta = numZero; + int scale = 0; + }; + +private: std::vector afterVault_ = {}; std::vector afterMPTs_ = {}; std::vector beforeVault_ = {}; std::vector beforeMPTs_ = {}; - std::unordered_map deltas_ = {}; + std::unordered_map deltas_ = {}; public: void @@ -905,7 +913,7 @@ public: [[nodiscard]] static std::int32_t computeMinScale( Asset const& asset, - std::initializer_list numbers); + std::initializer_list numbers); }; // additional invariant checks can be declared above and then added to this