Improve invariant checking:

Add a new invariant checker that verifies that we never charge a
fee higher than specified in the transaction; we will charge less
in some corner cases where the transacting account cannot afford
the fee.

Detect more anomalous conditions, and improve the logged error
messages.

Clarify the code flow associated with invoking the invariant checker
from `Transactor`, add extra comments and improve naming to make the
code self-documenting.

src/ripple/app/tx/impl/ApplyContext.cpp

@@ -70,9 +70,25 @@ ApplyContext::visit (std::function <void (
     view_->visit(base_, func);
 }
 
+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<std::size_t... Is>
 TER
-ApplyContext::checkInvariantsHelper(TER terResult, std::index_sequence<Is...>)
+ApplyContext::checkInvariantsHelper(
+    TER const result,
+    XRPAmount const fee,
+    std::index_sequence<Is...>)
 {
     if (view_->rules().enabled(featureEnforceInvariants))
     {
@@ -97,40 +113,40 @@ ApplyContext::checkInvariantsHelper(TER terResult, std::index_sequence<Is...>)
             // Sean Parent for_each_argument trick (a fold expression with `&&`
             // would be really nice here when we move to C++-17)
             std::array<bool, sizeof...(Is)> finalizers {{
-                std::get<Is>(checkers).finalize(tx, terResult, journal)...}};
+                std::get<Is>(checkers).finalize(tx, result, fee, journal)...}};
 
             // call each check's finalizer to see that it passes
             if (! std::all_of( finalizers.cbegin(), finalizers.cend(),
                     [](auto const& b) { return b; }))
             {
-                terResult = (terResult == tecINVARIANT_FAILED) ?
-                    TER {tefINVARIANT_FAILED} :
-                    TER {tecINVARIANT_FAILED} ;
                 JLOG(journal.fatal()) <<
                     "Transaction has failed one or more invariants: " <<
                     to_string(tx.getJson (0));
+
+                return failInvariantCheck (result);
             }
         }
         catch(std::exception const& ex)
         {
-            terResult = (terResult == tecINVARIANT_FAILED) ?
-                TER {tefINVARIANT_FAILED} :
-                TER {tecINVARIANT_FAILED} ;
             JLOG(journal.fatal()) <<
                 "Transaction caused an exception in an invariant" <<
                 ", ex: " << ex.what() <<
                 ", tx: " << to_string(tx.getJson (0));
+
+            return failInvariantCheck (result);
         }
     }
 
-    return terResult;
+    return result;
 }
 
 TER
-ApplyContext::checkInvariants(TER terResult)
+ApplyContext::checkInvariants(TER const result, XRPAmount const fee)
 {
-    return checkInvariantsHelper(
+    assert (isTesSuccess(result) || isTecClaim(result));
-        terResult, std::make_index_sequence<std::tuple_size<InvariantChecks>::value>{});
+
+    return checkInvariantsHelper(result, fee,
+        std::make_index_sequence<std::tuple_size<InvariantChecks>::value>{});
 }
 
 } // ripple

src/ripple/app/tx/impl/ApplyContext.h

@@ -101,12 +101,22 @@ 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);
+    checkInvariants(TER const result, XRPAmount const fee);
 
 private:
+    TER
+    failInvariantCheck (TER const result);
+
     template<std::size_t... Is>
-    TER checkInvariantsHelper(TER terResult, std::index_sequence<Is...>);
+    TER
+    checkInvariantsHelper(TER const result, XRPAmount const fee, std::index_sequence<Is...>);
 
     OpenView& base_;
     ApplyFlags flags_;

src/ripple/app/tx/impl/InvariantCheck.cpp

@@ -22,6 +22,52 @@
 
 namespace ripple {
 
+void
+TransactionFeeCheck::visitEntry(
+    uint256 const&,
+    bool,
+    std::shared_ptr<SLE const> const&,
+    std::shared_ptr<SLE const> const&)
+{
+    // nothing to do
+}
+
+bool
+TransactionFeeCheck::finalize(
+    STTx const& tx,
+    TER const result,
+    XRPAmount const fee,
+    beast::Journal const& j)
+{
+    // We should never charge a negative fee
+    if (fee.drops() < 0)
+    {
+        JLOG(j.fatal()) << "Invariant failed: fee paid was negative: " << fee.drops();
+        return false;
+    }
+
+    // We should never charge a fee that's greater than or equal to the
+    // entire XRP supply.
+    if (fee.drops() >= SYSTEM_CURRENCY_START)
+    {
+        JLOG(j.fatal()) << "Invariant failed: fee paid exceeds system limit: " << fee.drops();
+        return false;
+    }
+
+    // We should never charge more for a transaction than the transaction
+    // authorizes. It's possible to charge less in some circumstances.
+    if (fee > tx.getFieldAmount(sfFee).xrp())
+    {
+        JLOG(j.fatal()) << "Invariant failed: fee paid is " << fee.drops() <<
+            " exceeds fee specified in transaction.";
+        return false;
+    }
+
+    return true;
+}
+
+//------------------------------------------------------------------------------
+
 void
 XRPNotCreated::visitEntry(
     uint256 const&,
@@ -29,6 +75,13 @@ XRPNotCreated::visitEntry(
     std::shared_ptr <SLE const> const& before,
     std::shared_ptr <SLE const> const& after)
 {
+    /* We go through all modified ledger entries, looking only at account roots,
+     * escrow payments, and payment channels. We remove from the total any
+     * previous XRP values and add to the total any new XRP values. The net
+     * balance of a payment channel is computed from two fields (amount and
+     * balance) and deletions are ignored for paychan and escrow because the
+     * amount fields have not been adjusted for those in the case of deletion.
+     */
     if(before)
     {
         switch (before->getType())
@@ -69,17 +122,33 @@ XRPNotCreated::visitEntry(
 }
 
 bool
-XRPNotCreated::finalize(STTx const& tx, TER /*tec*/, beast::Journal const& j)
+XRPNotCreated::finalize(
+    STTx const& tx,
+    TER const,
+    XRPAmount const fee,
+    beast::Journal const& j)
 {
-    auto fee = tx.getFieldAmount(sfFee).xrp().drops();
+    // The net change should never be positive, as this would mean that the
-    if(-1*fee <= drops_ && drops_ <= 0)
+    // transaction created XRP out of thin air. That's not possible.
-        return true;
+    if (drops_ > 0)
-
+    {
-    JLOG(j.fatal()) << "Invariant failed: XRP net change was " << drops_ <<
+        JLOG(j.fatal()) <<
-        " on a fee of " << fee;
+            "Invariant failed: XRP net change was positive: " << drops_;
         return false;
     }
 
+    // The negative of the net change should be equal to actual fee charged.
+    if (-drops_ != fee.drops())
+    {
+        JLOG(j.fatal()) <<
+            "Invariant failed: XRP net change of " << drops_ <<
+            " doesn't match fee " << fee.drops();
+        return false;
+    }
+
+    return true;
+}
+
 //------------------------------------------------------------------------------
 
 void
@@ -116,7 +185,7 @@ XRPBalanceChecks::visitEntry(
 }
 
 bool
-XRPBalanceChecks::finalize(STTx const&, TER, beast::Journal const& j)
+XRPBalanceChecks::finalize(STTx const&, TER const, XRPAmount const, beast::Journal const& j)
 {
     if (bad_)
     {
@@ -157,7 +226,7 @@ NoBadOffers::visitEntry(
 }
 
 bool
-NoBadOffers::finalize(STTx const& tx, TER, beast::Journal const& j)
+NoBadOffers::finalize(STTx const& tx, TER const, XRPAmount const, beast::Journal const& j)
 {
     if (bad_)
     {
@@ -199,7 +268,7 @@ NoZeroEscrow::visitEntry(
 }
 
 bool
-NoZeroEscrow::finalize(STTx const& tx, TER, beast::Journal const& j)
+NoZeroEscrow::finalize(STTx const& tx, TER const, XRPAmount const, beast::Journal const& j)
 {
     if (bad_)
     {
@@ -224,7 +293,7 @@ AccountRootsNotDeleted::visitEntry(
 }
 
 bool
-AccountRootsNotDeleted::finalize(STTx const&, TER, beast::Journal const& j)
+AccountRootsNotDeleted::finalize(STTx const&, TER const, XRPAmount const, beast::Journal const& j)
 {
     if (! accountDeleted_)
         return true;
@@ -270,7 +339,7 @@ LedgerEntryTypesMatch::visitEntry(
 }
 
 bool
-LedgerEntryTypesMatch::finalize(STTx const&, TER, beast::Journal const& j)
+LedgerEntryTypesMatch::finalize(STTx const&, TER const, XRPAmount const, beast::Journal const& j)
 {
     if ((! typeMismatch_) && (! invalidTypeAdded_))
         return true;
@@ -309,7 +378,7 @@ NoXRPTrustLines::visitEntry(
 }
 
 bool
-NoXRPTrustLines::finalize(STTx const&, TER, beast::Journal const& j)
+NoXRPTrustLines::finalize(STTx const&, TER const, XRPAmount const, beast::Journal const& j)
 {
     if (! xrpTrustLine_)
         return true;

src/ripple/app/tx/impl/InvariantCheck.h

@@ -25,7 +25,9 @@
 #include <ripple/protocol/STTx.h>
 #include <ripple/protocol/TER.h>
 #include <ripple/beast/utility/Journal.h>
+#include <map>
 #include <tuple>
+#include <utility>
 #include <cstdint>
 
 namespace ripple {
@@ -65,6 +67,7 @@ public:
      *
      * @param tx the transaction being applied
      * @param tec the current TER result of the transaction
+     * @param fee the fee actually charged for this transaction
      * @param j journal for logging
      *
      * @return true if check passes, false if it fails
@@ -72,33 +75,45 @@ public:
     bool
     finalize(
         STTx const& tx,
-        TER tec,
+        TER const tec,
+        XRPAmount const fee,
         beast::Journal const& j);
 };
 #endif
 
+/**
+ * @brief Invariant: We should never charge a transaction a negative fee or a
+ * fee that is larger than what the transaction itself specifies.
+ *
+ * We can, in some circumstances, charge less.
+ */
+class TransactionFeeCheck
+{
+public:
+    void
+    visitEntry(
+        uint256 const&,
+        bool,
+        std::shared_ptr<SLE const> const&,
+        std::shared_ptr<SLE const> const&);
+
+    bool
+    finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&);
+};
+
 /**
  * @brief Invariant: A transaction must not create XRP and should only destroy
- * XRP, up to the transaction fee.
+ * the XRP fee.
- *
- * For this check, we start with a signed 64-bit integer set to zero. As we go
- * through the ledger entries, look only at account roots, escrow payments,
- * and payment channels.  Remove from the total any previous XRP values and add
- * to the total any new XRP values. The net balance of a payment channel is
- * computed from two fields (amount and balance) and deletions are ignored
- * for paychan and escrow because the amount fields have not been adjusted for
- * those in the case of deletion.
- *
- * The final total must be less than or equal to zero and greater than or equal
- * to the negative of the tx fee.
  *
+ * We iterate through all account roots, payment channels and escrow entries
+ * that were modified and calculate the net change in XRP caused by the
+ * transactions.
  */
 class XRPNotCreated
 {
     std::int64_t drops_ = 0;
 
 public:
-
     void
     visitEntry(
         uint256 const&,
@@ -107,20 +122,21 @@ public:
         std::shared_ptr<SLE const> const&);
 
     bool
-    finalize(STTx const&, TER, beast::Journal const&);
+    finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&);
 };
 
 /**
  * @brief Invariant: we cannot remove an account ledger entry
  *
- * an account root should never be the target of a delete
+ * We iterate all accounts roots that were modified, and ensure that any that
+ * were present before the transaction was applied continue to be present
+ * afterwards.
  */
 class AccountRootsNotDeleted
 {
     bool accountDeleted_ = false;
 
 public:
-
     void
     visitEntry(
         uint256 const&,
@@ -129,12 +145,15 @@ public:
         std::shared_ptr<SLE const> const&);
 
     bool
-    finalize(STTx const&, TER, beast::Journal const&);
+    finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&);
 };
 
 /**
  * @brief Invariant: An account XRP balance must be in XRP and take a value
-                     between 0 and SYSTEM_CURRENCY_START drops, inclusive.
+ *                   between 0 and SYSTEM_CURRENCY_START drops, inclusive.
+ *
+ * We iterate all account roots modified by the transaction and ensure that
+ * their XRP balances are reasonable.
  */
 class XRPBalanceChecks
 {
@@ -149,7 +168,7 @@ public:
         std::shared_ptr<SLE const> const&);
 
     bool
-    finalize(STTx const&, TER, beast::Journal const&);
+    finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&);
 };
 
 /**
@@ -162,7 +181,6 @@ class LedgerEntryTypesMatch
     bool invalidTypeAdded_ = false;
 
 public:
-
     void
     visitEntry(
         uint256 const&,
@@ -171,18 +189,20 @@ public:
         std::shared_ptr<SLE const> const&);
 
     bool
-    finalize(STTx const&, TER, beast::Journal const&);
+    finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&);
 };
 
 /**
  * @brief Invariant: Trust lines using XRP are not allowed.
+ *
+ * We iterate all the trust lines created by this transaction and ensure
+ * that they are against a valid issuer.
  */
 class NoXRPTrustLines
 {
     bool xrpTrustLine_ = false;
 
 public:
-
     void
     visitEntry(
         uint256 const&,
@@ -191,19 +211,21 @@ public:
         std::shared_ptr<SLE const> const&);
 
     bool
-    finalize(STTx const&, TER, beast::Journal const&);
+    finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&);
 };
 
 /**
  * @brief Invariant: offers should be for non-negative amounts and must not
  *                   be XRP to XRP.
+ *
+ * Examine all offers modified by the transaction and ensure that there are
+ * no offers which contain negative amounts or which exchange XRP for XRP.
  */
 class NoBadOffers
 {
     bool bad_ = false;
 
 public:
-
     void
     visitEntry(
         uint256 const&,
@@ -212,7 +234,7 @@ public:
         std::shared_ptr<SLE const> const&);
 
     bool
-    finalize(STTx const&, TER, beast::Journal const&);
+    finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&);
 
 };
 
@@ -225,7 +247,6 @@ class NoZeroEscrow
     bool bad_ = false;
 
 public:
-
     void
     visitEntry(
         uint256 const&,
@@ -234,13 +255,14 @@ public:
         std::shared_ptr<SLE const> const&);
 
     bool
-    finalize(STTx const&, TER, beast::Journal const&);
+    finalize(STTx const&, TER const, XRPAmount const, beast::Journal const&);
 
 };
 
 // additional invariant checks can be declared above and then added to this
 // tuple
 using InvariantChecks = std::tuple<
+    TransactionFeeCheck,
     AccountRootsNotDeleted,
     LedgerEntryTypesMatch,
     XRPBalanceChecks,

src/ripple/app/tx/impl/Transactor.cpp

@@ -317,9 +317,10 @@ TER Transactor::apply ()
 
         setSeq();
 
-        auto terResult = payFee ();
+        auto result = payFee ();
 
-        if (terResult != tesSUCCESS) return terResult;
+        if (result  != tesSUCCESS)
+            return result;
 
         view().update (sle);
     }
@@ -350,8 +351,8 @@ Transactor::checkSingleSign (PreclaimContext const& ctx)
         keylet::account(id));
     auto const hasAuthKey     = sle->isFieldPresent (sfRegularKey);
 
-    // Consistency: Check signature
+    // Consistency: Check signature & verify the transaction's signing
-    // Verify the transaction's signing public key is authorized for signing.
+    // public key is authorized for signing.
     auto const spk = ctx.tx.getSigningPubKey();
     if (!publicKeyType (makeSlice (spk)))
     {
@@ -568,8 +569,9 @@ void removeUnfundedOffers (ApplyView& view, std::vector<uint256> const& offers,
     }
 }
 
-void
+/** Reset the context, discarding any changes made and adjust the fee */
-Transactor::claimFee (XRPAmount& fee, TER terResult, std::vector<uint256> const& removedOffers)
+XRPAmount
+Transactor::reset(XRPAmount fee)
 {
     ctx_.discard();
 
@@ -578,33 +580,29 @@ Transactor::claimFee (XRPAmount& fee, TER terResult, std::vector<uint256> const&
 
     auto const balance = txnAcct->getFieldAmount (sfBalance).xrp ();
 
-    // balance should have already been
+    // balance should have already been checked in checkFee / preFlight.
-    // checked in checkFee / preFlight.
     assert(balance != zero && (!view().open() || balance >= fee));
-    // We retry/reject the transaction if the account
+
-    // balance is zero or we're applying against an open
+    // We retry/reject the transaction if the account balance is zero or we're
-    // ledger and the balance is less than the fee
+    // applying against an open ledger and the balance is less than the fee
     if (fee > balance)
         fee = balance;
+
+    // Since we reset the context, we need to charge the fee and update
+    // the account's sequence number again.
     txnAcct->setFieldAmount (sfBalance, balance - fee);
     txnAcct->setFieldU32 (sfSequence, ctx_.tx.getSequence() + 1);
 
-    if (terResult == tecOVERSIZE)
-        removeUnfundedOffers (view(), removedOffers, ctx_.app.journal ("View"));
-
     view().update (txnAcct);
+
+    return fee;
 }
 
 //------------------------------------------------------------------------------
 std::pair<TER, bool>
 Transactor::operator()()
 {
-    JLOG(j_.trace()) <<
+    JLOG(j_.trace()) << "apply: " << ctx_.tx.getTransactionID ();
-        "applyTransaction>";
-
-    auto const txID = ctx_.tx.getTransactionID ();
-
-    JLOG(j_.debug()) << "Transactor for id: " << txID;
 
 #ifdef BEAST_DEBUG
     {
@@ -624,42 +622,31 @@ Transactor::operator()()
     }
 #endif
 
-    auto terResult = ctx_.preclaimResult;
+    auto result = ctx_.preclaimResult;
-    if (terResult == tesSUCCESS)
+    if (result == tesSUCCESS)
-        terResult = apply();
+        result = apply();
 
     // No transaction can return temUNKNOWN from apply,
     // and it can't be passed in from a preclaim.
-    assert(terResult != temUNKNOWN);
+    assert(result != temUNKNOWN);
 
-    if (auto stream = j_.debug())
+    if (auto stream = j_.trace())
-    {
+        stream << "preclaim result: " << transToken(result);
-        std::string strToken;
-        std::string strHuman;
 
-        transResultInfo (terResult, strToken, strHuman);
+    bool applied = isTesSuccess (result);
-
-        stream <<
-            "applyTransaction: terResult=" << strToken <<
-            " : " << terResult <<
-            " : " << strHuman;
-    }
-
-    bool didApply = isTesSuccess (terResult);
     auto fee = ctx_.tx.getFieldAmount(sfFee).xrp ();
 
     if (ctx_.size() > oversizeMetaDataCap)
-        terResult = tecOVERSIZE;
+        result = tecOVERSIZE;
 
-    if ((terResult == tecOVERSIZE) ||
+    if ((result == tecOVERSIZE) ||
-        (isTecClaim (terResult) && !(view().flags() & tapRETRY)))
+        (isTecClaim (result) && !(view().flags() & tapRETRY)))
     {
-        // only claim the transaction fee
+        JLOG(j_.trace()) << "reapplying because of " << transToken(result);
-        JLOG(j_.debug()) <<
-            "Reprocessing tx " << txID << " to only claim fee";
 
         std::vector<uint256> removedOffers;
-        if (terResult == tecOVERSIZE)
+
+        if (result == tecOVERSIZE)
         {
             ctx_.visit (
                 [&removedOffers](
@@ -682,62 +669,64 @@ Transactor::operator()()
                 });
         }
 
-        claimFee(fee, terResult, removedOffers);
+        // Reset the context, potentially adjusting the fee
-        didApply = true;
+        fee = reset(fee);
+
+        // If necessary, remove any offers found unfunded during processing
+        if (result == tecOVERSIZE)
+            removeUnfundedOffers (view(), removedOffers, ctx_.app.journal ("View"));
+
+        applied = true;
     }
 
-    if (didApply)
+    if (applied)
     {
-        // Check invariants
+        // Check invariants: if `tecINVARIANT_FAILED` is not returned, we can
-        // if `tecINVARIANT_FAILED` not returned, we can proceed to apply the tx
+        // proceed to apply the tx
-        terResult = ctx_.checkInvariants(terResult);
+        result = ctx_.checkInvariants(result, fee);
-        if (terResult == tecINVARIANT_FAILED)
+
+        if (result == tecINVARIANT_FAILED)
         {
-            // if invariants failed, claim a fee still
+            // if invariants checking failed again, reset the context and
-            claimFee(fee, terResult, {});
+            // attempt to only claim a fee.
-            //Check invariants *again* to ensure the fee claiming doesn't
+            fee = reset(fee);
+
+            // Check invariants again to ensure the fee claiming doesn't
             // violate invariants.
-            terResult = ctx_.checkInvariants(terResult);
+            result = ctx_.checkInvariants(result, fee);
-            didApply = isTecClaim(terResult);
-        }
         }
 
-    if (didApply)
+        // We ran through the invariant checker, which can, in some cases,
-    {
+        // return a tef error code. Don't apply the transaction in that case.
-        // Transaction succeeded fully or (retries are
+        if (!isTecClaim(result) && !isTesSuccess(result))
-        // not allowed and the transaction could claim a fee)
+            applied = false;
+    }
 
-        if (!view().open())
+    if (applied)
     {
-            // Charge whatever fee they specified.
+        // Transaction succeeded fully or (retries are not allowed and the
+        // transaction could claim a fee)
 
-            // The transactor guarantees this will never trigger
+        // 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 < zero)
-            {
+            Throw<std::logic_error> ("fee charged is negative!");
-                // VFALCO Log to journal here
-                // JLOG(journal.fatal()) << "invalid fee";
-                Throw<std::logic_error> ("amount is negative!");
-            }
 
-            if (fee != zero)
+        // 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 != zero)
             ctx_.destroyXRP (fee);
+
+        // Once we call apply, we will no longer be able to look at view()
+        ctx_.apply(result);
     }
 
-        ctx_.apply(terResult);
+    JLOG(j_.trace()) << (applied ? "applied" : "not applied") << transToken(result);
-        // since we called apply(), it is not okay to look
-        // at view() past this point.
-    }
-    else
-    {
-        JLOG(j_.debug()) << "Not applying transaction " << txID;
-    }
 
-
+    return { result, applied };
-    JLOG(j_.trace()) <<
-        "apply: " << transToken(terResult) <<
-        ", " << (didApply ? "true" : "false");
-
-    return { terResult, didApply };
 }
 
 }

src/ripple/app/tx/impl/Transactor.h

@@ -171,9 +171,10 @@ protected:
     virtual TER doApply () = 0;
 
 private:
+    XRPAmount reset(XRPAmount fee);
+
     void setSeq ();
     TER payFee ();
-    void claimFee (XRPAmount& fee, TER terResult, std::vector<uint256> const& removedOffers);
     static NotTEC checkSingleSign (PreclaimContext const& ctx);
     static NotTEC checkMultiSign (PreclaimContext const& ctx);
 };

src/test/ledger/Invariants_test.cpp

@@ -52,15 +52,21 @@ class Invariants_test : public beast::unit_test::suite
     // this is common setup/method for running a failing invariant check. The
     // precheck function is used to manipulate the ApplyContext with view
     // changes that will cause the check to fail.
-    void
+    using Precheck = std::function <
-    doInvariantCheck( bool enabled,
-        std::vector<std::string> const& expect_logs,
-        std::function <
         bool (
             test::jtx::Account const& a,
             test::jtx::Account const& b,
-                ApplyContext& ac)>
+            ApplyContext& ac)>;
-            const& precheck )
+
+    using TXMod = std::function <
+        void (STTx& tx)>;
+
+    void
+    doInvariantCheck( bool enabled,
+        std::vector<std::string> const& expect_logs,
+        Precheck const& precheck,
+        XRPAmount fee = XRPAmount{},
+        TXMod txmod = [](STTx&){})
     {
         using namespace test::jtx;
         Env env {*this};
@@ -79,6 +85,7 @@ class Invariants_test : public beast::unit_test::suite
 
         // dummy/empty tx to setup the AccountContext
         auto tx = STTx {ttACCOUNT_SET, [](STObject&){  } };
+        txmod(tx);
         OpenView ov {*env.current()};
         TestSink sink;
         beast::Journal jlog {sink};
@@ -98,7 +105,7 @@ class Invariants_test : public beast::unit_test::suite
         // invoke check twice to cover tec and tef cases
         for (auto i : {0,1})
         {
-            tr = ac.checkInvariants(tr);
+            tr = ac.checkInvariants(tr, fee);
             if (enabled)
             {
                 BEAST_EXPECT(tr == (i == 0
@@ -144,7 +151,7 @@ class Invariants_test : public beast::unit_test::suite
         testcase << "checks " << (enabled ? "enabled" : "disabled") <<
             " - XRP created";
         doInvariantCheck (enabled,
-            {{ "XRP net change was 500 on a fee of 0" }},
+            {{ "XRP net change was positive: 500" }},
             [](Account const& A1, Account const&, ApplyContext& ac)
             {
                 // put a single account in the view and "manufacture" some XRP
@@ -185,7 +192,7 @@ class Invariants_test : public beast::unit_test::suite
             " - LE types don't match";
         doInvariantCheck (enabled,
             {{ "ledger entry type mismatch" },
-             { "XRP net change was -1000000000 on a fee of 0" }},
+             { "XRP net change of -1000000000 doesn't match fee 0" }},
             [](Account const& A1, Account const&, ApplyContext& ac)
             {
                 // replace an entry in the table with an SLE of a different type
@@ -258,7 +265,7 @@ class Invariants_test : public beast::unit_test::suite
 
         doInvariantCheck (enabled,
             {{ "incorrect account XRP balance" },
-             {  "XRP net change was 99999999000000001 on a fee of 0" }},
+             {  "XRP net change was positive: 99999999000000001" }},
             [](Account const& A1, Account const&, ApplyContext& ac)
             {
                 // balance exceeds genesis amount
@@ -272,7 +279,7 @@ class Invariants_test : public beast::unit_test::suite
 
         doInvariantCheck (enabled,
             {{ "incorrect account XRP balance" },
-             { "XRP net change was -1000000001 on a fee of 0" }},
+             { "XRP net change of -1000000001 doesn't match fee 0" }},
             [](Account const& A1, Account const&, ApplyContext& ac)
             {
                 // balance is negative
@@ -285,6 +292,37 @@ class Invariants_test : public beast::unit_test::suite
             });
     }
 
+    void
+    testTransactionFeeCheck(bool enabled)
+    {
+        using namespace test::jtx;
+        using namespace std::string_literals;
+        testcase << "checks " << (enabled ? "enabled" : "disabled") <<
+            " - Transaction fee checks";
+
+        doInvariantCheck (enabled,
+            {{ "fee paid was negative: -1" },
+             { "XRP net change of 0 doesn't match fee -1" }},
+            [](Account const&, Account const&, ApplyContext&) { return true; },
+            XRPAmount{-1});
+
+        doInvariantCheck (enabled,
+            {{ "fee paid exceeds system limit: "s +
+                std::to_string(SYSTEM_CURRENCY_START) },
+             { "XRP net change of 0 doesn't match fee "s +
+                std::to_string(SYSTEM_CURRENCY_START) }},
+            [](Account const&, Account const&, ApplyContext&) { return true; },
+            XRPAmount{SYSTEM_CURRENCY_START});
+
+         doInvariantCheck (enabled,
+            {{ "fee paid is 20 exceeds fee specified in transaction." },
+             { "XRP net change of 0 doesn't match fee 20" }},
+            [](Account const&, Account const&, ApplyContext&) { return true; },
+            XRPAmount{20},
+            [](STTx& tx) { tx.setFieldAmount(sfFee, XRPAmount{10}); } );
+    }
+
+
     void
     testNoBadOffers(bool enabled)
     {
@@ -373,7 +411,7 @@ class Invariants_test : public beast::unit_test::suite
             });
 
         doInvariantCheck (enabled,
-            {{ "XRP net change was -1000000 on a fee of 0"},
+            {{ "XRP net change of -1000000 doesn't match fee 0"},
              {  "escrow specifies invalid amount" }},
             [](Account const& A1, Account const&, ApplyContext& ac)
             {
@@ -389,7 +427,7 @@ class Invariants_test : public beast::unit_test::suite
             });
 
         doInvariantCheck (enabled,
-            {{ "XRP net change was 100000000000000001 on a fee of 0" },
+            {{ "XRP net change was positive: 100000000000000001" },
              {  "escrow specifies invalid amount" }},
             [](Account const& A1, Account const&, ApplyContext& ac)
             {
@@ -419,6 +457,7 @@ public:
             testTypesMatch (b);
             testNoXRPTrustLine (b);
             testXRPBalanceCheck (b);
+            testTransactionFeeCheck(b);
             testNoBadOffers (b);
             testNoZeroEscrow (b);
         }