feat: Create an invariant to validated failed transactions

include/xrpl/protocol/detail/features.macro

@@ -15,6 +15,7 @@
 // Add new amendments to the top of this list.
 // Keep it sorted in reverse chronological order.
 
+XRPL_FIX    (TecInvariant,                Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (Cleanup3_4_0,                Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(Sponsor,                     Supported::Yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(BatchV1_1,                   Supported::Yes, VoteBehavior::DefaultNo)

include/xrpl/tx/Transactor.h

@@ -336,6 +336,10 @@ class Transactor
         uint256 const& ticketIndex,
         beast::Journal j);
 
+    // Interface used by processPersistentChanges and Invariants
+    static std::unordered_set<LedgerEntryType>
+    typesForResult(TER const ter);
+
 protected:
     TER
     apply();

include/xrpl/tx/invariants/InvariantCheck.h

@@ -113,6 +113,53 @@ class InvariantChecker_PROTOTYPE
 };
 #endif
 
+/**
+ * @brief An unsuccessful transaction claiming a fee can only make a very small set of changes.
+ *
+ * 1. Reduce at most one AccountRoot or Sponsorship's XRP balance (pay a fee). (A transaction may
+ *    pay 0.)
+ * 2. Increment one AccountRoot's sequence or delete a Ticket, not both.
+ * 3. Delete expired objects, depending on the failure code.
+ *      For tecOVERSIZE and tecKILLED, ltOFFER
+ *      For tecINCOMPLETE, ltRIPPLE_STATE
+ *      For tecEXPIRED, ltNFTOKEN_OFFER or ltCREDENTIAL
+ * 4. Modify or delete Directory Nodes, only if expired objects were deleted.
+ *
+ * Anything outside of that is bad.
+ *
+ * Collect change data for the known allowed types, and collect the info in errors_ for anything
+ * else.
+ *
+ * Note that finalize() will always return true on a `tesSUCCESS`, even if there are messages
+ * collected in errors_. The errors_ only apply if the transaction was NOT successful. It will also
+ * do additional checks based on the transaction data.
+ */
+class FailedTransaction
+{
+    struct DeletedEntry
+    {
+        SLE::const_pointer before;
+        SLE::const_pointer after;
+    };
+
+    // accountPaidFee and accountIncreasedSequence are usually the same account, but they don't have
+    // to be.
+    SLE::const_pointer accountPaidFee_;
+    SLE::const_pointer sponsorPaidFee_;
+    SLE::const_pointer accountIncreasedSequence_;
+    SLE::const_pointer deletedTicket_;
+    std::vector<DeletedEntry> deletedObjects_;
+    std::vector<SLE::const_pointer> directorySideEffects_;
+    std::vector<std::string> errors_;
+
+public:
+    void
+    visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after);
+
+    [[nodiscard]] bool
+    finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&);
+};
+
 /**
  * @brief Invariant: We should never charge a transaction a negative fee or a
  * fee that is larger than what the transaction itself specifies.
@@ -432,6 +479,7 @@ class ObjectHasPseudoAccount
 // additional invariant checks can be declared above and then added to this
 // tuple
 using InvariantChecks = std::tuple<
+    FailedTransaction,
     TransactionFeeCheck,
     AccountRootsNotDeleted,
     AccountRootsDeletedClean,

src/libxrpl/tx/Transactor.cpp

@@ -4,6 +4,7 @@
 #include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/basics/contract.h>
+#include <xrpl/basics/scope.h>
 #include <xrpl/beast/utility/Zero.h>
 #include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/core/NetworkIDService.h>
@@ -1426,6 +1427,26 @@ Transactor::trapTransaction(uint256 txHash) const
     JLOG(j_.debug()) << "Transaction trapped: " << txHash;
 }
 
+std::unordered_set<LedgerEntryType>
+Transactor::typesForResult(TER const ter)
+{
+    std::unordered_set<LedgerEntryType> types;
+    if ((ter == tecOVERSIZE) || (ter == tecKILLED))
+    {
+        types.insert(ltOFFER);
+    }
+    else if (ter == tecINCOMPLETE)
+    {
+        types.insert(ltRIPPLE_STATE);
+    }
+    else if (ter == tecEXPIRED)
+    {
+        types.insert(ltNFTOKEN_OFFER);
+        types.insert(ltCREDENTIAL);
+    }
+    return types;
+}
+
 std::tuple<TER, XRPAmount, bool>
 Transactor::processPersistentChanges(TER result, XRPAmount fee)
 {
@@ -1436,24 +1457,6 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee)
     //        should be used, making it possible to do more useful work
     //        when transactions fail with a `tec` code.
 
-    auto typesForResult = [](TER const ter) {
-        std::unordered_set<LedgerEntryType> types;
-        if ((ter == tecOVERSIZE) || (ter == tecKILLED))
-        {
-            types.insert(ltOFFER);
-        }
-        else if (ter == tecINCOMPLETE)
-        {
-            types.insert(ltRIPPLE_STATE);
-        }
-        else if (ter == tecEXPIRED)
-        {
-            types.insert(ltNFTOKEN_OFFER);
-            types.insert(ltCREDENTIAL);
-        }
-        return types;
-    };
-
     // Build a list of ledger entry types to collect, based on the
     // result code. Only deleted objects of these types will be
     // re-applied after the context is reset.
@@ -1637,7 +1640,7 @@ Transactor::operator()()
     if (auto stream = j_.trace())
         stream << "preclaim result: " << transToken(result);
 
-    bool applied = isTesSuccess(result);
+    bool canApply = isTesSuccess(result);
     auto fee = ctx_.tx.getFieldAmount(sfFee).xrp();
 
     if (ctx_.size() > kOversizeMetaDataCap)
@@ -1648,74 +1651,76 @@ Transactor::operator()()
         // If the TapFailHard flag is set, a tec result
         // must not do anything
         ctx_.discard();
-        applied = false;
+        canApply = false;
     }
     else if (
         (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) ||
         (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags())))
     {
-        std::tie(result, fee, applied) = processPersistentChanges(result, fee);
+        // This is and must remain the only place where `canApply` can change from false to true.
+        // Changing from true to false is no problem.
+        std::tie(result, fee, canApply) = processPersistentChanges(result, fee);
     }
 
-    if (applied)
+    ScopeExit const logger{[&] {
+        JLOG(j_.trace()) << (canApply ? "applied " : "not applied ") << transToken(result);
+    }};
+
+    if (!canApply)
+        return {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)
     {
-        // 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);
-        }
+        // Reset to fee-claim only
+        auto const resetResult = reset(fee);
+        if (!isTesSuccess(resetResult.first))
+            result = resetResult.first;
 
-        // 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;
+        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 {result, false};
+
     std::optional<TxMeta> 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<std::logic_error>("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<std::logic_error>("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 {result, false, metadata};
     }
 
-    JLOG(j_.trace()) << (applied ? "applied " : "not applied ") << transToken(result);
-
-    return {result, applied, metadata};
+    return {result, canApply, metadata};
 }
 
 }  // namespace xrpl