Retried transactions that tec move from TxQ to open ledger:

* Unit test of tec code handling.
* Extra TxQ debug logging

src/ripple/app/consensus/RCLConsensus.cpp

@@ -654,7 +654,7 @@ RCLConsensus::Adaptor::buildLCL(
         if (replayData)
         {
             assert(replayData->parent()->info().hash == previousLedger.id());
-            return buildLedger(*replayData, tapNO_CHECK_SIGN, app_, j_);
+            return buildLedger(*replayData, tapNONE, app_, j_);
         }
         return buildLedger(
             previousLedger.ledger_,

src/ripple/app/ledger/impl/BuildLedger.cpp

@@ -139,7 +139,7 @@ applyTransactions(
             try
             {
                 switch (applyTransaction(
-                    app, view, *it->second, certainRetry, tapNO_CHECK_SIGN, j))
+                    app, view, *it->second, certainRetry, tapNONE, j))
                 {
                     case ApplyResult::Success:
                         it = retriableTxs.erase(it);

src/ripple/app/misc/NetworkOPs.cpp

@@ -1028,8 +1028,8 @@ void NetworkOPsImp::apply (std::unique_lock<std::mutex>& batchLock)
             {
                 for (TransactionStatus& e : transactions)
                 {
-                    // we check before addingto the batch
-                    ApplyFlags flags = tapNO_CHECK_SIGN;
+                    // we check before adding to the batch
+                    ApplyFlags flags = tapNONE;
                     if (e.admin)
                         flags = flags | tapUNLIMITED;
 

src/ripple/app/misc/TxQ.h

@@ -335,7 +335,7 @@ private:
                     PreflightResult const& pfresult);
 
         std::pair<TER, bool>
-        apply(Application& app, OpenView& view);
+        apply(Application& app, OpenView& view, beast::Journal j);
     };
 
     class GreaterFee

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

@@ -262,7 +262,7 @@ TxQ::MaybeTx::MaybeTx(
 }
 
 std::pair<TER, bool>
-TxQ::MaybeTx::apply(Application& app, OpenView& view)
+TxQ::MaybeTx::apply(Application& app, OpenView& view, beast::Journal j)
 {
     boost::optional<STAmountSO> saved;
     if (view.rules().enabled(fix1513))
@@ -272,6 +272,10 @@ TxQ::MaybeTx::apply(Application& app, OpenView& view)
     if (pfresult->rules != view.rules() ||
         pfresult->flags != flags)
     {
+        JLOG(j.debug()) << "Queued transaction " <<
+            txID << " rules or flags have changed. Flags from " <<
+            pfresult->flags << " to " << flags ;
+
         pfresult.emplace(
             preflight(app, view.rules(),
                 pfresult->tx,
@@ -504,7 +508,7 @@ TxQ::tryClearAccountQueue(Application& app, OpenView& view,
     // Attempt to apply the queued transactions.
     for (auto it = beginTxIter; it != endTxIter; ++it)
     {
-        auto txResult = it->second.apply(app, view);
+        auto txResult = it->second.apply(app, view, j);
         // Succeed or fail, use up a retry, because if the overall
         // process fails, we want the attempt to count. If it all
         // succeeds, the MaybeTx will be destructed, so it'll be
@@ -1009,7 +1013,7 @@ TxQ::apply(Application& app, OpenView& view,
 
         std::tie(txnResult, didApply) = doApply(pcresult, app, view);
 
-        JLOG(j_.trace()) << "Transaction " <<
+        JLOG(j_.trace()) << "New transaction " <<
             transactionID <<
                 (didApply ? " applied successfully with " :
                     " failed with ") <<
@@ -1111,6 +1115,17 @@ TxQ::apply(Application& app, OpenView& view,
         (void)created;
         assert(created);
     }
+    // Modify the flags for use when coming out of the queue.
+    // These changes _may_ cause an extra `preflight`, but as long as
+    // the `HashRouter` still knows about the transaction, the signature
+    // will not be checked again, so the cost should be minimal.
+
+    // Don't allow soft failures, which can lead to retries
+    flags &= ~tapRETRY;
+
+    // Don't queue because we're already in the queue
+    flags &= ~tapPREFER_QUEUE;
+
     auto& candidate = accountIter->second.add(
         { tx, transactionID, feeLevelPaid, flags, pfresult });
     /* Normally we defer figuring out the consequences until
@@ -1122,8 +1137,10 @@ TxQ::apply(Application& app, OpenView& view,
     // Then index it into the byFee lookup.
     byFee_.insert(candidate);
     JLOG(j_.debug()) << "Added transaction " << candidate.txID <<
+        " with result " << transToken(pfresult.ter) <<
         " from " << (accountExists ? "existing" : "new") <<
-            " account " << candidate.account << " to queue.";
+            " account " << candidate.account << " to queue." <<
+            " Flags: " << flags;
 
     return { terQUEUED, false };
 }
@@ -1278,14 +1295,15 @@ TxQ::accept(Application& app,
 
             TER txnResult;
             bool didApply;
-            std::tie(txnResult, didApply) = candidateIter->apply(app, view);
+            std::tie(txnResult, didApply) = candidateIter->apply(app, view, j_);
 
             if (didApply)
             {
                 // Remove the candidate from the queue
                 JLOG(j_.debug()) << "Queued transaction " <<
                     candidateIter->txID <<
-                    " applied successfully. Remove from queue.";
+                    " applied successfully with " <<
+                    transToken(txnResult) << ". Remove from queue.";
 
                 candidateIter = eraseAndAdvance(candidateIter);
                 ledgerChanged = true;
@@ -1304,9 +1322,12 @@ TxQ::accept(Application& app,
             }
             else
             {
-                JLOG(j_.debug()) << "Transaction " <<
+                JLOG(j_.debug()) << "Queued transaction " <<
                     candidateIter->txID << " failed with " <<
-                    transToken(txnResult) << ". Leave in queue.";
+                    transToken(txnResult) << ". Leave in queue." <<
+                    " Applied: " << didApply <<
+                    ". Flags: " <<
+                    candidateIter->flags;
                 if (account.retryPenalty &&
                         candidateIter->retriesRemaining > 2)
                     candidateIter->retriesRemaining = 1;

src/ripple/app/tx/applySteps.h

@@ -28,6 +28,16 @@ namespace ripple {
 class Application;
 class STTx;
 
+/** Return true if the transaction can claim a fee (tec),
+    and the `ApplyFlags` do not allow soft failures.
+ */
+inline
+bool
+isTecClaimHardFail(TER ter, ApplyFlags flags)
+{
+    return isTecClaim(ter) && !(flags & tapRETRY);
+}
+
 /** Describes the results of the `preflight` check
 
     @note All members are const to make it more difficult
@@ -99,7 +109,7 @@ public:
         , j(ctx_.j)
         , ter(ter_)
         , likelyToClaimFee(ter == tesSUCCESS
-            || isTecClaim(ter))
+            || isTecClaimHardFail(ter, flags))
     {
     }
 
@@ -268,4 +278,4 @@ doApply(PreclaimResult const& preclaimResult,
 
 }
 
-#endif
+#endif

src/ripple/app/tx/impl/Transactor.cpp

@@ -88,16 +88,13 @@ preflight1 (PreflightContext const& ctx)
 NotTEC
 preflight2 (PreflightContext const& ctx)
 {
-    if(!( ctx.flags & tapNO_CHECK_SIGN))
+    auto const sigValid = checkValidity(ctx.app.getHashRouter(),
+        ctx.tx, ctx.rules, ctx.app.config());
+    if (sigValid.first == Validity::SigBad)
     {
-        auto const sigValid = checkValidity(ctx.app.getHashRouter(),
-            ctx.tx, ctx.rules, ctx.app.config());
-        if (sigValid.first == Validity::SigBad)
-        {
-            JLOG(ctx.j.debug()) <<
-                "preflight2: bad signature. " << sigValid.second;
-            return temINVALID;
-        }
+        JLOG(ctx.j.debug()) <<
+            "preflight2: bad signature. " << sigValid.second;
+        return temINVALID;
     }
     return tesSUCCESS;
 }
@@ -638,7 +635,7 @@ Transactor::operator()()
         result = tecOVERSIZE;
 
     if ((result == tecOVERSIZE) ||
-        (isTecClaim (result) && !(view().flags() & tapRETRY)))
+        (isTecClaimHardFail (result, view().flags())))
     {
         JLOG(j_.trace()) << "reapplying because of " << transToken(result);