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

@@ -54,7 +54,7 @@ AcceptedLedgerTx::AcceptedLedgerTx (
 
 AcceptedLedgerTx::AcceptedLedgerTx (
     std::shared_ptr<ReadView const> const& ledger,
-    STTx::ref txn,
+    std::shared_ptr<STTx const> const& txn,
     TER result,
     AccountIDCache const& accountCache,
     Logs& logs)

src/ripple/app/ledger/AcceptedLedgerTx.h

@@ -62,7 +62,7 @@ public:
         Logs&);
     AcceptedLedgerTx (
         std::shared_ptr<ReadView const> const&,
-        STTx::ref,
+        std::shared_ptr<STTx const> const&,
         TER,
         AccountIDCache const&,
         Logs&);

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<STTx> (std::ref (sit));
+            auto stx = std::make_shared<STTx const> (std::ref (sit));
             assert (stx->getTransactionID () == nodeHash);
             auto const pap = &app_;
             app_.getJobQueue ().addJob (

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

@@ -1164,7 +1164,7 @@ void LedgerConsensusImp::accept (std::shared_ptr<SHAMap> set)
                     << " not get in";
                 SerialIter sit (it.second->peekTransaction().slice());
 
-                auto txn = std::make_shared<STTx>(sit);
+                auto txn = std::make_shared<STTx const>(sit);
 
                 retriableTxs.insert (txn);
 

src/ripple/app/misc/NetworkOPs.cpp

@@ -155,7 +155,7 @@ public:
     //
 
     // Must complete immediately.
-    void submitTransaction (STTx::pointer) override;
+    void submitTransaction (std::shared_ptr<STTx const> const&) override;
 
     void processTransaction (
         Transaction::pointer& transaction,
@@ -339,7 +339,7 @@ public:
     void pubLedger (Ledger::ref lpAccepted) override;
     void pubProposedTransaction (
         std::shared_ptr<ReadView const> const& lpCurrent,
-        STTx::ref stTxn, TER terResult) override;
+        std::shared_ptr<STTx const> 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<STTx const> const& iTrans)
 {
     if (isNeedNetworkLedger ())
     {
@@ -648,11 +648,7 @@ void NetworkOPsImp::submitTransaction (STTx::pointer iTrans)
     }
 
     // this is an asynchronous interface
-    Serializer s;
+    auto const trans = sterilize(*iTrans);
-    iTrans->add (s);
-
-    SerialIter sit (s.slice());
-    auto trans = std::make_shared<STTx> (std::ref (sit));
 
     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<ReadView const> const& lpCurrent,
-    STTx::ref stTxn, TER terResult)
+    std::shared_ptr<STTx const> const& stTxn, TER terResult)
 {
     Json::Value jvObj   = transJson (*stTxn, terResult, false, lpCurrent);
 

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<STTx const> 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<ReadView const> const& lpCurrent,
-        STTx::ref stTxn, TER terResult) = 0;
+        std::shared_ptr<STTx const> const& stTxn, TER terResult) = 0;
 };
 
 //------------------------------------------------------------------------------

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

@@ -40,7 +40,7 @@ convertBlobsToTxResult (
     Application& app)
 {
     SerialIter it (makeSlice(rawTxn));
-    STTx::pointer txn = std::make_shared<STTx> (it);
+    auto txn = std::make_shared<STTx const> (it);
     std::string reason;
 
     auto tr = std::make_shared<Transaction> (txn, reason, app);

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<STTx const> const& txn) = 0;
 
     // Return the set of local transactions to a new open ledger
     virtual CanonicalTXSet getTxSet () = 0;

src/ripple/app/tx/Transaction.h

@@ -64,7 +64,7 @@ public:
 
 public:
     Transaction (
-        STTx::ref, std::string&, Application&) noexcept;
+        std::shared_ptr<STTx const> const&, std::string&, Application&) noexcept;
 
     static
     Transaction::pointer
@@ -90,7 +90,7 @@ public:
     TransStatus
     sqlTransactionStatus(boost::optional<std::string> const& status);
 
-    STTx::ref getSTransaction ()
+    std::shared_ptr<STTx const> const& getSTransaction ()
     {
         return mTransaction;
     }
@@ -170,7 +170,7 @@ private:
     TER             mResult = temUNCERTAIN;
     bool            mApplying = false;
 
-    STTx::pointer   mTransaction;
+    std::shared_ptr<STTx const>   mTransaction;
     Application&    mApp;
     beast::Journal  j_;
 };

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);
+    Transaction::pointer
-    STTx::pointer  fetch (std::shared_ptr<SHAMapItem> const& item, SHAMapTreeNode:: TNType type,
+    fetch (uint256 const& , bool checkDisk);
-                                           bool checkDisk, std::uint32_t uCommitLedger);
+
+    std::shared_ptr<STTx const>
+    fetch (std::shared_ptr<SHAMapItem> 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 <uint256, Transaction>& getCache();
+
+    TaggedCache <uint256, Transaction>&
+    getCache();
 
 private:
     Application& mApp;

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<STTx const> 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<STTx const> const& getTX () const
     {
         return m_txn;
     }
@@ -97,7 +97,7 @@ public:
 
 private:
 
-    STTx::pointer m_txn;
+    std::shared_ptr<STTx const> 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<STTx const> const& txn) override
     {
         std::lock_guard <std::mutex> lock (m_lock);
 

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

@@ -31,7 +31,7 @@
 
 namespace ripple {
 
-Transaction::Transaction (STTx::ref stx,
+Transaction::Transaction (std::shared_ptr<STTx const> 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<STTx>(sit);
+        auto txn = std::make_shared<STTx const>(sit);
         std::string reason;
         auto result = std::make_shared<Transaction> (
             txn, reason, app);
@@ -125,7 +125,7 @@ Transaction::pointer Transaction::transactionFromSQL (
         rangeCheckedCast<std::uint32_t>(ledgerSeq.value_or (0));
 
     SerialIter it (makeSlice(rawTxn));
-    auto txn = std::make_shared<STTx> (it);
+    auto txn = std::make_shared<STTx const> (it);
     std::string reason;
     auto tr = std::make_shared<Transaction> (
         txn, reason, app);

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<SHAMapItem> const& item,
+std::shared_ptr<STTx const>
+TransactionMaster::fetch (std::shared_ptr<SHAMapItem> const& item,
         SHAMapTreeNode::TNType type,
         bool checkDisk, std::uint32_t uCommitLedger)
 {
-    STTx::pointer  txn;
+    std::shared_ptr<STTx const>  txn;
     auto iTx = fetch (item->key(), false);
 
     if (!iTx)
@@ -73,12 +74,12 @@ STTx::pointer TransactionMaster::fetch (std::shared_ptr<SHAMapItem> const& item,
         if (type == SHAMapTreeNode::tnTRANSACTION_NM)
         {
             SerialIter sit (item->slice());
-            txn = std::make_shared<STTx> (std::ref (sit));
+            txn = std::make_shared<STTx const> (std::ref (sit));
         }
         else if (type == SHAMapTreeNode::tnTRANSACTION_MD)
         {
             auto blob = SerialIter{item->data(), item->size()}.getVL();
-            txn = std::make_shared<STTx>(SerialIter{blob.data(), blob.size()});
+            txn = std::make_shared<STTx const>(SerialIter{blob.data(), blob.size()});
         }
     }
     else

src/ripple/overlay/impl/PeerImp.cpp

@@ -1031,7 +1031,7 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMTransaction> const& m)
 
     try
     {
-        auto stx = std::make_shared<STTx>(sit);
+        auto stx = std::make_shared<STTx const>(sit);
         uint256 txID = stx->getTransactionID ();
 
         int flags;
@@ -1716,7 +1716,7 @@ PeerImp::doFetchPack (const std::shared_ptr<protocol::TMGetObjectByHash>& packet
 
 void
 PeerImp::checkTransaction (int flags,
-    bool checkSignature, STTx::pointer stx)
+    bool checkSignature, std::shared_ptr<STTx const> const& stx)
 {
     // VFALCO TODO Rewrite to not use exceptions
     try

src/ripple/overlay/impl/PeerImp.h

@@ -449,7 +449,8 @@ private:
     doFetchPack (const std::shared_ptr<protocol::TMGetObjectByHash>& packet);
 
     void
-    checkTransaction (int flags, bool checkSignature, STTx::pointer stx);
+    checkTransaction (int flags, bool checkSignature,
+        std::shared_ptr<STTx const> const& stx);
 
     void
     checkPropose (Job& job,

src/ripple/protocol/STTx.h

@@ -24,6 +24,7 @@
 #include <ripple/protocol/TxFormats.h>
 #include <boost/container/flat_set.hpp>
 #include <boost/logic/tribool.hpp>
+#include <boost/optional.hpp>
 
 namespace ripple {
 
@@ -42,9 +43,6 @@ class STTx final
 public:
     static char const* getCountedObjectName () { return "STTx"; }
 
-    using pointer = std::shared_ptr<STTx>;
-    using ref     = const std::shared_ptr<STTx>&;
-
     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<uint256> 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<STTx const>
+sterilize (STTx const& stx);
+
 } // ripple
 
 #endif

src/ripple/protocol/impl/STTx.cpp

@@ -27,6 +27,7 @@
 #include <ripple/protocol/STArray.h>
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/types.h>
+#include <ripple/basics/contract.h>
 #include <ripple/basics/Log.h>
 #include <ripple/basics/StringUtilities.h>
 #include <ripple/json/to_string.h>
@@ -34,6 +35,7 @@
 #include <beast/cxx14/memory.h> // <memory>
 #include <boost/format.hpp>
 #include <array>
+#include <utility>
 
 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<STTx const>
+sterilize (STTx const& stx)
+{
+    Serializer s;
+    stx.add(s);
+    SerialIter sit(s.slice());
+    return std::make_shared<STTx const>(std::ref(sit));
+}
+
 } // ripple

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<STTx const> stpTrans;
 
     try
     {
-        stpTrans = std::make_shared<STTx> (std::ref (sitTrans));
+        stpTrans = std::make_shared<STTx const> (std::ref (sitTrans));
     }
     catch (std::exception& e)
     {

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<STTx const> from transactionPreProcessImpl ().
 struct transactionPreProcessResult
 {
     Json::Value const first;
-    STTx::pointer const second;
+    std::shared_ptr<STTx const> const second;
 
     transactionPreProcessResult () = delete;
     transactionPreProcessResult (transactionPreProcessResult const&) = delete;
@@ -318,7 +318,7 @@ struct transactionPreProcessResult
     , second ()
     { }
 
-    transactionPreProcessResult (STTx::pointer&& st)
+    transactionPreProcessResult (std::shared_ptr<STTx const>&& st)
     : first ()
     , second (std::move (st))
     { }
@@ -463,7 +463,7 @@ transactionPreProcessImpl (
         return std::move (err);
     }
 
-    STTx::pointer stpTrans;
+    std::shared_ptr<STTx> 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 <Json::Value, Transaction::pointer>
-transactionConstructImpl (STTx::pointer stpTrans,
+transactionConstructImpl (std::shared_ptr<STTx const> const& stpTrans,
     Rules const& rules, Application& app)
 {
     std::pair <Json::Value, Transaction::pointer> 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<STTx> stpTrans;
     {
         STParsedJSONObject parsedTx_json ("tx_json", tx_json);
         if (!parsedTx_json.object)
@@ -944,8 +944,8 @@ Json::Value transactionSubmitMultiSigned (
         }
         try
         {
-            stpTrans =
+            stpTrans = std::make_shared<STTx>(
-                std::make_shared<STTx> (std::move(parsedTx_json.object.get()));
+                std::move(parsedTx_json.object.get()));
         }
         catch (std::exception& ex)
         {

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 (jt.stx)
-    if (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<STTx>(
+        return sterilize(STTx{std::move(*obj)});
-            std::move(*obj));
     }
     catch(...)
     {