From 8296d81edfef2fed8d7efffdf67caaab88e3ee80 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Wed, 14 Oct 2015 15:18:37 -0700 Subject: [PATCH] Cache tid in STTx: The digest for a transaction (its transaction ID, or tid) is computed once upon constructed when the STTx is deserialized. Subsequent calls to retrieve the digest use the cached value. Any code which modifies the STTx and then attempts to retrieve the digest will terminate the process with a logic error contract violation. * Nested types removed * All STTx are contained as const (Except in transaction sign, which must modify) * tid in STTx is computed once on deserialization --- src/ripple/app/ledger/AcceptedLedgerTx.cpp | 2 +- src/ripple/app/ledger/AcceptedLedgerTx.h | 2 +- src/ripple/app/ledger/ConsensusTransSetSF.cpp | 2 +- .../app/ledger/impl/LedgerConsensusImp.cpp | 2 +- src/ripple/app/misc/NetworkOPs.cpp | 14 +++++-------- src/ripple/app/misc/NetworkOPs.h | 4 ++-- src/ripple/app/misc/impl/AccountTxPaging.cpp | 2 +- src/ripple/app/tx/LocalTxs.h | 2 +- src/ripple/app/tx/Transaction.h | 6 +++--- src/ripple/app/tx/TransactionMaster.h | 16 ++++++++++---- src/ripple/app/tx/impl/LocalTxs.cpp | 8 +++---- src/ripple/app/tx/impl/Transaction.cpp | 6 +++--- src/ripple/app/tx/impl/TransactionMaster.cpp | 9 ++++---- src/ripple/overlay/impl/PeerImp.cpp | 4 ++-- src/ripple/overlay/impl/PeerImp.h | 3 ++- src/ripple/protocol/STTx.h | 15 ++++++++++--- src/ripple/protocol/impl/STTx.cpp | 21 ++++++++++++++++++- src/ripple/rpc/handlers/Submit.cpp | 4 ++-- src/ripple/rpc/impl/TransactionSign.cpp | 18 ++++++++-------- src/ripple/test/jtx/impl/Env.cpp | 10 ++++----- 20 files changed, 91 insertions(+), 59 deletions(-) diff --git a/src/ripple/app/ledger/AcceptedLedgerTx.cpp b/src/ripple/app/ledger/AcceptedLedgerTx.cpp index 61dfc3063..4a8a7a482 100644 --- a/src/ripple/app/ledger/AcceptedLedgerTx.cpp +++ b/src/ripple/app/ledger/AcceptedLedgerTx.cpp @@ -54,7 +54,7 @@ AcceptedLedgerTx::AcceptedLedgerTx ( AcceptedLedgerTx::AcceptedLedgerTx ( std::shared_ptr const& ledger, - STTx::ref txn, + std::shared_ptr const& txn, TER result, AccountIDCache const& accountCache, Logs& logs) diff --git a/src/ripple/app/ledger/AcceptedLedgerTx.h b/src/ripple/app/ledger/AcceptedLedgerTx.h index e4552b791..b3e4c2f9a 100644 --- a/src/ripple/app/ledger/AcceptedLedgerTx.h +++ b/src/ripple/app/ledger/AcceptedLedgerTx.h @@ -62,7 +62,7 @@ public: Logs&); AcceptedLedgerTx ( std::shared_ptr const&, - STTx::ref, + std::shared_ptr const&, TER, AccountIDCache const&, Logs&); diff --git a/src/ripple/app/ledger/ConsensusTransSetSF.cpp b/src/ripple/app/ledger/ConsensusTransSetSF.cpp index 87c8f9fd6..8382fce8f 100644 --- a/src/ripple/app/ledger/ConsensusTransSetSF.cpp +++ b/src/ripple/app/ledger/ConsensusTransSetSF.cpp @@ -57,7 +57,7 @@ void ConsensusTransSetSF::gotNode ( // skip prefix Serializer s (nodeData.data() + 4, nodeData.size() - 4); SerialIter sit (s.slice()); - STTx::pointer stx = std::make_shared (std::ref (sit)); + auto stx = std::make_shared (std::ref (sit)); assert (stx->getTransactionID () == nodeHash); auto const pap = &app_; app_.getJobQueue ().addJob ( diff --git a/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp b/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp index 8f156b968..65ed57e10 100644 --- a/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp +++ b/src/ripple/app/ledger/impl/LedgerConsensusImp.cpp @@ -1164,7 +1164,7 @@ void LedgerConsensusImp::accept (std::shared_ptr set) << " not get in"; SerialIter sit (it.second->peekTransaction().slice()); - auto txn = std::make_shared(sit); + auto txn = std::make_shared(sit); retriableTxs.insert (txn); diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 482799efc..706297a00 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -155,7 +155,7 @@ public: // // Must complete immediately. - void submitTransaction (STTx::pointer) override; + void submitTransaction (std::shared_ptr const&) override; void processTransaction ( Transaction::pointer& transaction, @@ -339,7 +339,7 @@ public: void pubLedger (Ledger::ref lpAccepted) override; void pubProposedTransaction ( std::shared_ptr const& lpCurrent, - STTx::ref stTxn, TER terResult) override; + std::shared_ptr const& stTxn, TER terResult) override; //-------------------------------------------------------------------------- // @@ -639,7 +639,7 @@ std::string NetworkOPsImp::strOperatingMode () const return paStatusToken[mMode]; } -void NetworkOPsImp::submitTransaction (STTx::pointer iTrans) +void NetworkOPsImp::submitTransaction (std::shared_ptr const& iTrans) { if (isNeedNetworkLedger ()) { @@ -648,11 +648,7 @@ void NetworkOPsImp::submitTransaction (STTx::pointer iTrans) } // this is an asynchronous interface - Serializer s; - iTrans->add (s); - - SerialIter sit (s.slice()); - auto trans = std::make_shared (std::ref (sit)); + auto const trans = sterilize(*iTrans); auto const txid = trans->getTransactionID (); auto const flags = app_.getHashRouter().getFlags(txid); @@ -2005,7 +2001,7 @@ Json::Value NetworkOPsImp::getLedgerFetchInfo () void NetworkOPsImp::pubProposedTransaction ( std::shared_ptr const& lpCurrent, - STTx::ref stTxn, TER terResult) + std::shared_ptr const& stTxn, TER terResult) { Json::Value jvObj = transJson (*stTxn, terResult, false, lpCurrent); diff --git a/src/ripple/app/misc/NetworkOPs.h b/src/ripple/app/misc/NetworkOPs.h index 27a422cb8..ec141bde8 100644 --- a/src/ripple/app/misc/NetworkOPs.h +++ b/src/ripple/app/misc/NetworkOPs.h @@ -110,7 +110,7 @@ public: // // must complete immediately - virtual void submitTransaction (STTx::pointer) = 0; + virtual void submitTransaction (std::shared_ptr const&) = 0; /** * Process transactions as they arrive from the network or which are @@ -229,7 +229,7 @@ public: virtual void pubLedger (Ledger::ref lpAccepted) = 0; virtual void pubProposedTransaction ( std::shared_ptr const& lpCurrent, - STTx::ref stTxn, TER terResult) = 0; + std::shared_ptr const& stTxn, TER terResult) = 0; }; //------------------------------------------------------------------------------ diff --git a/src/ripple/app/misc/impl/AccountTxPaging.cpp b/src/ripple/app/misc/impl/AccountTxPaging.cpp index ad0d276f8..82fadaff4 100644 --- a/src/ripple/app/misc/impl/AccountTxPaging.cpp +++ b/src/ripple/app/misc/impl/AccountTxPaging.cpp @@ -40,7 +40,7 @@ convertBlobsToTxResult ( Application& app) { SerialIter it (makeSlice(rawTxn)); - STTx::pointer txn = std::make_shared (it); + auto txn = std::make_shared (it); std::string reason; auto tr = std::make_shared (txn, reason, app); diff --git a/src/ripple/app/tx/LocalTxs.h b/src/ripple/app/tx/LocalTxs.h index b8366b700..4985f8467 100644 --- a/src/ripple/app/tx/LocalTxs.h +++ b/src/ripple/app/tx/LocalTxs.h @@ -36,7 +36,7 @@ public: virtual ~LocalTxs () = default; // Add a new local transaction - virtual void push_back (LedgerIndex index, STTx::ref txn) = 0; + virtual void push_back (LedgerIndex index, std::shared_ptr const& txn) = 0; // Return the set of local transactions to a new open ledger virtual CanonicalTXSet getTxSet () = 0; diff --git a/src/ripple/app/tx/Transaction.h b/src/ripple/app/tx/Transaction.h index 6bba01785..f7ee52804 100644 --- a/src/ripple/app/tx/Transaction.h +++ b/src/ripple/app/tx/Transaction.h @@ -64,7 +64,7 @@ public: public: Transaction ( - STTx::ref, std::string&, Application&) noexcept; + std::shared_ptr const&, std::string&, Application&) noexcept; static Transaction::pointer @@ -90,7 +90,7 @@ public: TransStatus sqlTransactionStatus(boost::optional const& status); - STTx::ref getSTransaction () + std::shared_ptr const& getSTransaction () { return mTransaction; } @@ -170,7 +170,7 @@ private: TER mResult = temUNCERTAIN; bool mApplying = false; - STTx::pointer mTransaction; + std::shared_ptr mTransaction; Application& mApp; beast::Journal j_; }; diff --git a/src/ripple/app/tx/TransactionMaster.h b/src/ripple/app/tx/TransactionMaster.h index 8ddb567b8..b257e4e2b 100644 --- a/src/ripple/app/tx/TransactionMaster.h +++ b/src/ripple/app/tx/TransactionMaster.h @@ -37,15 +37,23 @@ public: TransactionMaster (TransactionMaster const&) = delete; TransactionMaster& operator= (TransactionMaster const&) = delete; - Transaction::pointer fetch (uint256 const& , bool checkDisk); - STTx::pointer fetch (std::shared_ptr const& item, SHAMapTreeNode:: TNType type, - bool checkDisk, std::uint32_t uCommitLedger); + Transaction::pointer + fetch (uint256 const& , bool checkDisk); + + std::shared_ptr + fetch (std::shared_ptr const& item, + SHAMapTreeNode:: TNType type, + bool checkDisk, std::uint32_t uCommitLedger); // return value: true = we had the transaction already bool inLedger (uint256 const& hash, std::uint32_t ledger); + void canonicalize (Transaction::pointer* pTransaction); + void sweep (void); - TaggedCache & getCache(); + + TaggedCache & + getCache(); private: Application& mApp; diff --git a/src/ripple/app/tx/impl/LocalTxs.cpp b/src/ripple/app/tx/impl/LocalTxs.cpp index 204434584..a3f1bd343 100644 --- a/src/ripple/app/tx/impl/LocalTxs.cpp +++ b/src/ripple/app/tx/impl/LocalTxs.cpp @@ -59,7 +59,7 @@ public: // get into a fully-validated ledger. static int const holdLedgers = 5; - LocalTx (LedgerIndex index, STTx::ref txn) + LocalTx (LedgerIndex index, std::shared_ptr const& txn) : m_txn (txn) , m_expire (index + holdLedgers) , m_id (txn->getTransactionID ()) @@ -85,7 +85,7 @@ public: return i > m_expire; } - STTx::ref getTX () const + std::shared_ptr const& getTX () const { return m_txn; } @@ -97,7 +97,7 @@ public: private: - STTx::pointer m_txn; + std::shared_ptr m_txn; LedgerIndex m_expire; uint256 m_id; AccountID m_account; @@ -113,7 +113,7 @@ public: LocalTxsImp() = default; // Add a new transaction to the set of local transactions - void push_back (LedgerIndex index, STTx::ref txn) override + void push_back (LedgerIndex index, std::shared_ptr const& txn) override { std::lock_guard lock (m_lock); diff --git a/src/ripple/app/tx/impl/Transaction.cpp b/src/ripple/app/tx/impl/Transaction.cpp index 83c79338d..2d61cad9f 100644 --- a/src/ripple/app/tx/impl/Transaction.cpp +++ b/src/ripple/app/tx/impl/Transaction.cpp @@ -31,7 +31,7 @@ namespace ripple { -Transaction::Transaction (STTx::ref stx, +Transaction::Transaction (std::shared_ptr const& stx, std::string& reason, Application& app) noexcept : mTransaction (stx) @@ -62,7 +62,7 @@ Transaction::pointer Transaction::sharedTransaction ( try { SerialIter sit (makeSlice(vucTransaction)); - auto txn = std::make_shared(sit); + auto txn = std::make_shared(sit); std::string reason; auto result = std::make_shared ( txn, reason, app); @@ -125,7 +125,7 @@ Transaction::pointer Transaction::transactionFromSQL ( rangeCheckedCast(ledgerSeq.value_or (0)); SerialIter it (makeSlice(rawTxn)); - auto txn = std::make_shared (it); + auto txn = std::make_shared (it); std::string reason; auto tr = std::make_shared ( txn, reason, app); diff --git a/src/ripple/app/tx/impl/TransactionMaster.cpp b/src/ripple/app/tx/impl/TransactionMaster.cpp index 98bd047e0..88b1c9dda 100644 --- a/src/ripple/app/tx/impl/TransactionMaster.cpp +++ b/src/ripple/app/tx/impl/TransactionMaster.cpp @@ -60,11 +60,12 @@ Transaction::pointer TransactionMaster::fetch (uint256 const& txnID, bool checkD return txn; } -STTx::pointer TransactionMaster::fetch (std::shared_ptr const& item, +std::shared_ptr +TransactionMaster::fetch (std::shared_ptr const& item, SHAMapTreeNode::TNType type, bool checkDisk, std::uint32_t uCommitLedger) { - STTx::pointer txn; + std::shared_ptr txn; auto iTx = fetch (item->key(), false); if (!iTx) @@ -73,12 +74,12 @@ STTx::pointer TransactionMaster::fetch (std::shared_ptr const& item, if (type == SHAMapTreeNode::tnTRANSACTION_NM) { SerialIter sit (item->slice()); - txn = std::make_shared (std::ref (sit)); + txn = std::make_shared (std::ref (sit)); } else if (type == SHAMapTreeNode::tnTRANSACTION_MD) { auto blob = SerialIter{item->data(), item->size()}.getVL(); - txn = std::make_shared(SerialIter{blob.data(), blob.size()}); + txn = std::make_shared(SerialIter{blob.data(), blob.size()}); } } else diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index bf341a890..3e8849ee9 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/src/ripple/overlay/impl/PeerImp.cpp @@ -1031,7 +1031,7 @@ PeerImp::onMessage (std::shared_ptr const& m) try { - auto stx = std::make_shared(sit); + auto stx = std::make_shared(sit); uint256 txID = stx->getTransactionID (); int flags; @@ -1716,7 +1716,7 @@ PeerImp::doFetchPack (const std::shared_ptr& packet void PeerImp::checkTransaction (int flags, - bool checkSignature, STTx::pointer stx) + bool checkSignature, std::shared_ptr const& stx) { // VFALCO TODO Rewrite to not use exceptions try diff --git a/src/ripple/overlay/impl/PeerImp.h b/src/ripple/overlay/impl/PeerImp.h index 884868e65..a4d59c89b 100644 --- a/src/ripple/overlay/impl/PeerImp.h +++ b/src/ripple/overlay/impl/PeerImp.h @@ -449,7 +449,8 @@ private: doFetchPack (const std::shared_ptr& packet); void - checkTransaction (int flags, bool checkSignature, STTx::pointer stx); + checkTransaction (int flags, bool checkSignature, + std::shared_ptr const& stx); void checkPropose (Job& job, diff --git a/src/ripple/protocol/STTx.h b/src/ripple/protocol/STTx.h index c8360c62b..d2fe8b85e 100644 --- a/src/ripple/protocol/STTx.h +++ b/src/ripple/protocol/STTx.h @@ -24,6 +24,7 @@ #include #include #include +#include namespace ripple { @@ -42,9 +43,6 @@ class STTx final public: static char const* getCountedObjectName () { return "STTx"; } - using pointer = std::shared_ptr; - using ref = const std::shared_ptr&; - static std::size_t const minMultiSigners = 1; static std::size_t const maxMultiSigners = 8; @@ -134,11 +132,22 @@ private: bool checkSingleSign () const; bool checkMultiSign () const; + boost::optional tid_; TxType tx_type_; }; bool passesLocalChecks (STObject const& st, std::string&); +/** Sterilize a transaction. + + The transaction is serialized and then deserialized, + ensuring that all equivalent transactions are in canonical + form. This also ensures that program metadata such as + the transaction's digest, are all computed. +*/ +std::shared_ptr +sterilize (STTx const& stx); + } // ripple #endif diff --git a/src/ripple/protocol/impl/STTx.cpp b/src/ripple/protocol/impl/STTx.cpp index 42835f755..f2e0a1c1c 100644 --- a/src/ripple/protocol/impl/STTx.cpp +++ b/src/ripple/protocol/impl/STTx.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -34,6 +35,7 @@ #include // #include #include +#include namespace ripple { @@ -74,6 +76,8 @@ STTx::STTx (STObject&& object) "Transaction not legal for format"; throw std::runtime_error ("transaction not valid"); } + + tid_ = getHash(HashPrefix::transactionID); } STTx::STTx (SerialIter& sit) @@ -106,6 +110,8 @@ STTx::STTx (SerialIter& sit) "Transaction not legal for format"; throw std::runtime_error ("transaction not valid"); } + + tid_ = getHash(HashPrefix::transactionID); } std::string @@ -161,7 +167,10 @@ STTx::getSigningHash () const uint256 STTx::getTransactionID () const { - return getHash (HashPrefix::transactionID); + assert(tid_); + if (! tid_) + LogicError("digest is undefined"); + return *tid_; } Blob STTx::getSignature () const @@ -180,6 +189,7 @@ void STTx::sign (RippleAddress const& private_key) { Blob const signature = private_key.accountPrivateSign (getSigningData (*this)); setFieldVL (sfTxnSignature, signature); + tid_ = getHash(HashPrefix::transactionID); } bool STTx::checkSign(bool allowMultiSign) const @@ -499,4 +509,13 @@ bool passesLocalChecks (STObject const& st, std::string& reason) return true; } +std::shared_ptr +sterilize (STTx const& stx) +{ + Serializer s; + stx.add(s); + SerialIter sit(s.slice()); + return std::make_shared(std::ref(sit)); +} + } // ripple diff --git a/src/ripple/rpc/handlers/Submit.cpp b/src/ripple/rpc/handlers/Submit.cpp index c39ebe6c5..18962cbc4 100644 --- a/src/ripple/rpc/handlers/Submit.cpp +++ b/src/ripple/rpc/handlers/Submit.cpp @@ -67,11 +67,11 @@ Json::Value doSubmit (RPC::Context& context) SerialIter sitTrans (makeSlice(ret.first)); - STTx::pointer stpTrans; + std::shared_ptr stpTrans; try { - stpTrans = std::make_shared (std::ref (sitTrans)); + stpTrans = std::make_shared (std::ref (sitTrans)); } catch (std::exception& e) { diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 4e33d8628..8d07065fe 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -295,11 +295,11 @@ checkTxJsonFields ( //------------------------------------------------------------------------------ // A move-only struct that makes it easy to return either a Json::Value or a -// STTx::pointer from transactionPreProcessImpl (). +// std::shared_ptr from transactionPreProcessImpl (). struct transactionPreProcessResult { Json::Value const first; - STTx::pointer const second; + std::shared_ptr const second; transactionPreProcessResult () = delete; transactionPreProcessResult (transactionPreProcessResult const&) = delete; @@ -318,7 +318,7 @@ struct transactionPreProcessResult , second () { } - transactionPreProcessResult (STTx::pointer&& st) + transactionPreProcessResult (std::shared_ptr&& st) : first () , second (std::move (st)) { } @@ -463,7 +463,7 @@ transactionPreProcessImpl ( return std::move (err); } - STTx::pointer stpTrans; + std::shared_ptr stpTrans; try { // If we're generating a multi-signature the SigningPubKey must be @@ -501,12 +501,12 @@ transactionPreProcessImpl ( stpTrans->sign (keypair.secretKey); } - return std::move (stpTrans); + return transactionPreProcessResult {std::move (stpTrans)}; } static std::pair -transactionConstructImpl (STTx::pointer stpTrans, +transactionConstructImpl (std::shared_ptr const& stpTrans, Rules const& rules, Application& app) { std::pair ret; @@ -931,7 +931,7 @@ Json::Value transactionSubmitMultiSigned ( } // Grind through the JSON in tx_json to produce a STTx - STTx::pointer stpTrans; + std::shared_ptr stpTrans; { STParsedJSONObject parsedTx_json ("tx_json", tx_json); if (!parsedTx_json.object) @@ -944,8 +944,8 @@ Json::Value transactionSubmitMultiSigned ( } try { - stpTrans = - std::make_shared (std::move(parsedTx_json.object.get())); + stpTrans = std::make_shared( + std::move(parsedTx_json.object.get())); } catch (std::exception& ex) { diff --git a/src/ripple/test/jtx/impl/Env.cpp b/src/ripple/test/jtx/impl/Env.cpp index c4be36dea..284c60fbd 100644 --- a/src/ripple/test/jtx/impl/Env.cpp +++ b/src/ripple/test/jtx/impl/Env.cpp @@ -270,15 +270,14 @@ void Env::submit (JTx const& jt) { bool didApply; - auto const& stx = jt.stx; - if (stx) + if (jt.stx) { - txid_ = stx->getTransactionID(); + txid_ = jt.stx->getTransactionID(); openLedger.modify( [&](OpenView& view, beast::Journal j) { std::tie(ter_, didApply) = ripple::apply( - app(), view, *stx, applyFlags(), + app(), view, *jt.stx, applyFlags(), beast::Journal{}); return didApply; }); @@ -385,8 +384,7 @@ Env::st (JTx const& jt) try { - return std::make_shared( - std::move(*obj)); + return sterilize(STTx{std::move(*obj)}); } catch(...) {