From 2f5d721ec17815c21cbfb13aae142e322b5219bf Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Mon, 13 Jul 2015 17:01:40 -0400 Subject: [PATCH] Track STTx validity with HashRouter. (RIPD-977) --- .../app/ledger/impl/LedgerConsensusImp.cpp | 12 ++++-- src/ripple/app/ledger/impl/LedgerMaster.cpp | 9 +++-- src/ripple/app/ledger/impl/OpenLedger.cpp | 6 ++- src/ripple/app/misc/HashRouter.cpp | 40 +++++++++++++++---- src/ripple/app/misc/IHashRouter.h | 12 ++++++ src/ripple/app/misc/NetworkOPs.cpp | 15 ++++--- src/ripple/app/misc/impl/AccountTxPaging.cpp | 3 +- src/ripple/app/tx/Transaction.h | 4 +- src/ripple/app/tx/apply.h | 7 ++-- src/ripple/app/tx/impl/Transaction.cpp | 19 ++++++--- src/ripple/app/tx/impl/Transactor.cpp | 24 +++++------ src/ripple/app/tx/impl/Transactor.h | 6 +-- src/ripple/app/tx/impl/apply.cpp | 8 ++-- src/ripple/overlay/impl/PeerImp.cpp | 4 +- src/ripple/protocol/STTx.h | 27 ++++--------- src/ripple/protocol/impl/STTx.cpp | 39 +++++++----------- src/ripple/rpc/handlers/Submit.cpp | 5 ++- src/ripple/rpc/impl/TransactionSign.cpp | 3 +- src/ripple/test/jtx/impl/Env.cpp | 5 ++- 19 files changed, 145 insertions(+), 103 deletions(-) diff --git a/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp b/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp index a5636245df..b215def9b3 100644 --- a/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp +++ b/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp @@ -1150,8 +1150,10 @@ void LedgerConsensusImp::accept (std::shared_ptr set) newLCL, retriableTransactions, tapNONE); } for (auto const& item : localTx) - apply (accum, *item.second, tapNONE, getConfig(), - deprecatedLogs().journal("LedgerConsensus")); + apply (accum, *item.second, tapNONE, + 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(), - deprecatedLogs().journal("LedgerConsensus")); + auto const result = apply(view, *txn, flags, + getApp().getHashRouter().sigVerify(), + getConfig(), deprecatedLogs(). + journal("LedgerConsensus")); if (result.second) { WriteLog (lsDEBUG, LedgerConsensus) diff --git a/src/ripple/app/ledger/impl/LedgerMaster.cpp b/src/ripple/app/ledger/impl/LedgerMaster.cpp index 45e5e47708..d0f86b1e35 100644 --- a/src/ripple/app/ledger/impl/LedgerMaster.cpp +++ b/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 (tepFlags | tapNO_CHECK_SIGN); - auto const ret = apply( - view, *it.second, tepFlags, getConfig(), - deprecatedLogs().journal("LedgerMaster")); + auto const ret = apply(view, *it.second, + tepFlags, getApp().getHashRouter().sigVerify(), + getConfig(), deprecatedLogs().journal("LedgerMaster")); if (ret.second) ++recovers; diff --git a/src/ripple/app/ledger/impl/OpenLedger.cpp b/src/ripple/app/ledger/impl/OpenLedger.cpp index 7f467cca55..6429b947f9 100644 --- a/src/ripple/app/ledger/impl/OpenLedger.cpp +++ b/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) || diff --git a/src/ripple/app/misc/HashRouter.cpp b/src/ripple/app/misc/HashRouter.cpp index 47ca8e0a31..1198871228 100644 --- a/src/ripple/app/misc/HashRouter.cpp +++ b/src/ripple/app/misc/HashRouter.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -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, int& flags); - bool addSuppressionFlags (uint256 const& index, int flag); - bool setFlag (uint256 const& index, int flag); - int getFlags (uint256 const& index); + bool addSuppressionPeer (uint256 const& index, PeerShortID peer) override; + bool addSuppressionPeer (uint256 const& index, PeerShortID peer, int& flags) override; + bool addSuppressionFlags (uint256 const& index, int flag) override; + bool setFlag (uint256 const& index, int flag) override; + int getFlags (uint256 const& index) override; - bool swapSet (uint256 const& index, std::set& peers, int flag); + bool swapSet (uint256 const& index, std::set& peers, int flag) override; + + std::function)> + sigVerify() override; private: Entry getEntry (uint256 const& ); @@ -125,6 +129,28 @@ private: //------------------------------------------------------------------------------ +std::function)> +HashRouter::sigVerify() +{ + 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)) + { + setFlag(id, SF_BAD); + return false; + } + setFlag(id, SF_SIGGOOD); + return true; + }; +} + HashRouter::Entry& HashRouter::findCreateEntry (uint256 const& index, bool& created) { hash_map::iterator fit = mSuppressionMap.find (index); diff --git a/src/ripple/app/misc/IHashRouter.h b/src/ripple/app/misc/IHashRouter.h index 074d3a20a2..7f056e69c9 100644 --- a/src/ripple/app/misc/IHashRouter.h +++ b/src/ripple/app/misc/IHashRouter.h @@ -22,10 +22,13 @@ #include #include +#include #include 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& 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)> + sigVerify() = 0; }; } // ripple diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 84cd775959..edb7d3040d 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/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 (trans, Validate::NO, reason), + std::make_shared (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& batchLock) std::tie (e.result, e.applied) = ripple::apply (*accum, *e.transaction->getSTransaction(), flags, - getConfig(), deprecatedLogs().journal( - "NetworkOPs")); + getApp().getHashRouter().sigVerify(), + getConfig(), deprecatedLogs().journal( + "NetworkOPs")); applied |= e.applied; #if RIPPLE_OPEN_LEDGER @@ -881,8 +883,9 @@ void NetworkOPsImp::apply (std::unique_lock& batchLock) [&](OpenView& view, beast::Journal j) { auto const result = ripple::apply( - view, *e.transaction->getSTransaction(), - flags, getConfig(), j); + view, *e.transaction->getSTransaction(), flags, + getApp().getHashRouter().sigVerify(), + getConfig(), j); if (result.first != e.result) mismatch(sle1, e.result, sle2, result.first, e.transaction->getSTransaction(), j); diff --git a/src/ripple/app/misc/impl/AccountTxPaging.cpp b/src/ripple/app/misc/impl/AccountTxPaging.cpp index 64e9986433..570358e64e 100644 --- a/src/ripple/app/misc/impl/AccountTxPaging.cpp +++ b/src/ripple/app/misc/impl/AccountTxPaging.cpp @@ -42,7 +42,8 @@ convertBlobsToTxResult ( STTx::pointer txn = std::make_shared (it); std::string reason; - auto tr = std::make_shared (txn, Validate::NO, reason); + auto tr = std::make_shared (txn, Validate::NO, + directSigVerify, reason); tr->setStatus (Transaction::sqlTransactionStatus(status)); tr->setLedger (ledger_index); diff --git a/src/ripple/app/tx/Transaction.h b/src/ripple/app/tx/Transaction.h index 1f65982e3c..e40e8ab922 100644 --- a/src/ripple/app/tx/Transaction.h +++ b/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 const& status); - bool checkSign (std::string&) const; + bool checkSign (std::string&, SigVerify) const; STTx::ref getSTransaction () { diff --git a/src/ripple/app/tx/apply.h b/src/ripple/app/tx/apply.h index a85935f17b..86f6c7a9bd 100644 --- a/src/ripple/app/tx/apply.h +++ b/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 apply (OpenView& view, STTx const& tx, ApplyFlags flags, - Config const& config, - beast::Journal journal); + SigVerify verify, + Config const& config, + beast::Journal journal); } // ripple diff --git a/src/ripple/app/tx/impl/Transaction.cpp b/src/ripple/app/tx/impl/Transaction.cpp index 60fc380674..9e6a7d7db2 100644 --- a/src/ripple/app/tx/impl/Transaction.cpp +++ b/src/ripple/app/tx/impl/Transaction.cpp @@ -23,12 +23,14 @@ #include #include #include +#include #include #include 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 (std::make_shared (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 (it); std::string reason; - auto tr = std::make_shared (txn, validate, reason); + auto tr = std::make_shared (txn, validate, + getApp().getHashRouter().sigVerify(), reason); tr->setStatus (sqlTransactionStatus (status)); tr->setLedger (inLedger); diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 8d31963172..f5b4558424 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/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 (tx.isKnownBad () || - (! (ctx.flags & tapNO_CHECK_SIGN) && !tx.checkSign( - ( + + if(! ctx.verify(tx, + [&, ctx] (STTx const& tx) + { + 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)"; - return temINVALID; - } - - tx.setGood(); + ); + })) + { + JLOG(j.debug) << "apply: Invalid transaction (bad signature)"; + return temINVALID; } return tesSUCCESS; diff --git a/src/ripple/app/tx/impl/Transactor.h b/src/ripple/app/tx/impl/Transactor.h index 08bb1b16bd..e1b6599a66 100644 --- a/src/ripple/app/tx/impl/Transactor.h +++ b/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; }; diff --git a/src/ripple/app/tx/impl/apply.cpp b/src/ripple/app/tx/impl/apply.cpp index eb44c1d9b3..1b8d49dd9c 100644 --- a/src/ripple/app/tx/impl/apply.cpp +++ b/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 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); diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index 3d635d207b..67e7119ca9 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/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 (stx, validate, reason); + auto tx = std::make_shared (stx, validate, + directSigVerify, + reason); if (tx->getStatus () == INVALID) { diff --git a/src/ripple/protocol/STTx.h b/src/ripple/protocol/STTx.h index 3d7e031e4f..f8356578b9 100644 --- a/src/ripple/protocol/STTx.h +++ b/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) > ; + +inline +bool directSigVerify(STTx const& tx, std::function sigCheck) +{ + return sigCheck(tx); +} + } // ripple #endif diff --git a/src/ripple/protocol/impl/STTx.cpp b/src/ripple/protocol/impl/STTx.cpp index b9159bb9f1..e9fd7ca9cf 100644 --- a/src/ripple/protocol/impl/STTx.cpp +++ b/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 (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. - // Otherwise we're single-signing. - Blob const& signingPubKey = getFieldVL (sfSigningPubKey); - sig_state_ = signingPubKey.empty () ? - checkMultiSign () : checkSingleSign (); - } - else - { - sig_state_ = checkSingleSign (); - } + // Determine whether we're single- or multi-signing by looking + // at the SigningPubKey. It it's empty we must be multi-signing. + // Otherwise we're single-signing. + Blob const& signingPubKey = getFieldVL (sfSigningPubKey); + sigGood = signingPubKey.empty () ? + checkMultiSign () : checkSingleSign (); } - catch (...) + else { - sig_state_ = false; + sigGood = checkSingleSign (); } } - - assert (!boost::indeterminate (sig_state_)); - - return static_cast (sig_state_); + catch (...) + { + } + return sigGood; } void STTx::setSigningPubKey (RippleAddress const& naSignPubKey) diff --git a/src/ripple/rpc/handlers/Submit.cpp b/src/ripple/rpc/handlers/Submit.cpp index 22186a12e3..26f79ab8f5 100644 --- a/src/ripple/rpc/handlers/Submit.cpp +++ b/src/ripple/rpc/handlers/Submit.cpp @@ -18,7 +18,9 @@ //============================================================================== #include +#include #include +#include #include #include #include @@ -79,7 +81,8 @@ Json::Value doSubmit (RPC::Context& context) Transaction::pointer tpTrans; std::string reason; - tpTrans = std::make_shared (stpTrans, Validate::YES, reason); + tpTrans = std::make_shared (stpTrans, Validate::YES, + getApp().getHashRouter().sigVerify(), reason); if (tpTrans->getStatus() != NEW) { jvResult[jss::error] = "invalidTransaction"; diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 2efb872eb2..bc3b8c30b3 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -721,7 +721,8 @@ transactionConstructImpl (STTx::pointer stpTrans) Transaction::pointer tpTrans; { std::string reason; - tpTrans = std::make_shared(stpTrans, Validate::NO, reason); + tpTrans = std::make_shared(stpTrans, Validate::NO, + directSigVerify, reason); if (tpTrans->getStatus () != NEW) { ret.first = RPC::make_error (rpcINTERNAL, diff --git a/src/ripple/test/jtx/impl/Env.cpp b/src/ripple/test/jtx/impl/Env.cpp index b63f079ff1..8e72f3faa2 100644 --- a/src/ripple/test/jtx/impl/Env.cpp +++ b/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, - beast::Journal{}); + view, *stx, applyFlags(), + directSigVerify, config, + beast::Journal{}); return didApply; }); }