From 097e0349fef82581d9df66f50108c80846f15f38 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Thu, 4 Jun 2026 16:57:28 +0200 Subject: [PATCH] refactor: Extract invariant invocation into free checkInvariants runner Move the invariant-check orchestration out of ApplyContext and Transactor into a free function xrpl::checkInvariants(ApplyContext&, TER, XRPAmount, optional>). The two previously separate traversals (one in ApplyContext driving the protocol tuple fold, one in Transactor driving the tx-specific check) are merged into a single ctx.visit walk. Per-layer try/catch inside the lambda isolates collection faults: a throw in one layer stops only that layer from visiting further entries while the other continues. A layer whose collection faulted skips its finalize phase. ApplyContext loses checkInvariants/checkInvariantsHelper/failInvariantCheck. Transactor delegates to the free runner via a private InvariantCheckAdapter that bridges visitInvariantEntry+finalizeInvariants into the InvariantCheck interface. A SkipTxInvariants::Yes/No enum makes the fee-claim-reset call site explicit about omitting the tx-specific check. Protocol checks remain duck-typed in the InvariantChecks tuple (static dispatch, no vtable on the hot path). InvariantCheck is the runtime interface used only by InvariantCheckAdapter. --- include/xrpl/tx/ApplyContext.h | 16 -- include/xrpl/tx/Transactor.h | 62 ++++-- include/xrpl/tx/invariants/CheckInvariants.h | 129 ++++++++++++ src/libxrpl/tx/ApplyContext.cpp | 76 -------- src/libxrpl/tx/Transactor.cpp | 69 ++----- src/libxrpl/tx/invariants/CheckInvariants.cpp | 184 ++++++++++++++++++ src/test/app/Invariants_test.cpp | 3 +- src/test/app/NFTokenBurn_test.cpp | 5 +- 8 files changed, 379 insertions(+), 165 deletions(-) create mode 100644 include/xrpl/tx/invariants/CheckInvariants.h create mode 100644 src/libxrpl/tx/invariants/CheckInvariants.cpp diff --git a/include/xrpl/tx/ApplyContext.h b/include/xrpl/tx/ApplyContext.h index 8540037601..584e665220 100644 --- a/include/xrpl/tx/ApplyContext.h +++ b/include/xrpl/tx/ApplyContext.h @@ -103,23 +103,7 @@ public: view_->rawDestroyXRP(fee); } - /** Applies all invariant checkers one by one. - - @param result the result generated by processing this transaction. - @param fee the fee charged for this transaction - @return the result code that should be returned for this transaction. - */ - TER - checkInvariants(TER const result, XRPAmount const fee); - private: - static TER - failInvariantCheck(TER const result); - - template - TER - checkInvariantsHelper(TER const result, XRPAmount const fee, std::index_sequence); - OpenView& base_; ApplyFlags flags_; std::optional view_; diff --git a/include/xrpl/tx/Transactor.h b/include/xrpl/tx/Transactor.h index 86b1e856b3..3b9f850997 100644 --- a/include/xrpl/tx/Transactor.h +++ b/include/xrpl/tx/Transactor.h @@ -6,6 +6,7 @@ #include #include #include +#include #include @@ -142,19 +143,30 @@ public: return ctx_.view(); } + /** Whether to run the transaction-specific invariant check. + * + * After a fee-claim reset the transaction's effects have been rolled back, + * so only the protocol invariants are meaningful; pass @c Yes to skip the + * transaction-specific check in that case. + */ + enum class SkipTxInvariants : bool { No = false, Yes = true }; + /** Check all invariants for the current transaction. * - * Runs transaction-specific invariants first (visitInvariantEntry + - * finalizeInvariants), then protocol-level invariants. Both layers - * always run; the worst failure code is returned. + * Delegates to the free @c xrpl::checkInvariants runner. Unless @p skip is + * @c SkipTxInvariants::Yes, the transaction-specific adapter is passed so + * both layers share a single walk of the modified ledger entries. + * Protocol faults (tefINVARIANT_FAILED) take priority over transaction + * faults (tecINVARIANT_FAILED). * * @param result the tentative TER from transaction processing. * @param fee the fee consumed by the transaction. + * @param skip whether to skip the transaction-specific invariant check. * * @return the final TER after all invariant checks. */ [[nodiscard]] TER - checkInvariants(TER result, XRPAmount fee); + checkInvariants(TER result, XRPAmount fee, SkipTxInvariants skip); ///////////////////////////////////////////////////// /* @@ -404,20 +416,34 @@ private: static NotTEC preflightUniversal(PreflightContext const& ctx); - /** Check transaction-specific invariants only. - * - * Walks every modified ledger entry via visitInvariantEntry, then - * calls finalizeInvariants on the derived transactor. Returns - * tecINVARIANT_FAILED if any transaction invariant is violated. - * - * @param result the tentative TER from transaction processing. - * @param fee the fee consumed by the transaction. - * - * @return the original result if all invariants pass, or - * tecINVARIANT_FAILED otherwise. - */ - [[nodiscard]] TER - checkTransactionInvariants(TER result, XRPAmount fee); + /** Bridges the transaction-specific two-phase invariant hooks + * (visitInvariantEntry + finalizeInvariants) into the InvariantCheck + * interface consumed by the free xrpl::checkInvariants runner. */ + class InvariantCheckAdapter : public InvariantCheck + { + Transactor& self_; + + public: + explicit InvariantCheckAdapter(Transactor& self) : self_(self) + { + } + + void + visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) override + { + self_.visitInvariantEntry(isDelete, before, after); + } + + [[nodiscard]] bool + finalize( + STTx const& tx, + TER result, + XRPAmount fee, + ReadView const& view, + beast::Journal const& j) const override; + }; + + InvariantCheckAdapter invariantCheck_{*this}; }; inline bool diff --git a/include/xrpl/tx/invariants/CheckInvariants.h b/include/xrpl/tx/invariants/CheckInvariants.h new file mode 100644 index 0000000000..dd3db92188 --- /dev/null +++ b/include/xrpl/tx/invariants/CheckInvariants.h @@ -0,0 +1,129 @@ +#pragma once + +#include +#include +#include +#include +#include +#include +#include + +namespace xrpl { + +/** + * @brief Runtime interface for a transaction-specific invariant check. + * + * The free @c checkInvariants runner drives two layers of checks over a single + * walk of the modified ledger entries: + * + * - **Protocol checks** are the concrete types in @c InvariantChecks, held in + * a @c std::tuple and dispatched statically by a compile-time fold (no + * virtual calls). They are duck-typed against the two-phase contract + * described below; see @c InvariantChecker_PROTOTYPE in InvariantCheck.h. + * - **The transaction-specific check** is injected at runtime through this + * interface, so the runner can call it without depending on the concrete + * transactor type. + * + * Both layers honour the same two-phase protocol: + * + * **Phase 1 — state collection** (`visitEntry`) + * Called once for each ledger entry created, modified, or deleted by the + * transaction. Implementations accumulate whatever state they need to + * evaluate their post-conditions. Must not throw. + * + * **Phase 2 — condition evaluation** (`finalize`) + * Called once after every modified entry has been visited. Returns true if + * all post-conditions hold, false to fail the transaction. + * + * ## Rules for implementing `finalize` + * + * ### Invariants must run regardless of transaction result + * + * `finalize` MUST perform meaningful checks even when the transaction has + * failed (`!isTesSuccess(result)`). A bug or exploit could cause a failed + * transaction to mutate ledger state in unexpected ways; invariants are the + * last line of defense. + * + * The typical pattern: an invariant that expects a domain-specific state + * change (e.g. a Vault being created) should expect that change only when + * the transaction succeeded. A failed VaultCreate must not have created a + * Vault. + * + * ### Privilege-gated checks apply to failed transactions too + * + * Failed transactions carry no privileges. Any privilege-gated assertion + * must therefore also be enforced for failed transactions. + */ +class InvariantCheck +{ +public: + virtual ~InvariantCheck() = default; + + /** + * @brief Called for each ledger entry modified by the transaction. + * + * @param isDelete true if the SLE is being deleted. + * @param before the entry's state before the transaction (nullptr for + * newly created entries). + * @param after the entry's state after the transaction. For deletions + * this is the SLE being erased; use @p isDelete rather than + * `after == nullptr` to detect deletions. + */ + virtual void + visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) = 0; + + /** + * @brief Called after all entries have been visited. + * + * @param tx the transaction being applied. + * @param result the tentative TER result of the transaction. + * @param fee the fee consumed by the transaction. + * @param view read-only view of the ledger after the transaction. + * @param j journal for logging invariant failures. + * @return true if all invariants hold; false to fail with + * tecINVARIANT_FAILED / tefINVARIANT_FAILED. + */ + [[nodiscard]] virtual bool + finalize( + STTx const& tx, + TER result, + XRPAmount fee, + ReadView const& view, + beast::Journal const& j) const = 0; +}; + +/** + * @brief Run all protocol invariant checks plus the transaction-specific check + * in a single pass over the modified entries. + * + * Both layers share one walk of the modified-entry set: @p txCheck's + * `visitEntry` accumulates state on the same traversal that drives the + * protocol checkers, then both layers' `finalize` run on the complete state. + * Protocol faults (tefINVARIANT_FAILED) take precedence over transaction + * faults (tecINVARIANT_FAILED). + * + * Every transaction is guaranteed to supply a @p txCheck (either a real check + * or a no-op stub). The invariant contract requires @c finalize to handle + * failed results gracefully, so passing the same @p txCheck after a fee-claim + * reset is safe. + * + * @param ctx the apply context for the current transaction. + * @param result the tentative TER from transaction processing. + * @param fee the fee consumed by the transaction. + * @param txCheck the transaction-specific invariant check. + * @return the final TER after all invariant checks. + */ +[[nodiscard]] TER +checkInvariants( + ApplyContext& ctx, + TER result, + XRPAmount fee, + std::optional> txCheck); + +[[nodiscard]] inline TER +checkInvariants(ApplyContext& ctx, TER result, XRPAmount fee) +{ + return checkInvariants(ctx, result, fee, std::nullopt); +} + +} // namespace xrpl diff --git a/src/libxrpl/tx/ApplyContext.cpp b/src/libxrpl/tx/ApplyContext.cpp index 93b0d101af..31535762af 100644 --- a/src/libxrpl/tx/ApplyContext.cpp +++ b/src/libxrpl/tx/ApplyContext.cpp @@ -12,15 +12,10 @@ #include #include #include -#include -#include #include -#include #include #include -#include -#include namespace xrpl { @@ -74,75 +69,4 @@ ApplyContext::visit( view_->visit(base_, func); // NOLINT(bugprone-unchecked-optional-access) } -TER -ApplyContext::failInvariantCheck(TER const result) -{ - // If we already failed invariant checks before and we are now attempting to - // only charge a fee, and even that fails the invariant checks something is - // very wrong. We switch to tefINVARIANT_FAILED, which does NOT get included - // in a ledger. - - return (result == tecINVARIANT_FAILED || result == tefINVARIANT_FAILED) - ? TER{tefINVARIANT_FAILED} - : TER{tecINVARIANT_FAILED}; -} - -template -TER -ApplyContext::checkInvariantsHelper( - TER const result, - XRPAmount const fee, - std::index_sequence) -{ - try - { - auto checkers = getInvariantChecks(); - - // call each check's per-entry method - visit( - [&checkers]( - uint256 const& index, bool isDelete, SLE::const_ref before, SLE::const_ref after) { - (..., std::get(checkers).visitEntry(isDelete, before, after)); - }); - - // Note: do not replace this logic with a `...&&` fold expression. - // The fold expression will only run until the first check fails (it - // short-circuits). While the logic is still correct, the log - // message won't be. Every failed invariant should write to the log, - // not just the first one. - std::array const finalizers{{std::get(checkers).finalize( - tx, result, fee, *view_, journal)...}}; // NOLINT(bugprone-unchecked-optional-access) - - // call each check's finalizer to see that it passes - if (!std::all_of(finalizers.cbegin(), finalizers.cend(), [](auto const& b) { return b; })) - { - JLOG(journal.fatal()) << "Transaction has failed one or more global invariants: " - << to_string(tx.getJson(JsonOptions::Values::None)); - - return failInvariantCheck(result); - } - } - catch (std::exception const& ex) - { - JLOG(journal.fatal()) << "Transaction caused an exception in a global invariant" - << ", ex: " << ex.what() - << ", tx: " << to_string(tx.getJson(JsonOptions::Values::None)); - - return failInvariantCheck(result); - } - - return result; -} - -TER -ApplyContext::checkInvariants(TER const result, XRPAmount const fee) -{ - XRPL_ASSERT( - isTesSuccess(result) || isTecClaim(result), - "xrpl::ApplyContext::checkInvariants : is tesSUCCESS or tecCLAIM"); - - return checkInvariantsHelper( - result, fee, std::make_index_sequence>{}); -} - } // namespace xrpl diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 51541cc2e3..78f166b37e 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -44,7 +44,6 @@ #include #include -#include #include #include #include @@ -1135,58 +1134,24 @@ Transactor::trapTransaction(uint256 txHash) const JLOG(j_.debug()) << "Transaction trapped: " << txHash; } -[[nodiscard]] TER -Transactor::checkTransactionInvariants(TER result, XRPAmount fee) +[[nodiscard]] bool +Transactor::InvariantCheckAdapter::finalize( + STTx const& tx, + TER const result, + XRPAmount const fee, + ReadView const& view, + beast::Journal const& j) const { - try - { - // Phase 1: visit modified entries - ctx_.visit( - [this](uint256 const&, bool isDelete, SLE::const_ref before, SLE::const_ref after) { - this->visitInvariantEntry(isDelete, before, after); - }); - - // Phase 2: finalize - if (!this->finalizeInvariants(ctx_.tx, result, fee, ctx_.view(), ctx_.journal)) - { - JLOG(ctx_.journal.fatal()) << // - "Transaction has failed one or more transaction invariants, tx: " << // - to_string(ctx_.tx.getJson(JsonOptions::Values::None)); - return tecINVARIANT_FAILED; - } - } - catch (std::exception const& ex) - { - JLOG(ctx_.journal.fatal()) << // - "Exception while checking transaction invariants: " << // - ex.what() << // - ", tx: " << // - to_string(ctx_.tx.getJson(JsonOptions::Values::None)); - - return tecINVARIANT_FAILED; - } - - return result; + return self_.finalizeInvariants(tx, result, fee, view, j); } [[nodiscard]] TER -Transactor::checkInvariants(TER result, XRPAmount fee) +Transactor::checkInvariants(TER result, XRPAmount fee, SkipTxInvariants skip) { - // Transaction invariants first (more specific). These check post-conditions of the specific - // transaction. If these fail, the transaction's core logic is wrong. - auto const txResult = checkTransactionInvariants(result, fee); + if (skip == SkipTxInvariants::Yes) + return xrpl::checkInvariants(ctx_, result, fee); - // Protocol invariants second (broader). These check properties that must hold regardless of - // transaction type. - auto const protoResult = ctx_.checkInvariants(result, fee); - - // Fail if either check failed. tef (fatal) takes priority over tec. - if (protoResult == tefINVARIANT_FAILED) - return tefINVARIANT_FAILED; - if (txResult == tecINVARIANT_FAILED || protoResult == tecINVARIANT_FAILED) - return tecINVARIANT_FAILED; - - return result; + return xrpl::checkInvariants(ctx_, result, fee, std::ref(invariantCheck_)); } //------------------------------------------------------------------------------ ApplyResult @@ -1364,7 +1329,7 @@ Transactor::operator()() { // Check invariants: if `tecINVARIANT_FAILED` is not returned, we can // proceed to apply the tx - result = checkInvariants(result, fee); + result = checkInvariants(result, fee, SkipTxInvariants::No); if (result == tecINVARIANT_FAILED) { // Reset to fee-claim only @@ -1375,11 +1340,11 @@ Transactor::operator()() 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. + // invariants. After reset, only protocol invariants are re-checked; + // the transaction's effects have been rolled back, so the + // transaction-specific invariants are no longer meaningful. if (isTesSuccess(result) || isTecClaim(result)) - result = ctx_.checkInvariants(result, fee); + result = checkInvariants(result, fee, SkipTxInvariants::Yes); } // We ran through the invariant checker, which can, in some cases, diff --git a/src/libxrpl/tx/invariants/CheckInvariants.cpp b/src/libxrpl/tx/invariants/CheckInvariants.cpp new file mode 100644 index 0000000000..f0099562da --- /dev/null +++ b/src/libxrpl/tx/invariants/CheckInvariants.cpp @@ -0,0 +1,184 @@ +#include + +#include +#include +#include // IWYU pragma: keep +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace xrpl { + +namespace { + +TER +failInvariantCheck(TER const result) +{ + return (result == tecINVARIANT_FAILED || result == tefINVARIANT_FAILED) + ? TER{tefINVARIANT_FAILED} + : TER{tecINVARIANT_FAILED}; +} + +template +TER +checkInvariantsHelper( + ApplyContext& ctx, + TER const result, + XRPAmount const fee, + std::optional> txCheck, + std::index_sequence) +{ + auto checkers = getInvariantChecks(); + + // Phase 1 — state collection. + // One walk feeds both layers. Per-layer try-catch isolates faults: a throw + // in txCheck stops only txCheck from visiting further entries; the protocol + // fold keeps going (and vice-versa). A layer that threw skips finalize. + bool txCollectionOk = true; + bool protoCollectionOk = true; + std::string txCollectionEx; + std::string protoCollectionEx; + + ctx.visit([&](uint256 const&, bool isDelete, SLE::const_ref before, SLE::const_ref after) { + if (txCheck && txCollectionOk) + { + try + { + txCheck->get().visitEntry(isDelete, before, after); + } + catch (std::exception const& ex) + { + txCollectionOk = false; + txCollectionEx = ex.what(); + } + } + + if (protoCollectionOk) + { + try + { + (..., std::get(checkers).visitEntry(isDelete, before, after)); + } + catch (std::exception const& ex) + { + protoCollectionOk = false; + protoCollectionEx = ex.what(); + } + } + }); + + // Phase 2 — evaluate invariant conditions. + auto const txResult = [&]() -> TER { + if (result == tecINVARIANT_FAILED || !txCheck) + return result; + + if (!txCollectionOk) + { + JLOG(ctx.journal.fatal()) + << "Transaction caused an exception while collecting transaction invariant state" + << ", ex: " << txCollectionEx + << ", tx: " << to_string(ctx.tx.getJson(JsonOptions::Values::None)); + return tecINVARIANT_FAILED; + } + + try + { + if (!txCheck->get().finalize(ctx.tx, result, fee, ctx.view(), ctx.journal)) + { + JLOG(ctx.journal.fatal()) + << "Transaction has failed one or more transaction invariants: " + << to_string(ctx.tx.getJson(JsonOptions::Values::None)); + return tecINVARIANT_FAILED; + } + } + catch (std::exception const& ex) + { + JLOG(ctx.journal.fatal()) + << "Transaction caused an exception in a transaction invariant" + << ", ex: " << ex.what() + << ", tx: " << to_string(ctx.tx.getJson(JsonOptions::Values::None)); + return tecINVARIANT_FAILED; + } + + return result; + }(); + + auto const protoResult = [&]() -> TER { + if (!protoCollectionOk) + { + JLOG(ctx.journal.fatal()) + << "Transaction caused an exception while collecting global invariant state" + << ", ex: " << protoCollectionEx + << ", tx: " << to_string(ctx.tx.getJson(JsonOptions::Values::None)); + return failInvariantCheck(result); + } + + bool protoOk = true; + try + { + // Note: do not replace this logic with a `...&&` fold expression. + // The fold expression will only run until the first check fails (it + // short-circuits). While the logic is still correct, the log + // message won't be. Every failed invariant should write to the log, + // not just the first one. + std::array const finalizers{{std::get(checkers).finalize( + ctx.tx, + result, + fee, + ctx.view(), + ctx.journal)...}}; // NOLINT(bugprone-unchecked-optional-access) + + protoOk = std::all_of( + finalizers.cbegin(), finalizers.cend(), [](auto const& b) { return b; }); + if (!protoOk) + { + JLOG(ctx.journal.fatal()) + << "Transaction has failed one or more global invariants: " + << to_string(ctx.tx.getJson(JsonOptions::Values::None)); + } + } + catch (std::exception const& ex) + { + JLOG(ctx.journal.fatal()) + << "Transaction caused an exception in a global invariant" + << ", ex: " << ex.what() + << ", tx: " << to_string(ctx.tx.getJson(JsonOptions::Values::None)); + protoOk = false; + } + + return protoOk ? result : failInvariantCheck(result); + }(); + + // Fail if either check failed. tef (fatal) takes priority over tec. + if (protoResult == tefINVARIANT_FAILED) + return tefINVARIANT_FAILED; + if (txResult == tecINVARIANT_FAILED || protoResult == tecINVARIANT_FAILED) + return tecINVARIANT_FAILED; + + return result; +} + +} // namespace + +TER +checkInvariants( + ApplyContext& ctx, + TER const result, + XRPAmount const fee, + std::optional> txCheck) +{ + XRPL_ASSERT( + isTesSuccess(result) || isTecClaim(result), + "xrpl::checkInvariants : is tesSUCCESS or tecCLAIM"); + + return checkInvariantsHelper( + ctx, result, fee, txCheck, std::make_index_sequence>{}); +} + +} // namespace xrpl diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 3654036869..3d4c3b5ab7 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -199,7 +199,8 @@ class Invariants_test : public beast::unit_test::Suite TER terActual = tesSUCCESS; for (TER const& terExpect : ters) { - terActual = transactor->checkInvariants(terActual, fee); + terActual = + transactor->checkInvariants(terActual, fee, Transactor::SkipTxInvariants::No); BEAST_EXPECTS( terExpect == terActual, "expected: " + transToken(terExpect) + " got: " + transToken(terActual)); diff --git a/src/test/app/NFTokenBurn_test.cpp b/src/test/app/NFTokenBurn_test.cpp index 482d275d51..e937a9004e 100644 --- a/src/test/app/NFTokenBurn_test.cpp +++ b/src/test/app/NFTokenBurn_test.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -789,7 +790,7 @@ class NFTokenBurn_test : public beast::unit_test::Suite TER terActual = tesSUCCESS; for (TER const& terExpect : {TER(tecINVARIANT_FAILED), TER(tefINVARIANT_FAILED)}) { - terActual = ac.checkInvariants(terActual, XRPAmount{}); + terActual = xrpl::checkInvariants(ac, terActual, XRPAmount{}); BEAST_EXPECT(terExpect == terActual); BEAST_EXPECT(sink.messages().str().starts_with("Invariant failed:")); // uncomment to log the invariant failure message @@ -826,7 +827,7 @@ class NFTokenBurn_test : public beast::unit_test::Suite TER terActual = tesSUCCESS; for (TER const& terExpect : {TER(tecINVARIANT_FAILED), TER(tefINVARIANT_FAILED)}) { - terActual = ac.checkInvariants(terActual, XRPAmount{}); + terActual = xrpl::checkInvariants(ac, terActual, XRPAmount{}); BEAST_EXPECT(terExpect == terActual); BEAST_EXPECT(sink.messages().str().starts_with("Invariant failed:")); // uncomment to log the invariant failure message