From b9903bbcc483a384decf8d2665f559d123baaba2 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 31 Dec 2021 21:31:37 -0800 Subject: [PATCH] Simplify and improve order book tracking: - Avoid using std::shared_ptr - Prefer using unordered maps to avoid linear searches - Increase the interval between full order book updates --- src/ripple/app/ledger/OrderBookDB.cpp | 257 ++++++++++++-------------- src/ripple/app/ledger/OrderBookDB.h | 21 +-- src/ripple/app/misc/NetworkOPs.cpp | 3 +- src/ripple/app/misc/OrderBook.h | 89 --------- src/ripple/app/paths/Pathfinder.cpp | 38 ++-- 5 files changed, 143 insertions(+), 265 deletions(-) delete mode 100644 src/ripple/app/misc/OrderBook.h diff --git a/src/ripple/app/ledger/OrderBookDB.cpp b/src/ripple/app/ledger/OrderBookDB.cpp index b9f72b71523..28ff53f0f81 100644 --- a/src/ripple/app/ledger/OrderBookDB.cpp +++ b/src/ripple/app/ledger/OrderBookDB.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -28,70 +29,72 @@ namespace ripple { OrderBookDB::OrderBookDB(Application& app) - : app_(app), mSeq(0), j_(app.journal("OrderBookDB")) + : app_(app), seq_(0), j_(app.journal("OrderBookDB")) { } -void -OrderBookDB::invalidate() -{ - std::lock_guard sl(mLock); - mSeq = 0; -} - void OrderBookDB::setup(std::shared_ptr const& ledger) { + if (!app_.config().standalone() && app_.getOPs().isNeedNetworkLedger()) { - std::lock_guard sl(mLock); - auto seq = ledger->info().seq; + JLOG(j_.warn()) << "Eliding full order book update: no ledger"; + return; + } - // Do a full update every 256 ledgers - if (mSeq != 0) - { - if (seq == mSeq) - return; - if ((seq > mSeq) && ((seq - mSeq) < 256)) - return; - if ((seq < mSeq) && ((mSeq - seq) < 16)) - return; - } + auto seq = seq_.load(); - JLOG(j_.debug()) << "Advancing from " << mSeq << " to " << seq; + if (seq != 0) + { + if ((seq > ledger->seq()) && ((ledger->seq() - seq) < 25600)) + return; - mSeq = seq; + if ((ledger->seq() <= seq) && ((seq - ledger->seq()) < 16)) + return; } + if (seq_.exchange(ledger->seq()) != seq) + return; + + JLOG(j_.debug()) << "Full order book update: " << seq << " to " + << ledger->seq(); + if (app_.config().PATH_SEARCH_MAX != 0) { if (app_.config().standalone()) update(ledger); else app_.getJobQueue().addJob( - jtUPDATE_PF, "OrderBookDB::update", [this, ledger]() { - update(ledger); - }); + jtUPDATE_PF, + "OrderBookDB::update: " + std::to_string(ledger->seq()), + [this, ledger]() { update(ledger); }); } } void OrderBookDB::update(std::shared_ptr const& ledger) { - hash_set seen; - OrderBookDB::IssueToOrderBook destMap; - OrderBookDB::IssueToOrderBook sourceMap; - hash_set XRPBooks; - - JLOG(j_.debug()) << "OrderBookDB::update>"; - if (app_.config().PATH_SEARCH_MAX == 0) + return; // pathfinding has been disabled + + // A newer full update job is pending + if (auto const seq = seq_.load(); seq > ledger->seq()) { - // pathfinding has been disabled + JLOG(j_.debug()) << "Eliding update for " << ledger->seq() + << " because of pending update to later " << seq; return; } + decltype(allBooks_) allBooks; + decltype(xrpBooks_) xrpBooks; + + allBooks.reserve(allBooks_.size()); + xrpBooks.reserve(xrpBooks_.size()); + + JLOG(j_.debug()) << "Beginning update (" << ledger->seq() << ")"; + // walk through the entire ledger looking for orderbook entries - int books = 0; + int cnt = 0; try { @@ -100,9 +103,8 @@ OrderBookDB::update(std::shared_ptr const& ledger) if (app_.isStopping()) { JLOG(j_.info()) - << "OrderBookDB::update exiting due to isStopping"; - std::lock_guard sl(mLock); - mSeq = 0; + << "Update halted because the process is stopping"; + seq_.store(0); return; } @@ -111,40 +113,38 @@ OrderBookDB::update(std::shared_ptr const& ledger) sle->getFieldH256(sfRootIndex) == sle->key()) { Book book; + book.in.currency = sle->getFieldH160(sfTakerPaysCurrency); book.in.account = sle->getFieldH160(sfTakerPaysIssuer); - book.out.account = sle->getFieldH160(sfTakerGetsIssuer); book.out.currency = sle->getFieldH160(sfTakerGetsCurrency); + book.out.account = sle->getFieldH160(sfTakerGetsIssuer); + + allBooks[book.in].insert(book.out); + + if (isXRP(book.out)) + xrpBooks.insert(book.in); - uint256 index = getBookBase(book); - if (seen.insert(index).second) - { - auto orderBook = std::make_shared(index, book); - sourceMap[book.in].push_back(orderBook); - destMap[book.out].push_back(orderBook); - if (isXRP(book.out)) - XRPBooks.insert(book.in); - ++books; - } + ++cnt; } } } catch (SHAMapMissingNode const& mn) { - JLOG(j_.info()) << "OrderBookDB::update: " << mn.what(); - std::lock_guard sl(mLock); - mSeq = 0; + JLOG(j_.info()) << "Missing node in " << ledger->seq() + << " during update: " << mn.what(); + seq_.store(0); return; } - JLOG(j_.debug()) << "OrderBookDB::update< " << books << " books found"; + JLOG(j_.debug()) << "Update completed (" << ledger->seq() << "): " << cnt + << " books found"; + { std::lock_guard sl(mLock); - - mXRPBooks.swap(XRPBooks); - mSourceMap.swap(sourceMap); - mDestMap.swap(destMap); + allBooks_.swap(allBooks); + xrpBooks_.swap(xrpBooks); } + app_.getLedgerMaster().newOrderBookDB(); } @@ -152,60 +152,50 @@ void OrderBookDB::addOrderBook(Book const& book) { bool toXRP = isXRP(book.out); + std::lock_guard sl(mLock); - if (toXRP) - { - // We don't want to search through all the to-XRP or from-XRP order - // books! - for (auto ob : mSourceMap[book.in]) - { - if (isXRP(ob->getCurrencyOut())) // also to XRP - return; - } - } - else - { - for (auto ob : mDestMap[book.out]) - { - if (ob->getCurrencyIn() == book.in.currency && - ob->getIssuerIn() == book.in.account) - { - return; - } - } - } - uint256 index = getBookBase(book); - auto orderBook = std::make_shared(index, book); + allBooks_[book.in].insert(book.out); - mSourceMap[book.in].push_back(orderBook); - mDestMap[book.out].push_back(orderBook); if (toXRP) - mXRPBooks.insert(book.in); + xrpBooks_.insert(book.in); } // return list of all orderbooks that want this issuerID and currencyID -OrderBook::List +std::vector OrderBookDB::getBooksByTakerPays(Issue const& issue) { - std::lock_guard sl(mLock); - auto it = mSourceMap.find(issue); - return it == mSourceMap.end() ? OrderBook::List() : it->second; + std::vector ret; + + { + std::lock_guard sl(mLock); + + if (auto it = allBooks_.find(issue); it != allBooks_.end()) + { + ret.reserve(it->second.size()); + + for (auto const& gets : it->second) + ret.push_back(Book(issue, gets)); + } + } + + return ret; } int OrderBookDB::getBookSize(Issue const& issue) { std::lock_guard sl(mLock); - auto it = mSourceMap.find(issue); - return it == mSourceMap.end() ? 0 : it->second.size(); + if (auto it = allBooks_.find(issue); it != allBooks_.end()) + return static_cast(it->second.size()); + return 0; } bool OrderBookDB::isBookToXRP(Issue const& issue) { std::lock_guard sl(mLock); - return mXRPBooks.count(issue) > 0; + return xrpBooks_.count(issue) > 0; } BookListeners::pointer @@ -247,63 +237,52 @@ OrderBookDB::processTxn( Json::Value const& jvObj) { std::lock_guard sl(mLock); - if (alTx.getResult() == tesSUCCESS) + + // For this particular transaction, maintain the set of unique + // subscriptions that have already published it. This prevents sending + // the transaction multiple times if it touches multiple ltOFFER + // entries for the same book, or if it touches multiple books and a + // single client has subscribed to those books. + hash_set havePublished; + + // Check if this is an offer or an offer cancel or a payment that + // consumes an offer. + // Check to see what the meta looks like. + for (auto& node : alTx.getMeta()->getNodes()) { - // For this particular transaction, maintain the set of unique - // subscriptions that have already published it. This prevents sending - // the transaction multiple times if it touches multiple ltOFFER - // entries for the same book, or if it touches multiple books and a - // single client has subscribed to those books. - hash_set havePublished; - - // Check if this is an offer or an offer cancel or a payment that - // consumes an offer. - // Check to see what the meta looks like. - for (auto& node : alTx.getMeta()->getNodes()) + try { - try + if (node.getFieldU16(sfLedgerEntryType) == ltOFFER) { - if (node.getFieldU16(sfLedgerEntryType) == ltOFFER) - { - SField const* field = nullptr; - - // We need a field that contains the TakerGets and TakerPays - // parameters. - if (node.getFName() == sfModifiedNode) - field = &sfPreviousFields; - else if (node.getFName() == sfCreatedNode) - field = &sfNewFields; - else if (node.getFName() == sfDeletedNode) - field = &sfFinalFields; - - if (field) + auto process = [&, this](SField const& field) { + if (auto data = dynamic_cast( + node.peekAtPField(field)); + data && data->isFieldPresent(sfTakerPays) && + data->isFieldPresent(sfTakerGets)) { - auto data = dynamic_cast( - node.peekAtPField(*field)); - - if (data && data->isFieldPresent(sfTakerPays) && - data->isFieldPresent(sfTakerGets)) - { - // determine the OrderBook - Book b{ - data->getFieldAmount(sfTakerGets).issue(), - data->getFieldAmount(sfTakerPays).issue()}; - - auto listeners = getBookListeners(b); - if (listeners) - { - listeners->publish(jvObj, havePublished); - } - } + auto listeners = getBookListeners( + {data->getFieldAmount(sfTakerGets).issue(), + data->getFieldAmount(sfTakerPays).issue()}); + if (listeners) + listeners->publish(jvObj, havePublished); } - } - } - catch (std::exception const&) - { - JLOG(j_.info()) - << "Fields not found in OrderBookDB::processTxn"; + }; + + // We need a field that contains the TakerGets and TakerPays + // parameters. + if (node.getFName() == sfModifiedNode) + process(sfPreviousFields); + else if (node.getFName() == sfCreatedNode) + process(sfNewFields); + else if (node.getFName() == sfDeletedNode) + process(sfFinalFields); } } + catch (std::exception const& ex) + { + JLOG(j_.info()) + << "processTxn: field not found (" << ex.what() << ")"; + } } } diff --git a/src/ripple/app/ledger/OrderBookDB.h b/src/ripple/app/ledger/OrderBookDB.h index 3b6939013a3..ea7c60c5f5b 100644 --- a/src/ripple/app/ledger/OrderBookDB.h +++ b/src/ripple/app/ledger/OrderBookDB.h @@ -23,7 +23,6 @@ #include #include #include -#include #include namespace ripple { @@ -37,15 +36,13 @@ class OrderBookDB setup(std::shared_ptr const& ledger); void update(std::shared_ptr const& ledger); - void - invalidate(); void addOrderBook(Book const&); /** @return a list of all orderbooks that want this issuerID and currencyID. */ - OrderBook::List + std::vector getBooksByTakerPays(Issue const&); /** @return a count of all orderbooks that want this issuerID and @@ -68,22 +65,14 @@ class OrderBookDB const AcceptedLedgerTx& alTx, Json::Value const& jvObj); - using IssueToOrderBook = hash_map; - private: - void - rawAddBook(Book const&); - Application& app_; - // by ci/ii - IssueToOrderBook mSourceMap; - - // by co/io - IssueToOrderBook mDestMap; + // Maps order books by "issue in" to "issue out": + hardened_hash_map> allBooks_; // does an order book to XRP exist - hash_set mXRPBooks; + hash_set xrpBooks_; std::recursive_mutex mLock; @@ -91,7 +80,7 @@ class OrderBookDB BookToListenersMap mListeners; - std::uint32_t mSeq; + std::atomic seq_; beast::Journal const j_; }; diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 2b9c5f316bd..1d1493758df 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -3072,7 +3072,8 @@ NetworkOPsImp::pubValidatedTransaction( it = mStreamMaps[sRTTransactions].erase(it); } } - app_.getOrderBookDB().processTxn(alAccepted, alTx, jvObj); + if (alTx.getResult() == tesSUCCESS) + app_.getOrderBookDB().processTxn(alAccepted, alTx, jvObj); pubAccountTransaction(alAccepted, alTx, true); } diff --git a/src/ripple/app/misc/OrderBook.h b/src/ripple/app/misc/OrderBook.h deleted file mode 100644 index ce8e0db9f38..00000000000 --- a/src/ripple/app/misc/OrderBook.h +++ /dev/null @@ -1,89 +0,0 @@ -//------------------------------------------------------------------------------ -/* - This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2012, 2013 Ripple Labs Inc. - - Permission to use, copy, modify, and/or distribute this software for any - purpose with or without fee is hereby granted, provided that the above - copyright notice and this permission notice appear in all copies. - - THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES - WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF - MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR - ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES - WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN - ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF - OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -*/ -//============================================================================== - -#ifndef RIPPLE_APP_MISC_ORDERBOOK_H_INCLUDED -#define RIPPLE_APP_MISC_ORDERBOOK_H_INCLUDED - -#include - -namespace ripple { - -/** Describes a serialized ledger entry for an order book. */ -class OrderBook : public CountedObject -{ -public: - using pointer = std::shared_ptr; - using ref = std::shared_ptr const&; - using List = std::vector; - - /** Construct from a currency specification. - - @param index ??? - @param book in and out currency/issuer pairs. - */ - // VFALCO NOTE what is the meaning of the index parameter? - OrderBook(uint256 const& base, Book const& book) - : mBookBase(base), mBook(book) - { - } - - uint256 const& - getBookBase() const - { - return mBookBase; - } - - Book const& - book() const - { - return mBook; - } - - Currency const& - getCurrencyIn() const - { - return mBook.in.currency; - } - - Currency const& - getCurrencyOut() const - { - return mBook.out.currency; - } - - AccountID const& - getIssuerIn() const - { - return mBook.in.account; - } - - AccountID const& - getIssuerOut() const - { - return mBook.out.account; - } - -private: - uint256 const mBookBase; - Book const mBook; -}; - -} // namespace ripple - -#endif diff --git a/src/ripple/app/paths/Pathfinder.cpp b/src/ripple/app/paths/Pathfinder.cpp index 158077c5708..4e81bebd3c3 100644 --- a/src/ripple/app/paths/Pathfinder.cpp +++ b/src/ripple/app/paths/Pathfinder.cpp @@ -1126,16 +1126,14 @@ Pathfinder::addLink( if (continueCallback && !continueCallback()) return; if (!currentPath.hasSeen( - xrpAccount(), - book->getCurrencyOut(), - book->getIssuerOut()) && - !issueMatchesOrigin(book->book().out) && + xrpAccount(), book.out.currency, book.out.account) && + !issueMatchesOrigin(book.out) && (!bDestOnly || - (book->getCurrencyOut() == mDstAmount.getCurrency()))) + (book.out.currency == mDstAmount.getCurrency()))) { STPath newPath(currentPath); - if (book->getCurrencyOut().isZero()) + if (book.out.currency.isZero()) { // to XRP // add the order book itself @@ -1158,9 +1156,9 @@ Pathfinder::addLink( incompletePaths.push_back(newPath); } else if (!currentPath.hasSeen( - book->getIssuerOut(), - book->getCurrencyOut(), - book->getIssuerOut())) + book.out.account, + book.out.currency, + book.out.account)) { // Don't want the book if we've already seen the issuer // book -> account -> book @@ -1173,8 +1171,8 @@ Pathfinder::addLink( STPathElement::typeCurrency | STPathElement::typeIssuer, xrpAccount(), - book->getCurrencyOut(), - book->getIssuerOut()); + book.out.currency, + book.out.account); } else { @@ -1183,19 +1181,19 @@ Pathfinder::addLink( STPathElement::typeCurrency | STPathElement::typeIssuer, xrpAccount(), - book->getCurrencyOut(), - book->getIssuerOut()); + book.out.currency, + book.out.account); } if (hasEffectiveDestination && - book->getIssuerOut() == mDstAccount && - book->getCurrencyOut() == mDstAmount.getCurrency()) + book.out.account == mDstAccount && + book.out.currency == mDstAmount.getCurrency()) { // We skipped a required issuer } else if ( - book->getIssuerOut() == mEffectiveDst && - book->getCurrencyOut() == mDstAmount.getCurrency()) + book.out.account == mEffectiveDst && + book.out.currency == mDstAmount.getCurrency()) { // with the destination account, this path is // complete JLOG(j_.trace()) @@ -1210,9 +1208,9 @@ Pathfinder::addLink( newPath, STPathElement( STPathElement::typeAccount, - book->getIssuerOut(), - book->getCurrencyOut(), - book->getIssuerOut())); + book.out.account, + book.out.currency, + book.out.account)); } } }