From 96dedf553e25428b1f00dfa4a344e782910ed19a Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Sun, 5 Oct 2014 12:53:39 -0700 Subject: [PATCH] Refactor SerializedTransaction: * Use boost:tribool instead of two intertwined bool variables * Trim down public interface, reduce member variables --- src/ripple/app/misc/SerializedTransaction.cpp | 224 ++++++++---------- src/ripple/app/misc/SerializedTransaction.h | 66 +++--- src/ripple/app/paths/cursor/AdvanceNode.cpp | 2 +- src/ripple/app/transactors/Transactor.cpp | 4 +- src/ripple/app/tx/LocalTxs.cpp | 5 +- src/ripple/app/tx/Transaction.cpp | 10 +- src/ripple/app/tx/TransactionEngine.cpp | 2 +- src/ripple/overlay/impl/PeerImp.cpp | 2 +- 8 files changed, 130 insertions(+), 185 deletions(-) diff --git a/src/ripple/app/misc/SerializedTransaction.cpp b/src/ripple/app/misc/SerializedTransaction.cpp index 6e8419c6d2..6e026fd669 100644 --- a/src/ripple/app/misc/SerializedTransaction.cpp +++ b/src/ripple/app/misc/SerializedTransaction.cpp @@ -25,73 +25,80 @@ namespace ripple { SerializedTransaction::SerializedTransaction (TxType type) : STObject (sfTransaction) - , mType (type) - , mSigGood (false) - , mSigBad (false) + , tx_type_ (type) + , sig_state_ (boost::indeterminate) { - mFormat = TxFormats::getInstance().findByType (type); + auto format = TxFormats::getInstance().findByType (type); - if (mFormat == nullptr) + if (format == nullptr) { - WriteLog (lsWARNING, SerializedTransaction) << "Transaction type: " << type; + WriteLog (lsWARNING, SerializedTransaction) << + "Transaction type: " << type; throw std::runtime_error ("invalid transaction type"); } - set (mFormat->elements); - setFieldU16 (sfTransactionType, mFormat->getType ()); + set (format->elements); + setFieldU16 (sfTransactionType, format->getType ()); } SerializedTransaction::SerializedTransaction (STObject const& object) : STObject (object) - , mSigGood (false) - , mSigBad (false) + , sig_state_ (boost::indeterminate) { - mType = static_cast (getFieldU16 (sfTransactionType)); + tx_type_ = static_cast (getFieldU16 (sfTransactionType)); - mFormat = TxFormats::getInstance().findByType (mType); + auto format = TxFormats::getInstance().findByType (tx_type_); - if (!mFormat) + if (!format) { - WriteLog (lsWARNING, SerializedTransaction) << "Transaction type: " << mType; + WriteLog (lsWARNING, SerializedTransaction) << + "Transaction type: " << tx_type_; throw std::runtime_error ("invalid transaction type"); } - if (!setType (mFormat->elements)) + if (!setType (format->elements)) { + WriteLog (lsWARNING, SerializedTransaction) << + "Transaction not legal for format"; throw std::runtime_error ("transaction not valid"); } } -SerializedTransaction::SerializedTransaction (SerializerIterator& sit) : STObject (sfTransaction), - mSigGood (false), mSigBad (false) +SerializedTransaction::SerializedTransaction (SerializerIterator& sit) + : STObject (sfTransaction) + , sig_state_ (boost::indeterminate) { int length = sit.getBytesLeft (); if ((length < Protocol::txMinSizeBytes) || (length > Protocol::txMaxSizeBytes)) { - WriteLog (lsERROR, SerializedTransaction) << "Transaction has invalid length: " << length; + WriteLog (lsERROR, SerializedTransaction) << + "Transaction has invalid length: " << length; throw std::runtime_error ("Transaction length invalid"); } set (sit); - mType = static_cast (getFieldU16 (sfTransactionType)); + tx_type_ = static_cast (getFieldU16 (sfTransactionType)); - mFormat = TxFormats::getInstance().findByType (mType); + auto format = TxFormats::getInstance().findByType (tx_type_); - if (!mFormat) + if (!format) { - WriteLog (lsWARNING, SerializedTransaction) << "Transaction type: " << mType; + WriteLog (lsWARNING, SerializedTransaction) << + "Invalid transaction type: " << tx_type_; throw std::runtime_error ("invalid transaction type"); } - if (!setType (mFormat->elements)) + if (!setType (format->elements)) { - WriteLog (lsWARNING, SerializedTransaction) << "Transaction not legal for format"; + WriteLog (lsWARNING, SerializedTransaction) << + "Transaction not legal for format"; throw std::runtime_error ("transaction not valid"); } } -std::string SerializedTransaction::getFullText () const +std::string +SerializedTransaction::getFullText () const { std::string ret = "\""; ret += to_string (getTransactionID ()); @@ -101,72 +108,47 @@ std::string SerializedTransaction::getFullText () const return ret; } -std::string SerializedTransaction::getText () const -{ - return STObject::getText (); -} - -std::vector SerializedTransaction::getMentionedAccounts () const +std::vector +SerializedTransaction::getMentionedAccounts () const { std::vector accounts; - BOOST_FOREACH (const SerializedType & it, peekData ()) + for (auto const& it : peekData ()) { - const STAccount* sa = dynamic_cast (&it); - - if (sa != nullptr) + if (auto sa = dynamic_cast (&it)) { - bool found = false; - RippleAddress na = sa->getValueNCA (); - BOOST_FOREACH (const RippleAddress & it, accounts) - { - if (it == na) - { - found = true; - break; - } - } + auto const na = sa->getValueNCA (); - if (!found) + if (std::find (accounts.cbegin (), accounts.cend (), na) == accounts.cend ()) accounts.push_back (na); } - - const STAmount* sam = dynamic_cast (&it); - - if (sam) + else if (auto sa = dynamic_cast (&it)) { - auto issuer = sam->getIssuer (); + auto const& issuer = sa->getIssuer (); - if (issuer.isNonZero ()) - { - RippleAddress na; - na.setAccountID (issuer); - bool found = false; - BOOST_FOREACH (const RippleAddress & it, accounts) - { - if (it == na) - { - found = true; - break; - } - } + if (isXRP (issuer)) + continue; - if (!found) - accounts.push_back (na); - } + RippleAddress na; + na.setAccountID (issuer); + + if (std::find (accounts.cbegin (), accounts.cend (), na) == accounts.cend ()) + accounts.push_back (na); } } + return accounts; } -uint256 SerializedTransaction::getSigningHash () const +uint256 +SerializedTransaction::getSigningHash () const { return STObject::getSigningHash (HashPrefix::txSign); } -uint256 SerializedTransaction::getTransactionID () const +uint256 +SerializedTransaction::getTransactionID () const { - // perhaps we should cache this return getHash (HashPrefix::transactionID); } @@ -182,48 +164,45 @@ Blob SerializedTransaction::getSignature () const } } -void SerializedTransaction::sign (RippleAddress const& naAccountPrivate) +void SerializedTransaction::sign (RippleAddress const& private_key) { Blob signature; - naAccountPrivate.accountPrivateSign (getSigningHash (), signature); + private_key.accountPrivateSign (getSigningHash (), signature); setFieldVL (sfTxnSignature, signature); } bool SerializedTransaction::checkSign () const { - if (mSigGood) - return true; - - if (mSigBad) - return false; - - try + if (boost::indeterminate (sig_state_)) { - RippleAddress n; - n.setAccountPublic (getFieldVL (sfSigningPubKey)); - - if (checkSign (n)) + try { - mSigGood = true; - return true; + RippleAddress n; + n.setAccountPublic (getFieldVL (sfSigningPubKey)); + + sig_state_ = checkSign (n); + } + catch (...) + { + sig_state_ = false; } } - catch (...) - { - ; - } - mSigBad = true; - return false; + assert (!boost::indeterminate (sig_state_)); + + return static_cast (sig_state_); } -bool SerializedTransaction::checkSign (RippleAddress const& naAccountPublic) const +bool SerializedTransaction::checkSign (RippleAddress const& public_key) const { try { - const ECDSA fullyCanonical = (getFlags() & tfFullyCanonicalSig) ? - ECDSA::strict : ECDSA::not_strict; - return naAccountPublic.accountPublicVerify (getSigningHash (), getFieldVL (sfTxnSignature), fullyCanonical); + ECDSA const fullyCanonical = (getFlags() & tfFullyCanonicalSig) + ? ECDSA::strict + : ECDSA::not_strict; + + return public_key.accountPublicVerify (getSigningHash (), + getFieldVL (sfTxnSignature), fullyCanonical); } catch (...) { @@ -261,26 +240,14 @@ Json::Value SerializedTransaction::getJson (int options, bool binary) const return getJson(options); } -std::string SerializedTransaction::getSQLValueHeader () +std::string const& +SerializedTransaction::getMetaSQLInsertReplaceHeader () { - return "(TransID, TransType, FromAcct, FromSeq, LedgerSeq, Status, RawTxn)"; -} + static std::string const sql = "INSERT OR REPLACE INTO Transactions " + "(TransID, TransType, FromAcct, FromSeq, LedgerSeq, Status, RawTxn, TxnMeta)" + " VALUES "; -std::string SerializedTransaction::getMetaSQLValueHeader () -{ - return "(TransID, TransType, FromAcct, FromSeq, LedgerSeq, Status, RawTxn, TxnMeta)"; -} - -std::string SerializedTransaction::getMetaSQLInsertReplaceHeader () -{ - return "INSERT OR REPLACE INTO Transactions " + getMetaSQLValueHeader () + " VALUES "; -} - -std::string SerializedTransaction::getSQL (std::uint32_t inLedger, char status) const -{ - Serializer s; - add (s); - return getSQL (s, inLedger, status); + return sql; } std::string SerializedTransaction::getMetaSQL (std::uint32_t inLedger, @@ -291,25 +258,18 @@ std::string SerializedTransaction::getMetaSQL (std::uint32_t inLedger, return getMetaSQL (s, inLedger, TXN_SQL_VALIDATED, escapedMetaData); } -std::string SerializedTransaction::getSQL (Serializer rawTxn, std::uint32_t inLedger, char status) const -{ - static boost::format bfTrans ("('%s', '%s', '%s', '%d', '%d', '%c', %s)"); - std::string rTxn = sqlEscape (rawTxn.peekData ()); - - return str (boost::format (bfTrans) - % to_string (getTransactionID ()) % getTransactionType () - % getSourceAccount ().humanAccountID () - % getSequence () % inLedger % status % rTxn); -} - -std::string SerializedTransaction::getMetaSQL (Serializer rawTxn, std::uint32_t inLedger, char status, - std::string const& escapedMetaData) const +std::string +SerializedTransaction::getMetaSQL (Serializer rawTxn, + std::uint32_t inLedger, char status, std::string const& escapedMetaData) const { static boost::format bfTrans ("('%s', '%s', '%s', '%d', '%d', '%c', %s, %s)"); - std::string rTxn = sqlEscape (rawTxn.peekData ()); + std::string rTxn = sqlEscape (rawTxn.peekData ()); + + auto format = TxFormats::getInstance().findByType (tx_type_); + assert (format != nullptr); return str (boost::format (bfTrans) - % to_string (getTransactionID ()) % getTransactionType () + % to_string (getTransactionID ()) % format->getName () % getSourceAccount ().humanAccountID () % getSequence () % inLedger % status % rTxn % escapedMetaData); } @@ -358,12 +318,14 @@ bool isMemoOkay (STObject const& st) } // Ensure all account fields are 160-bits -bool isAccountFieldOkay (STObject const& st) +static +bool +isAccountFieldOkay (STObject const& st) { for (int i = 0; i < st.getCount(); ++i) { - const STAccount* t = dynamic_cast(st.peekAtPIndex (i)); - if (t&& !t->isValueH160 ()) + auto t = dynamic_cast(st.peekAtPIndex (i)); + if (t && !t->isValueH160 ()) return false; } diff --git a/src/ripple/app/misc/SerializedTransaction.h b/src/ripple/app/misc/SerializedTransaction.h index 8fe9d2d380..1e61541bb6 100644 --- a/src/ripple/app/misc/SerializedTransaction.h +++ b/src/ripple/app/misc/SerializedTransaction.h @@ -20,10 +20,11 @@ #ifndef RIPPLE_SERIALIZEDTRANSACTION_H #define RIPPLE_SERIALIZEDTRANSACTION_H +#include + namespace ripple { // VFALCO TODO replace these macros with language constants - #define TXN_SQL_NEW 'N' #define TXN_SQL_CONFLICT 'C' #define TXN_SQL_HELD 'H' @@ -42,9 +43,16 @@ public: typedef const std::shared_ptr& ref; public: - explicit SerializedTransaction (SerializerIterator & sit); + SerializedTransaction () = delete; + SerializedTransaction& operator= (SerializedTransaction const& other) = delete; + + SerializedTransaction (SerializedTransaction const& other) = default; + + explicit SerializedTransaction (SerializerIterator& sit); explicit SerializedTransaction (TxType type); - explicit SerializedTransaction (const STObject & object); + + // Only called from ripple::RPC::transactionSign - can we eliminate this? + explicit SerializedTransaction (STObject const& object); // STObject functions SerializedTypeID getSType () const @@ -52,19 +60,15 @@ public: return STI_TRANSACTION; } std::string getFullText () const; - std::string getText () const; // outer transaction functions / signature functions Blob getSignature () const; - void setSignature (Blob const & s) - { - setFieldVL (sfTxnSignature, s); - } + uint256 getSigningHash () const; TxType getTxnType () const { - return mType; + return tx_type_; } STAmount getTransactionFee () const { @@ -86,11 +90,6 @@ public: void setSigningPubKey (const RippleAddress & naSignPubKey); void setSourceAccount (const RippleAddress & naSource); - std::string getTransactionType () const - { - return mFormat->getName (); - } - std::uint32_t getSequence () const { return getFieldU32 (sfSequence); @@ -107,41 +106,32 @@ public: virtual Json::Value getJson (int options) const; virtual Json::Value getJson (int options, bool binary) const; - void sign (const RippleAddress & naAccountPrivate); - bool checkSign (const RippleAddress & naAccountPublic) const; + void sign (RippleAddress const& private_key); + bool checkSign () const; + bool checkSign (RippleAddress const& public_key) const; + bool isKnownGood () const { - return mSigGood; + return (sig_state_ == true); } bool isKnownBad () const { - return mSigBad; + return (sig_state_ == false); } void setGood () const { - mSigGood = true; + sig_state_ = true; } void setBad () const { - mSigBad = true; + sig_state_ = false; } - // SQL Functions - static std::string getSQLValueHeader (); - static std::string getSQLInsertHeader (); - static std::string getSQLInsertIgnoreHeader (); - - std::string getSQL ( - std::string & sql, std::uint32_t inLedger, char status) const; - std::string getSQL ( - std::uint32_t inLedger, char status) const; - std::string getSQL ( - Serializer rawTxn, std::uint32_t inLedger, char status) const; - // SQL Functions with metadata - static std::string getMetaSQLValueHeader (); - static std::string getMetaSQLInsertReplaceHeader (); + static + std::string const& + getMetaSQLInsertReplaceHeader (); std::string getMetaSQL ( std::uint32_t inLedger, std::string const& escapedMetaData) const; @@ -153,16 +143,14 @@ public: std::string const& escapedMetaData) const; private: - TxType mType; - TxFormats::Item const* mFormat; - SerializedTransaction* duplicate () const override { return new SerializedTransaction (*this); } - mutable bool mSigGood; - mutable bool mSigBad; + TxType tx_type_; + + mutable boost::tribool sig_state_; }; bool passesLocalChecks (STObject const& st, std::string&); diff --git a/src/ripple/app/paths/cursor/AdvanceNode.cpp b/src/ripple/app/paths/cursor/AdvanceNode.cpp index bb19b1e645..ee1dc04137 100644 --- a/src/ripple/app/paths/cursor/AdvanceNode.cpp +++ b/src/ripple/app/paths/cursor/AdvanceNode.cpp @@ -234,7 +234,7 @@ TER PathCursor::advanceNode (bool const bReverse) const << "advanceNode: PAST INTERNAL ERROR" << " REVERSE: OFFER NON-POSITIVE:" << " node.saTakerPays=" << node().saTakerPays - << " node.saTakerGets=%s" << node().saTakerGets; + << " node.saTakerGets=" << node().saTakerGets; // Mark offer for always deletion. rippleCalc_.permanentlyUnfundedOffers_.insert ( diff --git a/src/ripple/app/transactors/Transactor.cpp b/src/ripple/app/transactors/Transactor.cpp index dae445062a..beaf87a1d8 100644 --- a/src/ripple/app/transactors/Transactor.cpp +++ b/src/ripple/app/transactors/Transactor.cpp @@ -198,9 +198,7 @@ TER Transactor::checkSeq () } else { - uint256 txID = mTxn.getTransactionID (); - - if (mEngine->getLedger ()->hasTransaction (txID)) + if (mEngine->getLedger ()->hasTransaction (mTxn.getTransactionID ())) return tefALREADY; } diff --git a/src/ripple/app/tx/LocalTxs.cpp b/src/ripple/app/tx/LocalTxs.cpp index 0f4bee4c5e..a7c1557db1 100644 --- a/src/ripple/app/tx/LocalTxs.cpp +++ b/src/ripple/app/tx/LocalTxs.cpp @@ -63,10 +63,7 @@ public: , m_seq (txn->getSequence()) { if (txn->isFieldPresent (sfLastLedgerSequence)) - { - LedgerIndex m_txnexpire = txn->getFieldU32 (sfLastLedgerSequence) + 1; - m_expire = std::min (m_expire, m_txnexpire); - } + m_expire = std::min (m_expire, txn->getFieldU32 (sfLastLedgerSequence) + 1); } uint256 const& getID () const diff --git a/src/ripple/app/tx/Transaction.cpp b/src/ripple/app/tx/Transaction.cpp index af163c96b0..1e8ec31f9e 100644 --- a/src/ripple/app/tx/Transaction.cpp +++ b/src/ripple/app/tx/Transaction.cpp @@ -253,11 +253,11 @@ bool Transaction::isHexTxID (std::string const& txid) if (txid.size () != 64) return false; - auto const ret = std::find_if_not (txid.begin (), txid.end (), - std::bind ( - std::isxdigit , - std::placeholders::_1, - std::locale ())); + auto const ret = std::find_if (txid.begin (), txid.end (), + [](std::string::value_type c) + { + return !std::isxdigit (c); + }); return (ret == txid.end ()); } diff --git a/src/ripple/app/tx/TransactionEngine.cpp b/src/ripple/app/tx/TransactionEngine.cpp index ceb3a73449..afab4ed310 100644 --- a/src/ripple/app/tx/TransactionEngine.cpp +++ b/src/ripple/app/tx/TransactionEngine.cpp @@ -100,7 +100,7 @@ TER TransactionEngine::applyTransaction ( } #endif - uint256 txID = txn.getTransactionID (); + uint256 const& txID = txn.getTransactionID (); if (!txID) { diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index 3e37395b78..4b876cf43e 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/src/ripple/overlay/impl/PeerImp.cpp @@ -1255,7 +1255,7 @@ PeerImp::on_message (std::shared_ptr const& m) SerializerIterator sit (s); SerializedTransaction::pointer stx = std::make_shared < SerializedTransaction> (std::ref (sit)); - uint256 txID = stx->getTransactionID(); + uint256 txID = stx->getTransactionID (); int flags;