Refactor SerializedTransaction:

* Use boost:tribool instead of two intertwined bool variables
* Trim down public interface, reduce member variables

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 <TxType> (getFieldU16 (sfTransactionType));
+    tx_type_ = static_cast <TxType> (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<TxType> (getFieldU16 (sfTransactionType));
+    tx_type_ = static_cast<TxType> (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<RippleAddress> SerializedTransaction::getMentionedAccounts () const
+std::vector<RippleAddress>
+SerializedTransaction::getMentionedAccounts () const
 {
     std::vector<RippleAddress> accounts;
 
-    BOOST_FOREACH (const SerializedType & it, peekData ())
+    for (auto const& it : peekData ())
     {
-        const STAccount* sa = dynamic_cast<const STAccount*> (&it);
-
-        if (sa != nullptr)
+        if (auto sa = dynamic_cast<STAccount const*> (&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<const STAmount*> (&it);
-
-        if (sam)
+        else if (auto sa = dynamic_cast<STAmount const*> (&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<bool> (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<STAccount const*>(st.peekAtPIndex (i));
-        if (t&& !t->isValueH160 ())
+        auto t = dynamic_cast<STAccount const*>(st.peekAtPIndex (i));
+        if (t && !t->isValueH160 ())
             return false;
     }
 

src/ripple/app/misc/SerializedTransaction.h

@@ -20,10 +20,11 @@
 #ifndef RIPPLE_SERIALIZEDTRANSACTION_H
 #define RIPPLE_SERIALIZEDTRANSACTION_H
 
+#include <boost/logic/tribool.hpp>
+
 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<SerializedTransaction>& 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&);

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 (

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

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

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::string::value_type>,
-            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 ());
 }

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

src/ripple/overlay/impl/PeerImp.cpp

@@ -1255,7 +1255,7 @@ PeerImp::on_message (std::shared_ptr <protocol::TMTransaction> 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;