diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 528934f6de..cc927512ea 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -283,8 +283,16 @@ jobs: - name: Show test failure summary if: ${{ failure() && !inputs.build_only }} - working-directory: ${{ runner.os == 'Windows' && format('{0}/{1}', env.BUILD_DIR, inputs.build_type) || env.BUILD_DIR }} + env: + WORKING_DIR: ${{ runner.os == 'Windows' && format('{0}\{1}', env.BUILD_DIR, inputs.build_type) || env.BUILD_DIR }} run: | + if [ ! -d "${WORKING_DIR}" ]; then + echo "Working directory '${WORKING_DIR}' does not exist." + exit 0 + fi + + cd "${WORKING_DIR}" + if [ ! -f unittest.log ]; then echo "unittest.log not found; embedded tests may not have run." exit 0 diff --git a/include/xrpl/basics/base_uint.h b/include/xrpl/basics/base_uint.h index 3f38d4049b..727768a69a 100644 --- a/include/xrpl/basics/base_uint.h +++ b/include/xrpl/basics/base_uint.h @@ -46,6 +46,11 @@ struct IsContiguousContainer : std::true_type { }; +template +struct AlwaysFalseT : std::bool_constant +{ +}; + } // namespace detail /** Integers of any length that is a multiple of 32-bits @@ -272,10 +277,28 @@ public: std::is_trivially_copyable_v>> explicit BaseUInt(Container const& c) { + // Use AlwaysFalseT so the static_assert condition is dependent + // and only triggers when this constructor template is instantiated. + static_assert( + detail::AlwaysFalseT::value, + "This constructor is not intended to be used and will be soon removed. " + "Use base_uint::fromRaw instead."); + } + + template < + class Container, + class = std::enable_if_t< + detail::IsContiguousContainer::value && + std::is_trivially_copyable_v>> + static BaseUInt + fromRaw(Container const& c) + { + BaseUInt result; XRPL_ASSERT( c.size() * sizeof(typename Container::value_type) == size(), - "xrpl::BaseUInt::BaseUInt(Container auto) : input size match"); - std::memcpy(data_.data(), c.data(), size()); + "xrpl::BaseUInt::fromRaw(Container auto) : input size match"); + std::memcpy(result.data_.data(), c.data(), size()); + return result; } template diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 78b78e132e..2c2cd14e97 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -46,9 +46,6 @@ XRPL_FEATURE(Credentials, Supported::Yes, VoteBehavior::DefaultNo XRPL_FEATURE(AMMClawback, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (AMMv1_2, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(MPTokensV1, Supported::Yes, VoteBehavior::DefaultNo) -// InvariantsV1_1 will be changes to Supported::yes when all the -// invariants expected to be included under it are complete. -XRPL_FEATURE(InvariantsV1_1, Supported::No, VoteBehavior::DefaultNo) XRPL_FIX (NFTokenPageLinks, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (InnerObjTemplate2, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (EnforceNFTokenTrustline, Supported::Yes, VoteBehavior::DefaultNo) diff --git a/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp b/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp index 40a41d236a..160629d835 100644 --- a/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp @@ -152,7 +152,7 @@ pseudoAccountAddress(ReadView const& view, uint256 const& pseudoOwnerKey) RipeshaHasher rsh; auto const hash = sha512Half(i, view.header().parentHash, pseudoOwnerKey); rsh(hash.data(), hash.size()); - AccountID const ret{static_cast(rsh)}; + AccountID const ret = AccountID::fromRaw(static_cast(rsh)); if (!view.read(keylet::account(ret))) return ret; } diff --git a/src/libxrpl/ledger/helpers/LendingHelpers.cpp b/src/libxrpl/ledger/helpers/LendingHelpers.cpp index 89d5ed8e35..a8bee7aecb 100644 --- a/src/libxrpl/ledger/helpers/LendingHelpers.cpp +++ b/src/libxrpl/ledger/helpers/LendingHelpers.cpp @@ -1963,8 +1963,18 @@ loanMakePayment( // ------------------------------------------------------------- // overpayment handling + // + // If the "fixSecurity3_1_3" amendment is enabled, truncate "amount", + // at the loan scale. If the raw value is used, the overpayment + // amount could be meaningless dust. Trying to process such a small + // amount will, at best, waste time when all the result values round + // to zero. At worst, it can cause logical errors with tiny amounts + // of interest that don't add up correctly. + auto const roundedAmount = view.rules().enabled(fixSecurity3_1_3) + ? roundToAsset(asset, amount, loanScale, Number::RoundingMode::TowardsZero) + : amount; if (paymentType == LoanPaymentType::Overpayment && loan->isFlag(lsfLoanOverpayment) && - paymentRemainingProxy > 0 && totalPaid < amount && + paymentRemainingProxy > 0 && totalPaid < roundedAmount && numPayments < kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION) { TenthBips32 const overpaymentInterestRate{loan->at(sfOverpaymentInterestRate)}; @@ -1973,7 +1983,7 @@ loanMakePayment( // It shouldn't be possible for the overpayment to be greater than // totalValueOutstanding, because that would have been processed as // another normal payment. But cap it just in case. - Number const overpayment = std::min(amount - totalPaid, *totalValueOutstandingProxy); + Number const overpayment = std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy); detail::ExtendedPaymentComponents const overpaymentComponents = detail::computeOverpaymentComponents( diff --git a/src/libxrpl/protocol/AccountID.cpp b/src/libxrpl/protocol/AccountID.cpp index 6639835b41..76982560de 100644 --- a/src/libxrpl/protocol/AccountID.cpp +++ b/src/libxrpl/protocol/AccountID.cpp @@ -105,7 +105,7 @@ parseBase58(std::string const& s) auto const result = decodeBase58Token(s, TokenType::AccountID); if (result.size() != AccountID::kBYTES) return std::nullopt; - return AccountID{result}; + return AccountID::fromRaw(result); } //------------------------------------------------------------------------------ @@ -150,7 +150,7 @@ calcAccountID(PublicKey const& pk) RipeshaHasher rsh; rsh(pk.data(), pk.size()); - return AccountID{static_cast(rsh)}; + return AccountID::fromRaw(static_cast(rsh)); } AccountID const& diff --git a/src/libxrpl/protocol/Indexes.cpp b/src/libxrpl/protocol/Indexes.cpp index a8ade0de0f..a74e6cedf4 100644 --- a/src/libxrpl/protocol/Indexes.cpp +++ b/src/libxrpl/protocol/Indexes.cpp @@ -385,7 +385,7 @@ nftpageMin(AccountID const& owner) { std::array buf{}; std::memcpy(buf.data(), owner.data(), owner.size()); - return {ltNFTOKEN_PAGE, uint256{buf}}; + return {ltNFTOKEN_PAGE, uint256::fromRaw(buf)}; } Keylet diff --git a/src/libxrpl/protocol/PublicKey.cpp b/src/libxrpl/protocol/PublicKey.cpp index ad88e60fe7..c38fb781c9 100644 --- a/src/libxrpl/protocol/PublicKey.cpp +++ b/src/libxrpl/protocol/PublicKey.cpp @@ -297,7 +297,7 @@ calcNodeID(PublicKey const& pk) RipeshaHasher h; h(pk.data(), pk.size()); - return NodeID{static_cast(h)}; + return NodeID::fromRaw(static_cast(h)); } } // namespace xrpl diff --git a/src/libxrpl/protocol/STIssue.cpp b/src/libxrpl/protocol/STIssue.cpp index c0019c334f..c9b8109e32 100644 --- a/src/libxrpl/protocol/STIssue.cpp +++ b/src/libxrpl/protocol/STIssue.cpp @@ -28,7 +28,7 @@ STIssue::STIssue(SerialIter& sit, SField const& name) : STBase{name} { auto const currencyOrAccount = sit.get160(); - if (isXRP(static_cast(currencyOrAccount))) + if (isXRP(Currency::fromRaw(currencyOrAccount))) { asset_ = xrpIssue(); } @@ -39,7 +39,7 @@ STIssue::STIssue(SerialIter& sit, SField const& name) : STBase{name} // - 160 bits MPT issuer account // - 160 bits black hole account // - 32 bits sequence - AccountID const account = static_cast(sit.get160()); + AccountID const account = AccountID::fromRaw(sit.get160()); // MPT if (noAccount() == account) { diff --git a/src/libxrpl/protocol/STPathSet.cpp b/src/libxrpl/protocol/STPathSet.cpp index 9c0cc20e96..35d5fd782b 100644 --- a/src/libxrpl/protocol/STPathSet.cpp +++ b/src/libxrpl/protocol/STPathSet.cpp @@ -93,7 +93,7 @@ STPathSet::STPathSet(SerialIter& sit, SField const& name) : STBase(name) XRPL_ASSERT( !(hasCurrency && hasMPT), "xrpl::STPathSet::STPathSet : not has Currency and MPT"); if (hasCurrency) - asset = static_cast(sit.get160()); + asset = Currency::fromRaw(sit.get160()); if (hasMPT) asset = sit.get192(); diff --git a/src/libxrpl/protocol/STVector256.cpp b/src/libxrpl/protocol/STVector256.cpp index 75b833dd7a..7aca309667 100644 --- a/src/libxrpl/protocol/STVector256.cpp +++ b/src/libxrpl/protocol/STVector256.cpp @@ -30,7 +30,7 @@ STVector256::STVector256(SerialIter& sit, SField const& name) : STBase(name) value_.reserve(cnt); for (std::size_t i = 0; i != cnt; ++i) - value_.emplace_back(slice.substr(i * uint256::size(), uint256::size())); + value_.push_back(uint256::fromRaw(slice.substr(i * uint256::size(), uint256::size()))); } STBase* diff --git a/src/libxrpl/protocol/Seed.cpp b/src/libxrpl/protocol/Seed.cpp index f15d4dcff0..42b2e85498 100644 --- a/src/libxrpl/protocol/Seed.cpp +++ b/src/libxrpl/protocol/Seed.cpp @@ -105,7 +105,7 @@ parseGenericSeed(std::string const& str, bool rfc1751) if (RFC1751::getKeyFromEnglish(key, str) == 1) { Blob const blob(key.rbegin(), key.rend()); - return Seed{uint128{blob}}; + return Seed{uint128::fromRaw(blob)}; } } diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 4f25a91bdf..e9c32bf14c 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -467,7 +467,7 @@ AccountRootsDeletedClean::finalize( // transaction processing results, however unlikely, only fail if the // feature is enabled. Enabled, or not, though, a fatal-level message will // be logged - [[maybe_unused]] bool const enforce = view.rules().enabled(featureInvariantsV1_1) || + [[maybe_unused]] bool const enforce = view.rules().enabled(fixCleanup3_2_0) || view.rules().enabled(featureSingleAssetVault) || view.rules().enabled(featureLendingProtocol); diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index ef93f0dc1b..d7147128cc 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -30,6 +30,7 @@ #include #include #include +#include namespace xrpl { @@ -162,6 +163,7 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) Number const numPaymentEstimate = static_cast(amount / regularPayment); // Charge one base fee per paymentsPerFeeIncrement payments, rounding up. + // This set round is safe because there's a mode guard just above Number::setround(Number::RoundingMode::Upward); auto const feeIncrements = std::max( std::int64_t(1), @@ -463,9 +465,10 @@ LoanPay::doApply() // Vault object state changes view.update(vaultSle); + Number const assetsAvailableBefore = *assetsAvailableProxy; + Number const assetsTotalBefore = *assetsTotalProxy; #if !NDEBUG { - Number const assetsAvailableBefore = *assetsAvailableProxy; Number const pseudoAccountBalanceBefore = accountHolds( view, vaultPseudoAccount, @@ -489,16 +492,6 @@ LoanPay::doApply() "xrpl::LoanPay::doApply", "assets available must not be greater than assets outstanding"); - if (*assetsAvailableProxy > *assetsTotalProxy) - { - // LCOV_EXCL_START - JLOG(j_.fatal()) << "Vault assets available must not be greater " - "than assets outstanding. Available: " - << *assetsAvailableProxy << ", Total: " << *assetsTotalProxy; - return tecINTERNAL; - // LCOV_EXCL_STOP - } - JLOG(j_.debug()) << "total paid to vault raw: " << totalPaidToVaultRaw << ", total paid to vault rounded: " << totalPaidToVaultRounded << ", total paid to broker: " << totalPaidToBroker @@ -524,12 +517,68 @@ LoanPay::doApply() associateAsset(*vaultSle, asset); // Duplicate some checks after rounding + Number const assetsAvailableAfter = *assetsAvailableProxy; + Number const assetsTotalAfter = *assetsTotalProxy; + XRPL_ASSERT_PARTS( - *assetsAvailableProxy <= *assetsTotalProxy, + assetsAvailableAfter <= assetsTotalAfter, "xrpl::LoanPay::doApply", "assets available must not be greater than assets outstanding"); + if (assetsAvailableAfter == assetsAvailableBefore) + { + // An unchanged assetsAvailable indicates that the amount paid to the + // vault was zero, or rounded to zero. That should be impossible, but I + // can't rule it out for extreme edge cases, so fail gracefully if it + // happens. + // + // LCOV_EXCL_START + JLOG(j_.warn()) << "LoanPay: Vault assets available unchanged after rounding: " // + << "Before: " << assetsAvailableBefore // + << ", After: " << assetsAvailableAfter; + return tecPRECISION_LOSS; + // LCOV_EXCL_STOP + } + if (paymentParts->valueChange != beast::kZERO && assetsTotalAfter == assetsTotalBefore) + { + // Non-zero valueChange with an unchanged assetsTotal indicates that the + // actual value change rounded to zero. That should be impossible, but I + // can't rule it out for extreme edge cases, so fail gracefully if it + // happens. + // + // LCOV_EXCL_START + JLOG(j_.warn()) + << "LoanPay: Vault assets expected change, but unchanged after rounding: " // + << "Before: " << assetsTotalBefore // + << ", After: " << assetsTotalAfter // + << ", ValueChange: " << paymentParts->valueChange; + return tecPRECISION_LOSS; + // LCOV_EXCL_STOP + } + if (paymentParts->valueChange == beast::kZERO && assetsTotalAfter != assetsTotalBefore) + { + // A change in assetsTotal when there was no valueChange indicates that + // something really weird happened. That should be flat out impossible. + // + // LCOV_EXCL_START + JLOG(j_.fatal()) << "LoanPay: Vault assets changed unexpectedly after rounding: " // + << "Before: " << assetsTotalBefore // + << ", After: " << assetsTotalAfter // + << ", ValueChange: " << paymentParts->valueChange; + return tecINTERNAL; + // LCOV_EXCL_STOP + } + if (assetsAvailableAfter > assetsTotalAfter) + { + // Assets available are not allowed to be larger than assets total. + // LCOV_EXCL_START + JLOG(j_.fatal()) << "LoanPay: Vault assets available must not be greater " + "than assets outstanding. Available: " + << assetsAvailableAfter << ", Total: " << assetsTotalAfter; + return tecINTERNAL; + // LCOV_EXCL_STOP + } -#if !NDEBUG + // These three values are used to check that funds are conserved after the transfers auto const accountBalanceBefore = accountHolds( view, account_, @@ -557,7 +606,6 @@ LoanPay::doApply() AuthHandling::IgnoreAuth, j_, SpendableHandling::FullBalance); -#endif if (totalPaidToVaultRounded != beast::kZERO) { @@ -593,19 +641,22 @@ LoanPay::doApply() return ter; #if !NDEBUG - Number const assetsAvailableAfter = *assetsAvailableProxy; - Number const pseudoAccountBalanceAfter = accountHolds( - view, - vaultPseudoAccount, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_); - XRPL_ASSERT_PARTS( - assetsAvailableAfter == pseudoAccountBalanceAfter, - "xrpl::LoanPay::doApply", - "vault pseudo balance agrees after"); + { + Number const pseudoAccountBalanceAfter = accountHolds( + view, + vaultPseudoAccount, + asset, + FreezeHandling::IgnoreFreeze, + AuthHandling::IgnoreAuth, + j_); + XRPL_ASSERT_PARTS( + assetsAvailableAfter == pseudoAccountBalanceAfter, + "xrpl::LoanPay::doApply", + "vault pseudo balance agrees after"); + } +#endif + // Check that funds are conserved auto const accountBalanceAfter = accountHolds( view, account_, @@ -633,14 +684,121 @@ LoanPay::doApply() AuthHandling::IgnoreAuth, j_, SpendableHandling::FullBalance); + auto const balanceScale = [&]() { + // Find a reasonable scale to use for the balance comparisons. + // + // First find the minimum and maximum exponent of all the non-zero balances, before and + // after. If min and max are equal, use that value. If they are not, use "max + 1" to reduce + // rounding discrepancies without making the result meaningless. Cap the scale at + // STAmount::kMAX_OFFSET, just in case the numbers are all very large. + std::vector exponents; + exponents.reserve(6); + for (auto const& a : { + accountBalanceBefore, + vaultBalanceBefore, + brokerBalanceBefore, + accountBalanceAfter, + vaultBalanceAfter, + brokerBalanceAfter, + }) + { + // Exclude zeroes + if (a != beast::kZERO) + exponents.push_back(a.exponent()); + } + if (exponents.empty()) + { + UNREACHABLE("xrpl::LoanPay::doApply : all zeroes"); + return 0; + } + auto const [minItr, maxItr] = std::ranges::minmax_element(exponents); + auto const min = *minItr; + auto const max = *maxItr; + JLOG(j_.trace()) << "Min scale: " << min << ", max scale: " << max; + // IOU rounding can be interesting. We want all the balance checks to agree, but don't want + // to round to such an extreme that it becomes meaningless. e.g. Everything rounds to one + // digit. So add 1 to the max (reducing the number of digits after the decimal point by 1) + // if the scales are not already all the same. + return std::min(min == max ? max : max + 1, STAmount::kMAX_OFFSET); + }(); + + // No object changes are made below this point XRPL_ASSERT_PARTS( - accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore == - accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter, + Number::getround() == Number::RoundingMode::ToNearest, "xrpl::LoanPay::doApply", - "funds are conserved (with rounding)"); + "Number rounding ToNearest"); + NumberRoundModeGuard const mg(Number::RoundingMode::ToNearest); + + auto const accountBalanceBeforeRounded = roundToScale(accountBalanceBefore, balanceScale); + auto const vaultBalanceBeforeRounded = roundToScale(vaultBalanceBefore, balanceScale); + auto const brokerBalanceBeforeRounded = roundToScale(brokerBalanceBefore, balanceScale); + + auto const totalBalanceBefore = accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore; + auto const totalBalanceBeforeRounded = roundToScale(totalBalanceBefore, balanceScale); + + JLOG(j_.trace()) << "Before: " // + << "account " << Number(accountBalanceBeforeRounded) << " (" + << Number(accountBalanceBefore) << ")" + << ", vault " << Number(vaultBalanceBeforeRounded) << " (" + << Number(vaultBalanceBefore) << ")" + << ", broker " << Number(brokerBalanceBeforeRounded) << " (" + << Number(brokerBalanceBefore) << ")" + << ", total " << Number(totalBalanceBeforeRounded) << " (" + << Number(totalBalanceBefore) << ")"; + + auto const accountBalanceAfterRounded = roundToScale(accountBalanceAfter, balanceScale); + auto const vaultBalanceAfterRounded = roundToScale(vaultBalanceAfter, balanceScale); + auto const brokerBalanceAfterRounded = roundToScale(brokerBalanceAfter, balanceScale); + + auto const totalBalanceAfter = accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter; + auto const totalBalanceAfterRounded = roundToScale(totalBalanceAfter, balanceScale); + + JLOG(j_.trace()) << "After: " // + << "account " << Number(accountBalanceAfterRounded) << " (" + << Number(accountBalanceAfter) << ")" + << ", vault " << Number(vaultBalanceAfterRounded) << " (" + << Number(vaultBalanceAfter) << ")" + << ", broker " << Number(brokerBalanceAfterRounded) << " (" + << Number(brokerBalanceAfter) << ")" + << ", total " << Number(totalBalanceAfterRounded) << " (" + << Number(totalBalanceAfter) << ")"; + + auto const accountBalanceChange = accountBalanceAfter - accountBalanceBefore; + auto const vaultBalanceChange = vaultBalanceAfter - vaultBalanceBefore; + auto const brokerBalanceChange = brokerBalanceAfter - brokerBalanceBefore; + + auto const totalBalanceChange = accountBalanceChange + vaultBalanceChange + brokerBalanceChange; + auto const totalBalanceChangeRounded = roundToScale(totalBalanceChange, balanceScale); + + JLOG(j_.trace()) << "Changes: " // + << "account " << to_string(accountBalanceChange) // + << ", vault " << to_string(vaultBalanceChange) // + << ", broker " << to_string(brokerBalanceChange) // + << ", total " << to_string(totalBalanceChangeRounded) << " (" + << Number(totalBalanceChange) << ")"; + + bool const goodRounding = totalBalanceBeforeRounded == totalBalanceAfterRounded || + totalBalanceChangeRounded == beast::kZERO; + if (totalBalanceBeforeRounded != totalBalanceAfterRounded) + { + JLOG((goodRounding ? j_.debug() : j_.warn())) + << "Total rounded balances don't match" + << (totalBalanceChangeRounded == beast::kZERO ? ", but total changes do" : ""); + } + if (totalBalanceChangeRounded != beast::kZERO) + { + JLOG((goodRounding ? j_.debug() : j_.warn())) + << "Total balance changes don't match" + << (totalBalanceBeforeRounded == totalBalanceAfterRounded ? ", but total balances do" + : ""); + } + + // Rounding for IOUs can be weird, so check a few different ways to show + // that funds are conserved. XRPL_ASSERT_PARTS( - accountBalanceAfter >= beast::kZERO, "xrpl::LoanPay::doApply", "positive account balance"); + goodRounding, "xrpl::LoanPay::doApply", "funds are conserved (with rounding)"); + XRPL_ASSERT_PARTS( accountBalanceAfter < accountBalanceBefore || account_ == asset.getIssuer(), "xrpl::LoanPay::doApply", @@ -661,7 +819,6 @@ LoanPay::doApply() vaultBalanceAfter > vaultBalanceBefore || brokerBalanceAfter > brokerBalanceBefore, "xrpl::LoanPay::doApply", "vault and/or broker balance increased"); -#endif return tesSUCCESS; } diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 09b16d7b67..c9f0a04a18 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -94,7 +94,7 @@ class Invariants_test : public beast::unit_test::Suite static FeatureBitset defaultAmendments() { - return xrpl::test::jtx::testableAmendments() | featureInvariantsV1_1 | fixSecurity3_1_3; + return xrpl::test::jtx::testableAmendments() | fixSecurity3_1_3 | fixCleanup3_2_0; } /** Run a specific test case to put the ledger into a state that will be @@ -3401,7 +3401,7 @@ class Invariants_test : public beast::unit_test::Suite sleShares->at(sfFlags) = 0; // Setting wrong pseudo account ID - sleShares->at(sfIssuer) = AccountID(uint160(42)); + sleShares->at(sfIssuer) = AccountID(42); sleShares->at(sfOutstandingAmount) = 0; sleShares->at(sfSequence) = sequence; diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 67780414c0..70997b1dcd 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -370,16 +370,11 @@ protected: env.balance(vaultPseudo, broker.asset).number()); if (ownerCount == 0) { - // Allow some slop for rounding IOUs - - // TODO: This needs to be an exact match once all the - // other rounding issues are worked out. + // The Vault must be perfectly balanced if there + // are no loans outstanding auto const total = vaultSle->at(sfAssetsTotal); auto const available = vaultSle->at(sfAssetsAvailable); - env.test.BEAST_EXPECT( - total == available || - (!broker.asset.integral() && available != 0 && - ((total - available) / available < Number(1, -6)))); + env.test.BEAST_EXPECT(total == available); env.test.BEAST_EXPECT(vaultSle->at(sfLossUnrealized) == 0); } } @@ -7177,6 +7172,144 @@ protected: BEAST_EXPECT(afterSecondCoverAvailable == 0); } + void + testYieldTheftRounding(std::uint32_t flags) + { + testcase("Rounding manipulation does not permit yield theft"); + using namespace jtx; + using namespace loan; + + // 1. Setup Environment + Env env(*this, all_); + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(1000), issuer, lender, borrower); + env.close(); + + // 2. Asset Selection + PrettyAsset const iou = issuer["USD"]; + env(trust(lender, iou(100'000'000))); + env(trust(borrower, iou(100'000'000))); + env(pay(issuer, lender, iou(100'000'000))); + env(pay(issuer, borrower, iou(100'000'000))); + env.close(); + + // 3. Create Vault and Broker with High Debt Limit (100M) + auto const brokerInfo = createVaultAndBroker( + env, + iou, + lender, + { + .vaultDeposit = 5'000'000, + .debtMax = Number{100'000'000}, + .coverDeposit = 500'000, + }); + auto const [currentSeq, vaultKeylet] = [&]() { + auto const brokerSle = env.le(keylet::loanbroker(brokerInfo.brokerID)); + if (!BEAST_EXPECT(brokerSle)) + return std::make_tuple(0u, keylet::unchecked(beast::kZERO)); + auto const currentSeq = brokerSle->at(sfLoanSequence); + auto const vaultKeylet = keylet::vault(brokerSle->at(sfVaultID)); + return std::make_tuple(currentSeq, vaultKeylet); + }(); + + // 4. Loan Parameters (Attack Vector) + Number const principal = 1'000'000; + TenthBips32 const interestRate = TenthBips32{1}; // 0.001% + std::uint32_t const paymentInterval = 86400; + std::uint32_t const paymentTotal = 3650; + + auto const loanSetFee = Fee(env.current()->fees().base * 2); + env(set(borrower, brokerInfo.brokerID, iou(principal).value(), flags), + Sig(sfCounterpartySignature, lender), + loan::kINTEREST_RATE(interestRate), + loan::kPAYMENT_INTERVAL(paymentInterval), + loan::kPAYMENT_TOTAL(paymentTotal), + Fee(loanSetFee)); + env.close(); + + // --- RETRIEVE OBJECTS & SETUP ATTACK --- + + auto borrowerBalance = [&]() { return env.balance(borrower, iou); }; + auto const borrowerScale = static_cast(borrowerBalance()).exponent(); + + auto const loanKeylet = keylet::loan(brokerInfo.brokerID, currentSeq); + auto const maybePeriodicPayment = [&]() -> std::optional { + auto const loanSle = env.le(loanKeylet); + if (!BEAST_EXPECT(loanSle)) + return std::nullopt; + // Construct Payment + return STAmount{iou, loanSle->at(sfPeriodicPayment)}; + }(); + if (!maybePeriodicPayment) + return; + auto const periodicPayment = *maybePeriodicPayment; + auto const roundedPayment = + roundToScale(periodicPayment, borrowerScale, Number::RoundingMode::Upward); + + // ATTACK: Add dust buffer (1e-9) to force 'excess' logic execution + STAmount const paymentBuffer{iou, Number(1, -9)}; + STAmount const attackPayment = periodicPayment + paymentBuffer; + + auto const maybeInitialVaultAssets = [&]() -> std::optional { + auto const vault = env.le(vaultKeylet); + if (!BEAST_EXPECT(vault)) + return std::nullopt; + return vault->at(sfAssetsTotal); + }(); + if (!maybeInitialVaultAssets) + return; + auto const initialVaultAssets = *maybeInitialVaultAssets; + + // 5. Execution Loop + int yieldTheftCount = 0; + auto previousAssetsTotal = initialVaultAssets; + + for (int i = 0; i < 100; ++i) + { + auto const balanceBefore = borrowerBalance(); + env(pay(borrower, loanKeylet.key, attackPayment, flags)); + env.close(); + auto const borrowerDelta = balanceBefore - borrowerBalance(); + BEAST_EXPECT(borrowerDelta.signum() == roundedPayment.signum()); + + auto const loanSle = env.le(loanKeylet); + if (!BEAST_EXPECT(loanSle)) + break; + auto const updatedPayment = STAmount{iou, loanSle->at(sfPeriodicPayment)}; + BEAST_EXPECT( + (roundToScale(updatedPayment, borrowerScale, Number::RoundingMode::Upward) == + roundedPayment)); + BEAST_EXPECT( + (updatedPayment == periodicPayment) || + (flags == tfLoanOverpayment && i >= 2 && updatedPayment < periodicPayment)); + + auto const currentVaultSle = env.le(vaultKeylet); + if (!BEAST_EXPECT(currentVaultSle)) + break; + + auto const currentAssetsTotal = currentVaultSle->at(sfAssetsTotal); + auto const delta = currentAssetsTotal - previousAssetsTotal; + + BEAST_EXPECT( + (delta == beast::kZERO && borrowerDelta <= roundedPayment) || + (delta > beast::kZERO && borrowerDelta > roundedPayment)); + + // If tx succeeded but Assets Total didn't change, interest was + // stolen. + if (delta == beast::kZERO && borrowerDelta > roundedPayment) + { + yieldTheftCount++; + } + + previousAssetsTotal = currentAssetsTotal; + } + + BEAST_EXPECTS(yieldTheftCount == 0, std::to_string(yieldTheftCount)); + } + // Tests that vault withdrawals work correctly when the vault has unrealized // loss from an impaired loan, ensuring the invariant check properly // accounts for the loss. @@ -7497,6 +7630,10 @@ public: testLoanPayLateFullPaymentBypassesPenalties(); testLoanCoverMinimumRoundingExploit(); #endif + for (auto const flags : {0u, tfLoanOverpayment}) + { + testYieldTheftRounding(flags); + } testBugInterestDueDeltaCrash(); testFullLifecycleVaultPnLNearZeroRate(); diff --git a/src/test/basics/base_uint_test.cpp b/src/test/basics/base_uint_test.cpp index 8d5b56d480..603536bbe0 100644 --- a/src/test/basics/base_uint_test.cpp +++ b/src/test/basics/base_uint_test.cpp @@ -134,7 +134,7 @@ struct base_uint_test : beast::unit_test::Suite Blob const raw{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}; BEAST_EXPECT(test96::kBYTES == raw.size()); - test96 u{raw}; + test96 u = test96::fromRaw(raw); uset.insert(u); BEAST_EXPECT(raw.size() == u.size()); BEAST_EXPECT(to_string(u) == "0102030405060708090A0B0C"); @@ -155,7 +155,7 @@ struct base_uint_test : beast::unit_test::Suite // back into another base_uint (w) for comparison with the original Nonhash<96> h{}; hash_append(h, u); - test96 const w{std::vector(h.data.begin(), h.data.end())}; + test96 const w = test96::fromRaw(std::vector(h.data.begin(), h.data.end())); BEAST_EXPECT(w == u); test96 v{~u}; diff --git a/src/test/protocol/STParsedJSON_test.cpp b/src/test/protocol/STParsedJSON_test.cpp index 6b863644a3..bc5c72b4fc 100644 --- a/src/test/protocol/STParsedJSON_test.cpp +++ b/src/test/protocol/STParsedJSON_test.cpp @@ -393,7 +393,7 @@ class STParsedJSON_test : public beast::unit_test::Suite 0xCD, 0xEF}; // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - BEAST_EXPECT(obj.object->getFieldH128(sfEmailHash) == uint128{expected}); + BEAST_EXPECT(obj.object->getFieldH128(sfEmailHash) == uint128::fromRaw(expected)); } // Valid lowercase hex string for UInt128 @@ -488,8 +488,9 @@ class STParsedJSON_test : public beast::unit_test::Suite std::array const expected = {0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF, 0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF, 0x01, 0x23, 0x45, 0x67}; - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - BEAST_EXPECT(obj.object->getFieldH160(sfTakerPaysCurrency) == uint160{expected}); + BEAST_EXPECT( + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) + obj.object->getFieldH160(sfTakerPaysCurrency) == uint160::fromRaw(expected)); } // Valid lowercase hex string for UInt160 { @@ -575,8 +576,9 @@ class STParsedJSON_test : public beast::unit_test::Suite std::array const expected = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}; - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - BEAST_EXPECT(obj.object->getFieldH192(sfMPTokenIssuanceID) == uint192{expected}); + BEAST_EXPECT( + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) + obj.object->getFieldH192(sfMPTokenIssuanceID) == uint192::fromRaw(expected)); } // Valid lowercase hex string for UInt192 @@ -676,7 +678,7 @@ class STParsedJSON_test : public beast::unit_test::Suite 0x67, 0x89, 0xAB, 0xCD, 0xEF, 0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF, 0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF}; // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - BEAST_EXPECT(obj.object->getFieldH256(sfLedgerHash) == uint256{expected}); + BEAST_EXPECT(obj.object->getFieldH256(sfLedgerHash) == uint256::fromRaw(expected)); } // Valid lowercase hex string for UInt256 { diff --git a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp index 19dba2e893..07738d99f4 100644 --- a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp +++ b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp @@ -56,8 +56,8 @@ LedgerReplayMsgHandler::processProofPathRequest( reply.set_ledgerhash(packet.ledgerhash()); reply.set_type(packet.type()); - uint256 const key(packet.key()); - uint256 const ledgerHash(packet.ledgerhash()); + uint256 const key = uint256::fromRaw(packet.key()); + uint256 const ledgerHash = uint256::fromRaw(packet.ledgerhash()); auto ledger = app_.getLedgerMaster().getLedgerByHash(ledgerHash); if (!ledger) { @@ -107,7 +107,8 @@ LedgerReplayMsgHandler::processProofPathResponse( { protocol::TMProofPathResponse const& reply = *msg; if (reply.has_error() || !reply.has_key() || !reply.has_ledgerhash() || !reply.has_type() || - !reply.has_ledgerheader() || reply.path_size() == 0) + !reply.has_ledgerheader() || reply.path_size() == 0 || + reply.ledgerhash().size() != uint256::size() || reply.key().size() != uint256::size()) { JLOG(journal_.debug()) << "Bad message: Error reply"; return false; @@ -121,7 +122,7 @@ LedgerReplayMsgHandler::processProofPathResponse( // deserialize the header auto info = deserializeHeader({reply.ledgerheader().data(), reply.ledgerheader().size()}); - uint256 const replyHash(reply.ledgerhash()); + uint256 const replyHash = uint256::fromRaw(reply.ledgerhash()); if (calculateLedgerHash(info) != replyHash) { JLOG(journal_.debug()) << "Bad message: Hash mismatch"; @@ -129,7 +130,7 @@ LedgerReplayMsgHandler::processProofPathResponse( } info.hash = replyHash; - uint256 const key(reply.key()); + uint256 const key = uint256::fromRaw(reply.key()); if (key != keylet::skip().key) { JLOG(journal_.debug()) << "Bad message: we only support the short skip list for now. " @@ -185,7 +186,7 @@ LedgerReplayMsgHandler::processReplayDeltaRequest( } reply.set_ledgerhash(packet.ledgerhash()); - uint256 const ledgerHash{packet.ledgerhash()}; + uint256 const ledgerHash = uint256::fromRaw(packet.ledgerhash()); auto ledger = app_.getLedgerMaster().getLedgerByHash(ledgerHash); if (!ledger || !ledger->isImmutable()) { @@ -214,14 +215,15 @@ LedgerReplayMsgHandler::processReplayDeltaResponse( std::shared_ptr const& msg) { protocol::TMReplayDeltaResponse const& reply = *msg; - if (reply.has_error() || !reply.has_ledgerheader()) + if (reply.has_error() || !reply.has_ledgerheader() || !reply.has_ledgerhash() || + reply.ledgerhash().size() != uint256::size()) { JLOG(journal_.debug()) << "Bad message: Error reply"; return false; } auto info = deserializeHeader({reply.ledgerheader().data(), reply.ledgerheader().size()}); - uint256 const replyHash(reply.ledgerhash()); + uint256 const replyHash = uint256::fromRaw(reply.ledgerhash()); if (calculateLedgerHash(info) != replyHash) { JLOG(journal_.debug()) << "Bad message: Hash mismatch"; diff --git a/src/xrpld/app/ledger/detail/LedgerToJson.cpp b/src/xrpld/app/ledger/detail/LedgerToJson.cpp index 10a3e5c0bd..7c03e22580 100644 --- a/src/xrpld/app/ledger/detail/LedgerToJson.cpp +++ b/src/xrpld/app/ledger/detail/LedgerToJson.cpp @@ -297,13 +297,24 @@ fillJsonQueue(json::Value& json, LedgerFill const& fill) txJson["last_result"] = transToken(*tx.lastResult); auto&& temp = fillJsonTx(fill, bBinary, bExpanded, tx.txn, nullptr); - if (fill.context->apiVersion > 1) + if (temp.isObject()) { - copyFrom(txJson, temp); + if (fill.context->apiVersion > 1) + { + copyFrom(txJson, temp); + } + else + { + copyFrom(txJson[jss::tx], temp); + } + } + else if (fill.context->apiVersion > 1) + { + txJson[jss::hash] = temp; } else { - copyFrom(txJson[jss::tx], temp); + txJson[jss::tx] = temp; } } } diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 3e572a297f..7d1dd27528 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -212,7 +212,7 @@ PeerImp::run() return ret; if (auto const s = base64Decode(value); s.size() == uint256::size()) - return uint256{s}; + return uint256::fromRaw(s); return std::nullopt; }; @@ -1831,7 +1831,7 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } - uint256 const ledgerHash{m->ledgerhash()}; + uint256 const ledgerHash = uint256::fromRaw(m->ledgerhash()); // Otherwise check if received data for a candidate transaction set if (m->type() == protocol::liTS_CANDIDATE) @@ -1893,8 +1893,8 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } - uint256 const proposeHash{set.currenttxhash()}; - uint256 const prevLedger{set.previousledger()}; + uint256 const proposeHash = uint256::fromRaw(set.currenttxhash()); + uint256 const prevLedger = uint256::fromRaw(set.previousledger()); NetClock::time_point const closeTime{NetClock::duration{set.closetime()}}; @@ -2177,7 +2177,7 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } - uint256 const hash{m->hash()}; + uint256 const hash = uint256::fromRaw(m->hash()); if (m->status() == protocol::tsHAVE) { @@ -2617,7 +2617,7 @@ PeerImp::onMessage(std::shared_ptr const& m) auto const& obj = packet.objects(i); if (obj.has_hash() && stringIsUInt256Sized(obj.hash())) { - uint256 const hash{obj.hash()}; + uint256 const hash = uint256::fromRaw(obj.hash()); // VFALCO TODO Move this someplace more sensible so we dont // need to inject the NodeStore interfaces. std::uint32_t const seq{obj.has_ledgerseq() ? obj.ledgerseq() : 0}; @@ -2687,7 +2687,7 @@ PeerImp::onMessage(std::shared_ptr const& m) if (pLDo) { - uint256 const hash{obj.hash()}; + uint256 const hash = uint256::fromRaw(obj.hash()); app_.getLedgerMaster().addFetchPack( hash, std::make_shared(obj.data().begin(), obj.data().end())); @@ -2739,7 +2739,7 @@ PeerImp::handleHaveTransactions(std::shared_ptr co return; } - uint256 hash(m->hashes(i)); + uint256 hash = uint256::fromRaw(m->hashes(i)); auto txn = app_.getMasterTransaction().fetchFromCache(hash); @@ -2873,7 +2873,7 @@ PeerImp::doFetchPack(std::shared_ptr const& packet) fee_.fee = Resource::kFEE_HEAVY_BURDEN_PEER; - uint256 const hash{packet->ledgerhash()}; + uint256 const hash = uint256::fromRaw(packet->ledgerhash()); std::weak_ptr const weak = shared_from_this(); auto elapsed = UptimeClock::now(); @@ -2908,7 +2908,7 @@ PeerImp::doTransactions(std::shared_ptr const& pack return; } - uint256 hash(obj.hash()); + uint256 hash = uint256::fromRaw(obj.hash()); auto txn = app_.getMasterTransaction().fetchFromCache(hash); @@ -3254,7 +3254,7 @@ PeerImp::getLedger(std::shared_ptr const& m) if (m->has_ledgerhash()) { // Attempt to find ledger by hash - uint256 const ledgerHash{m->ledgerhash()}; + uint256 const ledgerHash = uint256::fromRaw(m->ledgerhash()); ledger = app_.getLedgerMaster().getLedgerByHash(ledgerHash); if (!ledger) { @@ -3333,7 +3333,7 @@ PeerImp::getTxSet(std::shared_ptr const& m) const { JLOG(pJournal_.trace()) << "getTxSet: TX set"; - uint256 const txSetHash{m->ledgerhash()}; + uint256 const txSetHash = uint256::fromRaw(m->ledgerhash()); std::shared_ptr shaMap{app_.getInboundTransactions().getSet(txSetHash, false)}; if (!shaMap) {