Improve canonicalization of serialized amounts:

The existing code that deserialized an STAmount was sub-optimal and performed
poorly. In some rare cases the operation could result in otherwise valid
serialized amounts overflowing during deserialization. This commit will help
detect error conditions more quickly and eliminate the problematic corner cases.

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

@@ -290,6 +290,8 @@ TxQ::MaybeTx::apply(Application& app, OpenView& view, beast::Journal j)
 {
     // If the rules or flags change, preflight again
     assert(pfresult);
+    STAmountSO stAmountSO{view.rules().enabled(fixSTAmountCanonicalize)};
+
     if (pfresult->rules != view.rules() || pfresult->flags != flags)
     {
         JLOG(j.debug()) << "Queued transaction " << txID
@@ -728,6 +730,8 @@ TxQ::apply(
     ApplyFlags flags,
     beast::Journal j)
 {
+    STAmountSO stAmountSO{view.rules().enabled(fixSTAmountCanonicalize)};
+
     // See if the transaction paid a high enough fee that it can go straight
     // into the ledger.
     if (auto directApplied = tryDirectApply(app, view, tx, flags, j))

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

@@ -746,6 +746,8 @@ Transactor::operator()()
 {
     JLOG(j_.trace()) << "apply: " << ctx_.tx.getTransactionID();
 
+    STAmountSO stAmountSO{view().rules().enabled(fixSTAmountCanonicalize)};
+
 #ifdef DEBUG
     {
         Serializer ser;

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

@@ -113,6 +113,8 @@ apply(
     ApplyFlags flags,
     beast::Journal j)
 {
+    STAmountSO stAmountSO{view.rules().enabled(fixSTAmountCanonicalize)};
+
     auto pfresult = preflight(app, view.rules(), tx, flags, j);
     auto pcresult = preclaim(pfresult, app, view);
     return doApply(pcresult, app, view);

src/ripple/protocol/Feature.h

@@ -114,7 +114,9 @@ class FeatureCollections
         "fixAmendmentMajorityCalc",  // Fix Amendment majority calculation
         "NegativeUNL",
         "TicketBatch",
-        "FlowSortStrands"};
+        "FlowSortStrands",
+        "fixSTAmountCanonicalize",
+    };
 
     std::vector<uint256> features;
     boost::container::flat_map<uint256, std::size_t> featureToIndex;
@@ -373,6 +375,7 @@ extern uint256 const fixAmendmentMajorityCalc;
 extern uint256 const featureNegativeUNL;
 extern uint256 const featureTicketBatch;
 extern uint256 const featureFlowSortStrands;
+extern uint256 const fixSTAmountCanonicalize;
 
 }  // namespace ripple
 

src/ripple/protocol/STAmount.h

@@ -21,6 +21,7 @@
 #define RIPPLE_PROTOCOL_STAMOUNT_H_INCLUDED
 
 #include <ripple/basics/IOUAmount.h>
+#include <ripple/basics/LocalValue.h>
 #include <ripple/basics/XRPAmount.h>
 #include <ripple/protocol/Issue.h>
 #include <ripple/protocol/SField.h>
@@ -471,6 +472,32 @@ isXRP(STAmount const& amount)
     return isXRP(amount.issue().currency);
 }
 
+// Since `canonicalize` does not have access to a ledger, this is needed to put
+// the low-level routine stAmountCanonicalize on an amendment switch. Only
+// transactions need to use this switchover. Outside of a transaction it's safe
+// to unconditionally use the new behavior.
+extern LocalValue<bool> stAmountCanonicalizeSwitchover;
+
+/** RAII class to set and restore the STAmount canonicalize switchover.
+ */
+
+class STAmountSO
+{
+public:
+    explicit STAmountSO(bool v) : saved_(*stAmountCanonicalizeSwitchover)
+    {
+        *stAmountCanonicalizeSwitchover = v;
+    }
+
+    ~STAmountSO()
+    {
+        *stAmountCanonicalizeSwitchover = saved_;
+    }
+
+private:
+    bool saved_;
+};
+
 }  // namespace ripple
 
 #endif

src/ripple/protocol/impl/Feature.cpp

@@ -134,6 +134,7 @@ detail::supportedAmendments()
         //"NegativeUNL",      // Commented out to prevent automatic enablement
         "TicketBatch",
         "FlowSortStrands",
+        "fixSTAmountCanonicalize",
     };
     return supported;
 }
@@ -188,7 +189,8 @@ uint256 const
     fixAmendmentMajorityCalc        = *getRegisteredFeature("fixAmendmentMajorityCalc"),
     featureNegativeUNL              = *getRegisteredFeature("NegativeUNL"),
     featureTicketBatch              = *getRegisteredFeature("TicketBatch"),
-    featureFlowSortStrands          = *getRegisteredFeature("FlowSortStrands");
+    featureFlowSortStrands          = *getRegisteredFeature("FlowSortStrands"),
+    fixSTAmountCanonicalize         = *getRegisteredFeature("fixSTAmountCanonicalize");
 
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.

src/ripple/protocol/impl/STAmount.cpp

@@ -34,6 +34,8 @@
 
 namespace ripple {
 
+LocalValue<bool> stAmountCanonicalizeSwitchover(true);
+
 static const std::uint64_t tenTo14 = 100000000000000ull;
 static const std::uint64_t tenTo14m1 = tenTo14 - 1;
 static const std::uint64_t tenTo17 = tenTo14 * 1000;
@@ -662,13 +664,23 @@ STAmount::canonicalize()
         // native currency amounts should always have an offset of zero
         mIsNative = true;
 
-        if (mValue == 0)
+        // log(2^64,10) ~ 19.2
+        if (mValue == 0 || mOffset <= -20)
         {
+            mValue = 0;
             mOffset = 0;
             mIsNegative = false;
             return;
         }
 
+        if (*stAmountCanonicalizeSwitchover)
+        {
+            // log(cMaxNativeN, 10) == 17
+            if (mOffset > 17)
+                Throw<std::runtime_error>(
+                    "Native currency amount out of range");
+        }
+
         while (mOffset < 0)
         {
             mValue /= 10;
@@ -677,6 +689,14 @@ STAmount::canonicalize()
 
         while (mOffset > 0)
         {
+            if (*stAmountCanonicalizeSwitchover)
+            {
+                // N.B. do not move the overflow check to after the
+                // multiplication
+                if (mValue > cMaxNativeN)
+                    Throw<std::runtime_error>(
+                        "Native currency amount out of range");
+            }
             mValue *= 10;
             --mOffset;
         }