diff --git a/RefactoringNotes.txt b/RefactoringNotes.txt new file mode 100644 index 0000000000..8c76d2d960 --- /dev/null +++ b/RefactoringNotes.txt @@ -0,0 +1,51 @@ +-------------------------------------------------------------------------------- + +Naming + +Some names don't make sense. + + LedgerAcquire + Not a noun. + Is it really an InboundLedger ? + Does it continue to exist after the ledger is received? + +Inconsistent names + + We have full names like SerializedType and then acronyms like STObject + Two names for some things, e.g. SerializedLedgerEntry and SLE + + Shared/Smart pointer typedefs in classes have a variety of different names + for the same thing. e.g. "pointer", "ptr", "ptr_t", "wptr" + +Verbose names + + The prefix "Flat" is more appealing than "Serialized" because its shorter and + easier to pronounce. + +Ledger "Skip List" + + Is not really a skip list + +-------------------------------------------------------------------------------- + +Interfaces + + Serializer + + Upon analysis this class does two incompatible things. Flattening, and + unflattening. The interface should be reimplemented as two distinct + abstract classes, InputStream and OutputStream with suitable implementations + such as to and from a block of memory or dynamically allocated buffer. + + The name and conflation of dual roles serves to confuse code at the point + of call. Does set(Serializer& s) flatten or unflatten the data? This + would be more clear: + bool write (OutputStream& stream); + + + +Implementation + + LoadManager + + What is going on in the destructor diff --git a/StyleCheatSheet.txt b/StyleCheatSheet.txt index a168e8c22c..451b42f97a 100644 --- a/StyleCheatSheet.txt +++ b/StyleCheatSheet.txt @@ -3,3 +3,5 @@ - Place each ctor-initializer on its own line. - Create typedefs for primitive types to describe them. - Return descriptive local variables instead of constants. +- Use "explicit" for single-argument ctors + diff --git a/modules/ripple_data/protocol/ripple_FieldNames.h b/modules/ripple_data/protocol/ripple_FieldNames.h index cdfaf387cc..a5315f2ca5 100644 --- a/modules/ripple_data/protocol/ripple_FieldNames.h +++ b/modules/ripple_data/protocol/ripple_FieldNames.h @@ -23,6 +23,11 @@ enum SerializedTypeID STI_VALIDATION = 10003, }; +/** Identifies fields. + + Fields are necessary to tag data in signed transactions so that + the binary format of the transaction can be canonicalized. +*/ // VFALCO TODO rename this to NamedField class SField { @@ -43,40 +48,58 @@ public: const int fieldCode; // (type<<16)|index const SerializedTypeID fieldType; // STI_* const int fieldValue; // Code number for protocol - std::string fieldName; + std::string fieldName; int fieldMeta; int fieldNum; bool signingField; - SField(int fc, SerializedTypeID tid, int fv, const char* fn) : - fieldCode(fc), fieldType(tid), fieldValue(fv), fieldName(fn), fieldMeta(sMD_Default), signingField(true) + SField (int fc, SerializedTypeID tid, int fv, const char* fn) + : fieldCode (fc) + , fieldType (tid) + , fieldValue (fv) + , fieldName (fn) + , fieldMeta (sMD_Default) + , signingField (true) { boost::mutex::scoped_lock sl(mapMutex); - codeToField[fieldCode] = this; - fieldNum = ++num; + + codeToField[fieldCode] = this; + + fieldNum = ++num; } - SField(SerializedTypeID tid, int fv, const char *fn) : - fieldCode(FIELD_CODE(tid, fv)), fieldType(tid), fieldValue(fv), fieldName(fn), - fieldMeta(sMD_Default), signingField(true) + SField (SerializedTypeID tid, int fv, const char *fn) + : fieldCode (FIELD_CODE (tid, fv)) + , fieldType (tid) + , fieldValue (fv) + , fieldName (fn) + , fieldMeta (sMD_Default) + , signingField (true) { boost::mutex::scoped_lock sl(mapMutex); - codeToField[fieldCode] = this; - fieldNum = ++num; + + codeToField[fieldCode] = this; + + fieldNum = ++num; } - SField(int fc) : fieldCode(fc), fieldType(STI_UNKNOWN), fieldValue(0), fieldMeta(sMD_Never), signingField(true) + explicit SField (int fc) + : fieldCode (fc) + , fieldType (STI_UNKNOWN) + , fieldValue (0) + , fieldMeta (sMD_Never) + , signingField (true) { boost::mutex::scoped_lock sl(mapMutex); fieldNum = ++num; } - ~SField(); + ~SField (); - static SField::ref getField(int fieldCode); - static SField::ref getField(const std::string& fieldName); - static SField::ref getField(int type, int value) { return getField(FIELD_CODE(type, value)); } - static SField::ref getField(SerializedTypeID type, int value) { return getField(FIELD_CODE(type, value)); } + static SField::ref getField (int fieldCode); + static SField::ref getField (const std::string& fieldName); + static SField::ref getField (int type, int value) { return getField(FIELD_CODE(type, value)); } + static SField::ref getField (SerializedTypeID type, int value) { return getField(FIELD_CODE(type, value)); } std::string getName() const; bool hasName() const { return !fieldName.empty(); } @@ -86,8 +109,11 @@ public: bool isUseful() const { return fieldCode > 0; } bool isKnown() const { return fieldType != STI_UNKNOWN; } bool isBinary() const { return fieldValue < 256; } - bool isDiscardable() const { return fieldValue > 256; } - int getCode() const { return fieldCode; } + + // VFALCO NOTE What is a discardable field? + bool isDiscardable() const { return fieldValue > 256; } + + int getCode() const { return fieldCode; } int getNum() const { return fieldNum; } static int getNumFields() { return num; } @@ -99,8 +125,9 @@ public: bool shouldInclude(bool withSigningField) const { return (fieldValue < 256) && (withSigningField || signingField); } - bool operator==(const SField& f) const { return fieldCode == f.fieldCode; } - bool operator!=(const SField& f) const { return fieldCode != f.fieldCode; } + bool operator== (const SField& f) const { return fieldCode == f.fieldCode; } + + bool operator!= (const SField& f) const { return fieldCode != f.fieldCode; } static int compare(SField::ref f1, SField::ref f2); @@ -110,7 +137,7 @@ protected: static boost::mutex mapMutex; static int num; - SField(SerializedTypeID id, int val); + SField (SerializedTypeID id, int val); }; extern SField sfInvalid, sfGeneric, sfLedgerEntry, sfTransaction, sfValidation; diff --git a/modules/ripple_data/protocol/ripple_SerializeDeclarations.h b/modules/ripple_data/protocol/ripple_SerializeDeclarations.h index 45593cce70..0730d992bd 100644 --- a/modules/ripple_data/protocol/ripple_SerializeDeclarations.h +++ b/modules/ripple_data/protocol/ripple_SerializeDeclarations.h @@ -1,6 +1,15 @@ // This is not really a header file, but it can be used as one with // appropriate #define statements. +/* + Common type common field - 1 byte + Common type uncommon field - 2 bytes + ... + + Rarity of fields determines the number of bytes. + This is done to reduce the average size of the messages. +*/ + // types (common) TYPE(Int16, UINT16, 1) TYPE(Int32, UINT32, 2) diff --git a/modules/ripple_data/protocol/ripple_SerializedObject.cpp b/modules/ripple_data/protocol/ripple_SerializedObject.cpp index 6be78790b0..f4b7c362c4 100644 --- a/modules/ripple_data/protocol/ripple_SerializedObject.cpp +++ b/modules/ripple_data/protocol/ripple_SerializedObject.cpp @@ -62,6 +62,7 @@ UPTR_T STObject::makeDefaultObject(SerializedTypeID id, SField:: } } +// VFALCO TODO Remove the 'depth' parameter UPTR_T STObject::makeDeserializedObject(SerializedTypeID id, SField::ref name, SerializerIterator& sit, int depth) { @@ -184,7 +185,8 @@ bool STObject::setType(const SOTemplate &type) bool STObject::isValidForType() { boost::ptr_vector::iterator it = mData.begin(); - BOOST_FOREACH(const SOElement* elem, mType->peek()) + + BOOST_FOREACH(const SOElement* elem, mType->peek()) { if (it == mData.end()) return false; @@ -227,7 +229,7 @@ bool STObject::set(SerializerIterator& sit, int depth) */ // return true = terminated with end-of-object -bool STObject::set(SerializerIterator& sit, int depth) +bool STObject::set (SerializerIterator& sit, int depth) { bool reachedEndOfObject = false; diff --git a/modules/ripple_data/protocol/ripple_SerializedObject.h b/modules/ripple_data/protocol/ripple_SerializedObject.h index b687b2d2e6..cd3ce32bc9 100644 --- a/modules/ripple_data/protocol/ripple_SerializedObject.h +++ b/modules/ripple_data/protocol/ripple_SerializedObject.h @@ -38,8 +38,13 @@ public: virtual bool isDefault() const { return mData.empty(); } virtual void add(Serializer& s) const { add(s, true); } // just inner elements - void add(Serializer& s, bool withSignature) const; + + void add(Serializer& s, bool withSignature) const; + + // VFALCO NOTE does this return an expensive copy of an object with a dynamic buffer? + // VFALCO TODO Remove this function and fix the few callers. Serializer getSerializer() const { Serializer s; add(s); return s; } + std::string getFullText() const; std::string getText() const; virtual Json::Value getJson(int options) const; @@ -116,14 +121,24 @@ public: bool delField(SField::ref field); void delField(int index); - static UPTR_T makeDefaultObject(SerializedTypeID id, SField::ref name); - static UPTR_T makeDeserializedObject(SerializedTypeID id, SField::ref name, - SerializerIterator&, int depth); + static UPTR_T makeDefaultObject(SerializedTypeID id, SField::ref name); - static UPTR_T makeNonPresentObject(SField::ref name) - { return makeDefaultObject(STI_NOTPRESENT, name); } - static UPTR_T makeDefaultObject(SField::ref name) - { return makeDefaultObject(name.fieldType, name); } + // VFALCO TODO remove the 'depth' parameter + static UPTR_T makeDeserializedObject ( + SerializedTypeID id, + SField::ref name, + SerializerIterator &, + int depth); + + static UPTR_T makeNonPresentObject (SField::ref name) + { + return makeDefaultObject(STI_NOTPRESENT, name); + } + + static UPTR_T makeDefaultObject (SField::ref name) + { + return makeDefaultObject(name.fieldType, name); + } // field iterator stuff typedef boost::ptr_vector::iterator iterator; @@ -148,9 +163,7 @@ private: { mData.swap(data); } }; -// allow ptr_* collections of STObject's -inline STObject* new_clone(const STObject& s) { return s.oClone().release(); } -inline void delete_clone(const STObject* s) { boost::checked_delete(s); } +//------------------------------------------------------------------------------ inline STObject::iterator range_begin(STObject& x) { return x.begin(); } inline STObject::iterator range_end(STObject &x) { return x.end(); } diff --git a/modules/ripple_data/protocol/ripple_SerializedObjectTemplate.h b/modules/ripple_data/protocol/ripple_SerializedObjectTemplate.h index 23eac3f935..5b3bab9f41 100644 --- a/modules/ripple_data/protocol/ripple_SerializedObjectTemplate.h +++ b/modules/ripple_data/protocol/ripple_SerializedObjectTemplate.h @@ -50,6 +50,9 @@ public: */ SOTemplate (); + // VFALCO NOTE Why do we even bother with the 'private' keyword if + // this function is present? + // std::vector const& peek() const { return mTypes; @@ -64,8 +67,9 @@ public: int getIndex (SField::ref) const; private: - std::vector mTypes; - std::vector mIndex; // field num -> index + std::vector mTypes; + + std::vector mIndex; // field num -> index }; #endif diff --git a/newcoin.vcxproj b/newcoin.vcxproj index b56489ad5b..67d836442c 100644 --- a/newcoin.vcxproj +++ b/newcoin.vcxproj @@ -1838,6 +1838,7 @@ + diff --git a/newcoin.vcxproj.filters b/newcoin.vcxproj.filters index e9835473e8..9a48bf5bdd 100644 --- a/newcoin.vcxproj.filters +++ b/newcoin.vcxproj.filters @@ -1606,5 +1606,6 @@ + \ No newline at end of file diff --git a/src/cpp/ripple/LedgerConsensus.h b/src/cpp/ripple/LedgerConsensus.h index 17fb899735..f21683c4df 100644 --- a/src/cpp/ripple/LedgerConsensus.h +++ b/src/cpp/ripple/LedgerConsensus.h @@ -8,6 +8,7 @@ DEFINE_INSTANCE(LedgerConsensus); DEFINE_INSTANCE(TransactionAcquire); +// VFALCO TODO rename to PeerTxRequest // A transaction set we are trying to acquire class TransactionAcquire : private IS_INSTANCE (TransactionAcquire) @@ -18,7 +19,7 @@ public: typedef boost::shared_ptr pointer; public: - TransactionAcquire(uint256 const& hash); + explicit TransactionAcquire(uint256 const& hash); virtual ~TransactionAcquire() { ; } SHAMap::ref getMap() { return mMap; } diff --git a/src/cpp/ripple/LedgerMaster.cpp b/src/cpp/ripple/LedgerMaster.cpp index ecc6b3d0f9..ad5e22a276 100644 --- a/src/cpp/ripple/LedgerMaster.cpp +++ b/src/cpp/ripple/LedgerMaster.cpp @@ -132,7 +132,9 @@ Ledger::pointer LedgerMaster::closeLedger(bool recover) } } CondLog (recovers != 0, lsINFO, LedgerMaster) << "Recovered " << recovers << " held transactions"; - mHeldTransactions.reset(closingLedger->getHash()); + + // VFALCO TODO recreate the CanonicalTxSet object instead of resetting it + mHeldTransactions.reset(closingLedger->getHash()); } mCurrentLedger = boost::make_shared(boost::ref(*closingLedger), true); diff --git a/src/cpp/ripple/LedgerMaster.h b/src/cpp/ripple/LedgerMaster.h index 96aa89edab..b03b360fbd 100644 --- a/src/cpp/ripple/LedgerMaster.h +++ b/src/cpp/ripple/LedgerMaster.h @@ -14,10 +14,17 @@ public: typedef FUNCTION_TYPE callback; public: - LedgerMaster() : mHeldTransactions(uint256()), mMissingSeq(0), - mMinValidations(0), mLastValidateSeq(0), mPubThread(false), - mPathFindThread(false), mPathFindNewLedger(false), mPathFindNewRequest(false) - { ; } + LedgerMaster () + : mHeldTransactions (uint256()) + , mMissingSeq (0) + , mMinValidations (0) + , mLastValidateSeq (0) + , mPubThread (false) + , mPathFindThread (false) + , mPathFindNewLedger (false) + , mPathFindNewRequest (false) + { + } uint32 getCurrentLedgerIndex(); @@ -133,6 +140,7 @@ private: void pubThread(); void updatePaths(); +private: boost::recursive_mutex mLock; TransactionEngine mEngine; diff --git a/src/cpp/ripple/ripple_AcceptedLedger.h b/src/cpp/ripple/ripple_AcceptedLedger.h index d723bdd5fd..04a9d76082 100644 --- a/src/cpp/ripple/ripple_AcceptedLedger.h +++ b/src/cpp/ripple/ripple_AcceptedLedger.h @@ -2,7 +2,12 @@ #define ACCEPTED_LEDGER_H /** - + + An accepted ledger is a ledger that has a sufficient number of + validations to convince the local server that it is irrevocable. + + The existence of an accepted ledger implies all preceding ledgers + are accepted. */ class AcceptedLedger { diff --git a/src/cpp/ripple/ripple_AcceptedLedgerTx.h b/src/cpp/ripple/ripple_AcceptedLedgerTx.h index a7a29e5ea9..a62df63b8e 100644 --- a/src/cpp/ripple/ripple_AcceptedLedgerTx.h +++ b/src/cpp/ripple/ripple_AcceptedLedgerTx.h @@ -1,12 +1,20 @@ #ifndef RIPPLE_ACCEPTEDLEDGERTX_H #define RIPPLE_ACCEPTEDLEDGERTX_H -/*============================================================================*/ +//------------------------------------------------------------------------------ + /** A transaction that is in a closed ledger. Description + An accepted ledger transaction contains additional information that the + server needs to tell clients about the transaction. For example, + - The transaction in JSON form + - Which accounts are affected + * This is used by InfoSub to report to clients + - Cached stuff + @code @endcode @@ -17,7 +25,7 @@ class AcceptedLedgerTx { public: - typedef boost::shared_ptr pointer; + typedef boost::shared_ptr pointer; typedef const pointer& ref; public: diff --git a/src/cpp/ripple/ripple_CanonicalTXSet.h b/src/cpp/ripple/ripple_CanonicalTXSet.h index 9e89ec7ea3..4b4bcc3710 100644 --- a/src/cpp/ripple/ripple_CanonicalTXSet.h +++ b/src/cpp/ripple/ripple_CanonicalTXSet.h @@ -1,6 +1,14 @@ #ifndef RIPPLE_CANONICALTXSET_H #define RIPPLE_CANONICALTXSET_H +/** Holds transactions which were deferred to the next pass of consensus. + + "Canonical" refers to the order in which transactions are applied. + + - Puts transactions from the same account in sequence order + +*/ +// VFALCO TODO rename to SortedTxSet class CanonicalTXSet { public: @@ -25,7 +33,8 @@ public: uint256 const& getTXID() const { return mTXid; } private: - uint256 mAccount, mTXid; + uint256 mAccount; + uint256 mTXid; uint32 mSeq; }; @@ -33,13 +42,14 @@ public: typedef std::map ::const_iterator const_iterator; public: - CanonicalTXSet (LedgerHash const& lastClosedLedgerHash) + explicit CanonicalTXSet (LedgerHash const& lastClosedLedgerHash) : mSetHash (lastClosedLedgerHash) { } void push_back (SerializedTransaction::ref txn); + // VFALCO TODO remove this function void reset (LedgerHash const& newLastClosedLedgerHash) { mSetHash = newLastClosedLedgerHash; @@ -57,6 +67,7 @@ public: bool empty() const { return mMap.empty(); } private: + // Used to salt the accounts so people can't mine for low account numbers uint256 mSetHash; std::map mMap; diff --git a/src/cpp/ripple/ripple_LedgerAcquire.h b/src/cpp/ripple/ripple_LedgerAcquire.h index 2755d1200f..31c5df4768 100644 --- a/src/cpp/ripple/ripple_LedgerAcquire.h +++ b/src/cpp/ripple/ripple_LedgerAcquire.h @@ -3,7 +3,7 @@ DEFINE_INSTANCE(LedgerAcquire); -// VFALCO TODO Rename to IncomingLedger +// VFALCO TODO Rename to InboundLedger // A ledger we are trying to acquire class LedgerAcquire : private IS_INSTANCE(LedgerAcquire) , public PeerSet diff --git a/src/cpp/ripple/ripple_LedgerAcquireMaster.h b/src/cpp/ripple/ripple_LedgerAcquireMaster.h index c20dbfd4d1..0a21bf460c 100644 --- a/src/cpp/ripple/ripple_LedgerAcquireMaster.h +++ b/src/cpp/ripple/ripple_LedgerAcquireMaster.h @@ -1,9 +1,9 @@ #ifndef RIPPLE_LEDGERACQUIREMASTER_H #define RIPPLE_LEDGERACQUIREMASTER_H -/** Manages the acquisition of ledgers. +/** Manages the lifetime of inbound ledgers. */ -// VFALCO TODO Rename to IncomingLedgerManager +// VFALCO TODO Rename to InboundLedgerManager // VFALCO TODO Create abstract interface class LedgerAcquireMaster { diff --git a/src/cpp/ripple/ripple_SHAMap.h b/src/cpp/ripple/ripple_SHAMap.h index a12d9c6d89..5937ce612d 100644 --- a/src/cpp/ripple/ripple_SHAMap.h +++ b/src/cpp/ripple/ripple_SHAMap.h @@ -24,8 +24,8 @@ public: public: // build new map - SHAMap(SHAMapType t, uint32 seq = 1); - SHAMap(SHAMapType t, uint256 const& hash); + explicit SHAMap (SHAMapType t, uint32 seq = 1); + SHAMap (SHAMapType t, uint256 const& hash); ~SHAMap() { mState = smsInvalid; }