From 569b3a46a1ba85e35dedcf390b30ed82ca3b2504 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Mon, 3 Mar 2014 13:37:08 -0800 Subject: [PATCH] Cleanups: - Cleanup VotableInteger class and remove unused duplicate - Remove obsolete function and move to std functions - Fix typos - Make isMemoOkay a free function - Sanitize error returns --- src/ripple_app/misc/Features.cpp | 51 ---------- src/ripple_app/misc/FeeVote.cpp | 31 +++--- src/ripple_app/misc/NetworkOPs.cpp | 2 +- src/ripple_app/misc/SerializedTransaction.cpp | 22 +++-- src/ripple_app/misc/SerializedTransaction.h | 4 +- src/ripple_app/rpc/RPCHandler.cpp | 95 ++++++++++--------- src/ripple_app/tx/Transaction.cpp | 30 ++---- src/ripple_overlay/api/Peers.h | 16 ++-- src/ripple_rpc/api/ErrorCodes.h | 4 +- src/ripple_rpc/impl/ErrorCodes.cpp | 4 +- 10 files changed, 102 insertions(+), 157 deletions(-) diff --git a/src/ripple_app/misc/Features.cpp b/src/ripple_app/misc/Features.cpp index a4ccff2e8a..56a466b6a2 100644 --- a/src/ripple_app/misc/Features.cpp +++ b/src/ripple_app/misc/Features.cpp @@ -498,57 +498,6 @@ Json::Value Features::getJson (uint256 const& feature) return ret; } -template class VotableInteger -{ -protected: - INT mCurrent; // The current setting - INT mTarget; // The setting we want - std::map mVoteMap; - -public: - VotableInteger (INT current, INT target) : mCurrent (current), mTarget (target) - { - ++mVoteMap[mTarget]; // Add our vote - } - - bool mayVote () - { - return mCurrent != mTarget; // If we love the current setting, we will not vote - } - - void addVote (INT vote) - { - ++mVoteMap[vote]; - } - - void noVote () - { - addVote (mCurrent); - } - - INT getVotes () - { - INT ourVote = mCurrent; - int weight = 0; - - typedef typename std::map::value_type mapVType; - BOOST_FOREACH (const mapVType & value, mVoteMap) - { - // Take most voted value between current and target, inclusive - // FIXME: Should take best value that can get a significant majority - if ((value.first <= std::max (mTarget, mCurrent)) && - (value.first >= std::min (mTarget, mCurrent)) && - (value.second > weight)) - { - ourVote = value.first; - weight = value.second; - } - } - - return ourVote; - } -}; - IFeatures* IFeatures::New (uint32 majorityTime, int majorityFraction) { return new Features (majorityTime, majorityFraction); diff --git a/src/ripple_app/misc/FeeVote.cpp b/src/ripple_app/misc/FeeVote.cpp index 00dcd139a7..aa92b5ae88 100644 --- a/src/ripple_app/misc/FeeVote.cpp +++ b/src/ripple_app/misc/FeeVote.cpp @@ -24,22 +24,26 @@ class Features; class FeeVote : public IFeeVote { private: - // VFALCO TODO rename template parameter (wtf, looks like a macro) - template + + template class VotableInteger { public: - VotableInteger (INT current, INT target) : mCurrent (current), mTarget (target) + VotableInteger (Integer current, Integer target) + : mCurrent (current) + , mTarget (target) { - ++mVoteMap[mTarget]; // Add our vote + // Add our vote + ++mVoteMap[mTarget]; } - bool mayVote () + bool mayVote () const { - return mCurrent != mTarget; // If we love the current setting, we will not vote + // If we love the current setting, we will not vote + return mCurrent != mTarget; } - void addVote (INT vote) + void addVote (Integer vote) { ++mVoteMap[vote]; } @@ -49,12 +53,12 @@ private: addVote (mCurrent); } - INT getVotes () + Integer getVotes () { - INT ourVote = mCurrent; + Integer ourVote = mCurrent; int weight = 0; - typedef typename std::map::value_type mapVType; + typedef typename std::map::value_type mapVType; BOOST_FOREACH (const mapVType & value, mVoteMap) { // Take most voted value between current and target, inclusive @@ -71,10 +75,11 @@ private: } private: - INT mCurrent; // The current setting - INT mTarget; // The setting we want - std::map mVoteMap; + Integer mCurrent; // The current setting + Integer mTarget; // The setting we want + std::map mVoteMap; }; + public: FeeVote (uint64 targetBaseFee, uint32 targetReserveBase, uint32 targetReserveIncrement) : mTargetBaseFee (targetBaseFee) diff --git a/src/ripple_app/misc/NetworkOPs.cpp b/src/ripple_app/misc/NetworkOPs.cpp index 4b91e216e4..5b7f203fd1 100644 --- a/src/ripple_app/misc/NetworkOPs.cpp +++ b/src/ripple_app/misc/NetworkOPs.cpp @@ -733,7 +733,7 @@ void NetworkOPsImp::submitTransaction (Job&, SerializedTransaction::pointer iTra { try { - if (!trans->isMemoOkay () || !trans->checkSign ()) + if (!isMemoOkay (*trans) || !trans->checkSign ()) { m_journal.warning << "Submitted transaction has bad signature"; getApp().getHashRouter ().setFlag (suppress, SF_BAD); diff --git a/src/ripple_app/misc/SerializedTransaction.cpp b/src/ripple_app/misc/SerializedTransaction.cpp index b071666851..ab873fbfbf 100644 --- a/src/ripple_app/misc/SerializedTransaction.cpp +++ b/src/ripple_app/misc/SerializedTransaction.cpp @@ -307,17 +307,19 @@ std::string SerializedTransaction::getMetaSQL (Serializer rawTxn, uint32 inLedge % getSequence () % inLedger % status % rTxn % escapedMetaData); } -bool SerializedTransaction::isMemoOkay () +//------------------------------------------------------------------------------ +bool isMemoOkay (STObject const& st) { - bool ret = true; - if (isFieldPresent (sfMemos)) - { - Serializer s (2048); // Try to avoid allocate/copy/free's - getFieldArray (sfMemos).add (s); - if (s.getDataLength () > 1024) - ret = false; - } - return ret; + if (!st.isFieldPresent (sfMemos)) + return true; + // The number 2048 is a preallocation hint, not a hard limit + // to avoid allocate/copy/free's + Serializer s (2048); + st.getFieldArray (sfMemos).add (s); + // FIXME move the memo limit into a config tunable + if (s.getDataLength () > 1024) + return false; + return true; } //------------------------------------------------------------------------------ diff --git a/src/ripple_app/misc/SerializedTransaction.h b/src/ripple_app/misc/SerializedTransaction.h index 506aab3076..b1cf24c4c6 100644 --- a/src/ripple_app/misc/SerializedTransaction.h +++ b/src/ripple_app/misc/SerializedTransaction.h @@ -124,8 +124,6 @@ public: mSigBad = true; } - bool isMemoOkay (); - // SQL Functions static std::string getSQLValueHeader (); static std::string getSQLInsertHeader (); @@ -153,5 +151,7 @@ private: mutable bool mSigBad; }; +bool isMemoOkay (STObject const& st); + #endif // vim:ts=4 diff --git a/src/ripple_app/rpc/RPCHandler.cpp b/src/ripple_app/rpc/RPCHandler.cpp index 965ec0e8ca..85374fc904 100644 --- a/src/ripple_app/rpc/RPCHandler.cpp +++ b/src/ripple_app/rpc/RPCHandler.cpp @@ -338,7 +338,8 @@ Json::Value RPCHandler::transactionSign (Json::Value params, if (!bOffline) { - SLE::pointer sleAccountRoot = mNetOps->getSLEi (lSnapshot, Ledger::getAccountRootIndex (raSrcAddressID.getAccountID ())); + SLE::pointer sleAccountRoot = mNetOps->getSLEi (lSnapshot, + Ledger::getAccountRootIndex (raSrcAddressID.getAccountID ())); if (!sleAccountRoot) { @@ -350,23 +351,29 @@ Json::Value RPCHandler::transactionSign (Json::Value params, bool bHaveAuthKey = false; RippleAddress naAuthorizedPublic; - RippleAddress naSecret = RippleAddress::createSeedGeneric (params["secret"].asString ()); - RippleAddress naMasterGenerator = RippleAddress::createGeneratorPublic (naSecret); + RippleAddress naSecret = RippleAddress::createSeedGeneric ( + params["secret"].asString ()); + RippleAddress naMasterGenerator = RippleAddress::createGeneratorPublic ( + naSecret); - // Find the index of Account from the master generator, so we can generate the public and private keys. - RippleAddress naMasterAccountPublic; - unsigned int iIndex = 0; - bool bFound = false; + // Find the index of Account from the master generator, so we can generate + // the public and private keys. + RippleAddress naMasterAccountPublic; + unsigned int iIndex = 0; + bool bFound = false; - // Don't look at ledger entries to determine if the account exists. Don't want to leak to thin server that these accounts are - // related. + // Don't look at ledger entries to determine if the account exists. + // Don't want to leak to thin server that these accounts are related. while (!bFound && iIndex != getConfig ().ACCOUNT_PROBE_MAX) { naMasterAccountPublic.setAccountPublic (naMasterGenerator, iIndex); - WriteLog (lsWARNING, RPCHandler) << "authorize: " << iIndex << " : " << naMasterAccountPublic.humanAccountID () << " : " << raSrcAddressID.humanAccountID (); + WriteLog (lsWARNING, RPCHandler) << + "authorize: " << iIndex << + " : " << naMasterAccountPublic.humanAccountID () << + " : " << raSrcAddressID.humanAccountID (); - bFound = raSrcAddressID.getAccountID () == naMasterAccountPublic.getAccountID (); + bFound = raSrcAddressID.getAccountID () == naMasterAccountPublic.getAccountID (); if (!bFound) ++iIndex; @@ -378,9 +385,12 @@ Json::Value RPCHandler::transactionSign (Json::Value params, } // Use the generator to determine the associated public and private keys. - RippleAddress naGenerator = RippleAddress::createGeneratorPublic (naSecret); - RippleAddress naAccountPublic = RippleAddress::createAccountPublic (naGenerator, iIndex); - RippleAddress naAccountPrivate = RippleAddress::createAccountPrivate (naGenerator, naSecret, iIndex); + RippleAddress naGenerator = RippleAddress::createGeneratorPublic ( + naSecret); + RippleAddress naAccountPublic = RippleAddress::createAccountPublic ( + naGenerator, iIndex); + RippleAddress naAccountPrivate = RippleAddress::createAccountPrivate ( + naGenerator, naSecret, iIndex); if (bHaveAuthKey // The generated pair must match authorized... @@ -395,7 +405,7 @@ Json::Value RPCHandler::transactionSign (Json::Value params, return rpcError (rpcSRC_ACT_NOT_FOUND); } - std::unique_ptr sopTrans; + std::unique_ptr sopTrans; { STParsedJSON parsed ("tx_json", tx_json); @@ -424,30 +434,27 @@ Json::Value RPCHandler::transactionSign (Json::Value params, } catch (std::exception& e) { - jvResult["error"] = "invalidTransaction"; - jvResult["error_exception"] = e.what (); - - return jvResult; + return RPC::make_error (rpcINTERNAL, + "Exception occurred during transaction"); } - if (!stpTrans->isMemoOkay ()) + if (!isMemoOkay (*stpTrans)) { - jvResult["error"] = "invalidTransaction"; - jvResult["error_exception"] = "overlong memos"; - - return jvResult; + return RPC::make_error (rpcINVALID_PARAMS, + "The memo exceeds the maximum allowed size."); } if (params.isMember ("debug_signing")) { - jvResult["tx_unsigned"] = strHex (stpTrans->getSerializer ().peekData ()); + jvResult["tx_unsigned"] = strHex ( + stpTrans->getSerializer ().peekData ()); jvResult["tx_signing_hash"] = stpTrans->getSigningHash ().ToString (); } // FIXME: For performance, transactions should not be signed in this code path. stpTrans->sign (naAccountPrivate); - Transaction::pointer tpTrans; + Transaction::pointer tpTrans; try { @@ -455,37 +462,33 @@ Json::Value RPCHandler::transactionSign (Json::Value params, } catch (std::exception& e) { - jvResult["error"] = "internalTransaction"; - jvResult["error_exception"] = e.what (); - - return jvResult; + return RPC::make_error (rpcINTERNAL, + "Exception occurred during transaction"); } try { // FIXME: For performance, should use asynch interface - tpTrans = mNetOps->submitTransactionSync (tpTrans, mRole == Config::ADMIN, bFailHard, bSubmit); + tpTrans = mNetOps->submitTransactionSync (tpTrans, + mRole == Config::ADMIN, bFailHard, bSubmit); if (!tpTrans) { - jvResult["error"] = "invalidTransaction"; - jvResult["error_exception"] = "Unable to sterilize transaction."; - - return jvResult; + return RPC::make_error (rpcINTERNAL, + "Unable to sterilize transaction."); } } catch (std::exception& e) { - jvResult["error"] = "internalSubmit"; - jvResult["error_exception"] = e.what (); - - return jvResult; + return RPC::make_error (rpcINTERNAL, + "Exception occurred during transaction submission."); } try { - jvResult["tx_json"] = tpTrans->getJson (0); - jvResult["tx_blob"] = strHex (tpTrans->getSTransaction ()->getSerializer ().peekData ()); + jvResult["tx_json"] = tpTrans->getJson (0); + jvResult["tx_blob"] = strHex ( + tpTrans->getSTransaction ()->getSerializer ().peekData ()); if (temUNCERTAIN != tpTrans->getResult ()) { @@ -503,10 +506,8 @@ Json::Value RPCHandler::transactionSign (Json::Value params, } catch (std::exception& e) { - jvResult["error"] = "internalJson"; - jvResult["error_exception"] = e.what (); - - return jvResult; + return RPC::make_error (rpcINTERNAL, + "Exception occurred during JSON handling."); } } @@ -535,7 +536,7 @@ Json::Value RPCHandler::getMasterGenerator (Ledger::ref lrLedger, const RippleAd if (vucMasterGenerator.empty ()) { - return rpcError (rpcFAIL_GEN_DECRPYT); + return rpcError (rpcFAIL_GEN_DECRYPT); } naMasterGenerator.setGenerator (vucMasterGenerator); @@ -693,7 +694,7 @@ Json::Value RPCHandler::accountFromString (Ledger::ref lrLedger, RippleAddress& if (vucMasterGenerator.empty ()) { - rpcError (rpcNO_GEN_DECRPYT); + rpcError (rpcNO_GEN_DECRYPT); } naGenerator.setGenerator (vucMasterGenerator); diff --git a/src/ripple_app/tx/Transaction.cpp b/src/ripple_app/tx/Transaction.cpp index ee072a2053..27bf91749d 100644 --- a/src/ripple_app/tx/Transaction.cpp +++ b/src/ripple_app/tx/Transaction.cpp @@ -31,7 +31,7 @@ Transaction::Transaction (SerializedTransaction::ref sit, bool bValidate) return; } - if (!bValidate || (mTransaction->isMemoOkay () && checkSign ())) + if (!bValidate || (isMemoOkay (*mTransaction) && checkSign ())) mStatus = NEW; } @@ -350,29 +350,17 @@ Json::Value Transaction::getJson (int options, bool binary) const return ret; } -// -// Obsolete -// - -static bool isHex (char j) -{ - if ((j >= '0') && (j <= '9')) return true; - - if ((j >= 'A') && (j <= 'F')) return true; - - if ((j >= 'a') && (j <= 'f')) return true; - - return false; -} - bool Transaction::isHexTxID (const std::string& txid) { - if (txid.size () != 64) return false; + if (txid.size () != 64) + return false; - for (int i = 0; i < 64; ++i) - if (!isHex (txid[i])) return false; + auto const ret = std::find_if_not (txid.begin (), txid.end (), + std::bind ( + std::isxdigit , + std::placeholders::_1, + std::locale ())); - return true; + return (ret == txid.end ()); } -// vim:ts=4 diff --git a/src/ripple_overlay/api/Peers.h b/src/ripple_overlay/api/Peers.h index 4207bd8507..eda986b9a1 100644 --- a/src/ripple_overlay/api/Peers.h +++ b/src/ripple_overlay/api/Peers.h @@ -175,18 +175,18 @@ struct send_if_pred { typedef void return_type; - PackedMessage::pointer const& msg; - Predicate const& predicate; + PackedMessage::pointer const& msg; + Predicate const& predicate; - send_if_pred(PackedMessage::pointer const& m, Predicate const& p) - : msg(m), predicate(p) - { } + send_if_pred(PackedMessage::pointer const& m, Predicate const& p) + : msg(m), predicate(p) + { } void operator()(Peer::ref peer) const - { + { if (predicate (peer)) peer->sendPacket (msg, false); - } + } }; /** Helper function to aid in type deduction */ @@ -206,7 +206,7 @@ struct send_if_not_pred { typedef void return_type; - PackedMessage::pointer const& msg; + PackedMessage::pointer const& msg; Predicate const& predicate; send_if_not_pred(PackedMessage::pointer const& m, Predicate const& p) diff --git a/src/ripple_rpc/api/ErrorCodes.h b/src/ripple_rpc/api/ErrorCodes.h index b28a3481dd..7918ed0cc6 100644 --- a/src/ripple_rpc/api/ErrorCodes.h +++ b/src/ripple_rpc/api/ErrorCodes.h @@ -108,10 +108,10 @@ enum error_code_i // Internal error (should never happen) rpcINTERNAL, // Generic internal error. - rpcFAIL_GEN_DECRPYT, + rpcFAIL_GEN_DECRYPT, rpcNOT_IMPL, rpcNOT_SUPPORTED, - rpcNO_GEN_DECRPYT, + rpcNO_GEN_DECRYPT, }; //------------------------------------------------------------------------------ diff --git a/src/ripple_rpc/impl/ErrorCodes.cpp b/src/ripple_rpc/impl/ErrorCodes.cpp index 0b3dc6b525..90addee695 100644 --- a/src/ripple_rpc/impl/ErrorCodes.cpp +++ b/src/ripple_rpc/impl/ErrorCodes.cpp @@ -65,7 +65,7 @@ public: add (rpcDST_AMT_MALFORMED, "dstAmtMalformed", "Destination amount/currency/issuer is malformed."); add (rpcDST_ISR_MALFORMED, "dstIsrMalformed", "Destination issuer is malformed."); add (rpcFORBIDDEN, "forbidden", "Bad credentials."); - add (rpcFAIL_GEN_DECRPYT, "failGenDecrypt", "Failed to decrypt generator."); + add (rpcFAIL_GEN_DECRYPT, "failGenDecrypt", "Failed to decrypt generator."); add (rpcGETS_ACT_MALFORMED, "getsActMalformed", "Gets account malformed."); add (rpcGETS_AMT_MALFORMED, "getsAmtMalformed", "Gets amount malformed."); add (rpcHOST_IP_MALFORMED, "hostIpMalformed", "Host IP is malformed."); @@ -84,7 +84,7 @@ public: add (rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable."); add (rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable."); add (rpcNO_EVENTS, "noEvents", "Current transport does not support events."); - add (rpcNO_GEN_DECRPYT, "noGenDectypt", "Password failed to decrypt master public generator."); + add (rpcNO_GEN_DECRYPT, "noGenDecrypt", "Password failed to decrypt master public generator."); add (rpcNO_NETWORK, "noNetwork", "Network not available."); add (rpcNO_PATH, "noPath", "Unable to find a ripple path."); add (rpcNO_PERMISSION, "noPermission", "You don't have permission for this command.");