diff --git a/include/xrpl/tx/applySteps.h b/include/xrpl/tx/applySteps.h index bd495481f2..0a1ae9fa3a 100644 --- a/include/xrpl/tx/applySteps.h +++ b/include/xrpl/tx/applySteps.h @@ -12,6 +12,7 @@ #include #include +#include #include #include @@ -393,16 +394,21 @@ preclaim(PreflightResult const& preflightResult, ServiceRegistry& registry, Open * * No validation is done or implied by this function. * - * Caller is responsible for handling any exceptions. - * Since none should be thrown, that will usually - * mean terminating. - * + * Callers do not expect this function to throw; exceptions from a transactor's + * `calculateBaseFee` are caught and reported as an error instead. * @param view The current open ledger. * @param tx The transaction to be checked. * - * @return The base fee. + * @return The base fee on success. Returns `std::unexpected(temUNKNOWN)` if the transaction + * type is not recognized, and `std::unexpected(tefEXCEPTION)` if the transactor's + * `calculateBaseFee` threw. + * + * @note Failure is reported as an error rather than a fee of zero because a + * zero (or default) fee would pass checkFee and let the transaction be + * applied for less than it owes. Callers that only need a fee hint may fall + * back to a default; callers deciding whether to apply should reject. */ -XRPAmount +[[nodiscard]] std::expected calculateBaseFee(ReadView const& view, STTx const& tx); /** diff --git a/src/libxrpl/tx/applySteps.cpp b/src/libxrpl/tx/applySteps.cpp index 2c05c874d3..00ac9f9983 100644 --- a/src/libxrpl/tx/applySteps.cpp +++ b/src/libxrpl/tx/applySteps.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -195,7 +196,12 @@ invokePreclaim(PreclaimContext const& ctx) }()) return preSigResult; - if (TER const result = T::checkFee(ctx, calculateBaseFee(ctx.view, ctx.tx))) + // We can't check the fee if we can't compute it, so reject. + auto const baseFee = calculateBaseFee(ctx.view, ctx.tx); + if (!baseFee) + return baseFee.error(); + + if (TER const result = T::checkFee(ctx, *baseFee)) return result; } @@ -223,13 +229,12 @@ invokePreclaim(PreclaimContext const& ctx) * * @param view The ledger view to use for fee calculation. * @param tx The transaction for which the base fee is to be calculated. - * @return The calculated base fee as an XRPAmount. + * @return The calculated base fee. Returns `std::unexpected(temUNKNOWN)` if the transaction + * type is not recognized, and `std::unexpected(tefEXCEPTION)` if the transactor's + * `calculateBaseFee` threw. * - * @throws std::exception If an error occurs during fee calculation, including - * but not limited to unknown transaction types or internal errors, the function - * logs an error and returns an XRPAmount of zero. */ -static XRPAmount +static std::expected invokeCalculateBaseFee(ReadView const& view, STTx const& tx) { try @@ -238,13 +243,25 @@ invokeCalculateBaseFee(ReadView const& view, STTx const& tx) return T::calculateBaseFee(view, tx); }); } - catch (UnknownTxnType const& e) + catch (UnknownTxnType const&) { // LCOV_EXCL_START UNREACHABLE("xrpl::invoke_calculateBaseFee : unknown transaction type"); - return XRPAmount{0}; + return std::unexpected(temUNKNOWN); // LCOV_EXCL_STOP } + catch (std::exception const& e) + { + JLOG(debugLog().error()) << "calculateBaseFee: " << tx.getTransactionID() + << " threw an exception: " << e.what(); + return std::unexpected(tefEXCEPTION); + } + catch (...) + { + JLOG(debugLog().error()) << "calculateBaseFee: " << tx.getTransactionID() + << " threw an unknown exception"; + return std::unexpected(tefEXCEPTION); + } } TxConsequences::TxConsequences(NotTEC pfResult) @@ -416,7 +433,7 @@ preclaim(PreflightResult const& preflightResult, ServiceRegistry& registry, Open } } -XRPAmount +std::expected calculateBaseFee(ReadView const& view, STTx const& tx) { return invokeCalculateBaseFee(view, tx); @@ -441,13 +458,26 @@ doApply(PreclaimResult const& preclaimResult, ServiceRegistry& registry, OpenVie { if (!preclaimResult.likelyToClaimFee) return {preclaimResult.ter, false}; + + // For any tx with a real account, preclaim already computed this fee + // successfully against this same view. + auto const baseFee = calculateBaseFee(view, preclaimResult.tx); + if (!baseFee) + { + // LCOV_EXCL_START + JLOG(preclaimResult.j.error()) + << "apply: could not compute base fee: " << transToken(baseFee.error()); + return {tefINTERNAL, false}; + // LCOV_EXCL_STOP + } + ApplyContext ctx( registry, view, preclaimResult.parentBatchId, preclaimResult.tx, preclaimResult.ter, - calculateBaseFee(view, preclaimResult.tx), + *baseFee, preclaimResult.flags, preclaimResult.j); return invokeApply(ctx); diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index 18886b2682..624c4d0a84 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -36,6 +36,15 @@ namespace xrpl { namespace { +// Returns true if the transaction's payment amount is malformed. A loan +// payment must be strictly positive: zero would move nothing, and a negative +// amount is not a payment at all. +bool +isPaymentAmountInvalid(STAmount const& amount) +{ + return amount <= beast::kZero; +} + // Returns the account's true, unclamped balance in `asset`, for use only in // fund-conservation checks. accountHolds(..., SpendableHandling::FullBalance) // cannot be used for this: for XRP it always defers to xrpLiquid, which @@ -81,7 +90,7 @@ LoanPay::preflight(PreflightContext const& ctx) if (ctx.tx[sfLoanID] == beast::kZero) return temINVALID; - if (ctx.tx[sfAmount] <= beast::kZero) + if (isPaymentAmountInvalid(ctx.tx[sfAmount])) return temBAD_AMOUNT; // The loan payment flags are all mutually exclusive. If more than one is @@ -103,10 +112,19 @@ LoanPay::preflight(PreflightContext const& ctx) XRPAmount LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) { + auto fixEnabled313 = view.rules().enabled(fixCleanup3_1_3); + auto fixEnabled340 = view.rules().enabled(fixCleanup3_4_0); + using namespace lending; auto const normalCost = Transactor::calculateBaseFee(view, tx); + if (fixEnabled340 && isPaymentAmountInvalid(tx[sfAmount])) + { + // Let preflight worry about the error for this + return normalCost; + } + if (tx.isFlag(tfLoanFullPayment) || tx.isFlag(tfLoanLatePayment)) { // The loan will be making one set of calculations for one full or late @@ -179,8 +197,7 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) static constexpr std::int64_t kMaxFeeIncrements = kLoanMaximumPaymentsPerTransaction / kLoanPaymentsPerFeeIncrement; - if (view.rules().enabled(fixCleanup3_1_3) && - amount >= regularPayment * kLoanMaximumPaymentsPerTransaction) + if (fixEnabled313 && amount >= regularPayment * kLoanMaximumPaymentsPerTransaction) { // The payment handler will never process more than // loanMaximumPaymentsPerTransaction payments (including overpayments), diff --git a/src/libxrpl/tx/transactors/system/Batch.cpp b/src/libxrpl/tx/transactors/system/Batch.cpp index ccb113e07b..dcd06453aa 100644 --- a/src/libxrpl/tx/transactors/system/Batch.cpp +++ b/src/libxrpl/tx/transactors/system/Batch.cpp @@ -73,14 +73,23 @@ Batch::calculateBaseFeeImpl(ReadView const& view, STTx const& tx) for (auto const& stx : tx.getBatchTransactions()) { auto const fee = xrpl::calculateBaseFee(view, *stx); - // LCOV_EXCL_START - if (txnFees > maxAmount - fee) + if (!fee) { + JLOG(debugLog().error()) + << "BatchTrace: base fee of inner transaction " << stx->getTransactionID() + << " could not be computed: " << transToken(fee.error()); + return std::nullopt; + } + + // LCOV_EXCL_START + if (txnFees > maxAmount - *fee) + { + UNREACHABLE("XRPAmount overflow in txnFees calculation"); JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in txnFees calculation."; return std::nullopt; } // LCOV_EXCL_STOP - txnFees += fee; + txnFees += *fee; } // Calculate the Signers/BatchSigners Fees diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index a440c9ad1c..0dc6c8dcac 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1886,11 +1886,21 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) if (validatedLedgerIndex) { - auto [fee, accountSeq, availableSeq] = - registry_.get().getTxQ().getTxRequiredFeeAndSeq( - *newOL, e.transaction->getSTransaction()); - e.transaction->setCurrentLedgerState( - *validatedLedgerIndex, fee, accountSeq, availableSeq); + auto maybeFeeAndSeq = registry_.get().getTxQ().getTxRequiredFeeAndSeq( + *newOL, e.transaction->getSTransaction()); + if (maybeFeeAndSeq.has_value()) + { + auto [fee, accountSeq, availableSeq] = *maybeFeeAndSeq; + e.transaction->setCurrentLedgerState( + *validatedLedgerIndex, fee, accountSeq, availableSeq); + } + else + { + JLOG(journal_.debug()) + << "Unable to compute current ledger state for tx " + << e.transaction->getID() << " in validated ledger " + << *validatedLedgerIndex << ": " << transToken(maybeFeeAndSeq.error()); + } } } } diff --git a/src/xrpld/app/misc/TxQ.h b/src/xrpld/app/misc/TxQ.h index 65b4e9778c..4e6e9edbac 100644 --- a/src/xrpld/app/misc/TxQ.h +++ b/src/xrpld/app/misc/TxQ.h @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -390,10 +391,10 @@ public: * and first available sequence for transaction * @param view current open ledger * @param tx the transaction - * @return minimum required fee, first sequence in the ledger + * @return minimum required fee or an error, first sequence in the ledger * and first available sequence */ - FeeAndSeq + std::expected getTxRequiredFeeAndSeq(OpenView const& view, std::shared_ptr const& tx) const; /** diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index b9cfc1d65c..19dc351444 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -54,22 +55,29 @@ namespace xrpl { ////////////////////////////////////////////////////////////////////////// -static FeeLevel64 +/** + * Compute the fee level that a transaction pays. + * @return The fee level paid, or the error reported by `calculateBaseFee`. + */ +static std::expected getFeeLevelPaid(ReadView const& view, STTx const& tx) { - auto const [baseFee, effectiveFeePaid] = [&view, &tx]() { - XRPAmount const baseFee = calculateBaseFee(view, tx); + auto const computedBaseFee = calculateBaseFee(view, tx); + if (!computedBaseFee) + return std::unexpected(computedBaseFee.error()); + + auto const [baseFee, effectiveFeePaid] = [&view, &tx, fee = *computedBaseFee]() { XRPAmount const feePaid = tx[sfFee].xrp(); // If baseFee is 0 then the cost of a basic transaction is free, but we // need the effective fee level to be non-zero. - XRPAmount const mod = [&view, &tx, baseFee]() { - if (baseFee.signum() > 0) + XRPAmount const mod = [&view, &tx, fee]() { + if (fee.signum() > 0) return XRPAmount{0}; auto def = calculateDefaultBaseFee(view, tx); return def.signum() == 0 ? XRPAmount{1} : def; }(); - return std::pair{baseFee + mod, feePaid + mod}; + return std::pair{fee + mod, feePaid + mod}; }(); XRPL_ASSERT(baseFee.signum() > 0, "xrpl::getFeeLevelPaid : positive fee"); @@ -112,10 +120,20 @@ TxQ::FeeMetrics::update( auto const size = std::distance(txBegin, txEnd); feeLevels.reserve(size); std::for_each(txBegin, txEnd, [&](auto const& tx) { - feeLevels.push_back(getFeeLevelPaid(view, *tx.first)); + auto const maybeFeeLevel = getFeeLevelPaid(view, *tx.first); + if (maybeFeeLevel.has_value()) + { + feeLevels.push_back(*maybeFeeLevel); + } + else + { + // Excluded from the median sample below. + JLOG(j_.warn()) << "Unable to compute the fee level for a validated transaction " + << tx.first->getTransactionID() << " in ledger " << view.header().seq + << ": " << transToken(maybeFeeLevel.error()); + } }); std::ranges::sort(feeLevels); - XRPL_ASSERT(size == feeLevels.size(), "xrpl::TxQ::FeeMetrics::update : fee levels size"); JLOG((timeLeap ? j_.warn() : j_.debug())) << "Ledger " << view.header().seq << " has " << size << " transactions. " @@ -159,7 +177,10 @@ TxQ::FeeMetrics::update( txnsExpected_ = std::min(next, maximumTxnCount_.value_or(next)); } - if (size == 0) + // The median is taken over the transactions whose fee level could be + // computed, while txnsExpected_ above deliberately uses the full + // transaction count. + if (feeLevels.empty()) { escalationMultiplier_ = setup.minimumEscalationMultiplier; } @@ -169,8 +190,9 @@ TxQ::FeeMetrics::update( // evaluates to the middle element; for an even // number of elements, it will add the two elements // on either side of the "middle" and average them. + auto const count = feeLevels.size(); escalationMultiplier_ = - (feeLevels[size / 2] + feeLevels[(size - 1) / 2] + FeeLevel64{1}) / 2; + (feeLevels[count / 2] + feeLevels[(count - 1) / 2] + FeeLevel64{1}) / 2; escalationMultiplier_ = std::max(escalationMultiplier_, setup.minimumEscalationMultiplier); } JLOG(j_.debug()) << "Expected transactions updated to " << txnsExpected_ @@ -878,7 +900,14 @@ TxQ::apply( // We may need the base fee for multiple transactions or transaction // replacement, so just pull it up now. auto const metricsSnapshot = feeMetrics_.getSnapshot(); - auto const feeLevelPaid = getFeeLevelPaid(view, *tx); + auto const computedFeeLevelPaid = getFeeLevelPaid(view, *tx); + // Without a fee level there is no way to tell whether the transaction + // pays enough, so it can be neither applied nor queued. + if (!computedFeeLevelPaid.has_value()) + { + return {computedFeeLevelPaid.error(), false}; + } + FeeLevel64 const feeLevelPaid = *computedFeeLevelPaid; auto const requiredFeeLevel = getRequiredFeeLevel(view, flags, metricsSnapshot, lock); // Is there a blocker already in the account's queue? If so, don't @@ -1684,7 +1713,14 @@ TxQ::tryDirectApply( // If the transaction's fee is high enough we may be able to put the // transaction straight into the ledger. - FeeLevel64 const feeLevelPaid = getFeeLevelPaid(view, *tx); + auto const computedFeeLevelPaid = getFeeLevelPaid(view, *tx); + // The fee level is unknown, so the transaction cannot be applied here, + // and queueing it would only run into the same failure. Reject it. + if (!computedFeeLevelPaid.has_value()) + { + return ApplyResult{computedFeeLevelPaid.error(), false}; + } + FeeLevel64 const feeLevelPaid = *computedFeeLevelPaid; if (feeLevelPaid >= requiredFeeLevel) { @@ -1768,7 +1804,7 @@ TxQ::getMetrics(OpenView const& view) const return result; } -TxQ::FeeAndSeq +std::expected TxQ::getTxRequiredFeeAndSeq(OpenView const& view, std::shared_ptr const& tx) const { auto const account = (*tx)[sfAccount]; @@ -1776,14 +1812,19 @@ TxQ::getTxRequiredFeeAndSeq(OpenView const& view, std::shared_ptr co std::scoped_lock const lock(mutex_); auto const snapshot = feeMetrics_.getSnapshot(); - auto const baseFee = calculateBaseFee(view, *tx); + auto const maybeBaseFee = calculateBaseFee(view, *tx); + if (!maybeBaseFee.has_value()) + { + return std::unexpected(maybeBaseFee.error()); + } + auto const baseFee = *maybeBaseFee; auto const fee = FeeMetrics::scaleFeeLevel(snapshot, view); auto const sle = view.read(keylet::account(account)); std::uint32_t const accountSeq = sle ? (*sle)[sfSequence] : 0; std::uint32_t const availableSeq = nextQueuableSeqImpl(sle, lock).value(); - return { + return FeeAndSeq{ .fee = mulDiv(fee, baseFee, kBaseLevel) .value_or(XRPAmount(std::numeric_limits::max())), .accountSeq = accountSeq, diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index 3e9f62214e..6ea879aa2c 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -887,7 +887,10 @@ getTxFee(Application const& app, Config const& config, json::Value tx) if (!passesLocalChecks(stTx, reason)) return config.fees.referenceFee; - return calculateBaseFee(*app.getOpenLedger().current(), stTx); + // This fee is only a suggestion returned to the caller, so fall back to + // the reference fee as the other failure paths in this function do. + return calculateBaseFee(*app.getOpenLedger().current(), stTx) + .value_or(config.fees.referenceFee); } catch (std::exception& e) {