diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 51f99ede9..6b22faab6 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -77,10 +77,10 @@ class NetworkOPsImp final class TransactionStatus { public: - std::shared_ptr transaction; - bool admin; - bool local; - FailHard failType; + std::shared_ptr const transaction; + bool const admin; + bool const local; + FailHard const failType; bool applied; TER result; @@ -93,7 +93,9 @@ class NetworkOPsImp final , admin (a) , local (l) , failType (f) - {} + { + assert(local || failType == FailHard::no); + } }; /** @@ -1055,7 +1057,10 @@ void NetworkOPsImp::apply (std::unique_lock& batchLock) // we check before adding to the batch ApplyFlags flags = tapNONE; if (e.admin) - flags = flags | tapUNLIMITED; + flags |= tapUNLIMITED; + + if (e.failType == FailHard::yes) + flags |= tapFAIL_HARD; auto const result = app_.getTxQ().apply( app_, view, e.transaction->getSTransaction(), @@ -1133,8 +1138,9 @@ void NetworkOPsImp::apply (std::unique_lock& batchLock) } else if (e.result == terQUEUED) { - JLOG(m_journal.debug()) << "Transaction is likely to claim a " << - "fee, but is queued until fee drops"; + JLOG(m_journal.debug()) << "Transaction is likely to claim a" + << " fee, but is queued until fee drops"; + e.transaction->setStatus(HELD); // Add to held transactions, because it could get // kicked out of the queue, and this will try to @@ -1145,11 +1151,7 @@ void NetworkOPsImp::apply (std::unique_lock& batchLock) } else if (isTerRetry (e.result)) { - if (e.failType == FailHard::yes) - { - addLocal = false; - } - else + if (e.failType != FailHard::yes) { // transaction should be held JLOG(m_journal.debug()) @@ -1166,7 +1168,10 @@ void NetworkOPsImp::apply (std::unique_lock& batchLock) e.transaction->setStatus (INVALID); } - if (addLocal) + auto const enforceFailHard = e.failType == FailHard::yes && + !isTesSuccess(e.result); + + if (addLocal && !enforceFailHard) { m_localTX->push_back ( m_ledgerMaster.getCurrentLedgerIndex(), @@ -1174,9 +1179,9 @@ void NetworkOPsImp::apply (std::unique_lock& batchLock) e.transaction->setKept(); } - if (e.applied || ((mMode != OperatingMode::FULL) && + if ((e.applied || ((mMode != OperatingMode::FULL) && (e.failType != FailHard::yes) && e.local) || - (e.result == terQUEUED)) + (e.result == terQUEUED)) && !enforceFailHard) { auto const toSkip = app_.getHashRouter().shouldRelay( e.transaction->getID()); diff --git a/src/ripple/app/misc/TxQ.h b/src/ripple/app/misc/TxQ.h index 72640739f..a3e56bb0e 100644 --- a/src/ripple/app/misc/TxQ.h +++ b/src/ripple/app/misc/TxQ.h @@ -717,7 +717,7 @@ private: /** Checks if the indicated transaction fits the conditions for being stored in the queue. */ - bool canBeHeld(STTx const&, OpenView const&, + bool canBeHeld(STTx const&, ApplyFlags const, OpenView const&, AccountMap::iterator, boost::optional); diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index 7f0500f4d..a5e669b0b 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -360,16 +360,18 @@ TxQ::isFull() const } bool -TxQ::canBeHeld(STTx const& tx, OpenView const& view, +TxQ::canBeHeld(STTx const& tx, ApplyFlags const flags, OpenView const& view, AccountMap::iterator accountIter, boost::optional replacementIter) { // PreviousTxnID is deprecated and should never be used // AccountTxnID is not supported by the transaction // queue yet, but should be added in the future + // tapFAIL_HARD transactions are never held bool canBeHeld = ! tx.isFieldPresent(sfPreviousTxnID) && - ! tx.isFieldPresent(sfAccountTxnID); + ! tx.isFieldPresent(sfAccountTxnID) && + ! (flags & tapFAIL_HARD); if (canBeHeld) { /* To be queued and relayed, the transaction needs to @@ -798,7 +800,7 @@ TxQ::apply(Application& app, OpenView& view, // object to hold the info we need to adjust for // prior txns. Otherwise, let preclaim fail as if // we didn't have the queue at all. - if (canBeHeld(*tx, view, accountIter, replacedItemDeleteIter)) + if (canBeHeld(*tx, flags, view, accountIter, replacedItemDeleteIter)) multiTxn.emplace(); } @@ -1048,7 +1050,7 @@ TxQ::apply(Application& app, OpenView& view, // If `multiTxn` has a value, then `canBeHeld` has already been verified if (! multiTxn && - ! canBeHeld(*tx, view, accountIter, replacedItemDeleteIter)) + ! canBeHeld(*tx, flags, view, accountIter, replacedItemDeleteIter)) { // Bail, transaction cannot be held JLOG(j_.trace()) << "Transaction " << diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 0c093b1ed..bd52c8260 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -670,7 +670,15 @@ Transactor::operator()() if (ctx_.size() > oversizeMetaDataCap) result = tecOVERSIZE; - if ((result == tecOVERSIZE) || (result == tecKILLED) || + if (isTecClaim(result) && (view().flags() & tapFAIL_HARD)) + { + // If the tapFAIL_HARD flag is set, a tec result + // must not do anything + + ctx_.discard(); + applied = false; + } + else if ((result == tecOVERSIZE) || (result == tecKILLED) || (isTecClaimHardFail (result, view().flags()))) { JLOG(j_.trace()) << "reapplying because of " << transToken(result); diff --git a/src/ripple/ledger/ApplyView.h b/src/ripple/ledger/ApplyView.h index 131607178..bd5be0ac6 100644 --- a/src/ripple/ledger/ApplyView.h +++ b/src/ripple/ledger/ApplyView.h @@ -32,6 +32,10 @@ enum ApplyFlags { tapNONE = 0x00, + // This is a local transaction with the + // fail_hard flag set. + tapFAIL_HARD = 0x10, + // This is not the transaction's last pass // Transaction can be retried, soft failures allowed tapRETRY = 0x20, diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index d1e638be3..3c8181682 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -81,7 +81,7 @@ enum TEMcodes : TERUnderlyingType // - Not applied // - Not forwarded // - Reject - // - Can not succeed in any imagined ledger. + // - Cannot succeed in any imagined ledger. temMALFORMED = -299, temBAD_AMOUNT, diff --git a/src/test/jtx/Env_test.cpp b/src/test/jtx/Env_test.cpp index 86bf2db27..2ea9a335a 100644 --- a/src/test/jtx/Env_test.cpp +++ b/src/test/jtx/Env_test.cpp @@ -17,7 +17,6 @@ */ //============================================================================== -#include #include #include #include @@ -25,6 +24,8 @@ #include #include #include +#include +#include #include #include #include @@ -348,6 +349,81 @@ public: env(noop("alice")); } + // Rudimentary test to ensure fail_hard + // transactions are neither queued nor + // held. + void + testFailHard() + { + using namespace jtx; + Env env(*this); + auto const gw = Account("gateway"); + auto const USD = gw["USD"]; + + auto const alice = Account {"alice"}; + env.fund (XRP(10000), alice); + + auto const localTxCnt = env.app().getOPs().getLocalTxCount(); + auto const queueTxCount = env.app().getTxQ().getMetrics(*env.current()).txCount; + auto const openTxCount = env.current()->txCount(); + BEAST_EXPECT(localTxCnt == 2 && queueTxCount == 0 && openTxCount == 2); + + auto applyTxn = [&env](auto&& ...txnArgs) + { + auto jt = env.jt (txnArgs...); + Serializer s; + jt.stx->add (s); + + Json::Value args {Json::objectValue}; + + args[jss::tx_blob] = strHex (s.slice ()); + args[jss::fail_hard] = true; + + return env.rpc ("json", "submit", args.toStyledString()); + }; + + auto jr = applyTxn(noop (alice), fee (1)); + + BEAST_EXPECT(jr[jss::result][jss::engine_result] == "telINSUF_FEE_P"); + BEAST_EXPECT(env.app().getTxQ().getMetrics(*env.current()).txCount == queueTxCount); + BEAST_EXPECT(env.app().getOPs().getLocalTxCount () == localTxCnt); + BEAST_EXPECT(env.current()->txCount() == openTxCount); + + jr = applyTxn(noop (alice), sig("bob")); + + BEAST_EXPECT(jr[jss::result][jss::engine_result] == "tefBAD_AUTH"); + BEAST_EXPECT(env.app().getTxQ().getMetrics(*env.current()).txCount == queueTxCount); + BEAST_EXPECT(env.app().getOPs().getLocalTxCount () == localTxCnt); + BEAST_EXPECT(env.current()->txCount() == openTxCount); + + jr = applyTxn(noop (alice), seq(20)); + + BEAST_EXPECT(jr[jss::result][jss::engine_result] == "terPRE_SEQ"); + BEAST_EXPECT(env.app().getTxQ().getMetrics(*env.current()).txCount == queueTxCount); + BEAST_EXPECT(env.app().getOPs().getLocalTxCount () == localTxCnt); + BEAST_EXPECT(env.current()->txCount() == openTxCount); + + jr = applyTxn(offer(alice, XRP(1000), USD(1000))); + + BEAST_EXPECT(jr[jss::result][jss::engine_result] == "tecUNFUNDED_OFFER"); + BEAST_EXPECT(env.app().getTxQ().getMetrics(*env.current()).txCount == queueTxCount); + BEAST_EXPECT(env.app().getOPs().getLocalTxCount () == localTxCnt); + BEAST_EXPECT(env.current()->txCount() == openTxCount); + + jr = applyTxn(noop (alice), fee (drops (-10))); + + BEAST_EXPECT(jr[jss::result][jss::engine_result] == "temBAD_FEE"); + BEAST_EXPECT(env.app().getTxQ().getMetrics(*env.current()).txCount == queueTxCount); + BEAST_EXPECT(env.app().getOPs().getLocalTxCount () == localTxCnt); + BEAST_EXPECT(env.current()->txCount() == openTxCount); + + jr = applyTxn(noop (alice)); + + BEAST_EXPECT(jr[jss::result][jss::engine_result] == "tesSUCCESS"); + BEAST_EXPECT(env.app().getOPs().getLocalTxCount () == localTxCnt + 1); + BEAST_EXPECT(env.current()->txCount() == openTxCount + 1); + } + // Multi-sign basics void testMultiSign() @@ -774,6 +850,7 @@ public: testRequire(); testKeyType(); testPayments(); + testFailHard(); testMultiSign(); testTicket(); testJTxProperties();