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,7 +1168,10 @@ 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(),
@@ -1174,9 +1179,9 @@ void NetworkOPsImp::apply (std::unique_lock<std::mutex>& 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());

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