Clean up and modernize code:

This commit removes obsolete comments, dead or no longer useful
code, and workarounds for several issues that were present in older
compilers that we no longer support.

Specifically:

- It improves the transaction metadata handling class, simplifying
  its use and making it less error-prone.
- It reduces the footprint of the Serializer class by consolidating
  code and leveraging templates.
- It cleanups the ST* class hierarchy, removing dead code, improving
  and consolidating code to reduce complexity and code duplication.
- It shores up the handling of currency codes and the conversation
  between 160-bit currency codes and their string representation.
- It migrates beast::secure_erase to the ripple namespace and uses
  a call to OpenSSL_cleanse instead of the custom implementation.

src/ripple/protocol/PayChan.h

@@ -34,7 +34,7 @@ serializePayChanAuthorization(
     XRPAmount const& amt)
 {
     msg.add32(HashPrefix::paymentChannelClaim);
-    msg.add256(key);
+    msg.addBitString(key);
     msg.add64(amt.drops());
 }
 

src/ripple/protocol/STArray.h

@@ -30,14 +30,9 @@ class STArray final : public STBase, public CountedObject<STArray>
 private:
     using list_type = std::vector<STObject>;
 
-    enum { reserveSize = 8 };
-
     list_type v_;
 
 public:
-    // Read-only iteration
-    class Items;
-
     static char const*
     getCountedObjectName()
     {
@@ -48,7 +43,7 @@ public:
     using iterator = list_type::iterator;
     using const_iterator = list_type::const_iterator;
 
-    STArray();
+    STArray() = default;
     STArray(STArray&&);
     STArray(STArray const&) = default;
     STArray(SField const& f, int n);

src/ripple/protocol/STObject.h

@@ -239,8 +239,6 @@ private:
         }
     };
 
-    enum { reserveSize = 20 };
-
     using list_type = std::vector<detail::STVar>;
 
     list_type v_;
@@ -281,7 +279,7 @@ public:
 
     explicit STObject(SField const& name);
 
-    virtual ~STObject();
+    virtual ~STObject() = default;
 
     STBase*
     copy(std::size_t n, void* buf) const override

src/ripple/protocol/Serializer.h

@@ -26,20 +26,16 @@
 #include <ripple/basics/base_uint.h>
 #include <ripple/basics/contract.h>
 #include <ripple/basics/safe_cast.h>
-#include <ripple/beast/crypto/secure_erase.h>
 #include <ripple/protocol/HashPrefix.h>
 #include <ripple/protocol/SField.h>
 #include <cassert>
 #include <cstdint>
 #include <cstring>
 #include <iomanip>
-#include <sstream>
 #include <type_traits>
 
 namespace ripple {
 
-class CKey;  // forward declaration
-
 class Serializer
 {
 private:
@@ -83,34 +79,24 @@ public:
 
     // assemble functions
     int
-    add8(unsigned char byte);
-    int add16(std::uint16_t);
-    int add32(std::uint32_t);  // ledger indexes, account sequence, timestamps
-    int add32(HashPrefix);
-    int add64(std::uint64_t);  // native currency amounts
+    add8(unsigned char i);
     int
-    add128(const uint128&);  // private key generators
+    add16(std::uint16_t i);
     int
-    add256(uint256 const&);  // transaction and ledger hashes
+    add32(std::uint32_t i);  // ledger indexes, account sequence, timestamps
+    int
+    add32(HashPrefix p);
+    int
+    add64(std::uint64_t i);  // native currency amounts
 
     template <typename Integer>
     int addInteger(Integer);
 
-    template <int Bits, class Tag>
+    template <std::size_t Bits, class Tag>
     int
     addBitString(base_uint<Bits, Tag> const& v)
     {
-        int ret = mData.size();
-        mData.insert(mData.end(), v.begin(), v.end());
-        return ret;
-    }
-
-    // TODO(tom): merge with add128 and add256.
-    template <class Tag>
-    int
-    add160(base_uint<160, Tag> const& i)
-    {
-        return addBitString<160, Tag>(i);
+        return addRaw(v.data(), v.size());
     }
 
     int
@@ -119,8 +105,6 @@ public:
     addRaw(const void* ptr, int len);
     int
     addRaw(const Serializer& s);
-    int
-    addZeros(size_t uBytes);
 
     int
     addVL(Blob const& vector);
@@ -135,8 +119,6 @@ public:
     // disassemble functions
     bool
     get8(int&, int offset) const;
-    bool
-    get256(uint256&, int offset) const;
 
     template <typename Integer>
     bool
@@ -157,7 +139,7 @@ public:
         return true;
     }
 
-    template <int Bits, typename Tag = void>
+    template <std::size_t Bits, typename Tag = void>
     bool
     getBitString(base_uint<Bits, Tag>& data, int offset) const
     {
@@ -167,24 +149,6 @@ public:
         return success;
     }
 
-    // TODO(tom): merge with get128 and get256.
-    template <class Tag>
-    bool
-    get160(base_uint<160, Tag>& o, int offset) const
-    {
-        return getBitString<160, Tag>(o, offset);
-    }
-
-    bool
-    getRaw(Blob&, int offset, int length) const;
-    Blob
-    getRaw(int offset, int length) const;
-
-    bool
-    getVL(Blob& objectVL, int offset, int& length) const;
-    bool
-    getVLLength(int& length, int offset) const;
-
     int
     addFieldID(int type, int name);
     int
@@ -240,12 +204,6 @@ public:
         return std::string(static_cast<const char*>(getDataPtr()), size());
     }
     void
-    secureErase()
-    {
-        beast::secure_erase(mData.data(), mData.size());
-        mData.clear();
-    }
-    void
     erase()
     {
         mData.clear();
@@ -311,19 +269,6 @@ public:
         return v.mData != mData;
     }
 
-    std::string
-    getHex() const
-    {
-        std::stringstream h;
-
-        for (unsigned char const& element : mData)
-        {
-            h << std::setw(2) << std::hex << std::setfill('0')
-              << safe_cast<unsigned int>(element);
-        }
-        return h.str();
-    }
-
     static int
     decodeLengthLength(int b1);
     static int
@@ -334,11 +279,6 @@ public:
     decodeVLLength(int b1, int b2, int b3);
 
 private:
-    static int
-    lengthVL(int length)
-    {
-        return length + encodeLengthLength(length);
-    }
     static int
     encodeLengthLength(int length);  // length to encode length
     int
@@ -414,7 +354,7 @@ public:
     std::uint64_t
     get64();
 
-    template <int Bits, class Tag = void>
+    template <std::size_t Bits, class Tag = void>
     base_uint<Bits, Tag>
     getBitString();
 
@@ -467,7 +407,7 @@ public:
     getRawHelper(int size);
 };
 
-template <int Bits, class Tag>
+template <std::size_t Bits, class Tag>
 base_uint<Bits, Tag>
 SerialIter::getBitString()
 {

src/ripple/protocol/Sign.h

@@ -84,7 +84,7 @@ startMultiSigningData(STObject const& obj);
 inline void
 finishMultiSigningData(AccountID const& signingID, Serializer& s)
 {
-    s.add160(signingID);
+    s.addBitString(signingID);
 }
 
 }  // namespace ripple

src/ripple/protocol/UintTypes.h

@@ -81,11 +81,22 @@ isXRP(Currency const& c)
 std::string
 to_string(Currency const& c);
 
-/** Tries to convert a string to a Currency, returns true on success. */
+/** Tries to convert a string to a Currency, returns true on success.
+
+    @note This function will return success if the resulting currency is
+          badCurrency(). This legacy behavior is unfortunate; changing this
+          will require very careful checking everywhere and may mean having
+          to rewrite some unit test code.
+*/
 bool
 to_currency(Currency&, std::string const&);
 
-/** Tries to convert a string to a Currency, returns noCurrency() on failure. */
+/** Tries to convert a string to a Currency, returns noCurrency() on failure.
+
+    @note This function can return badCurrency(). This legacy behavior is
+          unfortunate; changing this will require very careful checking
+          everywhere and may mean having to rewrite some unit test code.
+*/
 Currency
 to_currency(std::string const&);
 

src/ripple/protocol/digest.h

@@ -208,7 +208,7 @@ private:
 
     inline void erase(std::true_type)
     {
-        beast::secure_erase(&h_, sizeof(h_));
+        secure_erase(&h_, sizeof(h_));
     }
 };
 
@@ -221,16 +221,6 @@ using sha512_half_hasher_s = detail::basic_sha512_half_hasher<true>;
 
 //------------------------------------------------------------------------------
 
-#ifdef _MSC_VER
-// Call from main to fix magic statics pre-VS2015
-inline void
-sha512_deprecatedMSVCWorkaround()
-{
-    beast::sha512_hasher h;
-    auto const digest = static_cast<beast::sha512_hasher::result_type>(h);
-}
-#endif
-
 /** Returns the SHA512-Half of a series of objects. */
 template <class... Args>
 sha512_half_hasher::result_type

src/ripple/protocol/impl/STAmount.cpp

@@ -624,8 +624,8 @@ STAmount::add(Serializer& s) const
                 (static_cast<std::uint64_t>(mOffset + 512 + 256 + 97)
                  << (64 - 10)));
 
-        s.add160(mIssue.currency);
-        s.add160(mIssue.account);
+        s.addBitString(mIssue.currency);
+        s.addBitString(mIssue.account);
     }
 }
 

src/ripple/protocol/impl/STArray.cpp

@@ -24,14 +24,6 @@
 
 namespace ripple {
 
-STArray::STArray()
-{
-    // VFALCO NOTE We need to determine if this is
-    //             the right thing to do, and consider
-    //             making it optional.
-    // v_.reserve(reserveSize);
-}
-
 STArray::STArray(STArray&& other)
     : STBase(other.getFName()), v_(std::move(other.v_))
 {
@@ -52,7 +44,6 @@ STArray::STArray(int n)
 
 STArray::STArray(SField const& f) : STBase(f)
 {
-    v_.reserve(reserveSize);
 }
 
 STArray::STArray(SField const& f, int n) : STBase(f)

src/ripple/protocol/impl/STObject.cpp

@@ -26,15 +26,6 @@
 
 namespace ripple {
 
-STObject::~STObject()
-{
-#if 0
-    // Turn this on to get a histogram on exit
-    static Log log;
-    log(v_.size());
-#endif
-}
-
 STObject::STObject(STObject&& other)
     : STBase(other.getFName()), v_(std::move(other.v_)), mType(other.mType)
 {
@@ -42,8 +33,6 @@ STObject::STObject(STObject&& other)
 
 STObject::STObject(SField const& name) : STBase(name), mType(nullptr)
 {
-    // VFALCO TODO See if this is the right thing to do
-    // v_.reserve(reserveSize);
 }
 
 STObject::STObject(SOTemplate const& type, SField const& name) : STBase(name)

src/ripple/protocol/impl/STPathSet.cpp

@@ -202,13 +202,13 @@ STPathSet::add(Serializer& s) const
             s.add8(iType);
 
             if (iType & STPathElement::typeAccount)
-                s.add160(speElement.getAccountID());
+                s.addBitString(speElement.getAccountID());
 
             if (iType & STPathElement::typeCurrency)
-                s.add160(speElement.getCurrency());
+                s.addBitString(speElement.getCurrency());
 
             if (iType & STPathElement::typeIssuer)
-                s.add160(speElement.getIssuerID());
+                s.addBitString(speElement.getIssuerID());
         }
 
         first = false;

src/ripple/protocol/impl/SecretKey.cpp

@@ -19,10 +19,10 @@
 
 #include <ripple/basics/contract.h>
 #include <ripple/basics/strHex.h>
-#include <ripple/beast/crypto/secure_erase.h>
 #include <ripple/beast/utility/rngfill.h>
 #include <ripple/crypto/GenerateDeterministicKey.h>
 #include <ripple/crypto/csprng.h>
+#include <ripple/crypto/secure_erase.h>
 #include <ripple/protocol/SecretKey.h>
 #include <ripple/protocol/digest.h>
 #include <ripple/protocol/impl/secp256k1.h>
@@ -33,7 +33,7 @@ namespace ripple {
 
 SecretKey::~SecretKey()
 {
-    beast::secure_erase(buf_, sizeof(buf_));
+    secure_erase(buf_, sizeof(buf_));
 }
 
 SecretKey::SecretKey(std::array<std::uint8_t, 32> const& key)
@@ -86,8 +86,8 @@ public:
         auto gpk = generatePublicDeterministicKey(gen_, ordinal);
         SecretKey const sk(Slice{gsk.data(), gsk.size()});
         PublicKey const pk(Slice{gpk.data(), gpk.size()});
-        beast::secure_erase(ui.data(), ui.size());
-        beast::secure_erase(gsk.data(), gsk.size());
+        secure_erase(ui.data(), ui.size());
+        secure_erase(gsk.data(), gsk.size());
         return {pk, sk};
     }
 };
@@ -169,7 +169,7 @@ randomSecretKey()
     std::uint8_t buf[32];
     beast::rngfill(buf, sizeof(buf), crypto_prng());
     SecretKey sk(Slice{buf, sizeof(buf)});
-    beast::secure_erase(buf, sizeof(buf));
+    secure_erase(buf, sizeof(buf));
     return sk;
 }
 
@@ -182,7 +182,7 @@ generateSecretKey(KeyType type, Seed const& seed)
     {
         auto key = sha512Half_s(Slice(seed.data(), seed.size()));
         SecretKey sk = Slice{key.data(), key.size()};
-        beast::secure_erase(key.data(), key.size());
+        secure_erase(key.data(), key.size());
         return sk;
     }
 
@@ -194,7 +194,7 @@ generateSecretKey(KeyType type, Seed const& seed)
         std::memcpy(ps.data(), seed.data(), seed.size());
         auto const upk = generateRootDeterministicPrivateKey(ps);
         SecretKey sk = Slice{upk.data(), upk.size()};
-        beast::secure_erase(ps.data(), ps.size());
+        secure_erase(ps.data(), ps.size());
         return sk;
     }
 

src/ripple/protocol/impl/Seed.cpp

@@ -19,10 +19,10 @@
 
 #include <ripple/basics/Buffer.h>
 #include <ripple/basics/contract.h>
-#include <ripple/beast/crypto/secure_erase.h>
 #include <ripple/beast/utility/rngfill.h>
 #include <ripple/crypto/RFC1751.h>
 #include <ripple/crypto/csprng.h>
+#include <ripple/crypto/secure_erase.h>
 #include <ripple/protocol/AccountID.h>
 #include <ripple/protocol/PublicKey.h>
 #include <ripple/protocol/SecretKey.h>
@@ -36,7 +36,7 @@ namespace ripple {
 
 Seed::~Seed()
 {
-    beast::secure_erase(buf_.data(), buf_.size());
+    secure_erase(buf_.data(), buf_.size());
 }
 
 Seed::Seed(Slice const& slice)
@@ -61,7 +61,7 @@ randomSeed()
     std::array<std::uint8_t, 16> buffer;
     beast::rngfill(buffer.data(), buffer.size(), crypto_prng());
     Seed seed(makeSlice(buffer));
-    beast::secure_erase(buffer.data(), buffer.size());
+    secure_erase(buffer.data(), buffer.size());
     return seed;
 }
 

src/ripple/protocol/impl/Serializer.cpp

@@ -25,17 +25,6 @@
 
 namespace ripple {
 
-int
-Serializer::addZeros(size_t uBytes)
-{
-    int ret = mData.size();
-
-    while (uBytes--)
-        mData.push_back(0);
-
-    return ret;
-}
-
 int
 Serializer::add16(std::uint16_t i)
 {
@@ -107,22 +96,6 @@ Serializer::addInteger(std::uint64_t i)
     return add64(i);
 }
 
-int
-Serializer::add128(const uint128& i)
-{
-    int ret = mData.size();
-    mData.insert(mData.end(), i.begin(), i.end());
-    return ret;
-}
-
-int
-Serializer::add256(uint256 const& i)
-{
-    int ret = mData.size();
-    mData.insert(mData.end(), i.begin(), i.end());
-    return ret;
-}
-
 int
 Serializer::addRaw(Blob const& vector)
 {
@@ -147,16 +120,6 @@ Serializer::addRaw(const void* ptr, int len)
     return ret;
 }
 
-bool
-Serializer::get256(uint256& o, int offset) const
-{
-    if ((offset + (256 / 8)) > mData.size())
-        return false;
-
-    memcpy(o.begin(), &(mData.front()) + offset, (256 / 8));
-    return true;
-}
-
 int
 Serializer::addFieldID(int type, int name)
 {
@@ -219,28 +182,6 @@ Serializer::chop(int bytes)
     return true;
 }
 
-bool
-Serializer::getRaw(Blob& o, int offset, int length) const
-{
-    if ((offset + length) > mData.size())
-        return false;
-
-    o.assign(mData.begin() + offset, mData.begin() + offset + length);
-    return true;
-}
-
-Blob
-Serializer::getRaw(int offset, int length) const
-{
-    Blob o;
-
-    if ((offset + length) > mData.size())
-        return o;
-
-    o.assign(mData.begin() + offset, mData.begin() + offset + length);
-    return o;
-}
-
 uint256
 Serializer::getSHA512Half() const
 {
@@ -278,99 +219,6 @@ Serializer::addVL(const void* ptr, int len)
     return ret;
 }
 
-bool
-Serializer::getVL(Blob& objectVL, int offset, int& length) const
-{
-    int b1;
-
-    if (!get8(b1, offset++))
-        return false;
-
-    int datLen, lenLen = decodeLengthLength(b1);
-
-    try
-    {
-        if (lenLen == 1)
-            datLen = decodeVLLength(b1);
-        else if (lenLen == 2)
-        {
-            int b2;
-
-            if (!get8(b2, offset++))
-                return false;
-
-            datLen = decodeVLLength(b1, b2);
-        }
-        else if (lenLen == 3)
-        {
-            int b2, b3;
-
-            if (!get8(b2, offset++))
-                return false;
-
-            if (!get8(b3, offset++))
-                return false;
-
-            datLen = decodeVLLength(b1, b2, b3);
-        }
-        else
-            return false;
-    }
-    catch (std::exception const&)
-    {
-        return false;
-    }
-
-    length = lenLen + datLen;
-    return getRaw(objectVL, offset, datLen);
-}
-
-bool
-Serializer::getVLLength(int& length, int offset) const
-{
-    int b1;
-
-    if (!get8(b1, offset++))
-        return false;
-
-    int lenLen = decodeLengthLength(b1);
-
-    try
-    {
-        if (lenLen == 1)
-            length = decodeVLLength(b1);
-        else if (lenLen == 2)
-        {
-            int b2;
-
-            if (!get8(b2, offset++))
-                return false;
-
-            length = decodeVLLength(b1, b2);
-        }
-        else if (lenLen == 3)
-        {
-            int b2, b3;
-
-            if (!get8(b2, offset++))
-                return false;
-
-            if (!get8(b3, offset++))
-                return false;
-
-            length = decodeVLLength(b1, b2, b3);
-        }
-        else
-            return false;
-    }
-    catch (std::exception const&)
-    {
-        return false;
-    }
-
-    return true;
-}
-
 int
 Serializer::addEncoded(int length)
 {

src/ripple/protocol/impl/UintTypes.cpp

@@ -21,20 +21,34 @@
 #include <ripple/protocol/Serializer.h>
 #include <ripple/protocol/SystemParameters.h>
 #include <ripple/protocol/UintTypes.h>
+#include <string_view>
 
 namespace ripple {
 
+// For details on the protocol-level serialization please visit
+// https://xrpl.org/serialization.html#currency-codes
+
+namespace detail {
+
+// Characters we are willing to allow in the ASCII representation of a
+// three-letter currency code.
+constexpr std::string_view isoCharSet =
+    "abcdefghijklmnopqrstuvwxyz"
+    "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+    "0123456789"
+    "<>(){}[]|?!@#$%^&*";
+
+// The location (in bytes) of the 3 digit currency inside a 160-bit value
+constexpr std::size_t isoCodeOffset = 12;
+
+// The length of an ISO-4217 like code
+constexpr std::size_t isoCodeLength = 3;
+
+}  // namespace detail
+
 std::string
 to_string(Currency const& currency)
 {
-    // Characters we are willing to allow in the ASCII representation of a
-    // three-letter currency code.
-    static std::string const allowed_characters =
-        "abcdefghijklmnopqrstuvwxyz"
-        "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-        "0123456789"
-        "<>(){}[]|?!@#$%^&*";
-
     if (currency == beast::zero)
         return systemCurrencyCode();
 
@@ -46,16 +60,14 @@ to_string(Currency const& currency)
 
     if ((currency & sIsoBits).isZero())
     {
-        // The offset of the 3 character ISO code in the currency descriptor
-        int const isoOffset = 12;
-
         std::string const iso(
-            currency.data() + isoOffset, currency.data() + isoOffset + 3);
+            currency.data() + detail::isoCodeOffset,
+            currency.data() + detail::isoCodeOffset + detail::isoCodeLength);
 
         // Specifying the system currency code using ISO-style representation
         // is not allowed.
         if ((iso != systemCurrencyCode()) &&
-            (iso.find_first_not_of(allowed_characters) == std::string::npos))
+            (iso.find_first_not_of(detail::isoCharSet) == std::string::npos))
         {
             return iso;
         }
@@ -73,30 +85,27 @@ to_currency(Currency& currency, std::string const& code)
         return true;
     }
 
-    static const int CURRENCY_CODE_LENGTH = 3;
-    if (code.size() == CURRENCY_CODE_LENGTH)
+    // Handle ISO-4217-like 3-digit character codes.
+    if (code.size() == detail::isoCodeLength)
     {
-        Blob codeBlob(CURRENCY_CODE_LENGTH);
+        if (code.find_first_not_of(detail::isoCharSet) != std::string::npos)
+            return false;
 
-        std::transform(code.begin(), code.end(), codeBlob.begin(), [](auto c) {
-            return ::toupper(static_cast<unsigned char>(c));
-        });
+        currency = beast::zero;
 
-        Serializer s;
+        std::transform(
+            code.begin(),
+            code.end(),
+            currency.begin() + detail::isoCodeOffset,
+            [](auto c) {
+                return static_cast<unsigned char>(
+                    ::toupper(static_cast<unsigned char>(c)));
+            });
 
-        s.addZeros(96 / 8);
-        s.addRaw(codeBlob);
-        s.addZeros(16 / 8);
-        s.addZeros(24 / 8);
-
-        s.get160(currency, 0);
         return true;
     }
 
-    if (40 == code.size())
-        return currency.SetHex(code);
-
-    return false;
+    return currency.SetHexExact(code);
 }
 
 Currency