Consolidate transaction signature checking.

* All checks flow through ripple::checkValidity, which transparently caches result flags.
* All external transaction submission code paths use checkValidity.
* SF_SIGGOOD flag no longer appears outside of HashRouter / checkValidity.
* Validity can be forced in known or trusted scenarios.

Builds/VisualStudio2015/RippleD.vcxproj

@@ -1688,6 +1688,10 @@
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
     </ClCompile>
+    <ClCompile Include="..\..\src\ripple\app\tests\HashRouter_test.cpp">
+      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
+      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
+    </ClCompile>
     <ClCompile Include="..\..\src\ripple\app\tests\MultiSign.test.cpp">
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>

Builds/VisualStudio2015/RippleD.vcxproj.filters

@@ -2436,6 +2436,9 @@
     <ClCompile Include="..\..\src\ripple\app\tests\DeliverMin.test.cpp">
       <Filter>ripple\app\tests</Filter>
     </ClCompile>
+    <ClCompile Include="..\..\src\ripple\app\tests\HashRouter_test.cpp">
+      <Filter>ripple\app\tests</Filter>
+    </ClCompile>
     <ClCompile Include="..\..\src\ripple\app\tests\MultiSign.test.cpp">
       <Filter>ripple\app\tests</Filter>
     </ClCompile>

src/ripple/app/ledger/OpenLedger.h

@@ -28,7 +28,6 @@
 #include <ripple/basics/Log.h>
 #include <ripple/basics/UnorderedContainers.h>
 #include <ripple/core/Config.h>
-#include <beast/container/aged_unordered_map.h>
 #include <beast/utility/Journal.h>
 #include <cassert>
 #include <mutex>
@@ -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<OpenView const> 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,
+                HashRouter& router, beast::Journal j);
-                    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,
+                HashRouter& router, beast::Journal j);
-                    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,
+            HashRouter& router, beast::Journal j)
-                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;

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

@@ -42,8 +42,8 @@
 #include <ripple/json/to_string.h>
 #include <ripple/overlay/Overlay.h>
 #include <ripple/overlay/predicates.h>
-#include <ripple/protocol/STValidation.h>
+#include <ripple/protocol/st.h>
-#include <ripple/protocol/UintTypes.h>
+#include <ripple/protocol/Feature.h>
 #include <beast/module/core/text/LexicalCast.h>
 #include <beast/utility/make_lock.h>
 #include <type_traits>
@@ -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,
+        auto const result = apply(app,
-            app.getHashRouter().sigVerify(),
+            view, *txn, flags, j);
-                app.config(), j);
         if (result.second)
         {
             JLOG (j.debug)

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(
+                        *it.second, flags, j);
-                            ).sigVerify(), app_.config(), j);
                     if (result.second)
                         any = true;
                 }

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

@@ -21,17 +21,17 @@
 #include <ripple/app/ledger/OpenLedger.h>
 #include <ripple/app/tx/apply.h>
 #include <ripple/ledger/CachedView.h>
+#include <ripple/protocol/Feature.h>
 #include <boost/range/adaptor/transformed.hpp>
 
 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<std::shared_ptr<
                 STTx const>>;
         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,
+        ripple::apply(app, *next,
-            flags, router.sigVerify(),
+            *item.second, flags, j_);
-                config_, 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<STTx const> const& tx,
         bool retry, ApplyFlags flags,
-            HashRouter& router, Config const& config,
+            HashRouter& router,beast::Journal j) -> Result
-                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.
+        app, view, *tx, flags, j);
-            sigVerify(), config, j);
     if (result.second)
         return Result::success;
     if (isTefFailure (result.first) ||

src/ripple/app/main/Application.cpp

@@ -44,6 +44,7 @@
 #include <ripple/app/misc/UniqueNodeList.h>
 #include <ripple/app/tx/InboundTransactions.h>
 #include <ripple/app/tx/TransactionMaster.h>
+#include <ripple/app/tx/apply.h>
 #include <ripple/basics/Log.h>
 #include <ripple/basics/ResolverAsio.h>
 #include <ripple/basics/Sustain.h>
@@ -1117,8 +1118,8 @@ ApplicationImp::startGenesisLedger()
     next->setClosed ();
     next->setImmutable (*config_);
     m_networkOPs->setLastCloseTime (next->info().closeTime);
-    openLedger_.emplace(next, *config_,
+    openLedger_.emplace(next, cachedSLEs_,
-        cachedSLEs_, logs_->journal("OpenLedger"));
+        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_,
+        openLedger_.emplace(loadLedger, cachedSLEs_,
-            cachedSLEs_, logs_->journal("OpenLedger"));
+            logs_->journal("OpenLedger"));
 
         if (replay)
         {
@@ -1395,7 +1396,8 @@ bool ApplicationImp::loadOldLedger (
                 auto s = std::make_shared <Serializer> ();
                 txPair.first->add(*s);
 
-                getHashRouter().setFlags (txID, SF_SIGGOOD);
+                forceValidity(getHashRouter(),
+                    txID, Validity::SigGoodOnly);
 
                 replayData->txns_.emplace (txIndex, txPair.first);
 

src/ripple/app/misc/HashRouter.cpp

@@ -19,131 +19,85 @@
 
 #include <BeastConfig.h>
 #include <ripple/app/misc/HashRouter.h>
-#include <ripple/protocol/STTx.h>
 #include <ripple/basics/UptimeTimer.h>
 #include <map>
 #include <mutex>
 
 namespace ripple {
 
-std::function<bool(STTx const&, std::function<bool(STTx const&)>)>
+auto
-HashRouter::sigVerify()
+HashRouter::emplace (uint256 const& index)
+    -> std::pair<Entry&, bool>
 {
-    return
+    auto fit = mSuppressionMap.find (index);
-    [&] (STTx const& tx, std::function<bool(STTx const&)> 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<uint256, Entry>::iterator fit = mSuppressionMap.find (index);
 
     if (fit != mSuppressionMap.end ())
     {
-        created = false;
+        return std::make_pair(
-        return fit->second;
+            std::ref(fit->second), false);
     }
 
-    created = true;
+    int elapsed = UptimeTimer::getInstance ().getElapsedSeconds ();
-
+    int expireCutoff = elapsed - mHoldTime;
-    int now = UptimeTimer::getInstance ().getElapsedSeconds ();
-    int expireTime = now - 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);
+    mSuppressionTimes[elapsed].push_back (index);
-    return mSuppressionMap.emplace (index, Entry ()).first->second;
+    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 <std::mutex> lock (mMutex);
 
-    bool created;
+    emplace (index);
-    findCreateEntry (index, created);
-    return created;
-}
-
-HashRouter::Entry HashRouter::getEntry (uint256 const& index)
-{
-    ScopedLockType lock (mMutex);
-
-    bool created;
-    return findCreateEntry (index, created);
 }
 
 bool HashRouter::addSuppressionPeer (uint256 const& index, PeerShortID peer)
 {
-    ScopedLockType lock (mMutex);
+    std::lock_guard <std::mutex> lock (mMutex);
 
-    bool created;
+    auto result = emplace(index);
-    findCreateEntry (index, created).addPeer (peer);
+    result.first.addPeer(peer);
-    return created;
+    return result.second;
 }
 
 bool HashRouter::addSuppressionPeer (uint256 const& index, PeerShortID peer, int& flags)
 {
-    ScopedLockType lock (mMutex);
+    std::lock_guard <std::mutex> lock (mMutex);
 
-    bool created;
+    auto result = emplace(index);
-    Entry& s = findCreateEntry (index, created);
+    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 <std::mutex> lock (mMutex);
 
-    bool created;
+    return emplace(index).first.getFlags ();
-    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;
 }
 
 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 <std::mutex> lock (mMutex);
 
-    bool created;
+    auto& s = emplace(index).first;
-    Entry& s = findCreateEntry (index, created);
 
     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<PeerShortID>& peers, int flag)
 {
-    ScopedLockType lock (mMutex);
+    std::lock_guard <std::mutex> lock (mMutex);
 
-    bool created;
+    auto& s = emplace(index).first;
-    Entry& s = findCreateEntry (index, created);
 
     if ((s.getFlags () & flag) == flag)
         return false;

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_BAD          0x02    // Temporarily bad
-#define SF_SIGGOOD      0x04    // Signature is good
+#define SF_SAVED        0x04
-#define SF_SAVED        0x08
+#define SF_RETRY        0x08    // Transaction can be retried
-#define SF_RETRY        0x10    // Transaction can be retried
+#define SF_TRUSTED      0x10    // comes from trusted source
-#define SF_TRUSTED      0x20    // 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 <PeerShortID> 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 <PeerShortID>& other)
         {
-            mPeers.swap (other);
+            peers_.swap (other);
         }
 
     private:
-        int mFlags;
+        int flags_;
-        std::set <PeerShortID> mPeers;
+        std::set <PeerShortID> 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<PeerShortID>& peers, int flag);
 
-    /**
-        Function wrapper that will check the signature status
-        of a STTx before calling an expensive signature
-        checking function.
-    */
-    std::function<bool(STTx const&, std::function<bool(STTx const&)>)>
-    sigVerify();
-
 private:
-    Entry getEntry (uint256 const& );
+    // pair.second indicates whether the entry was created
+    std::pair<Entry&, bool> emplace (uint256 const&);
 
-    Entry& findCreateEntry (uint256 const& , bool& created);
+    std::mutex mutable mMutex;
-
-    using MutexType = std::mutex;
-    using ScopedLockType = std::lock_guard <MutexType>;
-    MutexType mMutex;
 
     // Stores all suppressed hashes and their expiration time
     hash_map <uint256, Entry> mSuppressionMap;
@@ -172,7 +164,7 @@ private:
     // Stores all expiration times and the hashes indexed for them
     std::map< int, std::list<uint256> > mSuppressionTimes;
 
-    int mHoldTime;
+    int const mHoldTime;
 };
 
 } // ripple

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<STTx> (std::ref (sit));
 
-    uint256 suppress = trans->getTransactionID ();
+    auto const txid = trans->getTransactionID ();
-    int flags;
+    auto const flags = app_.getHashRouter().getFlags(txid);
 
-    if (app_.getHashRouter ().addSuppressionPeer (suppress, 0, flags) &&
+    if ((flags & SF_RETRY) != 0)
-        ((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<Transaction> (
-        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,22 +716,27 @@ 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.
-        // signature not checked
+    // If so, only cost is looking up HashRouter flags.
-        std::string reason;
+    auto const view = m_ledgerMaster.getCurrentLedger();
+    auto const validity = checkValidity(
+        app_.getHashRouter(),
+            *transaction->getSTransaction(),
+                view->rules(), app_.config());
+    assert(validity.first == Validity::Valid);
 
-        if (! transaction->checkSign (reason, directSigVerify))
+    // Not concerned with local checks at this point.
+    if (validity.first == Validity::SigBad)
     {
-            m_journal.info << "Transaction has bad signature: " << reason;
+        m_journal.info << "Transaction has bad signature: " <<
+            validity.second;
         transaction->setStatus(INVALID);
         transaction->setResult(temBAD_SIGNATURE);
-            app_.getHashRouter ().setFlags (transaction->getID (), SF_BAD);
+        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<std::mutex>& 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<std::mutex>& batchLock)
                 app_.openLedger().modify(
                     [&](OpenView& view, beast::Journal j)
                     {
-                        auto const result = ripple::apply(app_,
+                        auto const result = ripple::apply(app_, view,
-                            view, *e.transaction->getSTransaction(), flags,
+                            *e.transaction->getSTransaction(), flags, j);
-                                app_.getHashRouter().sigVerify(),
-                                    app_.config(), 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,6 +1684,7 @@ NetworkOPs::AccountTxs NetworkOPsImp::getAccountTxs (
                     pendSaveValidated(app_, ledger, false, false);
             }
 
+            if (txn)
                 ret.emplace_back (txn, std::make_shared<TxMeta> (
                     txn->getID (), txn->getLedger (), txnMeta,
                         app_.journal("TxMeta")));

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

@@ -43,8 +43,7 @@ convertBlobsToTxResult (
     STTx::pointer txn = std::make_shared<STTx> (it);
     std::string reason;
 
-    auto tr = std::make_shared<Transaction> (txn, Validate::NO,
+    auto tr = std::make_shared<Transaction> (txn, reason, app);
-        directSigVerify, reason, app);
 
     tr->setStatus (Transaction::sqlTransactionStatus(status));
     tr->setLedger (ledger_index);

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 <BeastConfig.h>
+#include <ripple/app/misc/HashRouter.h>
+#include <ripple/basics/UptimeTimer.h>
+#include <beast/unit_test/suite.h>
+
+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<HashRouter::PeerShortID> peers1;
+        std::set<HashRouter::PeerShortID> 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)
+
+}
+}

src/ripple/app/tests/Regression_test.cpp

@@ -67,7 +67,6 @@ 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);
             expect(result.first == tesSUCCESS);
             expect(result.second);
@@ -95,7 +94,6 @@ 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);
             expect(result.first == tecINSUFF_FEE);
             expect(result.second);

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<std::uint64_t> const& ledgerSeq,
         boost::optional<std::string> const& status,
         Blob const& rawTxn,
-        Validate validate,
+        Application& app);
+
+    static
+    Transaction::pointer
+    transactionFromSQLValidated (
+        boost::optional<std::uint64_t> const& ledgerSeq,
+        boost::optional<std::string> const& status,
+        Blob const& rawTxn,
         Application& app);
 
     static
     TransStatus
     sqlTransactionStatus(boost::optional<std::string> 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;

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<Validity, std::string>
+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<Validity, std::string>
+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,8 +108,6 @@ class Application;
 std::pair<TER, bool>
 apply (Application& app, OpenView& view,
     STTx const& tx, ApplyFlags flags,
-        SigVerify verify,
-            Config const& config,
         beast::Journal journal);
 
 } // ripple

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;

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

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;

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

@@ -76,7 +76,7 @@ SetSignerList::preflight (PreflightContext const& ctx)
 {
     if (! (ctx.flags & tapENABLE_TESTING) &&
             ! ctx.rules.enabled(featureMultiSign,
-            ctx.config.features))
+                ctx.app.config().features))
         return temDISABLED;
 
     auto const ret = preflight1 (ctx);

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);

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

@@ -19,6 +19,7 @@
 
 #include <BeastConfig.h>
 #include <ripple/app/tx/Transaction.h>
+#include <ripple/app/tx/apply.h>
 #include <ripple/basics/Log.h>
 #include <ripple/core/DatabaseCon.h>
 #include <ripple/app/ledger/LedgerMaster.h>
@@ -30,8 +31,8 @@
 
 namespace ripple {
 
-Transaction::Transaction (STTx::ref stx, Validate validate,
+Transaction::Transaction (STTx::ref stx,
-    SigVerify sigVerify, std::string& reason, Application& app)
+    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;
 }
-}
 
 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<STTx>(sit);
         std::string reason;
-
+        auto result = std::make_shared<Transaction> (
-        return std::make_shared<Transaction> (std::make_shared<STTx> (sit),
+            txn, reason, app);
-            validate, app.getHashRouter().sigVerify(), 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<Transaction> ();
+    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<std::uint64_t> const& ledgerSeq,
     boost::optional<std::string> const& status,
     Blob const& rawTxn,
-    Validate validate,
     Application& app)
 {
     std::uint32_t const inLedger =
@@ -146,14 +127,32 @@ Transaction::pointer Transaction::transactionFromSQL (
     SerialIter it (makeSlice(rawTxn));
     auto txn = std::make_shared<STTx> (it);
     std::string reason;
-    auto tr = std::make_shared<Transaction> (txn, validate,
+    auto tr = std::make_shared<Transaction> (
-        app.getHashRouter().sigVerify(), reason, app);
+        txn, reason, app);
 
     tr->setStatus (sqlTransactionStatus (status));
     tr->setLedger (inLedger);
     return tr;
 }
 
+Transaction::pointer Transaction::transactionFromSQLValidated(
+    boost::optional<std::uint64_t> const& ledgerSeq,
+    boost::optional<std::string> 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 "
@@ -177,8 +176,8 @@ Transaction::pointer Transaction::load (uint256 const& id, Application& app)
         convert(sociRawTxnBlob, rawTxn);
     }
 
-    return Transaction::transactionFromSQL (
+    return Transaction::transactionFromSQLValidated (
-        ledgerSeq, status, rawTxn, Validate::YES, app);
+        ledgerSeq, status, rawTxn, app);
 }
 
 // options 1 to include the date of the transaction

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

@@ -19,6 +19,7 @@
 
 #include <BeastConfig.h>
 #include <ripple/app/main/Application.h>
+#include <ripple/app/tx/apply.h>
 #include <ripple/app/tx/impl/Transactor.h>
 #include <ripple/app/tx/impl/SignerEntries.h>
 #include <ripple/basics/Log.h>
@@ -87,14 +88,10 @@ preflight2 (PreflightContext const& ctx)
     auto const pk = RippleAddress::createAccountPublic(
         ctx.tx.getSigningPubKey());
 
-    if(! ctx.verify(ctx.tx,
+    if(!( ctx.flags & tapNO_CHECK_SIGN) &&
-        [&, ctx] (STTx const& tx)
+        checkValidity(ctx.app.getHashRouter(),
-        {
+            ctx.tx, ctx.rules, ctx.app.config(),
-            return (ctx.flags & tapNO_CHECK_SIGN) ||
+                ctx.flags).first == Validity::SigBad)
-                tx.checkSign(
-                    (ctx.flags & tapENABLE_TESTING) ||
-                    (ctx.rules.enabled(featureMultiSign, ctx.config.features)));
-        }))
     {
         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 ();
 

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_);
-                beast::Journal j_)
-        : tx(tx_)
-        , rules(rules_)
-        , flags(flags_)
-        , verify(verify_)
-        , config(config_)
-        , j(j_)
-    {
-    }
 };
 
 struct PreflightResult

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

@@ -18,6 +18,7 @@
 //==============================================================================
 
 #include <BeastConfig.h>
+#include <ripple/app/misc/HashRouter.h>
 #include <ripple/app/tx/apply.h>
 #include <ripple/app/tx/impl/applyImpl.h>
 #include <ripple/app/tx/impl/ApplyContext.h>
@@ -32,9 +33,16 @@
 #include <ripple/app/tx/impl/SetSignerList.h>
 #include <ripple/app/tx/impl/SetTrust.h>
 #include <ripple/app/tx/impl/SusPay.h>
+#include <ripple/protocol/Feature.h>
 
 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
+std::pair<Validity, std::string>
-preflight (Rules const& rules, STTx const& tx,
+checkValidity(HashRouter& router, STTx const& tx, bool allowMultiSign)
-    ApplyFlags flags, SigVerify verify,
-        Config const& config, beast::Journal j)
 {
-    PreflightContext const pfctx(tx,
+    auto const id = tx.getTransactionID();
-        rules, flags, verify, config, j);
+    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<Validity, std::string>
+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,9 +278,8 @@ preclaim (PreflightResult const& preflightResult,
     boost::optional<PreclaimContext const> 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);
         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(
+        ApplyContext ctx(app, view,
-            app, view, preclaimResult.ctx.tx, preclaimResult.ter,
+            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<TER, bool>
 apply (Application& app, OpenView& view,
     STTx const& tx, ApplyFlags flags,
-        SigVerify verify, Config const& config,
         beast::Journal j)
 {
-    auto pfresult = preflight(view.rules(),
+    auto pfresult = preflight(app, view.rules(),
-        tx, flags, verify, config, j);
+        tx, flags, j);
     auto pcresult = preclaim(pfresult, app, view);
     return doApply(pcresult, app, view);
 }

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,
+preflight(Application& app, Rules const& rules,
-    ApplyFlags flags, SigVerify verify,
+    STTx const& tx, ApplyFlags flags,
-    Config const& config, beast::Journal j);
+        beast::Journal j);
 
 /** Gate a transaction based on static ledger information.
 

src/ripple/overlay/impl/PeerImp.cpp

@@ -28,6 +28,7 @@
 #include <ripple/overlay/ClusterNodeStatus.h>
 #include <ripple/app/misc/UniqueNodeList.h>
 #include <ripple/app/tx/InboundTransactions.h>
+#include <ripple/app/tx/apply.h>
 #include <ripple/protocol/digest.h>
 #include <ripple/basics/StringUtilities.h>
 #include <ripple/basics/UptimeTimer.h>
@@ -1052,6 +1053,7 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMTransaction> 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 <protocol::TMTransaction> 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 <protocol::TMTransaction> const& m)
             std::weak_ptr<PeerImp> 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<protocol::TMGetObjectByHash>& 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<Transaction> (stx, validate,
+        auto tx = std::make_shared<Transaction> (
-            directSigVerify,
+            stx, reason, app_);
-            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 (

src/ripple/overlay/impl/PeerImp.h

@@ -449,7 +449,7 @@ private:
     doFetchPack (const std::shared_ptr<protocol::TMGetObjectByHash>& packet);
 
     void
-    checkTransaction (int flags, STTx::pointer stx);
+    checkTransaction (int flags, bool checkSignature, STTx::pointer stx);
 
     void
     checkPropose (Job& job,

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<bool(STTx const&)>) > ;
-
-inline
-bool directSigVerify(STTx const& tx, std::function<bool(STTx const&)> sigCheck)
-{
-    return sigCheck(tx);
-}
-
 } // ripple
 
 #endif

src/ripple/rpc/handlers/Submit.cpp

@@ -20,6 +20,7 @@
 #include <BeastConfig.h>
 #include <ripple/app/ledger/LedgerMaster.h>
 #include <ripple/app/misc/HashRouter.h>
+#include <ripple/app/tx/apply.h>
 #include <ripple/net/RPCErr.h>
 #include <ripple/protocol/ErrorCodes.h>
 #include <ripple/resource/Fees.h>
@@ -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<Transaction> (stpTrans, Validate::YES,
+    tpTrans = std::make_shared<Transaction> (
-        context.app.getHashRouter().sigVerify(), reason, context.app);
+        stpTrans, reason, context.app);
     if (tpTrans->getStatus() != NEW)
     {
         jvResult[jss::error]            = "invalidTransaction";

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));
         }
     }

src/ripple/rpc/impl/TransactionSign.cpp

@@ -506,7 +506,8 @@ transactionPreProcessImpl (
 
 static
 std::pair <Json::Value, Transaction::pointer>
-transactionConstructImpl (STTx::pointer stpTrans, Application& app)
+transactionConstructImpl (STTx::pointer stpTrans,
+    Rules const& rules, Application& app)
 {
     std::pair <Json::Value, Transaction::pointer> ret;
 
@@ -514,8 +515,8 @@ transactionConstructImpl (STTx::pointer stpTrans, Application& app)
     Transaction::pointer tpTrans;
     {
         std::string reason;
-        tpTrans = std::make_shared<Transaction>(stpTrans, Validate::NO,
+        tpTrans = std::make_shared<Transaction>(
-            directSigVerify, reason, app);
+            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 <Json::Value, Transaction::pointer> 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 <Json::Value, Transaction::pointer> 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 <Json::Value, Transaction::pointer> 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 <Json::Value, Transaction::pointer> txn =
-        transactionConstructImpl (stpTrans, app);
+        transactionConstructImpl (stpTrans,
+            ledger->rules(), app);
 
     if (!txn.second)
         return txn.first;

src/ripple/test/jtx/impl/Env.cpp

@@ -43,6 +43,7 @@
 #include <ripple/protocol/TER.h>
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/types.h>
+#include <ripple/protocol/Feature.h>
 #include <memory>
 
 namespace ripple {
@@ -84,7 +85,7 @@ Env::Env (beast::unit_test::suite& test_)
     , closed_ (std::make_shared<Ledger>(
         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,7 +279,6 @@ Env::submit (JTx const& jt)
             {
                 std::tie(ter_, didApply) = ripple::apply(
                     app(), view, *stx, applyFlags(),
-                        directSigVerify, app().config(),
                         beast::Journal{});
                 return didApply;
             });

src/ripple/unity/app_tests.cpp

@@ -23,6 +23,7 @@
 #include <ripple/app/tests/AmendmentTable.test.cpp>
 #include <ripple/app/tests/CrossingLimits_test.cpp>
 #include <ripple/app/tests/DeliverMin.test.cpp>
+#include <ripple/app/tests/HashRouter_test.cpp>
 #include <ripple/app/tests/MultiSign.test.cpp>
 #include <ripple/app/tests/OfferStream.test.cpp>
 #include <ripple/app/tests/Offer.test.cpp>