From fd914e11823c712e90fa70c8a44020088a27b096 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Thu, 23 Jul 2026 15:28:11 +0200 Subject: [PATCH] refactor: Drop InvariantCheckAdapter, dispatch tx invariants directly Address open PR #7404 review feedback: - Rename CheckInvariants.h/.cpp to InvariantRunner.h/.cpp; the old name read as invariants for the Checks (ttCHECK*) feature. - Remove the InvariantCheck interface and Transactor::InvariantCheckAdapter wrapper. The free checkInvariants runner now takes an optional Transactor& directly and calls visitInvariantEntry/finalizeInvariants on it, since the adapter only existed to bridge to a single concrete type. --- include/xrpl/tx/Transactor.h | 127 +++++++---------- include/xrpl/tx/invariants/CheckInvariants.h | 132 ------------------ include/xrpl/tx/invariants/InvariantRunner.h | 70 ++++++++++ src/libxrpl/tx/Transactor.cpp | 15 +- ...heckInvariants.cpp => InvariantRunner.cpp} | 11 +- src/test/app/NFTokenBurn_test.cpp | 2 +- 6 files changed, 127 insertions(+), 230 deletions(-) delete mode 100644 include/xrpl/tx/invariants/CheckInvariants.h create mode 100644 include/xrpl/tx/invariants/InvariantRunner.h rename src/libxrpl/tx/invariants/{CheckInvariants.cpp => InvariantRunner.cpp} (89%) diff --git a/include/xrpl/tx/Transactor.h b/include/xrpl/tx/Transactor.h index b67e3bdeac..7acc10afb6 100644 --- a/include/xrpl/tx/Transactor.h +++ b/include/xrpl/tx/Transactor.h @@ -20,7 +20,7 @@ #include #include #include -#include +#include #include #include @@ -197,8 +197,8 @@ public: * Check all invariants for the current transaction. * * Delegates to the free @c xrpl::checkInvariants runner. When @p check is - * @c CheckTxInvariants::Yes, the transaction-specific adapter is passed so - * both layers share a single walk of the modified ledger entries. + * @c CheckTxInvariants::Yes, this transactor 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). * @@ -211,6 +211,51 @@ public: [[nodiscard]] TER checkInvariants(TER result, XRPAmount fee, CheckTxInvariants check); + /** + * Inspect a single ledger entry modified by this transaction. + * + * Called once for every SLE created, modified, or deleted by the + * transaction, before finalizeInvariants. Implementations should + * accumulate whatever state they need to verify transaction-specific + * post-conditions. + * + * @param isDelete true if the entry was erased from the ledger. + * @param before the entry's state before the transaction (nullptr + * for newly created entries). + * @param after the entry's state as supplied by the apply logic + * for this transaction. For deletions, this is the + * SLE being erased and is not guaranteed to be null; + * callers must use isDelete rather than after == nullptr + * to detect deletions. + */ + virtual void + visitInvariantEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) = 0; + + /** + * Check transaction-specific post-conditions after all entries have + * been visited. + * + * Called once after every modified ledger entry has been passed to + * visitInvariantEntry. Returns true if all transaction-specific + * invariants hold, or false to fail the transaction with + * tecINVARIANT_FAILED. + * + * @param tx the transaction being applied. + * @param result the tentative TER result so far. + * @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 pass; false otherwise. + */ + [[nodiscard]] virtual bool + finalizeInvariants( + STTx const& tx, + TER result, + XRPAmount fee, + ReadView const& view, + beast::Journal const& j) = 0; + ///////////////////////////////////////////////////// /* These static functions are called from invoke_preclaim @@ -361,51 +406,6 @@ protected: virtual TER doApply() = 0; - /** - * Inspect a single ledger entry modified by this transaction. - * - * Called once for every SLE created, modified, or deleted by the - * transaction, before finalizeInvariants. Implementations should - * accumulate whatever state they need to verify transaction-specific - * post-conditions. - * - * @param isDelete true if the entry was erased from the ledger. - * @param before the entry's state before the transaction (nullptr - * for newly created entries). - * @param after the entry's state as supplied by the apply logic - * for this transaction. For deletions, this is the - * SLE being erased and is not guaranteed to be null; - * callers must use isDelete rather than after == nullptr - * to detect deletions. - */ - virtual void - visitInvariantEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) = 0; - - /** - * Check transaction-specific post-conditions after all entries have - * been visited. - * - * Called once after every modified ledger entry has been passed to - * visitInvariantEntry. Returns true if all transaction-specific - * invariants hold, or false to fail the transaction with - * tecINVARIANT_FAILED. - * - * @param tx the transaction being applied. - * @param result the tentative TER result so far. - * @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 pass; false otherwise. - */ - [[nodiscard]] virtual bool - finalizeInvariants( - STTx const& tx, - TER result, - XRPAmount fee, - ReadView const& view, - beast::Journal const& j) = 0; - /** * Compute the minimum fee required to process a transaction * with a given baseFee based on the current server load. @@ -549,37 +549,6 @@ private: */ static NotTEC preflightUniversal(PreflightContext const& ctx); - - /** - * 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 deleted file mode 100644 index 21b72082e9..0000000000 --- a/include/xrpl/tx/invariants/CheckInvariants.h +++ /dev/null @@ -1,132 +0,0 @@ -#pragma once - -#include -#include -#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. @p after is - * never null. - */ - 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. - * - * Any failure (a `finalize` returning false or an exception anywhere in the - * check) returns @c failInvariantCheck(result). On the first pass that yields - * @c tecINVARIANT_FAILED. If that triggers a fee-claim reset and invariants - * are checked again, a second failure escalates to @c tefINVARIANT_FAILED, - * which excludes the transaction from the ledger entirely. - * - * @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/include/xrpl/tx/invariants/InvariantRunner.h b/include/xrpl/tx/invariants/InvariantRunner.h new file mode 100644 index 0000000000..15dabcb5c5 --- /dev/null +++ b/include/xrpl/tx/invariants/InvariantRunner.h @@ -0,0 +1,70 @@ +#pragma once + +#include +#include +#include + +#include +#include + +namespace xrpl { + +class Transactor; + +/** + * @brief Run all protocol invariant checks plus the transaction-specific check + * in a single pass over the modified entries. + * + * Two layers of checks share one walk of the modified-entry set: + * + * - **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**, when @p txCheck is seated, is + * dispatched at runtime through @c Transactor::visitInvariantEntry and + * @c Transactor::finalizeInvariants. + * + * Both layers honour the same two-phase protocol: + * + * **Phase 1 — state collection** (`visitEntry` / `visitInvariantEntry`) + * 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` / `finalizeInvariants`) + * Called once after every modified entry has been visited. Returns true if + * all post-conditions hold, false to fail the transaction. + * + * `txCheck`'s phase 1 accumulates state on the same traversal that drives the + * protocol checkers, then both layers' phase 2 run on the complete state. + * + * Any failure (a finalize step returning false or an exception anywhere in + * the check) returns @c failInvariantCheck(result). On the first pass that + * yields @c tecINVARIANT_FAILED. If that triggers a fee-claim reset and + * invariants are checked again, a second failure escalates to + * @c tefINVARIANT_FAILED, which excludes the transaction from the ledger + * entirely. + * + * @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 transactor whose transaction-specific invariants should + * also be checked, or @c std::nullopt to run only the + * protocol checks. + * @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/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 32e69a6058..08bd9c3ec3 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -41,7 +41,7 @@ #include #include #include -#include +#include #include #include @@ -1426,17 +1426,6 @@ Transactor::trapTransaction(uint256 txHash) const JLOG(j_.debug()) << "Transaction trapped: " << txHash; } -[[nodiscard]] bool -Transactor::InvariantCheckAdapter::finalize( - STTx const& tx, - TER const result, - XRPAmount const fee, - ReadView const& view, - beast::Journal const& j) const -{ - return self_.finalizeInvariants(tx, result, fee, view, j); -} - std::tuple Transactor::processPersistentChanges(TER result, XRPAmount fee) { @@ -1555,7 +1544,7 @@ Transactor::checkInvariants(TER result, XRPAmount fee, CheckTxInvariants check) if (check == CheckTxInvariants::No) return xrpl::checkInvariants(ctx_, result, fee); - return xrpl::checkInvariants(ctx_, result, fee, std::ref(invariantCheck_)); + return xrpl::checkInvariants(ctx_, result, fee, std::ref(*this)); } //------------------------------------------------------------------------------ diff --git a/src/libxrpl/tx/invariants/CheckInvariants.cpp b/src/libxrpl/tx/invariants/InvariantRunner.cpp similarity index 89% rename from src/libxrpl/tx/invariants/CheckInvariants.cpp rename to src/libxrpl/tx/invariants/InvariantRunner.cpp index ca58641494..5da27baadf 100644 --- a/src/libxrpl/tx/invariants/CheckInvariants.cpp +++ b/src/libxrpl/tx/invariants/InvariantRunner.cpp @@ -1,4 +1,4 @@ -#include +#include #include #include @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -38,7 +39,7 @@ checkInvariantsHelper( ApplyContext& ctx, TER const result, XRPAmount const fee, - std::optional> txCheck, + std::optional> txCheck, std::index_sequence) { auto checkers = getInvariantChecks(); @@ -48,13 +49,13 @@ checkInvariantsHelper( { ctx.visit([&](uint256 const&, bool isDelete, SLE::const_ref before, SLE::const_ref after) { if (txCheck) - txCheck->get().visitEntry(isDelete, before, after); + txCheck->get().visitInvariantEntry(isDelete, before, after); (..., std::get(checkers).visitEntry(isDelete, before, after)); }); if (txCheck) { - if (!txCheck->get().finalize(ctx.tx, result, fee, ctx.view(), ctx.journal)) + if (!txCheck->get().finalizeInvariants(ctx.tx, result, fee, ctx.view(), ctx.journal)) { JLOG(ctx.journal.fatal()) << "Transaction has failed one or more transaction invariants: " @@ -100,7 +101,7 @@ checkInvariants( ApplyContext& ctx, TER const result, XRPAmount const fee, - std::optional> txCheck) + std::optional> txCheck) { XRPL_ASSERT( isTesSuccess(result) || isTecClaim(result), diff --git a/src/test/app/NFTokenBurn_test.cpp b/src/test/app/NFTokenBurn_test.cpp index 0b841ba019..9713d9a379 100644 --- a/src/test/app/NFTokenBurn_test.cpp +++ b/src/test/app/NFTokenBurn_test.cpp @@ -31,7 +31,7 @@ #include #include #include -#include +#include #include #include