Improved local tx error messages (RIPD-720)

Failed local built transactions report the specific error.

src/ripple/app/misc/NetworkOPs.cpp

@@ -900,13 +900,16 @@ void NetworkOPsImp::submitTransaction (
         return;
     }
 
+    std::string reason;
+
     if ((flags & SF_SIGGOOD) == 0)
     {
         try
         {
-            if (!passesLocalChecks (*trans) || !trans->checkSign ())
+            if (! passesLocalChecks (*trans, reason) || ! trans->checkSign ())
             {
-                m_journal.warning << "Submitted transaction has bad signature";
+                m_journal.warning << "Submitted transaction " <<
+                    (reason.empty () ? "has bad signature" : "error: " + reason);
                 getApp().getHashRouter ().setFlag (suppress, SF_BAD);
                 return;
             }
@@ -915,7 +918,9 @@ void NetworkOPsImp::submitTransaction (
         }
         catch (...)
         {
-            m_journal.warning << "Exception checking transaction " << suppress;
+            m_journal.warning << "Exception checking transaction " << suppress
+                << (reason.empty () ? "" : ". error: " + reason);
+
             return;
         }
     }
@@ -923,7 +928,7 @@ void NetworkOPsImp::submitTransaction (
     m_job_queue.addJob (jtTRANSACTION, "submitTxn",
         std::bind (&NetworkOPsImp::processTransactionCbVoid,
                    this,
-                   std::make_shared<Transaction> (trans, Validate::NO),
+                   std::make_shared<Transaction> (trans, Validate::NO, reason),
                    false,
                    false,
                    false,
@@ -986,9 +991,11 @@ Transaction::pointer NetworkOPsImp::processTransactionCb (
     if ((newFlags & SF_SIGGOOD) == 0)
     {
         // signature not checked
-        if (!trans->checkSign ())
+        std::string reason;
+
+        if (! trans->checkSign (reason))
         {
-            m_journal.info << "Transaction has bad signature";
+            m_journal.info << "Transaction has bad signature: " << reason;
             trans->setStatus (INVALID);
             trans->setResult (temBAD_SIGNATURE);
             getApp().getHashRouter ().setFlag (trans->getID (), SF_BAD);

src/ripple/app/tx/Transaction.cpp

@@ -28,7 +28,8 @@
 
 namespace ripple {
 
-Transaction::Transaction (STTx::ref sit, Validate validate)
+Transaction::Transaction (STTx::ref sit, Validate validate, std::string& reason)
+    noexcept
     : mInLedger (0),
       mStatus (INVALID),
       mResult (temUNCERTAIN),
@@ -40,13 +41,19 @@ Transaction::Transaction (STTx::ref sit, Validate validate)
         mTransactionID  = mTransaction->getTransactionID ();
         mAccountFrom    = mTransaction->getSourceAccount ();
     }
+    catch (std::exception& e)
+    {
+        reason = e.what();
+        return;
+    }
     catch (...)
     {
+        reason = "Unexpected exception";
         return;
     }
 
     if (validate == Validate::NO ||
-        (passesLocalChecks (*mTransaction) && checkSign ()))
+        (passesLocalChecks (*mTransaction, reason) && checkSign (reason)))
     {
         mStatus = NEW;
     }
@@ -59,30 +66,39 @@ Transaction::pointer Transaction::sharedTransaction (
     {
         Serializer s (vucTransaction);
         SerialIter sit (s);
+        std::string reason;
 
-        return std::make_shared<Transaction> (
+        return std::make_shared<Transaction> (std::make_shared<STTx> (sit),
-            std::make_shared<STTx> (sit),
+            validate, reason);
-            validate);
+    }
+    catch (std::exception& e)
+    {
+        WriteLog(lsWARNING, Ledger) << "Exception constructing transaction" <<
+            e.what ();
     }
     catch (...)
     {
-        WriteLog (lsWARNING, Ledger) << "Exception constructing transaction";
+        WriteLog(lsWARNING, Ledger) << "Exception constructing transaction";
-        return std::shared_ptr<Transaction> ();
     }
+
+    return std::shared_ptr<Transaction> ();
 }
 
 //
 // Misc.
 //
 
-bool Transaction::checkSign () const
+bool Transaction::checkSign (std::string& reason) const
 {
-    if (mFromPubKey.isValid ())
+    if (! mFromPubKey.isValid ())
-        return mTransaction->checkSign();
+        reason = "Transaction has bad source public key";
+    else if (! mTransaction->checkSign ())
+        reason = "Transaction has bad signature";
+    else
+        return true;
 
-    WriteLog (lsWARNING, Ledger) << "Transaction has bad source public key";
+    WriteLog (lsWARNING, Ledger) << reason;
     return false;
-
 }
 
 void Transaction::setStatus (TransStatus ts, std::uint32_t lseq)
@@ -102,7 +118,8 @@ Transaction::pointer Transaction::transactionFromSQL (
 
     SerialIter it (rawTxn);
     auto txn = std::make_shared<STTx> (it);
-    auto tr = std::make_shared<Transaction> (txn, validate);
+    std::string reason;
+    auto tr = std::make_shared<Transaction> (txn, validate, reason);
 
     TransStatus st (INVALID);
 

src/ripple/app/tx/Transaction.h

@@ -62,7 +62,7 @@ public:
     typedef const pointer& ref;
 
 public:
-    Transaction (STTx::ref, Validate);
+    Transaction (STTx::ref, Validate, std::string&) noexcept;
 
     static Transaction::pointer sharedTransaction (Blob const&, Validate);
     static Transaction::pointer transactionFromSQL (
@@ -71,7 +71,7 @@ public:
         Blob const& rawTxn,
         Validate validate);
 
-    bool checkSign () const;
+    bool checkSign (std::string&) const;
 
     STTx::ref getSTransaction ()
     {

src/ripple/overlay/impl/PeerImp.cpp

@@ -1437,10 +1437,14 @@ PeerImp::checkTransaction (Job&, int flags,
         }
 
         auto validate = (flags & SF_SIGGOOD) ? Validate::NO : Validate::YES;
-        auto tx = std::make_shared<Transaction> (stx, validate);
+        std::string reason;
+        auto tx = std::make_shared<Transaction> (stx, validate, reason);
 
         if (tx->getStatus () == INVALID)
         {
+            if (! reason.empty ())
+                p_journal_.trace << "Exception checking transaction: " << reason;
+
             getApp().getHashRouter ().setFlag (stx->getTransactionID (), SF_BAD);
             charge (Resource::feeInvalidSignature);
             return;

src/ripple/protocol/STTx.h

@@ -156,7 +156,6 @@ private:
 };
 
 bool passesLocalChecks (STObject const& st, std::string&);
-bool passesLocalChecks (STObject const& st);
 
 } // ripple
 

src/ripple/protocol/impl/STTx.cpp

@@ -404,10 +404,4 @@ bool passesLocalChecks (STObject const& st, std::string& reason)
     return true;
 }
 
-bool passesLocalChecks (STObject const& st)
-{
-    std::string reason;
-    return passesLocalChecks (st, reason);
-}
-
 } // ripple

src/ripple/rpc/handlers/Submit.cpp

@@ -64,23 +64,12 @@ Json::Value doSubmit (RPC::Context& context)
     }
 
     Transaction::pointer            tpTrans;
-
+    std::string reason;
-    try
+    tpTrans = std::make_shared<Transaction> (stpTrans, Validate::YES, reason);
-    {
-        tpTrans     = std::make_shared<Transaction> (stpTrans, Validate::YES);
-    }
-    catch (std::exception& e)
-    {
-        jvResult[jss::error]           = "internalTransaction";
-        jvResult[jss::error_exception] = e.what ();
-
-        return jvResult;
-    }
-
     if (tpTrans->getStatus() != NEW)
     {
-        jvResult[jss::error]            = "invalidTransactions";
+        jvResult[jss::error]            = "invalidTransaction";
-        jvResult[jss::error_exception] = "fails local checks";
+        jvResult[jss::error_exception] = "fails local checks: " + reason;
 
         return jvResult;
     }

src/ripple/rpc/impl/TransactionSign.cpp

@@ -485,17 +485,8 @@ transactionSign (
 
     stpTrans->sign (keypair.secretKey);
 
-    Transaction::pointer tpTrans;
+    Transaction::pointer tpTrans = std::make_shared<Transaction> (stpTrans,
-
+        Validate::NO, reason);
-    try
-    {
-        tpTrans = std::make_shared<Transaction> (stpTrans, Validate::NO);
-    }
-    catch (std::exception&)
-    {
-        return RPC::make_error (rpcINTERNAL,
-            "Exception occurred during transaction");
-    }
 
     try
     {