Change how fail_hard transactions are handled.

FIXES: XRPLF#2847

* Transactions that are submitted with the fail_hard flag
  and that result in any TER code besides tesSUCCESS shall
  be neither queued nor held.

src/ripple/app/misc/NetworkOPs.cpp

@@ -1055,7 +1055,10 @@ void NetworkOPsImp::apply (std::unique_lock<std::mutex>& batchLock)
                     // we check before adding to the batch
                     ApplyFlags flags = tapNONE;
                     if (e.admin)
-                        flags = flags | tapUNLIMITED;
+                        flags |= tapUNLIMITED;
+
+                    if (e.local && e.failType == FailHard::yes)
+                        flags |= tapFAIL_HARD;
 
                     auto const result = app_.getTxQ().apply(
                         app_, view, e.transaction->getSTransaction(),
@@ -1126,8 +1129,9 @@ void NetworkOPsImp::apply (std::unique_lock<std::mutex>& 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
@@ -1136,11 +1140,7 @@ void NetworkOPsImp::apply (std::unique_lock<std::mutex>& 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())
@@ -1156,16 +1156,18 @@ void NetworkOPsImp::apply (std::unique_lock<std::mutex>& batchLock)
                 e.transaction->setStatus (INVALID);
             }
 
-            if (addLocal)
+            auto const enforceFailHard = e.failType == FailHard::yes && e.result != tesSUCCESS;
+
+            if (addLocal && !enforceFailHard)
             {
                 m_localTX->push_back (
                     m_ledgerMaster.getCurrentLedgerIndex(),
                     e.transaction->getSTransaction());
             }
 
-            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());

src/ripple/app/misc/TxQ.h

@@ -697,7 +697,7 @@ class TxQ
     /** 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<FeeMultiSet::iterator>);
 

src/ripple/app/misc/impl/TxQ.cpp

@@ -358,16 +358,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<FeeMultiSet::iterator> 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
@@ -795,7 +797,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();
             }
 
@@ -1044,7 +1046,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 " <<

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,

src/ripple/protocol/TER.h

@@ -75,7 +75,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,

src/test/jtx/Env_test.cpp

@@ -17,14 +17,15 @@
 */
 //==============================================================================
 
-#include <ripple/basics/Log.h>
 #include <test/jtx.h>
 #include <ripple/json/to_string.h>
 #include <ripple/protocol/Feature.h>
 #include <ripple/protocol/jss.h>
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/beast/hash/uhash.h>
 #include <ripple/beast/unit_test.h>
+#include <ripple/app/misc/NetworkOPs.h>
+#include <ripple/app/misc/TxQ.h>
 #include <boost/lexical_cast.hpp>
 #include <boost/optional.hpp>
 #include <utility>
@@ -348,6 +349,79 @@ class Env_test : public beast::unit_test::suite
         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 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);
+
+        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);
+
+        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);
+
+        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);
+
+        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);
+
+        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);
+
+        jr = applyTxn(noop (alice));
+
+        BEAST_EXPECT(jr[jss::result][jss::engine_result] == "tesSUCCESS");
+        BEAST_EXPECT(env.app().getOPs().getLocalTxCount () > localTxCnt);
+    }
+
     // Multi-sign basics
     void
     testMultiSign()
@@ -774,6 +848,7 @@ class Env_test : public beast::unit_test::suite
         testRequire();
         testKeyType();
         testPayments();
+        testFailHard();
         testMultiSign();
         testTicket();
         testJTxProperties();