Track STTx validity with HashRouter. (RIPD-977)

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

@@ -1150,8 +1150,10 @@ void LedgerConsensusImp::accept (std::shared_ptr<SHAMap> set)
                 newLCL, retriableTransactions, tapNONE);
         }
         for (auto const& item : localTx)
-            apply (accum, *item.second, tapNONE, getConfig(),
+            apply (accum, *item.second, tapNONE,
-                deprecatedLogs().journal("LedgerConsensus"));
+                getApp().getHashRouter().sigVerify(),
+                    getConfig(), deprecatedLogs().
+                        journal("LedgerConsensus"));
         accum.apply(*newOL);
         // We have a new Last Closed Ledger and new Open Ledger
         ledgerMaster_.pushLedger (newLCL, newOL);
@@ -1787,8 +1789,10 @@ applyTransaction (OpenView& view,
 
     try
     {
-        auto const result = apply(view, *txn, flags, getConfig(),
+        auto const result = apply(view, *txn, flags,
-            deprecatedLogs().journal("LedgerConsensus"));
+            getApp().getHashRouter().sigVerify(),
+                getConfig(), deprecatedLogs().
+                    journal("LedgerConsensus"));
         if (result.second)
         {
             WriteLog (lsDEBUG, LedgerConsensus)

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

@@ -379,7 +379,8 @@ public:
                     if (getApp().getHashRouter().addSuppressionFlags (it.first.getTXID (), SF_SIGGOOD))
                         flags = flags | tapNO_CHECK_SIGN;
                     auto const result = apply(view,
-                        *it.second, flags, getConfig(), j);
+                        *it.second, flags, getApp().getHashRouter(
+                            ).sigVerify(), getConfig(), j);
                     if (result.second)
                         any = true;
                 }
@@ -396,9 +397,9 @@ public:
                 if (getApp().getHashRouter ().addSuppressionFlags (it.first.getTXID (), SF_SIGGOOD))
                     tepFlags = static_cast<ApplyFlags> (tepFlags | tapNO_CHECK_SIGN);
 
-                auto const ret = apply(
+                auto const ret = apply(view, *it.second,
-                    view, *it.second, tepFlags, getConfig(),
+                    tepFlags, getApp().getHashRouter().sigVerify(),
-                        deprecatedLogs().journal("LedgerMaster"));
+                        getConfig(), deprecatedLogs().journal("LedgerMaster"));
 
                 if (ret.second)
                     ++recovers;

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

@@ -111,7 +111,8 @@ OpenLedger::accept(Rules const& rules,
     // Apply local tx
     for (auto const& item : locals)
         ripple::apply(*next, *item.second,
-            flags, config_, j_);
+            flags, router.sigVerify(),
+                config_, j_);
     // Switch to the new open view
     std::lock_guard<
         std::mutex> lock2(current_mutex_);
@@ -144,7 +145,8 @@ OpenLedger::apply_one (OpenView& view,
             SF_SIGGOOD)
         flags = flags | tapNO_CHECK_SIGN;
     auto const result = ripple::apply(
-        view, *tx, flags, config, j);
+        view, *tx, flags, router.
+            sigVerify(), config, j);
     if (result.second)
         return Result::success;
     if (isTefFailure (result.first) ||

src/ripple/app/misc/HashRouter.cpp

@@ -19,6 +19,7 @@
 
 #include <BeastConfig.h>
 #include <ripple/app/misc/IHashRouter.h>
+#include <ripple/protocol/STTx.h>
 #include <ripple/basics/CountedObject.h>
 #include <ripple/basics/UnorderedContainers.h>
 #include <ripple/basics/UptimeTimer.h>
@@ -95,15 +96,18 @@ public:
     {
     }
 
-    bool addSuppression (uint256 const& index);
+    bool addSuppression (uint256 const& index) override;
 
-    bool addSuppressionPeer (uint256 const& index, PeerShortID peer);
+    bool addSuppressionPeer (uint256 const& index, PeerShortID peer) override;
-    bool addSuppressionPeer (uint256 const& index, PeerShortID peer, int& flags);
+    bool addSuppressionPeer (uint256 const& index, PeerShortID peer, int& flags) override;
-    bool addSuppressionFlags (uint256 const& index, int flag);
+    bool addSuppressionFlags (uint256 const& index, int flag) override;
-    bool setFlag (uint256 const& index, int flag);
+    bool setFlag (uint256 const& index, int flag) override;
-    int getFlags (uint256 const& index);
+    int getFlags (uint256 const& index) override;
 
-    bool swapSet (uint256 const& index, std::set<PeerShortID>& peers, int flag);
+    bool swapSet (uint256 const& index, std::set<PeerShortID>& peers, int flag) override;
+
+    std::function<bool(STTx const&, std::function<bool(STTx const&)>)>
+    sigVerify() override;
 
 private:
     Entry getEntry (uint256 const& );
@@ -125,6 +129,28 @@ private:
 
 //------------------------------------------------------------------------------
 
+std::function<bool(STTx const&, std::function<bool(STTx const&)>)>
+HashRouter::sigVerify()
+{
+    return
+    [&] (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))
+        {
+            setFlag(id, SF_BAD);
+            return false;
+        }
+        setFlag(id, SF_SIGGOOD);
+        return true;
+    };
+}
+
 HashRouter::Entry& HashRouter::findCreateEntry (uint256 const& index, bool& created)
 {
     hash_map<uint256, Entry>::iterator fit = mSuppressionMap.find (index);

src/ripple/app/misc/IHashRouter.h

@@ -22,10 +22,13 @@
 
 #include <ripple/basics/base_uint.h>
 #include <cstdint>
+#include <functional>
 #include <set>
 
 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
@@ -80,6 +83,15 @@ public:
     virtual int getFlags (uint256 const& index) = 0;
 
     virtual bool swapSet (uint256 const& index, std::set<PeerShortID>& peers, int flag) = 0;
+
+    /**
+        Function wrapper that will check the signature status
+        of a STTx before calling an expensive signature
+        checking function.
+    */
+    virtual
+    std::function<bool(STTx const&, std::function<bool(STTx const&)>)>
+    sigVerify() = 0;
 };
 
 } // ripple

src/ripple/app/misc/NetworkOPs.cpp

@@ -697,7 +697,8 @@ void NetworkOPsImp::submitTransaction (Job&, STTx::pointer iTrans)
     m_job_queue.addJob (jtTRANSACTION, "submitTxn",
         std::bind (&NetworkOPsImp::processTransaction,
                    this,
-                   std::make_shared<Transaction> (trans, Validate::NO, reason),
+                   std::make_shared<Transaction> (trans, Validate::NO,
+                    directSigVerify, reason),
                    false,
                    false,
                    FailHard::no));
@@ -722,7 +723,7 @@ void NetworkOPsImp::processTransaction (Transaction::pointer& transaction,
         // signature not checked
         std::string reason;
 
-        if (! transaction->checkSign (reason))
+        if (! transaction->checkSign (reason, directSigVerify))
         {
             m_journal.info << "Transaction has bad signature: " << reason;
             transaction->setStatus (INVALID);
@@ -870,8 +871,9 @@ void NetworkOPsImp::apply (std::unique_lock<std::mutex>& batchLock)
                 std::tie (e.result, e.applied) =
                     ripple::apply (*accum,
                         *e.transaction->getSTransaction(), flags,
-                            getConfig(), deprecatedLogs().journal(
+                            getApp().getHashRouter().sigVerify(),
-                                "NetworkOPs"));
+                                getConfig(), deprecatedLogs().journal(
+                                    "NetworkOPs"));
                 applied |= e.applied;
 
             #if RIPPLE_OPEN_LEDGER
@@ -881,8 +883,9 @@ void NetworkOPsImp::apply (std::unique_lock<std::mutex>& batchLock)
                     [&](OpenView& view, beast::Journal j)
                     {
                         auto const result = ripple::apply(
-                            view, *e.transaction->getSTransaction(),
+                            view, *e.transaction->getSTransaction(), flags,
-                                flags, getConfig(), j);
+                                getApp().getHashRouter().sigVerify(),
+                                    getConfig(), j);
                         if (result.first != e.result)
                             mismatch(sle1,  e.result, sle2, result.first,
                                     e.transaction->getSTransaction(), j);

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

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

src/ripple/app/tx/Transaction.h

@@ -62,7 +62,7 @@ public:
     using ref = const pointer&;
 
 public:
-    Transaction (STTx::ref, Validate, std::string&) noexcept;
+    Transaction (STTx::ref, Validate, SigVerify, std::string&) noexcept;
 
     static
     Transaction::pointer
@@ -80,7 +80,7 @@ public:
     TransStatus
     sqlTransactionStatus(boost::optional<std::string> const& status);
 
-    bool checkSign (std::string&) const;
+    bool checkSign (std::string&, SigVerify) const;
 
     STTx::ref getSTransaction ()
     {

src/ripple/app/tx/apply.h

@@ -39,7 +39,7 @@ namespace ripple {
 */
 TER
 preflight (Rules const& rules, STTx const& tx,
-    ApplyFlags flags,
+    ApplyFlags flags, SigVerify verify,
         Config const& config, beast::Journal j);
 
 /** Apply a prechecked transaction to an OpenView.
@@ -86,8 +86,9 @@ doapply(OpenView& view, STTx const& tx,
 std::pair<TER, bool>
 apply (OpenView& view,
     STTx const& tx, ApplyFlags flags,
-        Config const& config,
+        SigVerify verify,
-            beast::Journal journal);
+            Config const& config,
+                beast::Journal journal);
 
 } // ripple
 

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

@@ -23,12 +23,14 @@
 #include <ripple/core/DatabaseCon.h>
 #include <ripple/app/ledger/LedgerMaster.h>
 #include <ripple/app/main/Application.h>
+#include <ripple/app/misc/IHashRouter.h>
 #include <ripple/protocol/JsonFields.h>
 #include <boost/optional.hpp>
 
 namespace ripple {
 
-Transaction::Transaction (STTx::ref stx, Validate validate, std::string& reason)
+Transaction::Transaction (STTx::ref stx, Validate validate,
+    SigVerify sigVerify, std::string& reason)
     noexcept
     : mTransaction (stx)
 {
@@ -50,7 +52,8 @@ Transaction::Transaction (STTx::ref stx, Validate validate, std::string& reason)
     }
 
     if (validate == Validate::NO ||
-        (passesLocalChecks (*mTransaction, reason) && checkSign (reason)))
+        (passesLocalChecks (*mTransaction, reason) &&
+            checkSign (reason, sigVerify)))
     {
         mStatus = NEW;
     }
@@ -65,7 +68,7 @@ Transaction::pointer Transaction::sharedTransaction (
         std::string reason;
 
         return std::make_shared<Transaction> (std::make_shared<STTx> (sit),
-            validate, reason);
+            validate, getApp().getHashRouter().sigVerify(), reason);
     }
     catch (std::exception& e)
     {
@@ -84,11 +87,14 @@ Transaction::pointer Transaction::sharedTransaction (
 // Misc.
 //
 
-bool Transaction::checkSign (std::string& reason) const
+bool Transaction::checkSign (std::string& reason, SigVerify sigVerify) const
 {
     if (! mFromPubKey.isValid ())
         reason = "Transaction has bad source public key";
-    else if (! mTransaction->checkSign ())
+    else if (!sigVerify(*mTransaction, [] (STTx const& tx)
+    {
+        return tx.checkSign();
+    }))
         reason = "Transaction has bad signature";
     else
         return true;
@@ -133,7 +139,8 @@ 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, reason);
+    auto tr = std::make_shared<Transaction> (txn, validate,
+        getApp().getHashRouter().sigVerify(), reason);
 
     tr->setStatus (sqlTransactionStatus (status));
     tr->setLedger (inLedger);

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

@@ -52,24 +52,22 @@ Transactor::preflight (PreflightContext const& ctx)
     auto const pk =
         RippleAddress::createAccountPublic(
             tx.getSigningPubKey());
-    if (!tx.isKnownGood ())
+
-    {
+    if(! ctx.verify(tx,
-        if (tx.isKnownBad () ||
+        [&, ctx] (STTx const& tx)
-            (! (ctx.flags & tapNO_CHECK_SIGN) && !tx.checkSign(
+        {
-                (
+            return (ctx.flags & tapNO_CHECK_SIGN) ||
+                tx.checkSign(
 #if RIPPLE_ENABLE_MULTI_SIGN
                     true
 #else
                     ctx.flags & tapENABLE_TESTING
 #endif
-                ))))
+                );
-        {
+        }))
-            tx.setBad();
+    {
-            j.debug << "apply: Invalid transaction (bad signature)";
+        JLOG(j.debug) << "apply: Invalid transaction (bad signature)";
-            return temINVALID;
+        return temINVALID;
-        }
-
-        tx.setGood();
     }
 
     return tesSUCCESS;

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

@@ -31,12 +31,12 @@ struct PreflightContext
 public:
     explicit PreflightContext(STTx const& tx_,
         Rules const& rules_, ApplyFlags flags_,
-            //SigVerify verify_,
+            SigVerify verify_,
                 beast::Journal j_ = {})
         : tx(tx_)
         , rules(rules_)
         , flags(flags_)
-        //, verify(verify_)
+        , verify(verify_)
         , j(j_)
     {
     }
@@ -44,7 +44,7 @@ public:
     STTx const& tx;
     Rules const& rules;
     ApplyFlags flags;
-    //SigVerify verify;
+    SigVerify verify;
     beast::Journal j;
 };
 

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

@@ -81,13 +81,13 @@ invoke_apply (ApplyContext& ctx)
 
 TER
 preflight (Rules const& rules, STTx const& tx,
-    ApplyFlags flags,
+    ApplyFlags flags, SigVerify verify,
         Config const& config, beast::Journal j)
 {
     try
     {
         PreflightContext pfctx(
-            tx, rules, flags, j);
+            tx, rules, flags, verify, j);
         return invoke_preflight(pfctx);
     }
     catch (std::exception const& e)
@@ -131,11 +131,11 @@ doapply(OpenView& view, STTx const& tx,
 
 std::pair<TER, bool>
 apply (OpenView& view, STTx const& tx,
-    ApplyFlags flags,
+    ApplyFlags flags, SigVerify verify,
         Config const& config, beast::Journal j)
 {
     auto pfresult = preflight(view.rules(),
-        tx, flags, config, j);
+        tx, flags, verify, config, j);
     if (pfresult != tesSUCCESS)
         return { pfresult, false };
     return doapply(view, tx, flags, config, j);

src/ripple/overlay/impl/PeerImp.cpp

@@ -1707,7 +1707,9 @@ PeerImp::checkTransaction (Job&, int flags,
 
         auto validate = (flags & SF_SIGGOOD) ? Validate::NO : Validate::YES;
         std::string reason;
-        auto tx = std::make_shared<Transaction> (stx, validate, reason);
+        auto tx = std::make_shared<Transaction> (stx, validate,
+            directSigVerify,
+            reason);
 
         if (tx->getStatus () == INVALID)
         {

src/ripple/protocol/STTx.h

@@ -129,23 +129,6 @@ public:
 #endif
             ) const;
 
-    bool isKnownGood () const
-    {
-        return (sig_state_ == true);
-    }
-    bool isKnownBad () const
-    {
-        return (sig_state_ == false);
-    }
-    void setGood () const
-    {
-        sig_state_ = true;
-    }
-    void setBad () const
-    {
-        sig_state_ = false;
-    }
-
     // SQL Functions with metadata.
     static
     std::string const&
@@ -165,12 +148,18 @@ private:
     bool checkMultiSign () const;
 
     TxType tx_type_;
-
-    mutable boost::tribool sig_state_;
 };
 
 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/protocol/impl/STTx.cpp

@@ -39,7 +39,6 @@ namespace ripple {
 STTx::STTx (TxType type)
     : STObject (sfTransaction)
     , tx_type_ (type)
-    , sig_state_ (boost::indeterminate)
 {
     auto format = TxFormats::getInstance().findByType (type);
 
@@ -56,7 +55,6 @@ STTx::STTx (TxType type)
 
 STTx::STTx (STObject&& object)
     : STObject (std::move (object))
-    , sig_state_ (boost::indeterminate)
 {
     tx_type_ = static_cast <TxType> (getFieldU16 (sfTransactionType));
 
@@ -79,7 +77,6 @@ STTx::STTx (STObject&& object)
 
 STTx::STTx (SerialIter& sit)
     : STObject (sfTransaction)
-    , sig_state_ (boost::indeterminate)
 {
     int length = sit.getBytesLeft ();
 
@@ -186,33 +183,27 @@ void STTx::sign (RippleAddress const& private_key)
 
 bool STTx::checkSign(bool allowMultiSign) const
 {
-    if (boost::indeterminate (sig_state_))
+    bool sigGood = false;
+    try
     {
-        try
+        if (allowMultiSign)
         {
-            if (allowMultiSign)
+            // Determine whether we're single- or multi-signing by looking
-            {
+            // at the SigningPubKey.  It it's empty we must be multi-signing.
-                // Determine whether we're single- or multi-signing by looking
+            // Otherwise we're single-signing.
-                // at the SigningPubKey.  It it's empty we must be multi-signing.
+            Blob const& signingPubKey = getFieldVL (sfSigningPubKey);
-                // Otherwise we're single-signing.
+            sigGood = signingPubKey.empty () ?
-                Blob const& signingPubKey = getFieldVL (sfSigningPubKey);
+                checkMultiSign () : checkSingleSign ();
-                sig_state_ = signingPubKey.empty () ?
-                    checkMultiSign () : checkSingleSign ();
-            }
-            else
-            {
-                sig_state_ = checkSingleSign ();
-            }
         }
-        catch (...)
+        else
         {
-            sig_state_ = false;
+            sigGood = checkSingleSign ();
         }
     }
-
+    catch (...)
-    assert (!boost::indeterminate (sig_state_));
+    {
-
+    }
-    return static_cast<bool> (sig_state_);
+    return sigGood;
 }
 
 void STTx::setSigningPubKey (RippleAddress const& naSignPubKey)

src/ripple/rpc/handlers/Submit.cpp

@@ -18,7 +18,9 @@
 //==============================================================================
 
 #include <BeastConfig.h>
+#include <ripple/app/main/Application.h>
 #include <ripple/app/misc/NetworkOPs.h>
+#include <ripple/app/misc/IHashRouter.h>
 #include <ripple/basics/StringUtilities.h>
 #include <ripple/basics/strHex.h>
 #include <ripple/net/RPCErr.h>
@@ -79,7 +81,8 @@ Json::Value doSubmit (RPC::Context& context)
 
     Transaction::pointer            tpTrans;
     std::string reason;
-    tpTrans = std::make_shared<Transaction> (stpTrans, Validate::YES, reason);
+    tpTrans = std::make_shared<Transaction> (stpTrans, Validate::YES,
+        getApp().getHashRouter().sigVerify(), reason);
     if (tpTrans->getStatus() != NEW)
     {
         jvResult[jss::error]            = "invalidTransaction";

src/ripple/rpc/impl/TransactionSign.cpp

@@ -721,7 +721,8 @@ transactionConstructImpl (STTx::pointer stpTrans)
     Transaction::pointer tpTrans;
     {
         std::string reason;
-        tpTrans = std::make_shared<Transaction>(stpTrans, Validate::NO, reason);
+        tpTrans = std::make_shared<Transaction>(stpTrans, Validate::NO,
+            directSigVerify, reason);
         if (tpTrans->getStatus () != NEW)
         {
             ret.first = RPC::make_error (rpcINTERNAL,

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

@@ -262,8 +262,9 @@ Env::submit (JTx const& jt)
             [&](OpenView& view, beast::Journal j)
             {
                 std::tie(ter, didApply) = ripple::apply(
-                    view, *stx, applyFlags(), config,
+                    view, *stx, applyFlags(), 
-                        beast::Journal{});
+                        directSigVerify, config,
+                            beast::Journal{});
                 return didApply;
             });
     }