diff --git a/src/ripple/app/tx/impl/ApplyContext.cpp b/src/ripple/app/tx/impl/ApplyContext.cpp index 59f0a07e3..3468b04a9 100644 --- a/src/ripple/app/tx/impl/ApplyContext.cpp +++ b/src/ripple/app/tx/impl/ApplyContext.cpp @@ -70,9 +70,25 @@ ApplyContext::visit (std::function 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 TER -ApplyContext::checkInvariantsHelper(TER terResult, std::index_sequence) +ApplyContext::checkInvariantsHelper( + TER const result, + XRPAmount const fee, + std::index_sequence) { if (view_->rules().enabled(featureEnforceInvariants)) { @@ -97,40 +113,40 @@ ApplyContext::checkInvariantsHelper(TER terResult, std::index_sequence) // Sean Parent for_each_argument trick (a fold expression with `&&` // would be really nice here when we move to C++-17) std::array finalizers {{ - std::get(checkers).finalize(tx, terResult, journal)...}}; + std::get(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( - terResult, std::make_index_sequence::value>{}); + assert (isTesSuccess(result) || isTecClaim(result)); + + return checkInvariantsHelper(result, fee, + std::make_index_sequence::value>{}); } } // ripple diff --git a/src/ripple/app/tx/impl/ApplyContext.h b/src/ripple/app/tx/impl/ApplyContext.h index a4cfb8cf5..53190931d 100644 --- a/src/ripple/app/tx/impl/ApplyContext.h +++ b/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 - TER checkInvariantsHelper(TER terResult, std::index_sequence); + TER + checkInvariantsHelper(TER const result, XRPAmount const fee, std::index_sequence); OpenView& base_; ApplyFlags flags_; diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index dfe22bcba..fd97b43f2 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -22,6 +22,52 @@ namespace ripple { +void +TransactionFeeCheck::visitEntry( + uint256 const&, + bool, + std::shared_ptr const&, + std::shared_ptr 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 const& before, std::shared_ptr 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,15 +122,31 @@ 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(); - if(-1*fee <= drops_ && drops_ <= 0) - return true; + // The net change should never be positive, as this would mean that the + // transaction created XRP out of thin air. That's not possible. + if (drops_ > 0) + { + JLOG(j.fatal()) << + "Invariant failed: XRP net change was positive: " << drops_; + return false; + } - JLOG(j.fatal()) << "Invariant failed: XRP net change was " << drops_ << - " on a fee of " << fee; - 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; } //------------------------------------------------------------------------------ @@ -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; diff --git a/src/ripple/app/tx/impl/InvariantCheck.h b/src/ripple/app/tx/impl/InvariantCheck.h index c494e7470..b1a2fbca3 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.h +++ b/src/ripple/app/tx/impl/InvariantCheck.h @@ -25,7 +25,9 @@ #include #include #include +#include #include +#include #include 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 const&, + std::shared_ptr 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. - * - * 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. + * the XRP 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 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 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 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 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 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,8 +234,8 @@ public: std::shared_ptr 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 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, diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index e6a2e1faa..dcae49e31 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/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 - // Verify the transaction's signing public key is authorized for signing. + // Consistency: Check signature & verify the transaction's 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 const& offers, } } -void -Transactor::claimFee (XRPAmount& fee, TER terResult, std::vector const& removedOffers) +/** Reset the context, discarding any changes made and adjust the fee */ +XRPAmount +Transactor::reset(XRPAmount fee) { ctx_.discard(); @@ -578,33 +580,29 @@ Transactor::claimFee (XRPAmount& fee, TER terResult, std::vector const& auto const balance = txnAcct->getFieldAmount (sfBalance).xrp (); - // balance should have already been - // checked in checkFee / preFlight. + // balance should have already been 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 - // ledger and the balance is less than the fee + + // We retry/reject the transaction if the account balance is zero or we're + // 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 Transactor::operator()() { - JLOG(j_.trace()) << - "applyTransaction>"; - - auto const txID = ctx_.tx.getTransactionID (); - - JLOG(j_.debug()) << "Transactor for id: " << txID; + JLOG(j_.trace()) << "apply: " << ctx_.tx.getTransactionID (); #ifdef BEAST_DEBUG { @@ -624,42 +622,31 @@ Transactor::operator()() } #endif - auto terResult = ctx_.preclaimResult; - if (terResult == tesSUCCESS) - terResult = apply(); + auto result = ctx_.preclaimResult; + if (result == tesSUCCESS) + 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()) - { - std::string strToken; - std::string strHuman; + if (auto stream = j_.trace()) + stream << "preclaim result: " << transToken(result); - transResultInfo (terResult, strToken, strHuman); - - stream << - "applyTransaction: terResult=" << strToken << - " : " << terResult << - " : " << strHuman; - } - - bool didApply = isTesSuccess (terResult); + bool applied = isTesSuccess (result); auto fee = ctx_.tx.getFieldAmount(sfFee).xrp (); if (ctx_.size() > oversizeMetaDataCap) - terResult = tecOVERSIZE; + result = tecOVERSIZE; - if ((terResult == tecOVERSIZE) || - (isTecClaim (terResult) && !(view().flags() & tapRETRY))) + if ((result == tecOVERSIZE) || + (isTecClaim (result) && !(view().flags() & tapRETRY))) { - // only claim the transaction fee - JLOG(j_.debug()) << - "Reprocessing tx " << txID << " to only claim fee"; + JLOG(j_.trace()) << "reapplying because of " << transToken(result); std::vector removedOffers; - if (terResult == tecOVERSIZE) + + if (result == tecOVERSIZE) { ctx_.visit ( [&removedOffers]( @@ -682,62 +669,64 @@ Transactor::operator()() }); } - claimFee(fee, terResult, removedOffers); - didApply = true; + // Reset the context, potentially adjusting the fee + 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 - // if `tecINVARIANT_FAILED` not returned, we can proceed to apply the tx - terResult = ctx_.checkInvariants(terResult); - if (terResult == tecINVARIANT_FAILED) + // Check invariants: if `tecINVARIANT_FAILED` is not returned, we can + // proceed to apply the tx + result = ctx_.checkInvariants(result, fee); + + if (result == tecINVARIANT_FAILED) { - // if invariants failed, claim a fee still - claimFee(fee, terResult, {}); - //Check invariants *again* to ensure the fee claiming doesn't - //violate invariants. - terResult = ctx_.checkInvariants(terResult); - didApply = isTecClaim(terResult); - } - } + // if invariants checking failed again, reset the context and + // attempt to only claim a fee. + fee = reset(fee); - if (didApply) - { - // Transaction succeeded fully or (retries are - // not allowed and the transaction could claim a fee) - - if (!view().open()) - { - // Charge whatever fee they specified. - - // The transactor guarantees this will never trigger - if (fee < zero) - { - // VFALCO Log to journal here - // JLOG(journal.fatal()) << "invalid fee"; - Throw ("amount is negative!"); - } - - if (fee != zero) - ctx_.destroyXRP (fee); + // Check invariants again to ensure the fee claiming doesn't + // violate invariants. + result = ctx_.checkInvariants(result, fee); } - ctx_.apply(terResult); - // since we called apply(), it is not okay to look - // at view() past this point. + // 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; } - else + + if (applied) { - JLOG(j_.debug()) << "Not applying transaction " << txID; + // 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 < zero) + Throw ("fee charged is negative!"); + + // 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); } + JLOG(j_.trace()) << (applied ? "applied" : "not applied") << transToken(result); - JLOG(j_.trace()) << - "apply: " << transToken(terResult) << - ", " << (didApply ? "true" : "false"); - - return { terResult, didApply }; + return { result, applied }; } } diff --git a/src/ripple/app/tx/impl/Transactor.h b/src/ripple/app/tx/impl/Transactor.h index 9ea5c7b0b..15ee64223 100644 --- a/src/ripple/app/tx/impl/Transactor.h +++ b/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 const& removedOffers); static NotTEC checkSingleSign (PreclaimContext const& ctx); static NotTEC checkMultiSign (PreclaimContext const& ctx); }; diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index 8919b9030..c5ff191de 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/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. + using Precheck = std::function < + bool ( + test::jtx::Account const& a, + test::jtx::Account const& b, + ApplyContext& ac)>; + + using TXMod = std::function < + void (STTx& tx)>; + void doInvariantCheck( bool enabled, std::vector const& expect_logs, - std::function < - bool ( - test::jtx::Account const& a, - test::jtx::Account const& b, - ApplyContext& ac)> - const& precheck ) + 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); }