From 43d842926a8c7a154062a90d389e526be3e89d2e Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 14 Aug 2026 20:18:33 +0000 Subject: [PATCH] refactor: Rewrite Transactor::operator() to early return (#8003) --- src/libxrpl/tx/Transactor.cpp | 136 ++++++++++++++++++---------------- 1 file changed, 72 insertions(+), 64 deletions(-) diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 5fc6942e20..594aa24940 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -46,6 +46,7 @@ #include #include #include +#include #include #include #include @@ -1637,85 +1638,92 @@ Transactor::operator()() if (auto stream = j_.trace()) stream << "preclaim result: " << transToken(result); - bool applied = isTesSuccess(result); auto fee = ctx_.tx.getFieldAmount(sfFee).xrp(); + bool const canApply = std::invoke([&result, &fee, this] { + bool canApplyTmp = isTesSuccess(result); - if (ctx_.size() > kOversizeMetaDataCap) - result = tecOVERSIZE; + if (ctx_.size() > kOversizeMetaDataCap) + result = tecOVERSIZE; - if (isTecClaim(result) && ((view().flags() & TapFailHard) != 0u)) - { - // If the TapFailHard flag is set, a tec result - // must not do anything - ctx_.discard(); - applied = false; - } - else if ( - (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) || - (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags()))) - { - std::tie(result, fee, applied) = processPersistentChanges(result, fee); - } - - if (applied) - { - // Check invariants: if `tecINVARIANT_FAILED` is not returned, we can - // proceed to apply the tx - result = checkInvariants(result, fee); - if (result == tecINVARIANT_FAILED) + if (isTecClaim(result) && ((view().flags() & TapFailHard) != 0u)) { - // Reset to fee-claim only - auto const resetResult = reset(fee); - if (!isTesSuccess(resetResult.first)) - result = resetResult.first; - - fee = resetResult.second; - - // Check invariants again to ensure the fee claiming doesn't violate - // invariants. After reset, only protocol invariants are re-checked. - // Transaction invariants are not meaningful here — the transaction's - // effects have been rolled back. - if (isTesSuccess(result) || isTecClaim(result)) - result = ctx_.checkInvariants(result, fee); + // If the TapFailHard flag is set, a tec result + // must not do anything + ctx_.discard(); + canApplyTmp = false; } + else if ( + (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) || + (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags()))) + { + // This is and must remain the only place where `canApplyTmp` can change from false to + // true. Changing from true to false is no problem. + std::tie(result, fee, canApplyTmp) = processPersistentChanges(result, fee); + } + return canApplyTmp; + }); - // We ran through the invariant checker, which can, in some cases, - // return a tef error code. Don't apply the transaction in that case. - if (!isTecClaim(result) && !isTesSuccess(result)) - applied = false; + auto const logger = [this]( + TER result, + bool canApply, + std::optional&& metadata = std::nullopt) -> ApplyResult { + JLOG(j_.trace()) << (canApply ? "applied " : "not applied ") << transToken(result); + return {result, canApply, std::move(metadata)}; + }; + + if (!canApply) + return logger(result, canApply); + + // Check invariants: if `tecINVARIANT_FAILED` is not returned, we can + // proceed to apply the tx + result = checkInvariants(result, fee); + if (result == tecINVARIANT_FAILED) + { + // Reset to fee-claim only + auto const resetResult = reset(fee); + if (!isTesSuccess(resetResult.first)) + result = resetResult.first; + + fee = resetResult.second; + + // Check invariants again to ensure the fee claiming doesn't violate + // invariants. After reset, only protocol invariants are re-checked. + // Transaction invariants are not meaningful here — the transaction's + // effects have been rolled back. + if (isTesSuccess(result) || isTecClaim(result)) + result = ctx_.checkInvariants(result, fee); } + // We ran through the invariant checker, which can, in some cases, + // return a tef error code. Don't apply the transaction in that case. + if (!isTecClaim(result) && !isTesSuccess(result)) + return logger(result, false); + std::optional metadata; - if (applied) - { - // Transaction succeeded fully or (retries are not allowed and the - // transaction could claim a fee) - // The transactor and invariant checkers guarantee that this will - // *never* trigger but if it, somehow, happens, don't allow a tx - // that charges a negative fee. - if (fee < beast::kZero) - Throw("fee charged is negative!"); + // Transaction succeeded fully or (retries are not allowed and the + // transaction could claim a fee) - // Charge whatever fee they specified. The fee has already been - // deducted from the balance of the account that issued the - // transaction. We just need to account for it in the ledger - // header. - if (!view().open() && fee != beast::kZero) - ctx_.destroyXRP(fee); + // The transactor and invariant checkers guarantee that this will + // *never* trigger but if it, somehow, happens, don't allow a tx + // that charges a negative fee. + if (fee < beast::kZero) + Throw("fee charged is negative!"); - // Once we call apply, we will no longer be able to look at view() - metadata = ctx_.apply(result); - } + // Charge whatever fee they specified. The fee has already been + // deducted from the balance of the account that issued the + // transaction. We just need to account for it in the ledger + // header. + if (!view().open() && fee != beast::kZero) + ctx_.destroyXRP(fee); + + // Once we call apply, we will no longer be able to look at view() + metadata = ctx_.apply(result); if ((ctx_.flags() & TapDryRun) != 0u) - { - applied = false; - } + return logger(result, false, std::move(metadata)); - JLOG(j_.trace()) << (applied ? "applied " : "not applied ") << transToken(result); - - return {result, applied, metadata}; + return logger(result, canApply, std::move(metadata)); } } // namespace xrpl