From 5e70db651d0837cb039a7df3f494610bf69e4a17 Mon Sep 17 00:00:00 2001 From: Miguel Portilla Date: Mon, 23 Mar 2015 13:45:08 -0400 Subject: [PATCH] Improved local tx error messages (RIPD-720) Failed local built transactions report the specific error. --- src/ripple/app/misc/NetworkOPs.cpp | 19 +++++++---- src/ripple/app/tx/Transaction.cpp | 43 +++++++++++++++++-------- src/ripple/app/tx/Transaction.h | 4 +-- src/ripple/overlay/impl/PeerImp.cpp | 6 +++- src/ripple/protocol/STTx.h | 1 - src/ripple/protocol/impl/STTx.cpp | 6 ---- src/ripple/rpc/handlers/Submit.cpp | 19 +++-------- src/ripple/rpc/impl/TransactionSign.cpp | 13 ++------ 8 files changed, 56 insertions(+), 55 deletions(-) diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 4b947b045..6b070ca7d 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/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 (trans, Validate::NO), + std::make_shared (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); diff --git a/src/ripple/app/tx/Transaction.cpp b/src/ripple/app/tx/Transaction.cpp index c2d7ca54f..39ed351cf 100644 --- a/src/ripple/app/tx/Transaction.cpp +++ b/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 ( - std::make_shared (sit), - validate); + return std::make_shared (std::make_shared (sit), + validate, reason); + } + catch (std::exception& e) + { + WriteLog(lsWARNING, Ledger) << "Exception constructing transaction" << + e.what (); } catch (...) { - WriteLog (lsWARNING, Ledger) << "Exception constructing transaction"; - return std::shared_ptr (); + WriteLog(lsWARNING, Ledger) << "Exception constructing transaction"; } + + return std::shared_ptr (); } // // Misc. // -bool Transaction::checkSign () const +bool Transaction::checkSign (std::string& reason) const { - if (mFromPubKey.isValid ()) - return mTransaction->checkSign(); + if (! mFromPubKey.isValid ()) + 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 (it); - auto tr = std::make_shared (txn, validate); + std::string reason; + auto tr = std::make_shared (txn, validate, reason); TransStatus st (INVALID); diff --git a/src/ripple/app/tx/Transaction.h b/src/ripple/app/tx/Transaction.h index 7c4626000..5c0dc595d 100644 --- a/src/ripple/app/tx/Transaction.h +++ b/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 () { diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index 350951963..e52ed791d 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/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 (stx, validate); + std::string reason; + auto tx = std::make_shared (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; diff --git a/src/ripple/protocol/STTx.h b/src/ripple/protocol/STTx.h index 995e00c2a..03699d4fe 100644 --- a/src/ripple/protocol/STTx.h +++ b/src/ripple/protocol/STTx.h @@ -156,7 +156,6 @@ private: }; bool passesLocalChecks (STObject const& st, std::string&); -bool passesLocalChecks (STObject const& st); } // ripple diff --git a/src/ripple/protocol/impl/STTx.cpp b/src/ripple/protocol/impl/STTx.cpp index 0f009d342..f2a5d875c 100644 --- a/src/ripple/protocol/impl/STTx.cpp +++ b/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 diff --git a/src/ripple/rpc/handlers/Submit.cpp b/src/ripple/rpc/handlers/Submit.cpp index ab8b4bfed..5f85b2eb8 100644 --- a/src/ripple/rpc/handlers/Submit.cpp +++ b/src/ripple/rpc/handlers/Submit.cpp @@ -64,23 +64,12 @@ Json::Value doSubmit (RPC::Context& context) } Transaction::pointer tpTrans; - - try - { - tpTrans = std::make_shared (stpTrans, Validate::YES); - } - catch (std::exception& e) - { - jvResult[jss::error] = "internalTransaction"; - jvResult[jss::error_exception] = e.what (); - - return jvResult; - } - + std::string reason; + tpTrans = std::make_shared (stpTrans, Validate::YES, reason); if (tpTrans->getStatus() != NEW) { - jvResult[jss::error] = "invalidTransactions"; - jvResult[jss::error_exception] = "fails local checks"; + jvResult[jss::error] = "invalidTransaction"; + jvResult[jss::error_exception] = "fails local checks: " + reason; return jvResult; } diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 6f1285a0f..02f1c3be0 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -485,17 +485,8 @@ transactionSign ( stpTrans->sign (keypair.secretKey); - Transaction::pointer tpTrans; - - try - { - tpTrans = std::make_shared (stpTrans, Validate::NO); - } - catch (std::exception&) - { - return RPC::make_error (rpcINTERNAL, - "Exception occurred during transaction"); - } + Transaction::pointer tpTrans = std::make_shared (stpTrans, + Validate::NO, reason); try {