diff --git a/include/xrpl/tx/Transactor.h b/include/xrpl/tx/Transactor.h index 7acc10afb6..74df857206 100644 --- a/include/xrpl/tx/Transactor.h +++ b/include/xrpl/tx/Transactor.h @@ -148,7 +148,7 @@ struct FeePayer FeePayerType type{FeePayerType::Account}; }; -class Transactor +class Transactor : public TxInvariantCheck { protected: ApplyContext& ctx_; @@ -159,7 +159,7 @@ protected: XRPAmount preFeeBalance_{}; // Balance before fees. public: - virtual ~Transactor() = default; + ~Transactor() override = default; Transactor(Transactor const&) = delete; Transactor& operator=(Transactor const&) = delete; @@ -198,9 +198,10 @@ public: * * Delegates to the free @c xrpl::checkInvariants runner. When @p check is * @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). + * share a single walk of the modified ledger entries. A failure in + * either layer fails the transaction the same way: tecINVARIANT_FAILED on + * the first pass, escalating to tefINVARIANT_FAILED if invariants are + * checked again after a fee-claim reset. * * @param result the tentative TER from transaction processing. * @param fee the fee consumed by the transaction. @@ -211,51 +212,6 @@ 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 @@ -406,6 +362,51 @@ 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,6 +550,32 @@ private: */ static NotTEC preflightUniversal(PreflightContext const& ctx); + + /** + * Bridges the two-phase TxInvariantCheck interface to this transactor's + * visitInvariantEntry/finalizeInvariants hooks. Declared private (rather + * than protected, like the hooks they forward to) so that neither this + * transactor nor any subclass can call them directly through a + * @c Transactor& — only through the @c TxInvariantCheck& that the free + * @c xrpl::checkInvariants runner holds, which is where the two-phase + * ordering is enforced. + */ + void + visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) final + { + visitInvariantEntry(isDelete, before, after); + } + + [[nodiscard]] bool + finalize( + STTx const& tx, + TER result, + XRPAmount fee, + ReadView const& view, + beast::Journal const& j) final + { + return finalizeInvariants(tx, result, fee, view, j); + } }; inline bool diff --git a/include/xrpl/tx/invariants/InvariantRunner.h b/include/xrpl/tx/invariants/InvariantRunner.h index 15dabcb5c5..369e913fe0 100644 --- a/include/xrpl/tx/invariants/InvariantRunner.h +++ b/include/xrpl/tx/invariants/InvariantRunner.h @@ -1,5 +1,9 @@ #pragma once +#include +#include +#include +#include #include #include #include @@ -9,49 +13,117 @@ namespace xrpl { -class Transactor; - /** - * @brief Run all protocol invariant checks plus the transaction-specific check - * in a single pass over the modified entries. + * @brief Runtime interface for a transaction-specific invariant check. * - * Two layers of checks share one walk of the modified-entry set: + * 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**, when @p txCheck is seated, is - * dispatched at runtime through @c Transactor::visitInvariantEntry and - * @c Transactor::finalizeInvariants. + * - **The transaction-specific check** is injected at runtime through this + * interface, so the runner can call it without depending on the concrete + * transactor type. @c Transactor implements this interface directly (see + * Transactor.h) so that the interface's access can stay narrower than + * Transactor's own public surface: calling through a @c TxInvariantCheck& + * (all the runner ever holds) is public, but calling through a + * @c Transactor& is not, since Transactor overrides these as private + * (forwarding to its own protected visitInvariantEntry/finalizeInvariants). * * Both layers honour the same two-phase protocol: * - * **Phase 1 — state collection** (`visitEntry` / `visitInvariantEntry`) + * **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` / `finalizeInvariants`) + * **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. * - * `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. + * ## Rules for implementing `finalize` * - * 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. + * ### 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 TxInvariantCheck +{ +public: + virtual ~TxInvariantCheck() = 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) = 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. + * + * The whole traversal — both layers' `visitEntry` calls and both layers' + * `finalize` calls — runs under a single try/catch. There is no per-layer + * isolation: an exception anywhere aborts the remaining traversal and + * finalize calls and fails the transaction. * * @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. + * @param txCheck the transaction-specific invariant check. * @return the final TER after all invariant checks. */ [[nodiscard]] TER @@ -59,7 +131,7 @@ checkInvariants( ApplyContext& ctx, TER result, XRPAmount fee, - std::optional> txCheck); + std::optional> txCheck); [[nodiscard]] inline TER checkInvariants(ApplyContext& ctx, TER result, XRPAmount fee) diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 08bd9c3ec3..0a38b181b3 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -46,7 +46,6 @@ #include #include #include -#include #include #include #include @@ -1544,7 +1543,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(*this)); + return xrpl::checkInvariants(ctx_, result, fee, *this); } //------------------------------------------------------------------------------ diff --git a/src/libxrpl/tx/invariants/InvariantRunner.cpp b/src/libxrpl/tx/invariants/InvariantRunner.cpp index 5da27baadf..55bff2d693 100644 --- a/src/libxrpl/tx/invariants/InvariantRunner.cpp +++ b/src/libxrpl/tx/invariants/InvariantRunner.cpp @@ -9,7 +9,6 @@ #include #include #include -#include #include #include @@ -39,23 +38,24 @@ checkInvariantsHelper( ApplyContext& ctx, TER const result, XRPAmount const fee, - std::optional> txCheck, + std::optional> txCheck, std::index_sequence) { - auto checkers = getInvariantChecks(); bool allOk = true; try { + auto checkers = getInvariantChecks(); + ctx.visit([&](uint256 const&, bool isDelete, SLE::const_ref before, SLE::const_ref after) { if (txCheck) - txCheck->get().visitInvariantEntry(isDelete, before, after); + txCheck->get().visitEntry(isDelete, before, after); (..., std::get(checkers).visitEntry(isDelete, before, after)); }); if (txCheck) { - if (!txCheck->get().finalizeInvariants(ctx.tx, result, fee, ctx.view(), ctx.journal)) + if (!txCheck->get().finalize(ctx.tx, result, fee, ctx.view(), ctx.journal)) { JLOG(ctx.journal.fatal()) << "Transaction has failed one or more transaction invariants: " @@ -69,12 +69,8 @@ checkInvariantsHelper( // 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) + std::array const finalizers{ + {std::get(checkers).finalize(ctx.tx, result, fee, ctx.view(), ctx.journal)...}}; if (!std::all_of(finalizers.cbegin(), finalizers.cend(), [](auto const& b) { return b; })) { @@ -101,7 +97,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/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index c5690fc920..a8876f4321 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -54,6 +55,7 @@ #include #include #include +#include #include #include @@ -64,6 +66,7 @@ #include #include #include +#include #include #include #include @@ -5916,6 +5919,54 @@ class Invariants_test : public beast::unit_test::Suite } } + void + testTxCheckException() + { + testcase << "txCheck exception"; + using namespace jtx; + + // A TxInvariantCheck that always throws from finalize, so we can + // exercise checkInvariantsHelper's catch block via the + // transaction-specific layer (as opposed to the protocol layer, + // which testObjectHasPseudoAccount's last case already covers via a + // real Transactor's finalizeInvariants). + struct ThrowingTxInvariantCheck : TxInvariantCheck + { + void + visitEntry(bool, SLE::const_ref, SLE::const_ref) override + { + } + + [[nodiscard]] bool + finalize(STTx const&, TER, XRPAmount, ReadView const&, beast::Journal const&) override + { + throw std::runtime_error("test-injected txCheck exception"); + } + }; + + Env env{*this}; + Account const alice{"alice"}; + env.fund(XRP(1000), alice); + env.close(); + + OpenView ov{*env.current()}; + STTx const tx{ttACCOUNT_SET, [](STObject&) {}}; + test::StreamSink sink{beast::Severity::Warning}; + beast::Journal const jlog{sink}; + ApplyContext ac{env.app(), ov, tx, tesSUCCESS, env.current()->fees().base, TapNone, jlog}; + CurrentTransactionRulesGuard const rulesGuard(ov.rules()); + + ThrowingTxInvariantCheck throwing; + TER terActual = tesSUCCESS; + for (TER const& terExpect : {TER(tecINVARIANT_FAILED), TER(tefINVARIANT_FAILED)}) + { + terActual = checkInvariants(ac, terActual, XRPAmount{}, throwing); + BEAST_EXPECT(terExpect == terActual); + BEAST_EXPECT(sink.messages().str().contains( + "Transaction caused an exception during invariant checks")); + } + } + void testConfidentialMPTTransfer() { @@ -6200,6 +6251,7 @@ public: testAMM(); testObjectHasPseudoAccount(); testSponsorship(); + testTxCheckException(); } };