diff --git a/Builds/VisualStudio2015/RippleD.vcxproj b/Builds/VisualStudio2015/RippleD.vcxproj index 739a4adf1..5b20f7352 100644 --- a/Builds/VisualStudio2015/RippleD.vcxproj +++ b/Builds/VisualStudio2015/RippleD.vcxproj @@ -1688,6 +1688,10 @@ True True + + True + True + True True diff --git a/Builds/VisualStudio2015/RippleD.vcxproj.filters b/Builds/VisualStudio2015/RippleD.vcxproj.filters index a3941b5ea..57854a358 100644 --- a/Builds/VisualStudio2015/RippleD.vcxproj.filters +++ b/Builds/VisualStudio2015/RippleD.vcxproj.filters @@ -2436,6 +2436,9 @@ ripple\app\tests + + ripple\app\tests + ripple\app\tests diff --git a/src/ripple/app/ledger/OpenLedger.h b/src/ripple/app/ledger/OpenLedger.h index 59bdd493e..1275b7083 100644 --- a/src/ripple/app/ledger/OpenLedger.h +++ b/src/ripple/app/ledger/OpenLedger.h @@ -28,7 +28,6 @@ #include #include #include -#include #include #include #include @@ -53,7 +52,6 @@ class OpenLedger private: beast::Journal j_; CachedSLEs& cache_; - Config const& config_; std::mutex mutable modify_mutex_; std::mutex mutable current_mutex_; std::shared_ptr current_; @@ -70,7 +68,7 @@ public: explicit OpenLedger(std::shared_ptr< Ledger const> const& ledger, - Config const& config, CachedSLEs& cache, + CachedSLEs& cache, beast::Journal journal); /** Returns `true` if there are no transactions. @@ -165,8 +163,7 @@ public: apply (Application& app, OpenView& view, ReadView const& check, FwdRange const& txs, OrderedTxs& retries, ApplyFlags flags, - HashRouter& router, Config const& config, - beast::Journal j); + HashRouter& router, beast::Journal j); private: enum Result @@ -185,8 +182,7 @@ private: apply_one (Application& app, OpenView& view, std::shared_ptr< STTx const> const& tx, bool retry, ApplyFlags flags, - HashRouter& router, Config const& config, - beast::Journal j); + HashRouter& router, beast::Journal j); }; //------------------------------------------------------------------------------ @@ -196,8 +192,7 @@ void OpenLedger::apply (Application& app, OpenView& view, ReadView const& check, FwdRange const& txs, OrderedTxs& retries, ApplyFlags flags, - HashRouter& router, Config const& config, - beast::Journal j) + HashRouter& router, beast::Journal j) { for (auto iter = txs.begin(); iter != txs.end(); ++iter) @@ -210,7 +205,7 @@ OpenLedger::apply (Application& app, OpenView& view, if (check.txExists(tx->getTransactionID())) continue; auto const result = apply_one(app, view, - tx, true, flags, router, config, j); + tx, true, flags, router, j); if (result == Result::retry) retries.insert(tx); } @@ -231,7 +226,7 @@ OpenLedger::apply (Application& app, OpenView& view, { switch (apply_one(app, view, iter->second, retry, flags, - router, config, j)) + router, j)) { case Result::success: ++changes; diff --git a/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp b/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp index bbcbccd19..60be31e4f 100644 --- a/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp +++ b/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp @@ -42,8 +42,8 @@ #include #include #include -#include -#include +#include +#include #include #include #include @@ -1820,10 +1820,6 @@ applyTransaction (Application& app, OpenView& view, if (retryAssured) flags = flags | tapRETRY; - if ((app.getHashRouter ().getFlags (txn->getTransactionID ()) - & SF_SIGGOOD) == SF_SIGGOOD) - flags = flags | tapNO_CHECK_SIGN; - JLOG (j.debug) << "TXN " << txn->getTransactionID () //<< (engine.view().open() ? " open" : " closed") @@ -1833,9 +1829,8 @@ applyTransaction (Application& app, OpenView& view, try { - auto const result = apply(app, view, *txn, flags, - app.getHashRouter().sigVerify(), - app.config(), j); + auto const result = apply(app, + view, *txn, flags, j); if (result.second) { JLOG (j.debug) diff --git a/src/ripple/app/ledger/impl/LedgerMaster.cpp b/src/ripple/app/ledger/impl/LedgerMaster.cpp index c98d530a7..b011834ba 100644 --- a/src/ripple/app/ledger/impl/LedgerMaster.cpp +++ b/src/ripple/app/ledger/impl/LedgerMaster.cpp @@ -378,13 +378,8 @@ public: for (auto const& it : mHeldTransactions) { ApplyFlags flags = tapNONE; - if (app_.getHashRouter().addSuppressionFlags ( - it.first.getTXID (), SF_SIGGOOD)) - flags = flags | tapNO_CHECK_SIGN; - auto const result = apply(app_, view, - *it.second, flags, app_.getHashRouter( - ).sigVerify(), app_.config(), j); + *it.second, flags, j); if (result.second) any = true; } diff --git a/src/ripple/app/ledger/impl/OpenLedger.cpp b/src/ripple/app/ledger/impl/OpenLedger.cpp index 1294bd104..658a47324 100644 --- a/src/ripple/app/ledger/impl/OpenLedger.cpp +++ b/src/ripple/app/ledger/impl/OpenLedger.cpp @@ -21,17 +21,17 @@ #include #include #include +#include #include namespace ripple { OpenLedger::OpenLedger(std::shared_ptr< Ledger const> const& ledger, - Config const& config, CachedSLEs& cache, + CachedSLEs& cache, beast::Journal journal) : j_ (journal) , cache_ (cache) - , config_ (config) , current_ (create(ledger->rules(), ledger)) { } @@ -89,7 +89,7 @@ OpenLedger::accept(Application& app, Rules const& rules, std::vector>; apply (app, *next, *ledger, empty{}, - retries, flags, router, config_, j_); + retries, flags, router, j_); } // Block calls to modify, otherwise // new tx going into the open ledger @@ -107,12 +107,11 @@ OpenLedger::accept(Application& app, Rules const& rules, { return p.first; }), - retries, flags, router, config_, j_); + retries, flags, router, j_); // Apply local tx for (auto const& item : locals) - ripple::apply(app, *next, *item.second, - flags, router.sigVerify(), - config_, j_); + ripple::apply(app, *next, + *item.second, flags, j_); // Switch to the new open view std::lock_guard< std::mutex> lock2(current_mutex_); @@ -135,18 +134,12 @@ auto OpenLedger::apply_one (Application& app, OpenView& view, std::shared_ptr const& tx, bool retry, ApplyFlags flags, - HashRouter& router, Config const& config, - beast::Journal j) -> Result + HashRouter& router,beast::Journal j) -> Result { if (retry) flags = flags | tapRETRY; - if ((router.getFlags( - tx->getTransactionID()) & SF_SIGGOOD) == - SF_SIGGOOD) - flags = flags | tapNO_CHECK_SIGN; auto const result = ripple::apply( - app, view, *tx, flags, router. - sigVerify(), config, j); + app, view, *tx, flags, j); if (result.second) return Result::success; if (isTefFailure (result.first) || diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 4701c16af..e06521c91 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -1117,8 +1118,8 @@ ApplicationImp::startGenesisLedger() next->setClosed (); next->setImmutable (*config_); m_networkOPs->setLastCloseTime (next->info().closeTime); - openLedger_.emplace(next, *config_, - cachedSLEs_, logs_->journal("OpenLedger")); + openLedger_.emplace(next, cachedSLEs_, + logs_->journal("OpenLedger")); m_ledgerMaster->switchLCL (next); } @@ -1372,8 +1373,8 @@ bool ApplicationImp::loadOldLedger ( m_ledgerMaster->switchLCL (loadLedger); m_ledgerMaster->forceValid(loadLedger); m_networkOPs->setLastCloseTime (loadLedger->info().closeTime); - openLedger_.emplace(loadLedger, *config_, - cachedSLEs_, logs_->journal("OpenLedger")); + openLedger_.emplace(loadLedger, cachedSLEs_, + logs_->journal("OpenLedger")); if (replay) { @@ -1395,7 +1396,8 @@ bool ApplicationImp::loadOldLedger ( auto s = std::make_shared (); txPair.first->add(*s); - getHashRouter().setFlags (txID, SF_SIGGOOD); + forceValidity(getHashRouter(), + txID, Validity::SigGoodOnly); replayData->txns_.emplace (txIndex, txPair.first); diff --git a/src/ripple/app/misc/HashRouter.cpp b/src/ripple/app/misc/HashRouter.cpp index 71e03c336..609704a0e 100644 --- a/src/ripple/app/misc/HashRouter.cpp +++ b/src/ripple/app/misc/HashRouter.cpp @@ -19,131 +19,85 @@ #include #include -#include #include #include #include namespace ripple { -std::function)> -HashRouter::sigVerify() +auto +HashRouter::emplace (uint256 const& index) + -> std::pair { - return - [&] (STTx const& tx, std::function sigCheck) - { - auto const id = tx.getTransactionID(); - auto const flags = getFlags(id); - if (flags & SF_SIGGOOD) - return true; - if (flags & SF_BAD) - return false; - if (! sigCheck(tx)) - { - setFlags(id, SF_BAD); - return false; - } - setFlags(id, SF_SIGGOOD); - return true; - }; -} - -HashRouter::Entry& HashRouter::findCreateEntry (uint256 const& index, bool& created) -{ - hash_map::iterator fit = mSuppressionMap.find (index); + auto fit = mSuppressionMap.find (index); if (fit != mSuppressionMap.end ()) { - created = false; - return fit->second; + return std::make_pair( + std::ref(fit->second), false); } - created = true; - - int now = UptimeTimer::getInstance ().getElapsedSeconds (); - int expireTime = now - mHoldTime; + int elapsed = UptimeTimer::getInstance ().getElapsedSeconds (); + int expireCutoff = elapsed - mHoldTime; // See if any supressions need to be expired auto it = mSuppressionTimes.begin (); - while ((it != mSuppressionTimes.end ()) && (it->first <= expireTime)) + while ((it != mSuppressionTimes.end ()) && (it->first <= expireCutoff)) { for(auto const& lit : it->second) mSuppressionMap.erase (lit); it = mSuppressionTimes.erase (it); } - mSuppressionTimes[now].push_back (index); - return mSuppressionMap.emplace (index, Entry ()).first->second; + mSuppressionTimes[elapsed].push_back (index); + return std::make_pair(std::ref( + mSuppressionMap.emplace ( + index, Entry ()).first->second), + true); } -bool HashRouter::addSuppression (uint256 const& index) +void HashRouter::addSuppression (uint256 const& index) { - ScopedLockType lock (mMutex); + std::lock_guard lock (mMutex); - bool created; - findCreateEntry (index, created); - return created; -} - -HashRouter::Entry HashRouter::getEntry (uint256 const& index) -{ - ScopedLockType lock (mMutex); - - bool created; - return findCreateEntry (index, created); + emplace (index); } bool HashRouter::addSuppressionPeer (uint256 const& index, PeerShortID peer) { - ScopedLockType lock (mMutex); + std::lock_guard lock (mMutex); - bool created; - findCreateEntry (index, created).addPeer (peer); - return created; + auto result = emplace(index); + result.first.addPeer(peer); + return result.second; } bool HashRouter::addSuppressionPeer (uint256 const& index, PeerShortID peer, int& flags) { - ScopedLockType lock (mMutex); + std::lock_guard lock (mMutex); - bool created; - Entry& s = findCreateEntry (index, created); + auto result = emplace(index); + auto& s = result.first; s.addPeer (peer); flags = s.getFlags (); - return created; + return result.second; } int HashRouter::getFlags (uint256 const& index) { - ScopedLockType lock (mMutex); + std::lock_guard lock (mMutex); - bool created; - return findCreateEntry (index, created).getFlags (); -} - -bool HashRouter::addSuppressionFlags (uint256 const& index, int flag) -{ - ScopedLockType lock (mMutex); - - bool created; - findCreateEntry (index, created).setFlags (flag); - return created; + return emplace(index).first.getFlags (); } bool HashRouter::setFlags (uint256 const& index, int flags) { - // VFALCO NOTE Comments like this belong in the HEADER file, - // and more importantly in a Javadoc comment so - // they appear in the generated documentation. - // - // return: true = changed, false = unchanged assert (flags != 0); - ScopedLockType lock (mMutex); + std::lock_guard lock (mMutex); - bool created; - Entry& s = findCreateEntry (index, created); + auto& s = emplace(index).first; if ((s.getFlags () & flags) == flags) return false; @@ -154,10 +108,9 @@ bool HashRouter::setFlags (uint256 const& index, int flags) bool HashRouter::swapSet (uint256 const& index, std::set& peers, int flag) { - ScopedLockType lock (mMutex); + std::lock_guard lock (mMutex); - bool created; - Entry& s = findCreateEntry (index, created); + auto& s = emplace(index).first; if ((s.getFlags () & flag) == flag) return false; diff --git a/src/ripple/app/misc/HashRouter.h b/src/ripple/app/misc/HashRouter.h index 35697e00b..e7abdb5da 100644 --- a/src/ripple/app/misc/HashRouter.h +++ b/src/ripple/app/misc/HashRouter.h @@ -29,17 +29,20 @@ namespace ripple { -class STTx; - // VFALCO NOTE Are these the flags?? Why aren't we using a packed struct? // VFALCO TODO convert these macros to int constants #define SF_RELAYED 0x01 // Has already been relayed to other nodes // VFALCO NOTE How can both bad and good be set on a hash? -#define SF_BAD 0x02 // Signature/format is bad -#define SF_SIGGOOD 0x04 // Signature is good -#define SF_SAVED 0x08 -#define SF_RETRY 0x10 // Transaction can be retried -#define SF_TRUSTED 0x20 // comes from trusted source +#define SF_BAD 0x02 // Temporarily bad +#define SF_SAVED 0x04 +#define SF_RETRY 0x08 // Transaction can be retried +#define SF_TRUSTED 0x10 // comes from trusted source +// Private flags, used internally in apply.cpp. +// Do not attempt to read, set, or reuse. +#define SF_PRIVATE1 0x100 +#define SF_PRIVATE2 0x200 +#define SF_PRIVATE3 0x400 +#define SF_PRIVATE4 0x800 /** Routing table for objects identified by hash. @@ -62,54 +65,54 @@ private: static char const* getCountedObjectName () { return "HashRouterEntry"; } Entry () - : mFlags (0) + : flags_ (0) { } std::set const& peekPeers () const { - return mPeers; + return peers_; } void addPeer (PeerShortID peer) { if (peer != 0) - mPeers.insert (peer); + peers_.insert (peer); } bool hasPeer (PeerShortID peer) const { - return mPeers.count (peer) > 0; + return peers_.count (peer) > 0; } int getFlags (void) const { - return mFlags; + return flags_; } bool hasFlag (int mask) const { - return (mFlags & mask) != 0; + return (flags_ & mask) != 0; } void setFlags (int flagsToSet) { - mFlags |= flagsToSet; + flags_ |= flagsToSet; } void clearFlag (int flagsToClear) { - mFlags &= ~flagsToClear; + flags_ &= ~flagsToClear; } void swapSet (std::set & other) { - mPeers.swap (other); + peers_.swap (other); } private: - int mFlags; - std::set mPeers; + int flags_; + std::set peers_; }; public: @@ -126,22 +129,22 @@ public: { } + HashRouter& operator= (HashRouter const&) = delete; + virtual ~HashRouter() = default; // VFALCO TODO Replace "Supression" terminology with something more // semantically meaningful. - bool addSuppression (uint256 const& index); + void addSuppression(uint256 const& index); bool addSuppressionPeer (uint256 const& index, PeerShortID peer); bool addSuppressionPeer (uint256 const& index, PeerShortID peer, int& flags); - bool addSuppressionFlags (uint256 const& index, int flag); - /** Set the flags on a hash. - @return `true` if the flags were changed. + @return `true` if the flags were changed. `false` if unchanged. */ bool setFlags (uint256 const& index, int flags); @@ -149,22 +152,11 @@ public: bool swapSet (uint256 const& index, std::set& peers, int flag); - /** - Function wrapper that will check the signature status - of a STTx before calling an expensive signature - checking function. - */ - std::function)> - sigVerify(); - private: - Entry getEntry (uint256 const& ); + // pair.second indicates whether the entry was created + std::pair emplace (uint256 const&); - Entry& findCreateEntry (uint256 const& , bool& created); - - using MutexType = std::mutex; - using ScopedLockType = std::lock_guard ; - MutexType mMutex; + std::mutex mutable mMutex; // Stores all suppressed hashes and their expiration time hash_map mSuppressionMap; @@ -172,7 +164,7 @@ private: // Stores all expiration times and the hashes indexed for them std::map< int, std::list > mSuppressionTimes; - int mHoldTime; + int const mHoldTime; }; } // ripple diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index b68f8406e..a1bb1215c 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -654,52 +654,47 @@ void NetworkOPsImp::submitTransaction (STTx::pointer iTrans) SerialIter sit (s.slice()); auto trans = std::make_shared (std::ref (sit)); - uint256 suppress = trans->getTransactionID (); - int flags; + auto const txid = trans->getTransactionID (); + auto const flags = app_.getHashRouter().getFlags(txid); - if (app_.getHashRouter ().addSuppressionPeer (suppress, 0, flags) && - ((flags & SF_RETRY) != 0)) + if ((flags & SF_RETRY) != 0) { - m_journal.warning << "Redundant transactions submitted"; + JLOG(m_journal.warning) << "Redundant transactions submitted"; return; } if ((flags & SF_BAD) != 0) { - m_journal.warning << "Submitted transaction cached bad"; + JLOG(m_journal.warning) << "Submitted transaction cached bad"; + return; + } + + try + { + auto const validity = checkValidity( + app_.getHashRouter(), *trans, + m_ledgerMaster.getValidatedRules(), + app_.config()); + + if (validity.first != Validity::Valid) + { + JLOG(m_journal.warning) << + "Submitted transaction invalid: " << + validity.second; + return; + } + } + catch (...) + { + JLOG(m_journal.warning) << "Exception checking transaction" << txid; + return; } std::string reason; - if ((flags & SF_SIGGOOD) == 0) - { - try - { - // Tell the call to checkSign() whether multisign is enabled. - if (!passesLocalChecks (*trans, reason) || - !trans->checkSign (m_ledgerMaster.getValidatedRules().enabled( - featureMultiSign, app_.config().features))) - { - m_journal.warning << "Submitted transaction " << - (reason.empty () ? "has bad signature" : "error: " + reason); - app_.getHashRouter ().setFlags (suppress, SF_BAD); - return; - } - - app_.getHashRouter ().setFlags (suppress, SF_SIGGOOD); - } - catch (...) - { - m_journal.warning << "Exception checking transaction " << suppress - << (reason.empty () ? "" : ". error: " + reason); - - return; - } - } - auto tx = std::make_shared ( - trans, Validate::NO, directSigVerify, reason, app_); + trans, reason, app_); m_job_queue.addJob (jtTRANSACTION, "submitTxn", [this, tx] (Job&) { auto t = tx; @@ -711,7 +706,7 @@ void NetworkOPsImp::processTransaction (Transaction::pointer& transaction, bool bAdmin, bool bLocal, FailHard failType) { auto ev = m_job_queue.getLoadEventAP (jtTXN_PROC, "ProcessTXN"); - int newFlags = app_.getHashRouter ().getFlags (transaction->getID ()); + auto const newFlags = app_.getHashRouter ().getFlags (transaction->getID ()); if ((newFlags & SF_BAD) != 0) { @@ -721,23 +716,28 @@ void NetworkOPsImp::processTransaction (Transaction::pointer& transaction, return; } - if ((newFlags & SF_SIGGOOD) == 0) + // NOTE eahennis - I think this check is redundant, + // but I'm not 100% sure yet. + // If so, only cost is looking up HashRouter flags. + auto const view = m_ledgerMaster.getCurrentLedger(); + auto const validity = checkValidity( + app_.getHashRouter(), + *transaction->getSTransaction(), + view->rules(), app_.config()); + assert(validity.first == Validity::Valid); + + // Not concerned with local checks at this point. + if (validity.first == Validity::SigBad) { - // signature not checked - std::string reason; - - if (! transaction->checkSign (reason, directSigVerify)) - { - m_journal.info << "Transaction has bad signature: " << reason; - transaction->setStatus (INVALID); - transaction->setResult (temBAD_SIGNATURE); - app_.getHashRouter ().setFlags (transaction->getID (), SF_BAD); - return; - } + m_journal.info << "Transaction has bad signature: " << + validity.second; + transaction->setStatus(INVALID); + transaction->setResult(temBAD_SIGNATURE); + app_.getHashRouter().setFlags(transaction->getID(), + SF_BAD); + return; } - app_.getHashRouter ().setFlags (transaction->getID (), SF_SIGGOOD); - // canonicalize can change our pointer app_.getMasterTransaction ().canonicalize (&transaction); @@ -835,6 +835,7 @@ void NetworkOPsImp::apply (std::unique_lock& batchLock) for (TransactionStatus& e : transactions) { ApplyFlags flags = tapNONE; + // All code paths to this point are gated by validity checks. flags = flags | tapNO_CHECK_SIGN; if (e.admin) flags = flags | tapADMIN; @@ -843,10 +844,8 @@ void NetworkOPsImp::apply (std::unique_lock& batchLock) app_.openLedger().modify( [&](OpenView& view, beast::Journal j) { - auto const result = ripple::apply(app_, - view, *e.transaction->getSTransaction(), flags, - app_.getHashRouter().sigVerify(), - app_.config(), j); + auto const result = ripple::apply(app_, view, + *e.transaction->getSTransaction(), flags, j); e.result = result.first; e.applied = result.second; return result.second; @@ -1672,7 +1671,7 @@ NetworkOPs::AccountTxs NetworkOPsImp::getAccountTxs ( txnMeta.clear (); auto txn = Transaction::transactionFromSQL ( - ledgerSeq, status, rawTxn, Validate::NO, app_); + ledgerSeq, status, rawTxn, app_); if (txnMeta.empty ()) { // Work around a bug that could leave the metadata missing @@ -1685,9 +1684,10 @@ NetworkOPs::AccountTxs NetworkOPsImp::getAccountTxs ( pendSaveValidated(app_, ledger, false, false); } - ret.emplace_back (txn, std::make_shared ( - txn->getID (), txn->getLedger (), txnMeta, - app_.journal ("TxMeta"))); + if (txn) + ret.emplace_back (txn, std::make_shared ( + txn->getID (), txn->getLedger (), txnMeta, + app_.journal("TxMeta"))); } } diff --git a/src/ripple/app/misc/impl/AccountTxPaging.cpp b/src/ripple/app/misc/impl/AccountTxPaging.cpp index fbe255007..ad0d276f8 100644 --- a/src/ripple/app/misc/impl/AccountTxPaging.cpp +++ b/src/ripple/app/misc/impl/AccountTxPaging.cpp @@ -43,8 +43,7 @@ convertBlobsToTxResult ( STTx::pointer txn = std::make_shared (it); std::string reason; - auto tr = std::make_shared (txn, Validate::NO, - directSigVerify, reason, app); + auto tr = std::make_shared (txn, reason, app); tr->setStatus (Transaction::sqlTransactionStatus(status)); tr->setLedger (ledger_index); diff --git a/src/ripple/app/tests/HashRouter_test.cpp b/src/ripple/app/tests/HashRouter_test.cpp new file mode 100644 index 000000000..95c9df759 --- /dev/null +++ b/src/ripple/app/tests/HashRouter_test.cpp @@ -0,0 +1,188 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2012-2015 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include + +namespace ripple { +namespace test { + +class HashRouter_test : public beast::unit_test::suite +{ + void + testExpiration() + { + HashRouter router(2); + + uint256 const key1(1); + uint256 const key2(2); + uint256 const key3(3); + uint256 const key4(4); + expect(key1 != key2 && + key2 != key3 && + key3 != key4); + + // t=0 + router.setFlags(key1, 12345); + expect(router.getFlags(key1) == 12345); + // key1 : 0 + // key2 : null + // key3 : null + + UptimeTimer::getInstance().incrementElapsedTime(); + + // Expiration is triggered by insertion, so + // key1 will be expired after the second + // call to setFlags. + // t=1 + router.setFlags(key2, 9999); + expect(router.getFlags(key1) == 12345); + expect(router.getFlags(key2) == 9999); + // key1 : 0 + // key2 : 1 + // key3 : null + + UptimeTimer::getInstance().incrementElapsedTime(); + + // t=2 + router.setFlags(key3, 2222); + expect(router.getFlags(key1) == 0); + expect(router.getFlags(key2) == 9999); + expect(router.getFlags(key3) == 2222); + // key1 : 2 + // key2 : 1 + // key3 : 2 + + UptimeTimer::getInstance().incrementElapsedTime(); + + // t=3 + // No insertion, no expiration + router.setFlags(key1, 7654); + expect(router.getFlags(key1) == 7654); + expect(router.getFlags(key2) == 9999); + expect(router.getFlags(key3) == 2222); + // key1 : 2 + // key2 : 1 + // key3 : 2 + + UptimeTimer::getInstance().incrementElapsedTime(); + UptimeTimer::getInstance().incrementElapsedTime(); + + // t=5 + router.setFlags(key4, 7890); + expect(router.getFlags(key1) == 0); + expect(router.getFlags(key2) == 0); + expect(router.getFlags(key3) == 0); + expect(router.getFlags(key4) == 7890); + // key1 : 5 + // key2 : 5 + // key3 : 5 + // key4 : 5 + } + + void testSuppression() + { + // Normal HashRouter + HashRouter router(2); + + uint256 const key1(1); + uint256 const key2(2); + uint256 const key3(3); + uint256 const key4(4); + expect(key1 != key2 && + key2 != key3 && + key3 != key4); + + int flags = 12345; // This value is ignored + router.addSuppression(key1); + expect(router.addSuppressionPeer(key2, 15)); + expect(router.addSuppressionPeer(key3, 20, flags)); + expect(flags == 0); + + UptimeTimer::getInstance().incrementElapsedTime(); + + expect(!router.addSuppressionPeer(key1, 2)); + expect(!router.addSuppressionPeer(key2, 3)); + expect(!router.addSuppressionPeer(key3, 4, flags)); + expect(flags == 0); + expect(router.addSuppressionPeer(key4, 5)); + } + + void + testSetFlags() + { + HashRouter router(2); + + uint256 const key1(1); + expect(router.setFlags(key1, 10)); + expect(!router.setFlags(key1, 10)); + expect(router.setFlags(key1, 20)); + } + + void + testSwapSet() + { + HashRouter router(2); + + uint256 const key1(1); + + std::set peers1; + std::set peers2; + + peers1.emplace(1); + peers1.emplace(3); + peers1.emplace(5); + peers2.emplace(2); + peers2.emplace(4); + + expect(router.swapSet(key1, peers1, 135)); + expect(peers1.empty()); + expect(router.getFlags(key1) == 135); + // No action, because flag matches + expect(!router.swapSet(key1, peers2, 135)); + expect(peers2.size() == 2); + expect(router.getFlags(key1) == 135); + // Do a swap + expect(router.swapSet(key1, peers2, 24)); + expect(peers2.size() == 3); + expect(router.getFlags(key1) == (135 | 24)); + } + +public: + + void + run() + { + UptimeTimer::getInstance().beginManualUpdates(); + + testExpiration(); + testSuppression(); + testSetFlags(); + testSwapSet(); + + UptimeTimer::getInstance().endManualUpdates(); + } +}; + +BEAST_DEFINE_TESTSUITE(HashRouter, app, ripple) + +} +} \ No newline at end of file diff --git a/src/ripple/app/tests/Regression_test.cpp b/src/ripple/app/tests/Regression_test.cpp index eb9042a99..e81bd32ba 100644 --- a/src/ripple/app/tests/Regression_test.cpp +++ b/src/ripple/app/tests/Regression_test.cpp @@ -67,8 +67,7 @@ struct Regression_test : public beast::unit_test::suite auto const result = ripple::apply(env.app(), accum, *jt.stx, tapENABLE_TESTING, - directSigVerify, env.app().config(), - env.journal); + env.journal); expect(result.first == tesSUCCESS); expect(result.second); @@ -95,8 +94,7 @@ struct Regression_test : public beast::unit_test::suite auto const result = ripple::apply(env.app(), accum, *jt.stx, tapENABLE_TESTING, - directSigVerify, env.app().config(), - env.journal); + env.journal); expect(result.first == tecINSUFF_FEE); expect(result.second); diff --git a/src/ripple/app/tx/Transaction.h b/src/ripple/app/tx/Transaction.h index ef6e68c1d..6bba01785 100644 --- a/src/ripple/app/tx/Transaction.h +++ b/src/ripple/app/tx/Transaction.h @@ -35,6 +35,7 @@ namespace ripple { class Application; class Database; +class Rules; enum TransStatus { @@ -49,8 +50,6 @@ enum TransStatus INCOMPLETE = 8 // needs more signatures }; -enum class Validate {NO, YES}; - // This class is for constructing and examining transactions. // Transactions are static so manipulation functions are unnecessary. class Transaction @@ -65,11 +64,11 @@ public: public: Transaction ( - STTx::ref, Validate, SigVerify, std::string&, Application&) noexcept; + STTx::ref, std::string&, Application&) noexcept; static Transaction::pointer - sharedTransaction (Blob const&, Validate, Application& app); + sharedTransaction (Blob const&, Rules const& rules, Application& app); static Transaction::pointer @@ -77,15 +76,20 @@ public: boost::optional const& ledgerSeq, boost::optional const& status, Blob const& rawTxn, - Validate validate, + Application& app); + + static + Transaction::pointer + transactionFromSQLValidated ( + boost::optional const& ledgerSeq, + boost::optional const& status, + Blob const& rawTxn, Application& app); static TransStatus sqlTransactionStatus(boost::optional const& status); - bool checkSign (std::string&, SigVerify) const; - STTx::ref getSTransaction () { return mTransaction; @@ -160,8 +164,6 @@ public: private: uint256 mTransactionID; - RippleAddress mFromPubKey; // Sign transaction with this. mSignPubKey - RippleAddress mSourcePrivate; // Sign transaction with this. LedgerIndex mInLedger = 0; TransStatus mStatus = INVALID; diff --git a/src/ripple/app/tx/apply.h b/src/ripple/app/tx/apply.h index 9164f6452..13d9ed9ce 100644 --- a/src/ripple/app/tx/apply.h +++ b/src/ripple/app/tx/apply.h @@ -31,6 +31,54 @@ namespace ripple { class Application; +class HashRouter; + +enum class Validity +{ + SigBad, // Signature is bad. Didn't do local checks. + SigGoodOnly, // Signature is good, but local checks fail. + Valid // Signature and local checks are good / passed. +}; + +/** Checks transaction signature and local checks. Returns + a Validity enum representing how valid the STTx is + and, if not Valid, a reason string. + Results are cached internally, so tests will not be + repeated over repeated calls, unless cache expires. + + @return std::pair, where `.first` is the status, and + `.second` is the reason if appropriate. +*/ +std::pair +checkValidity(HashRouter& router, + STTx const& tx, + bool allowMultiSign); + +/** Checks transaction signature and local checks. Returns + a Validity enum representing how valid the STTx is + and, if not Valid, a reason string. + Results are cached internally, so tests will not be + repeated over repeated calls, unless cache expires. + + @return std::pair, where `.first` is the status, and + `.second` is the reason if appropriate. +*/ +std::pair +checkValidity(HashRouter& router, + STTx const& tx, Rules const& rules, + Config const& config, + ApplyFlags const& flags = tapNONE); + + +/** Sets the validity of a given transaction in the cache. + Use with extreme care. + + @note Can only raise the validity to a more valid state, + and can not override anything cached bad. +*/ +void +forceValidity(HashRouter& router, uint256 const& txid, + Validity validity); /** Apply a transaction to a ReadView. @@ -60,9 +108,7 @@ class Application; std::pair apply (Application& app, OpenView& view, STTx const& tx, ApplyFlags flags, - SigVerify verify, - Config const& config, - beast::Journal journal); + beast::Journal journal); } // ripple diff --git a/src/ripple/app/tx/impl/Change.cpp b/src/ripple/app/tx/impl/Change.cpp index 36e7b967a..c1f28c948 100644 --- a/src/ripple/app/tx/impl/Change.cpp +++ b/src/ripple/app/tx/impl/Change.cpp @@ -50,7 +50,9 @@ Change::preflight (PreflightContext const& ctx) return temBAD_FEE; } - if (!ctx.tx.getSigningPubKey ().empty () || !ctx.tx.getSignature ().empty ()) + if (!ctx.tx.getSigningPubKey ().empty () || + !ctx.tx.getSignature ().empty () || + ctx.tx.isFieldPresent (sfSigners)) { JLOG(ctx.j.warning) << "Change: Bad signature"; return temBAD_SIGNATURE; diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index 73c305d69..3941f4def 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -649,7 +649,6 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel) auto saTakerGets = ctx_.tx[sfTakerGets]; auto const& uPaysIssuerID = saTakerPays.getIssuer (); - auto const& uPaysCurrency = saTakerPays.getCurrency (); auto const& uGetsIssuerID = saTakerGets.getIssuer (); @@ -663,7 +662,6 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel) deprecatedWrongOwnerCount_ = (*sleCreator)[sfOwnerCount]; - auto const uAccountSequenceNext = (*sleCreator)[sfSequence]; auto const uSequence = ctx_.tx.getSequence (); // This is the original rate of the offer, and is the rate at which diff --git a/src/ripple/app/tx/impl/Payment.cpp b/src/ripple/app/tx/impl/Payment.cpp index e2cf85ed2..d1c3ee372 100644 --- a/src/ripple/app/tx/impl/Payment.cpp +++ b/src/ripple/app/tx/impl/Payment.cpp @@ -207,8 +207,6 @@ Payment::preclaim(PreclaimContext const& ctx) return temMALFORMED; } - auto const id = ctx.tx[sfAccount]; - // Ripple if source or destination is non-native or if there are paths. std::uint32_t const uTxFlags = ctx.tx.getFlags(); bool const partialPaymentAllowed = uTxFlags & tfPartialPayment; diff --git a/src/ripple/app/tx/impl/SetSignerList.cpp b/src/ripple/app/tx/impl/SetSignerList.cpp index dc05ac8a9..6e3b426de 100644 --- a/src/ripple/app/tx/impl/SetSignerList.cpp +++ b/src/ripple/app/tx/impl/SetSignerList.cpp @@ -75,8 +75,8 @@ TER SetSignerList::preflight (PreflightContext const& ctx) { if (! (ctx.flags & tapENABLE_TESTING) && - ! ctx.rules.enabled(featureMultiSign, - ctx.config.features)) + ! ctx.rules.enabled(featureMultiSign, + ctx.app.config().features)) return temDISABLED; auto const ret = preflight1 (ctx); diff --git a/src/ripple/app/tx/impl/SusPay.cpp b/src/ripple/app/tx/impl/SusPay.cpp index 4d4fe3c63..94131db96 100644 --- a/src/ripple/app/tx/impl/SusPay.cpp +++ b/src/ripple/app/tx/impl/SusPay.cpp @@ -133,7 +133,7 @@ SusPayCreate::preflight (PreflightContext const& ctx) { if (! (ctx.flags & tapENABLE_TESTING) && ! ctx.rules.enabled(featureSusPay, - ctx.config.features)) + ctx.app.config().features)) return temDISABLED; auto const ret = preflight1 (ctx); @@ -253,7 +253,7 @@ SusPayFinish::preflight (PreflightContext const& ctx) { if (! (ctx.flags & tapENABLE_TESTING) && ! ctx.rules.enabled(featureSusPay, - ctx.config.features)) + ctx.app.config().features)) return temDISABLED; auto const ret = preflight1 (ctx); @@ -373,7 +373,7 @@ SusPayCancel::preflight (PreflightContext const& ctx) { if (! (ctx.flags & tapENABLE_TESTING) && ! ctx.rules.enabled(featureSusPay, - ctx.config.features)) + ctx.app.config().features)) return temDISABLED; auto const ret = preflight1 (ctx); diff --git a/src/ripple/app/tx/impl/Transaction.cpp b/src/ripple/app/tx/impl/Transaction.cpp index d4e0b3697..83c79338d 100644 --- a/src/ripple/app/tx/impl/Transaction.cpp +++ b/src/ripple/app/tx/impl/Transaction.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -30,8 +31,8 @@ namespace ripple { -Transaction::Transaction (STTx::ref stx, Validate validate, - SigVerify sigVerify, std::string& reason, Application& app) +Transaction::Transaction (STTx::ref stx, + std::string& reason, Application& app) noexcept : mTransaction (stx) , mApp (app) @@ -39,7 +40,6 @@ Transaction::Transaction (STTx::ref stx, Validate validate, { try { - mFromPubKey.setAccountPublic (mTransaction->getSigningPubKey ()); mTransactionID = mTransaction->getTransactionID (); } catch (std::exception& e) @@ -53,24 +53,26 @@ Transaction::Transaction (STTx::ref stx, Validate validate, return; } - if (validate == Validate::NO || - (passesLocalChecks (*mTransaction, reason) && - checkSign (reason, sigVerify))) - { - mStatus = NEW; - } + mStatus = NEW; } Transaction::pointer Transaction::sharedTransaction ( - Blob const& vucTransaction, Validate validate, Application& app) + Blob const& vucTransaction, Rules const& rules, Application& app) { try { SerialIter sit (makeSlice(vucTransaction)); + auto txn = std::make_shared(sit); std::string reason; - - return std::make_shared (std::make_shared (sit), - validate, app.getHashRouter().sigVerify(), reason, app); + auto result = std::make_shared ( + txn, reason, app); + if (checkValidity(app.getHashRouter(), + *txn, rules, app.config()).first + != Validity::Valid) + { + result->setStatus(INVALID); + } + return result; } catch (std::exception& e) { @@ -82,33 +84,13 @@ Transaction::pointer Transaction::sharedTransaction ( JLOG (app.journal ("Ledger").warning) << "Exception constructing transaction"; } - return std::shared_ptr (); + return {}; } // // Misc. // -bool Transaction::checkSign (std::string& reason, SigVerify sigVerify) const -{ - bool const allowMultiSign = mApp.getLedgerMaster(). - getValidatedRules().enabled (featureMultiSign, - mApp.config().features); - - if (! mFromPubKey.isValid ()) - reason = "Transaction has bad source public key"; - else if (!sigVerify(*mTransaction, [allowMultiSign] (STTx const& tx) - { - return tx.checkSign(allowMultiSign); - })) - reason = "Transaction has bad signature"; - else - return true; - - JLOG (j_.warning) << reason; - return false; -} - void Transaction::setStatus (TransStatus ts, std::uint32_t lseq) { mStatus = ts; @@ -137,7 +119,6 @@ Transaction::pointer Transaction::transactionFromSQL ( boost::optional const& ledgerSeq, boost::optional const& status, Blob const& rawTxn, - Validate validate, Application& app) { std::uint32_t const inLedger = @@ -146,15 +127,33 @@ Transaction::pointer Transaction::transactionFromSQL ( SerialIter it (makeSlice(rawTxn)); auto txn = std::make_shared (it); std::string reason; - auto tr = std::make_shared (txn, validate, - app.getHashRouter().sigVerify(), reason, app); + auto tr = std::make_shared ( + txn, reason, app); tr->setStatus (sqlTransactionStatus (status)); tr->setLedger (inLedger); return tr; } -Transaction::pointer Transaction::load (uint256 const& id, Application& app) +Transaction::pointer Transaction::transactionFromSQLValidated( + boost::optional const& ledgerSeq, + boost::optional const& status, + Blob const& rawTxn, + Application& app) +{ + auto ret = transactionFromSQL(ledgerSeq, status, rawTxn, app); + + if (checkValidity(app.getHashRouter(), + *ret->getSTransaction(), app. + getLedgerMaster().getValidatedRules(), + app.config()).first != + Validity::Valid) + return {}; + + return ret; +} + +Transaction::pointer Transaction::load(uint256 const& id, Application& app) { std::string sql = "SELECT LedgerSeq,Status,RawTxn " "FROM Transactions WHERE TransID='"; @@ -177,8 +176,8 @@ Transaction::pointer Transaction::load (uint256 const& id, Application& app) convert(sociRawTxnBlob, rawTxn); } - return Transaction::transactionFromSQL ( - ledgerSeq, status, rawTxn, Validate::YES, app); + return Transaction::transactionFromSQLValidated ( + ledgerSeq, status, rawTxn, app); } // options 1 to include the date of the transaction diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index e97bea1e3..d096b4e67 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -87,14 +88,10 @@ preflight2 (PreflightContext const& ctx) auto const pk = RippleAddress::createAccountPublic( ctx.tx.getSigningPubKey()); - if(! ctx.verify(ctx.tx, - [&, ctx] (STTx const& tx) - { - return (ctx.flags & tapNO_CHECK_SIGN) || - tx.checkSign( - (ctx.flags & tapENABLE_TESTING) || - (ctx.rules.enabled(featureMultiSign, ctx.config.features))); - })) + if(!( ctx.flags & tapNO_CHECK_SIGN) && + checkValidity(ctx.app.getHashRouter(), + ctx.tx, ctx.rules, ctx.app.config(), + ctx.flags).first == Validity::SigBad) { JLOG(ctx.j.debug) << "preflight2: bad signature"; return temINVALID; @@ -114,6 +111,19 @@ calculateFee(Application& app, std::uint64_t const baseFee, //------------------------------------------------------------------------------ +PreflightContext::PreflightContext(Application& app_, STTx const& tx_, + Rules const& rules_, ApplyFlags flags_, + beast::Journal j_) + : app(app_) + , tx(tx_) + , rules(rules_) + , flags(flags_) + , j(j_) +{ +} + +//------------------------------------------------------------------------------ + Transactor::Transactor( ApplyContext& ctx) : ctx_ (ctx) @@ -662,7 +672,6 @@ Transactor::operator()() keylet::account(ctx_.tx.getAccountID(sfAccount))); std::uint32_t t_seq = ctx_.tx.getSequence (); - std::uint32_t a_seq = txnAcct->getFieldU32 (sfSequence); auto const balance = txnAcct->getFieldAmount (sfBalance).xrp (); diff --git a/src/ripple/app/tx/impl/Transactor.h b/src/ripple/app/tx/impl/Transactor.h index 03d84363b..83921ae4c 100644 --- a/src/ripple/app/tx/impl/Transactor.h +++ b/src/ripple/app/tx/impl/Transactor.h @@ -30,25 +30,15 @@ namespace ripple { struct PreflightContext { public: + Application& app; STTx const& tx; Rules const& rules; ApplyFlags flags; - SigVerify verify; - Config const& config; beast::Journal j; - PreflightContext(STTx const& tx_, + PreflightContext(Application& app_, STTx const& tx_, Rules const& rules_, ApplyFlags flags_, - SigVerify verify_, Config const& config_, - beast::Journal j_) - : tx(tx_) - , rules(rules_) - , flags(flags_) - , verify(verify_) - , config(config_) - , j(j_) - { - } + beast::Journal j_); }; struct PreflightResult diff --git a/src/ripple/app/tx/impl/apply.cpp b/src/ripple/app/tx/impl/apply.cpp index 510abba7f..aca6c2901 100644 --- a/src/ripple/app/tx/impl/apply.cpp +++ b/src/ripple/app/tx/impl/apply.cpp @@ -18,6 +18,7 @@ //============================================================================== #include +#include #include #include #include @@ -32,9 +33,16 @@ #include #include #include +#include namespace ripple { +// These are the same flags defined as SF_PRIVATE1-4 in HashRouter.h +#define SF_SIGBAD SF_PRIVATE1 // Signature is bad +#define SF_SIGGOOD SF_PRIVATE2 // Signature is good +#define SF_LOCALBAD SF_PRIVATE3 // Local checks failed +#define SF_LOCALGOOD SF_PRIVATE4 // Local checks passed + static TER invoke_preflight (PreflightContext const& ctx) @@ -160,13 +168,91 @@ invoke_apply (ApplyContext& ctx) //------------------------------------------------------------------------------ -PreflightResult -preflight (Rules const& rules, STTx const& tx, - ApplyFlags flags, SigVerify verify, - Config const& config, beast::Journal j) +std::pair +checkValidity(HashRouter& router, STTx const& tx, bool allowMultiSign) { - PreflightContext const pfctx(tx, - rules, flags, verify, config, j); + auto const id = tx.getTransactionID(); + auto const flags = router.getFlags(id); + if (flags & SF_SIGBAD) + // Signature is known bad + return std::make_pair(Validity::SigBad, + "Transaction has bad signature."); + if (!(flags & SF_SIGGOOD)) + { + // Don't know signature state. Check it. + if (!tx.checkSign(allowMultiSign)) + { + router.setFlags(id, SF_SIGBAD); + return std::make_pair(Validity::SigBad, + "Transaction has bad signature."); + } + router.setFlags(id, SF_SIGGOOD); + } + + // Signature is now known good + if (flags & SF_LOCALBAD) + // ...but the local checks + // are known bad. + return std::make_pair(Validity::SigGoodOnly, + "Local checks failed."); + if (flags & SF_LOCALGOOD) + // ...and the local checks + // are known good. + return std::make_pair(Validity::Valid, ""); + + // Do the local checks + std::string reason; + if (!passesLocalChecks(tx, reason)) + { + router.setFlags(id, SF_LOCALBAD); + return std::make_pair(Validity::SigGoodOnly, reason); + } + router.setFlags(id, SF_LOCALGOOD); + return std::make_pair(Validity::Valid, ""); +} + +std::pair +checkValidity(HashRouter& router, + STTx const& tx, Rules const& rules, + Config const& config, + ApplyFlags const& flags) +{ + auto const allowMultiSign = + (flags & tapENABLE_TESTING) || + (rules.enabled(featureMultiSign, + config.features)); + + return checkValidity(router, tx, allowMultiSign); +} + +void +forceValidity(HashRouter& router, uint256 const& txid, + Validity validity) +{ + int flags = 0; + switch (validity) + { + case Validity::Valid: + flags |= SF_LOCALGOOD; + // fall through + case Validity::SigGoodOnly: + flags |= SF_SIGGOOD; + // fall through + case Validity::SigBad: + // would be silly to call directly + break; + } + if (flags) + router.setFlags(txid, flags); +} + +PreflightResult +preflight (Application& app, Rules const& rules, + STTx const& tx, ApplyFlags flags, + beast::Journal j) +{ + PreflightContext const pfctx(app, tx, + rules, flags, j); try { return{ pfctx, invoke_preflight(pfctx) }; @@ -192,10 +278,9 @@ preclaim (PreflightResult const& preflightResult, boost::optional ctx; if (preflightResult.ctx.rules != view.rules()) { - auto secondFlight = preflight(view.rules(), + auto secondFlight = preflight(app, view.rules(), preflightResult.ctx.tx, preflightResult.ctx.flags, - preflightResult.ctx.verify, preflightResult.ctx.config, - preflightResult.ctx.j); + preflightResult.ctx.j); ctx.emplace(app, view, secondFlight.ter, secondFlight.ctx.tx, secondFlight.ctx.flags, secondFlight.ctx.j); } @@ -240,8 +325,8 @@ doApply(PreclaimResult const& preclaimResult, if (preclaimResult.ter != tesSUCCESS && !isTecClaim(preclaimResult.ter)) return{ preclaimResult.ter, false }; - ApplyContext ctx( - app, view, preclaimResult.ctx.tx, preclaimResult.ter, + ApplyContext ctx(app, view, + preclaimResult.ctx.tx, preclaimResult.ter, preclaimResult.baseFee, preclaimResult.ctx.flags, preclaimResult.ctx.j); return invoke_apply(ctx); @@ -263,11 +348,10 @@ doApply(PreclaimResult const& preclaimResult, std::pair apply (Application& app, OpenView& view, STTx const& tx, ApplyFlags flags, - SigVerify verify, Config const& config, - beast::Journal j) + beast::Journal j) { - auto pfresult = preflight(view.rules(), - tx, flags, verify, config, j); + auto pfresult = preflight(app, view.rules(), + tx, flags, j); auto pcresult = preclaim(pfresult, app, view); return doApply(pcresult, app, view); } diff --git a/src/ripple/app/tx/impl/applyImpl.h b/src/ripple/app/tx/impl/applyImpl.h index f27361c4f..961638bbe 100644 --- a/src/ripple/app/tx/impl/applyImpl.h +++ b/src/ripple/app/tx/impl/applyImpl.h @@ -35,9 +35,9 @@ validity constraints that do not require a ledger. other things, the TER code. */ PreflightResult -preflight(Rules const& rules, STTx const& tx, - ApplyFlags flags, SigVerify verify, - Config const& config, beast::Journal j); +preflight(Application& app, Rules const& rules, + STTx const& tx, ApplyFlags flags, + beast::Journal j); /** Gate a transaction based on static ledger information. diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index 65ebd3227..bf341a890 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/src/ripple/overlay/impl/PeerImp.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -1052,6 +1053,7 @@ PeerImp::onMessage (std::shared_ptr const& m) p_journal_.debug << "Got tx " << txID; + bool checkSignature = true; if (cluster()) { if (! m->has_deferred () || ! m->deferred ()) @@ -1065,7 +1067,7 @@ PeerImp::onMessage (std::shared_ptr const& m) { // For now, be paranoid and have each validator // check each transaction, regardless of source - flags |= SF_SIGGOOD; + checkSignature = false; } } @@ -1078,9 +1080,10 @@ PeerImp::onMessage (std::shared_ptr const& m) std::weak_ptr weak = shared_from_this(); app_.getJobQueue ().addJob ( jtTRANSACTION, "recvTransaction->checkTransaction", - [weak, flags, stx] (Job&) { + [weak, flags, checkSignature, stx] (Job&) { if (auto peer = weak.lock()) - peer->checkTransaction(flags, stx); + peer->checkTransaction(flags, + checkSignature, stx); }); } } @@ -1712,7 +1715,8 @@ PeerImp::doFetchPack (const std::shared_ptr& packet } void -PeerImp::checkTransaction (int flags, STTx::pointer stx) +PeerImp::checkTransaction (int flags, + bool checkSignature, STTx::pointer stx) { // VFALCO TODO Rewrite to not use exceptions try @@ -1727,11 +1731,36 @@ PeerImp::checkTransaction (int flags, STTx::pointer stx) return; } - auto validate = (flags & SF_SIGGOOD) ? Validate::NO : Validate::YES; + if (checkSignature) + { + // Check the signature before handing off to the job queue. + auto valid = checkValidity(app_.getHashRouter(), *stx, + app_.getLedgerMaster().getValidatedRules(), + app_.config()); + if (valid.first != Validity::Valid) + { + if (!valid.second.empty()) + { + JLOG(p_journal_.trace) << + "Exception checking transaction: " << + valid.second; + } + + // Probably not necessary to set SF_BAD, but doesn't hurt. + app_.getHashRouter().setFlags(stx->getTransactionID(), SF_BAD); + charge(Resource::feeInvalidSignature); + return; + } + } + else + { + forceValidity(app_.getHashRouter(), + stx->getTransactionID(), Validity::Valid); + } + std::string reason; - auto tx = std::make_shared (stx, validate, - directSigVerify, - reason, app_); + auto tx = std::make_shared ( + stx, reason, app_); if (tx->getStatus () == INVALID) { @@ -1742,10 +1771,6 @@ PeerImp::checkTransaction (int flags, STTx::pointer stx) charge (Resource::feeInvalidSignature); return; } - else - { - app_.getHashRouter ().setFlags (stx->getTransactionID (), SF_SIGGOOD); - } bool const trusted (flags & SF_TRUSTED); app_.getOPs ().processTransaction ( diff --git a/src/ripple/overlay/impl/PeerImp.h b/src/ripple/overlay/impl/PeerImp.h index 0c14ea25f..884868e65 100644 --- a/src/ripple/overlay/impl/PeerImp.h +++ b/src/ripple/overlay/impl/PeerImp.h @@ -449,7 +449,7 @@ private: doFetchPack (const std::shared_ptr& packet); void - checkTransaction (int flags, STTx::pointer stx); + checkTransaction (int flags, bool checkSignature, STTx::pointer stx); void checkPropose (Job& job, diff --git a/src/ripple/protocol/STTx.h b/src/ripple/protocol/STTx.h index 84fa4de42..c8360c62b 100644 --- a/src/ripple/protocol/STTx.h +++ b/src/ripple/protocol/STTx.h @@ -139,14 +139,6 @@ private: bool passesLocalChecks (STObject const& st, std::string&); -using SigVerify = std::function < bool(STTx const&, std::function) > ; - -inline -bool directSigVerify(STTx const& tx, std::function sigCheck) -{ - return sigCheck(tx); -} - } // ripple #endif diff --git a/src/ripple/rpc/handlers/Submit.cpp b/src/ripple/rpc/handlers/Submit.cpp index ccbf29a21..c39ebe6c5 100644 --- a/src/ripple/rpc/handlers/Submit.cpp +++ b/src/ripple/rpc/handlers/Submit.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -80,10 +81,23 @@ Json::Value doSubmit (RPC::Context& context) return jvResult; } + { + auto validity = checkValidity(context.app.getHashRouter(), + *stpTrans, context.ledgerMaster.getCurrentLedger()->rules(), + context.app.config()); + if (validity.first != Validity::Valid) + { + jvResult[jss::error] = "invalidTransaction"; + jvResult[jss::error_exception] = "fails local checks: " + validity.second; + + return jvResult; + } + } + Transaction::pointer tpTrans; std::string reason; - tpTrans = std::make_shared (stpTrans, Validate::YES, - context.app.getHashRouter().sigVerify(), reason, context.app); + tpTrans = std::make_shared ( + stpTrans, reason, context.app); if (tpTrans->getStatus() != NEW) { jvResult[jss::error] = "invalidTransaction"; diff --git a/src/ripple/rpc/handlers/TxHistory.cpp b/src/ripple/rpc/handlers/TxHistory.cpp index 6b9fc4ce5..f06e5f981 100644 --- a/src/ripple/rpc/handlers/TxHistory.cpp +++ b/src/ripple/rpc/handlers/TxHistory.cpp @@ -81,7 +81,7 @@ Json::Value doTxHistory (RPC::Context& context) rawTxn.clear (); if (auto trans = Transaction::transactionFromSQL ( - ledgerSeq, status, rawTxn, Validate::NO, context.app)) + ledgerSeq, status, rawTxn, context.app)) txs.append (trans->getJson (0)); } } diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 972013cf4..4e33d8628 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -506,7 +506,8 @@ transactionPreProcessImpl ( static std::pair -transactionConstructImpl (STTx::pointer stpTrans, Application& app) +transactionConstructImpl (STTx::pointer stpTrans, + Rules const& rules, Application& app) { std::pair ret; @@ -514,8 +515,8 @@ transactionConstructImpl (STTx::pointer stpTrans, Application& app) Transaction::pointer tpTrans; { std::string reason; - tpTrans = std::make_shared(stpTrans, Validate::NO, - directSigVerify, reason, app); + tpTrans = std::make_shared( + stpTrans, reason, app); if (tpTrans->getStatus () != NEW) { ret.first = RPC::make_error (rpcINTERNAL, @@ -534,7 +535,7 @@ transactionConstructImpl (STTx::pointer stpTrans, Application& app) tpTrans->getSTransaction ()->add (s); Transaction::pointer tpTransNew = - Transaction::sharedTransaction(s.getData(), Validate::YES, app); + Transaction::sharedTransaction(s.getData(), rules, app); if (tpTransNew && ( !tpTransNew->getSTransaction ()->isEquivalent ( @@ -673,7 +674,8 @@ Json::Value transactionSign ( // Make sure the STTx makes a legitimate Transaction. std::pair txn = - transactionConstructImpl (preprocResult.second, app); + transactionConstructImpl (preprocResult.second, + ledger->rules(), app); if (!txn.second) return txn.first; @@ -707,7 +709,8 @@ Json::Value transactionSubmit ( // Make sure the STTx makes a legitimate Transaction. std::pair txn = - transactionConstructImpl (preprocResult.second, app); + transactionConstructImpl (preprocResult.second, + ledger->rules(), app); if (!txn.second) return txn.first; @@ -825,7 +828,8 @@ Json::Value transactionSignFor ( // Make sure the STTx makes a legitimate Transaction. std::pair txn = - transactionConstructImpl (preprocResult.second, app); + transactionConstructImpl (preprocResult.second, + ledger->rules(), app); if (!txn.second) return txn.first; @@ -1052,7 +1056,8 @@ Json::Value transactionSubmitMultiSigned ( // Make sure the SerializedTransaction makes a legitimate Transaction. std::pair txn = - transactionConstructImpl (stpTrans, app); + transactionConstructImpl (stpTrans, + ledger->rules(), app); if (!txn.second) return txn.first; diff --git a/src/ripple/test/jtx/impl/Env.cpp b/src/ripple/test/jtx/impl/Env.cpp index 11e7918d7..c4be36dea 100644 --- a/src/ripple/test/jtx/impl/Env.cpp +++ b/src/ripple/test/jtx/impl/Env.cpp @@ -43,6 +43,7 @@ #include #include #include +#include #include namespace ripple { @@ -84,7 +85,7 @@ Env::Env (beast::unit_test::suite& test_) , closed_ (std::make_shared( create_genesis, app().config(), app().family())) , cachedSLEs_ (std::chrono::seconds(5), stopwatch_) - , openLedger (closed_, app().config(), cachedSLEs_, journal) + , openLedger (closed_, cachedSLEs_, journal) { memoize(master); Pathfinder::initPathTable(); @@ -122,7 +123,7 @@ Env::close(NetClock::time_point const& closeTime) OpenView accum(&*next); OpenLedger::apply(app(), accum, *closed_, txs, retries, applyFlags(), *router, - app().config(), journal); + journal); accum.apply(*next); } // To ensure that the close time is exact and not rounded, we don't @@ -278,8 +279,7 @@ Env::submit (JTx const& jt) { std::tie(ter_, didApply) = ripple::apply( app(), view, *stx, applyFlags(), - directSigVerify, app().config(), - beast::Journal{}); + beast::Journal{}); return didApply; }); } diff --git a/src/ripple/unity/app_tests.cpp b/src/ripple/unity/app_tests.cpp index a236ac9e8..31be40ea9 100644 --- a/src/ripple/unity/app_tests.cpp +++ b/src/ripple/unity/app_tests.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include