Change how fail_hard transactions are handled.

FIXES: #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.

[FOLD] Keep tec results out of the open ledger when fail_hard:

* Improve TransactionStatus const correctness, and remove redundant
  `local` check
* Check open ledger tx count in fail_hard tests
* Fix some wrapping
* Remove duplicate test

src/ripple/app/misc/NetworkOPs.cpp

@@ -77,10 +77,10 @@ class NetworkOPsImp final
     class TransactionStatus
     {
     public:
-        std::shared_ptr<Transaction> transaction;
-        bool admin;
-        bool local;
-        FailHard failType;
+        std::shared_ptr<Transaction> 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<std::mutex>& 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<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
@@ -1145,11 +1151,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())
@@ -1166,17 +1168,20 @@ void NetworkOPsImp::apply (std::unique_lock<std::mutex>& 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(),
                     e.transaction->getSTransaction());
                 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());

src/ripple/app/misc/TxQ.h

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

@@ -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<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
@@ -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 " <<

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);

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

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

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,81 @@ 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 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 @@ class Env_test : public beast::unit_test::suite
         testRequire();
         testKeyType();
         testPayments();
+        testFailHard();
         testMultiSign();
         testTicket();
         testJTxProperties();