From 19b518a0af716317046ef3ff02834e1103cac16e Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 12 Oct 2012 17:01:37 -0700 Subject: [PATCH 1/6] Quick fix for the bug that was causing ledgers to diverge. LES used an unordered map, causing the traverse of modified ledger nodes to be in random order. This meant different nodes would thread transactions differently, causing ledger divergence. This change switches the LES code to use a standard map. This adds more overhead to LES search functions (because ordered map operations like search and insert are more expensive than unordered map opreations, so it may be worth a separate ordering step just for calcRawMeta instead. --- src/LedgerConsensus.cpp | 2 +- src/LedgerEntrySet.cpp | 24 +++++++++++++----------- src/LedgerEntrySet.h | 14 +++++++------- src/TransactionEngine.cpp | 2 +- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/LedgerConsensus.cpp b/src/LedgerConsensus.cpp index 624e8febf7..e8e9e06850 100644 --- a/src/LedgerConsensus.cpp +++ b/src/LedgerConsensus.cpp @@ -18,7 +18,7 @@ #define TRUST_NETWORK -// #define LC_DEBUG +#define LC_DEBUG typedef std::pair u160_prop_pair; typedef std::pair u256_lct_pair; diff --git a/src/LedgerEntrySet.cpp b/src/LedgerEntrySet.cpp index d59b920d0b..b1b4ed750c 100644 --- a/src/LedgerEntrySet.cpp +++ b/src/LedgerEntrySet.cpp @@ -51,7 +51,7 @@ void LedgerEntrySet::swapWith(LedgerEntrySet& e) // This is basically: copy-on-read. SLE::pointer LedgerEntrySet::getEntry(const uint256& index, LedgerEntryAction& action) { - boost::unordered_map::iterator it = mEntries.find(index); + std::map::iterator it = mEntries.find(index); if (it == mEntries.end()) { action = taaNONE; @@ -98,7 +98,7 @@ SLE::pointer LedgerEntrySet::entryCache(LedgerEntryType letType, const uint256& LedgerEntryAction LedgerEntrySet::hasEntry(const uint256& index) const { - boost::unordered_map::const_iterator it = mEntries.find(index); + std::map::const_iterator it = mEntries.find(index); if (it == mEntries.end()) return taaNONE; return it->second.mAction; @@ -106,7 +106,7 @@ LedgerEntryAction LedgerEntrySet::hasEntry(const uint256& index) const void LedgerEntrySet::entryCache(SLE::ref sle) { - boost::unordered_map::iterator it = mEntries.find(sle->getIndex()); + std::map::iterator it = mEntries.find(sle->getIndex()); if (it == mEntries.end()) { mEntries.insert(std::make_pair(sle->getIndex(), LedgerEntrySetEntry(sle, taaCACHED, mSeq))); @@ -127,7 +127,7 @@ void LedgerEntrySet::entryCache(SLE::ref sle) void LedgerEntrySet::entryCreate(SLE::ref sle) { - boost::unordered_map::iterator it = mEntries.find(sle->getIndex()); + std::map::iterator it = mEntries.find(sle->getIndex()); if (it == mEntries.end()) { mEntries.insert(std::make_pair(sle->getIndex(), LedgerEntrySetEntry(sle, taaCREATE, mSeq))); @@ -157,7 +157,7 @@ void LedgerEntrySet::entryCreate(SLE::ref sle) void LedgerEntrySet::entryModify(SLE::ref sle) { - boost::unordered_map::iterator it = mEntries.find(sle->getIndex()); + std::map::iterator it = mEntries.find(sle->getIndex()); if (it == mEntries.end()) { mEntries.insert(std::make_pair(sle->getIndex(), LedgerEntrySetEntry(sle, taaMODIFY, mSeq))); @@ -192,7 +192,7 @@ void LedgerEntrySet::entryModify(SLE::ref sle) void LedgerEntrySet::entryDelete(SLE::ref sle) { - boost::unordered_map::iterator it = mEntries.find(sle->getIndex()); + std::map::iterator it = mEntries.find(sle->getIndex()); if (it == mEntries.end()) { mEntries.insert(std::make_pair(sle->getIndex(), LedgerEntrySetEntry(sle, taaDELETE, mSeq))); @@ -233,7 +233,7 @@ Json::Value LedgerEntrySet::getJson(int) const Json::Value ret(Json::objectValue); Json::Value nodes(Json::arrayValue); - for (boost::unordered_map::const_iterator it = mEntries.begin(), + for (std::map::const_iterator it = mEntries.begin(), end = mEntries.end(); it != end; ++it) { Json::Value entry(Json::objectValue); @@ -269,7 +269,7 @@ Json::Value LedgerEntrySet::getJson(int) const SLE::pointer LedgerEntrySet::getForMod(const uint256& node, Ledger::ref ledger, boost::unordered_map& newMods) { - boost::unordered_map::iterator it = mEntries.find(node); + std::map::iterator it = mEntries.find(node); if (it != mEntries.end()) { if (it->second.mAction == taaDELETE) @@ -351,7 +351,7 @@ void LedgerEntrySet::calcRawMeta(Serializer& s) // Entries modified only as a result of building the transaction metadata boost::unordered_map newMod; - for (boost::unordered_map::const_iterator it = mEntries.begin(), + for (std::map::const_iterator it = mEntries.begin(), end = mEntries.end(); it != end; ++it) { int nType = TMNEndOfMetadata; @@ -410,8 +410,10 @@ void LedgerEntrySet::calcRawMeta(Serializer& s) if (origNode->getType() == ltRIPPLE_STATE) { - metaNode.addAccount(TMSLowID, NewcoinAddress::createAccountID(origNode->getFieldAmount(sfLowLimit).getIssuer())); - metaNode.addAccount(TMSHighID, NewcoinAddress::createAccountID(origNode->getFieldAmount(sfHighLimit).getIssuer())); + metaNode.addAccount(TMSLowID, + NewcoinAddress::createAccountID(origNode->getFieldAmount(sfLowLimit).getIssuer())); + metaNode.addAccount(TMSHighID, + NewcoinAddress::createAccountID(origNode->getFieldAmount(sfHighLimit).getIssuer())); } } diff --git a/src/LedgerEntrySet.h b/src/LedgerEntrySet.h index be44b0e973..11192c363d 100644 --- a/src/LedgerEntrySet.h +++ b/src/LedgerEntrySet.h @@ -32,11 +32,11 @@ class LedgerEntrySet { protected: Ledger::pointer mLedger; - boost::unordered_map mEntries; + std::map mEntries; // cannot be unordered! TransactionMetaSet mSet; int mSeq; - LedgerEntrySet(Ledger::ref ledger, const boost::unordered_map &e, + LedgerEntrySet(Ledger::ref ledger, const std::map &e, const TransactionMetaSet& s, int m) : mLedger(ledger), mEntries(e), mSet(s), mSeq(m) { ; } SLE::pointer getForMod(const uint256& node, Ledger::ref ledger, @@ -123,11 +123,11 @@ public: void calcRawMeta(Serializer&); // iterator functions - bool isEmpty() const { return mEntries.empty(); } - boost::unordered_map::const_iterator begin() const { return mEntries.begin(); } - boost::unordered_map::const_iterator end() const { return mEntries.end(); } - boost::unordered_map::iterator begin() { return mEntries.begin(); } - boost::unordered_map::iterator end() { return mEntries.end(); } + bool isEmpty() const { return mEntries.empty(); } + std::map::const_iterator begin() const { return mEntries.begin(); } + std::map::const_iterator end() const { return mEntries.end(); } + std::map::iterator begin() { return mEntries.begin(); } + std::map::iterator end() { return mEntries.end(); } static bool intersect(const LedgerEntrySet& lesLeft, const LedgerEntrySet& lesRight); }; diff --git a/src/TransactionEngine.cpp b/src/TransactionEngine.cpp index 38dbeb5610..36712c6a30 100644 --- a/src/TransactionEngine.cpp +++ b/src/TransactionEngine.cpp @@ -18,7 +18,7 @@ SETUP_LOG(); void TransactionEngine::txnWrite() { // Write back the account states - for (boost::unordered_map::iterator it = mNodes.begin(), end = mNodes.end(); + for (std::map::iterator it = mNodes.begin(), end = mNodes.end(); it != end; ++it) { const SLE::pointer& sleEntry = it->second.mEntry; From bfee7a30828eec866ef22880ec20b8653f65fa4f Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 12 Oct 2012 18:24:32 -0700 Subject: [PATCH 2/6] I apologize for the disaster that was toUInt64. It has been nuked from orbit. --- src/Amount.cpp | 45 ++++++++++++++++++++++++++----------------- src/SerializedTypes.h | 2 +- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/Amount.cpp b/src/Amount.cpp index ae518f8adf..9a12ea7e29 100644 --- a/src/Amount.cpp +++ b/src/Amount.cpp @@ -378,7 +378,7 @@ bool STAmount::setFullValue(const std::string& sAmount, const std::string& sCurr void STAmount::canonicalize() { - if (!mCurrency) + if (mCurrency.isZero()) { // native currency amounts should always have an offset of zero mIsNative = true; @@ -401,7 +401,11 @@ void STAmount::canonicalize() --mOffset; } - assert(mValue <= cMaxNative); + if (mValue > cMaxNative) + { + assert(false); + throw std::runtime_error("Native currency amount out of range"); + } return; } @@ -430,9 +434,9 @@ void STAmount::canonicalize() mValue /= 10; ++mOffset; } - assert((mValue == 0) || ((mValue >= cMinValue) && (mValue <= cMaxValue)) ); - assert((mValue == 0) || ((mOffset >= cMinOffset) && (mOffset <= cMaxOffset)) ); - assert((mValue != 0) || (mOffset != -100) ); + assert((mValue == 0) || ((mValue >= cMinValue) && (mValue <= cMaxValue))); + assert((mValue == 0) || ((mOffset >= cMinOffset) && (mOffset <= cMaxOffset))); + assert((mValue != 0) || (mOffset != -100)); } void STAmount::add(Serializer& s) const @@ -476,15 +480,20 @@ void STAmount::setValue(const STAmount &a) mIsNegative = a.mIsNegative; } -uint64 STAmount::toUInt64() const -{ // makes them sort easily - if (mIsNative) - return mValue; - if (mValue == 0) - return 0x4000000000000000ull; - if (mIsNegative) - return ((cMaxNative + 1) - mValue) | (static_cast(mOffset + 97) << (64 - 10)); - return mValue | (static_cast(mOffset + 256 + 97) << (64 - 10)); +int STAmount::compare(const STAmount& a) const +{ // Compares the value of a to the value of this STAmount, amounts must be comparable + if (mIsNegative != a.mIsNegative) return mIsNegative ? -1 : 1; + + if (!mValue) return a.mValue ? 1 : 0; + if (!a.mValue) return 1; + + if (mOffset > a.mOffset) return mIsNegative ? 1 : -1; + if (mOffset < a.mOffset) return mIsNegative ? -1 : 1; + + if (mValue > a.mValue) return mIsNegative ? 1 : -1; + if (mValue < a.mValue) return mIsNegative ? -1 : 1; + + return 0; } STAmount* STAmount::construct(SerializerIterator& sit, SField::ref name) @@ -633,25 +642,25 @@ bool STAmount::operator!=(const STAmount& a) const bool STAmount::operator<(const STAmount& a) const { throwComparable(a); - return toUInt64() < a.toUInt64(); + return compare(a) < 0; } bool STAmount::operator>(const STAmount& a) const { throwComparable(a); - return toUInt64() > a.toUInt64(); + return compare(a) > 0; } bool STAmount::operator<=(const STAmount& a) const { throwComparable(a); - return toUInt64() <= a.toUInt64(); + return compare(a) <= 0; } bool STAmount::operator>=(const STAmount& a) const { throwComparable(a); - return toUInt64() >= a.toUInt64(); + return compare(a) >= 0; } STAmount& STAmount::operator+=(const STAmount& a) diff --git a/src/SerializedTypes.h b/src/SerializedTypes.h index 90438442a8..d7df1d1b71 100644 --- a/src/SerializedTypes.h +++ b/src/SerializedTypes.h @@ -224,7 +224,6 @@ protected: : SerializedType(name), mCurrency(cur), mIssuer(iss), mValue(val), mOffset(off), mIsNative(isNat), mIsNegative(isNeg) { ; } - uint64 toUInt64() const; static uint64 muldiv(uint64, uint64, uint64); public: @@ -287,6 +286,7 @@ public: void negate() { if (!isZero()) mIsNegative = !mIsNegative; } void zero() { mOffset = mIsNative ? -100 : 0; mValue = 0; mIsNegative = false; } + int compare(const STAmount&) const; const uint160& getIssuer() const { return mIssuer; } void setIssuer(const uint160& uIssuer) { mIssuer = uIssuer; } From 08acbcd839cd0a412f9c0a0828fb476364e86c23 Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 12 Oct 2012 19:58:41 -0700 Subject: [PATCH 3/6] Fix the sense. --- src/Amount.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Amount.cpp b/src/Amount.cpp index 9a12ea7e29..f564e071fd 100644 --- a/src/Amount.cpp +++ b/src/Amount.cpp @@ -487,11 +487,11 @@ int STAmount::compare(const STAmount& a) const if (!mValue) return a.mValue ? 1 : 0; if (!a.mValue) return 1; - if (mOffset > a.mOffset) return mIsNegative ? 1 : -1; - if (mOffset < a.mOffset) return mIsNegative ? -1 : 1; + if (mOffset > a.mOffset) return mIsNegative ? -1 : 1; + if (mOffset < a.mOffset) return mIsNegative ? 1 : -1; - if (mValue > a.mValue) return mIsNegative ? 1 : -1; - if (mValue < a.mValue) return mIsNegative ? -1 : 1; + if (mValue > a.mValue) return mIsNegative ? -1 : 1; + if (mValue < a.mValue) return mIsNegative ? 1 : -1; return 0; } From 6fc5dc11895a42cc363a91a150213245b94ed9cc Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 12 Oct 2012 20:19:28 -0700 Subject: [PATCH 4/6] Spelling error. --- src/SHAMapSync.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SHAMapSync.cpp b/src/SHAMapSync.cpp index 7fdfac494c..028a5db1c8 100644 --- a/src/SHAMapSync.cpp +++ b/src/SHAMapSync.cpp @@ -415,7 +415,7 @@ BOOST_AUTO_TEST_SUITE( SHAMapSync ) BOOST_AUTO_TEST_CASE( SHAMapSync_test ) { - cLog(lsTRACE) << "being sync test"; + cLog(lsTRACE) << "begin sync test"; unsigned int seed; RAND_pseudo_bytes(reinterpret_cast(&seed), sizeof(seed)); srand(seed); From 730c6f3a5284e74c254a31a34f47040bc59cb2db Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 12 Oct 2012 20:19:36 -0700 Subject: [PATCH 5/6] Bugfix in STAmount::compare --- src/Amount.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Amount.cpp b/src/Amount.cpp index f564e071fd..7b9252e8fd 100644 --- a/src/Amount.cpp +++ b/src/Amount.cpp @@ -484,7 +484,11 @@ int STAmount::compare(const STAmount& a) const { // Compares the value of a to the value of this STAmount, amounts must be comparable if (mIsNegative != a.mIsNegative) return mIsNegative ? -1 : 1; - if (!mValue) return a.mValue ? 1 : 0; + if (!mValue) + { + if (a.mIsNegative) return 1; + return a.mValue ? -1 : 0; + } if (!a.mValue) return 1; if (mOffset > a.mOffset) return mIsNegative ? -1 : 1; From 9b2db30226809f42851297ddc0ebe0cba6eb1769 Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 12 Oct 2012 20:32:19 -0700 Subject: [PATCH 6/6] Cosmetic changes newcoin->ripple. --- rippled.cfg | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/rippled.cfg b/rippled.cfg index 6a3e97437f..c86e2324b3 100644 --- a/rippled.cfg +++ b/rippled.cfg @@ -1,11 +1,11 @@ # -# Sample newcoind.cfg +# Sample rippled.cfg # -# This file should be named newcoind.cfg. This file is UTF-8 with Dos, UNIX, +# This file should be named rippled.cfg. This file is UTF-8 with Dos, UNIX, # or Mac style end of lines. Blank lines and lines beginning with '#' are # ignored. Undefined sections are reserved. No escapes are currently defined. # -# When you launch newcoind, it will attempt to find this file. For details, +# When you launch rippled, it will attempt to find this file. For details, # refer to the manual page for --conf command line option. # # [debug_logfile] @@ -17,33 +17,33 @@ # Specifies where to find validators.txt for UNL boostrapping and RPC command unl_network. # During alpha testing, this defaults to: redstem.com # -# Example: newcoin.org +# Example: ripple.com # # [unl_default]: # XXX This should be called: [validators_file] # Specifies how to bootstrap the UNL list. The UNL list is based on a -# validators.txt file and is maintained in the databases. When newcoind +# validators.txt file and is maintained in the databases. When rippled # starts up, if the databases are missing or are obsolete due to an upgrade -# of newcoind, newcoind will reconstruct the UNL list as specified here. +# of rippled, rippled will reconstruct the UNL list as specified here. # -# If this entry is not present or empty, newcoind will look for a validators.txt in the +# If this entry is not present or empty, rippled will look for a validators.txt in the # config directory. If not found there, it will attempt to retrieve the file -# from the newcoin foundation's web site. +# from the Ripple foundation's web site. # # This entry is also used by the RPC command unl_load. # # Specify the file by specifying its full path. # # Examples: -# C:/home/johndoe/newcoin/validators.txt -# /home/johndoe/newcoin/validators.txt +# C:/home/johndoe/ripple/validators.txt +# /home/johndoe/ripple/validators.txt # # [validators]: -# Only valid in "newcoind.cfg", "newcoin.txt", and the referered [validators_url]. +# Only valid in "rippled.cfg", "ripple.txt", and the referered [validators_url]. # List of nodes to accept as validators speficied by public key or domain. # -# For domains, newcoind will probe for https web servers at the specied -# domain in the following order: newcoin.DOMAIN, www.DOMAIN, DOMAIN +# For domains, rippled will probe for https web servers at the specied +# domain in the following order: ripple.DOMAIN, www.DOMAIN, DOMAIN # # Examples: # redstem.com @@ -51,7 +51,7 @@ # n9MqiExBcoG19UXwoLjBJnhsxEhAZMuWwJDRdkyDz1EkEkwzQTNt John Doe # # [ips]: -# Only valid in "newcoind.cfg", "newcoin.txt", and the referered [ips_url]. +# Only valid in "rippled.cfg", "ripple.txt", and the referered [ips_url]. # List of ips where the Newcoin protocol is avialable. # One ipv4 or ipv6 address per line. # A port may optionally be specified after adding a space to the address.