From af36942e1f4364e54c729881de61679a77edbd33 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Thu, 16 Jul 2015 06:28:57 -0700 Subject: [PATCH] Tidy up offer crossing: * Clarify use of cancel view in OfferCreate transactor * Reduce OfferStream public interface * Reduce severity of some developer-only logging from ERROR to DEBUG --- src/ripple/app/ledger/LedgerHistory.cpp | 46 +++++++------- src/ripple/app/tx/impl/CreateOffer.cpp | 10 +-- src/ripple/app/tx/impl/OfferStream.cpp | 83 ++++++++++++------------- src/ripple/app/tx/impl/OfferStream.h | 37 +++-------- 4 files changed, 78 insertions(+), 98 deletions(-) diff --git a/src/ripple/app/ledger/LedgerHistory.cpp b/src/ripple/app/ledger/LedgerHistory.cpp index 436d3bb95..54bc65ea2 100644 --- a/src/ripple/app/ledger/LedgerHistory.cpp +++ b/src/ripple/app/ledger/LedgerHistory.cpp @@ -139,13 +139,13 @@ log_one(Ledger::pointer ledger, uint256 const& tx, char const* msg) if (metaData != nullptr) { - WriteLog (lsERROR, LedgerMaster) << "MISMATCH on TX " << tx << + WriteLog (lsDEBUG, LedgerMaster) << "MISMATCH on TX " << tx << ": " << msg << " is missing this transaction:\n" << metaData->getJson (0); } else { - WriteLog (lsERROR, LedgerMaster) << "MISMATCH on TX " << tx << + WriteLog (lsDEBUG, LedgerMaster) << "MISMATCH on TX " << tx << ": " << msg << " is missing this transaction."; } } @@ -185,31 +185,31 @@ log_metadata_difference(Ledger::pointer builtLedger, Ledger::pointer validLedger { if (result_diff && index_diff) { - WriteLog (lsERROR, LedgerMaster) << "MISMATCH on TX " << tx << + WriteLog (lsDEBUG, LedgerMaster) << "MISMATCH on TX " << tx << ": Different result and index!"; - WriteLog (lsERROR, LedgerMaster) << " Built:" << + WriteLog (lsDEBUG, LedgerMaster) << " Built:" << " Result: " << builtMetaData->getResult () << " Index: " << builtMetaData->getIndex (); - WriteLog (lsERROR, LedgerMaster) << " Valid:" << + WriteLog (lsDEBUG, LedgerMaster) << " Valid:" << " Result: " << validMetaData->getResult () << " Index: " << validMetaData->getIndex (); } else if (result_diff) { - WriteLog (lsERROR, LedgerMaster) << "MISMATCH on TX " << tx << + WriteLog (lsDEBUG, LedgerMaster) << "MISMATCH on TX " << tx << ": Different result!"; - WriteLog (lsERROR, LedgerMaster) << " Built:" << + WriteLog (lsDEBUG, LedgerMaster) << " Built:" << " Result: " << builtMetaData->getResult (); - WriteLog (lsERROR, LedgerMaster) << " Valid:" << + WriteLog (lsDEBUG, LedgerMaster) << " Valid:" << " Result: " << validMetaData->getResult (); } else if (index_diff) { - WriteLog (lsERROR, LedgerMaster) << "MISMATCH on TX " << tx << + WriteLog (lsDEBUG, LedgerMaster) << "MISMATCH on TX " << tx << ": Different index!"; - WriteLog (lsERROR, LedgerMaster) << " Built:" << + WriteLog (lsDEBUG, LedgerMaster) << " Built:" << " Index: " << builtMetaData->getIndex (); - WriteLog (lsERROR, LedgerMaster) << " Valid:" << + WriteLog (lsDEBUG, LedgerMaster) << " Valid:" << " Index: " << validMetaData->getIndex (); } } @@ -217,42 +217,42 @@ log_metadata_difference(Ledger::pointer builtLedger, Ledger::pointer validLedger { if (result_diff && index_diff) { - WriteLog (lsERROR, LedgerMaster) << "MISMATCH on TX " << tx << + WriteLog (lsDEBUG, LedgerMaster) << "MISMATCH on TX " << tx << ": Different result, index and nodes!"; - WriteLog (lsERROR, LedgerMaster) << " Built:\n" << + WriteLog (lsDEBUG, LedgerMaster) << " Built:\n" << builtMetaData->getJson (0); - WriteLog (lsERROR, LedgerMaster) << " Valid:\n" << + WriteLog (lsDEBUG, LedgerMaster) << " Valid:\n" << validMetaData->getJson (0); } else if (result_diff) { - WriteLog (lsERROR, LedgerMaster) << "MISMATCH on TX " << tx << + WriteLog (lsDEBUG, LedgerMaster) << "MISMATCH on TX " << tx << ": Different result and nodes!"; - WriteLog (lsERROR, LedgerMaster) << " Built:" << + WriteLog (lsDEBUG, LedgerMaster) << " Built:" << " Result: " << builtMetaData->getResult () << " Nodes:\n" << builtNodes.getJson (0); - WriteLog (lsERROR, LedgerMaster) << " Valid:" << + WriteLog (lsDEBUG, LedgerMaster) << " Valid:" << " Result: " << validMetaData->getResult () << " Nodes:\n" << validNodes.getJson (0); } else if (index_diff) { - WriteLog (lsERROR, LedgerMaster) << "MISMATCH on TX " << tx << + WriteLog (lsDEBUG, LedgerMaster) << "MISMATCH on TX " << tx << ": Different index and nodes!"; - WriteLog (lsERROR, LedgerMaster) << " Built:" << + WriteLog (lsDEBUG, LedgerMaster) << " Built:" << " Index: " << builtMetaData->getIndex () << " Nodes:\n" << builtNodes.getJson (0); - WriteLog (lsERROR, LedgerMaster) << " Valid:" << + WriteLog (lsDEBUG, LedgerMaster) << " Valid:" << " Index: " << validMetaData->getIndex () << " Nodes:\n" << validNodes.getJson (0); } else // nodes_diff { - WriteLog (lsERROR, LedgerMaster) << "MISMATCH on TX " << tx << + WriteLog (lsDEBUG, LedgerMaster) << "MISMATCH on TX " << tx << ": Different nodes!"; - WriteLog (lsERROR, LedgerMaster) << " Built:" << + WriteLog (lsDEBUG, LedgerMaster) << " Built:" << " Nodes:\n" << builtNodes.getJson (0); - WriteLog (lsERROR, LedgerMaster) << " Valid:" << + WriteLog (lsDEBUG, LedgerMaster) << " Valid:" << " Nodes:\n" << validNodes.getJson (0); } } diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index 02467c057..b11c914a0 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -563,8 +563,7 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel) // sequence to determine the transaction. Why is the offer sequence // number insufficient? - auto const sleCreator = view.peek ( - keylet::account(account_)); + auto const sleCreator = view.peek (keylet::account(account_)); deprecatedWrongOwnerCount_ = sleCreator->getFieldU32(sfOwnerCount); @@ -831,9 +830,12 @@ CreateOffer::doApply() // This is the ledger view that we work against. Transactions are applied // as we go on processing transactions. Sandbox view (&ctx_.view()); - // This is a checkpoint with just the fees paid. If something goes wrong - // with this transaction, we roll back to this ledger. + + // This is a ledger with just the fees paid and any unfunded or expired + // offers we encounter removed. It's used when handling Fill-or-Kill offers, + // if the order isn't going to be placed, to avoid wasting the work we did. Sandbox viewCancel (&ctx_.view()); + auto const result = applyGuts(view, viewCancel); if (result.second) view.apply(ctx_.rawView()); diff --git a/src/ripple/app/tx/impl/OfferStream.cpp b/src/ripple/app/tx/impl/OfferStream.cpp index 12124a3a1..cd274fd41 100644 --- a/src/ripple/app/tx/impl/OfferStream.cpp +++ b/src/ripple/app/tx/impl/OfferStream.cpp @@ -19,18 +19,19 @@ #include #include +#include namespace ripple { -OfferStream::OfferStream (ApplyView& view, ApplyView& view_cancel, +OfferStream::OfferStream (ApplyView& view, ApplyView& cancelView, BookRef book, Clock::time_point when, Config const& config, beast::Journal journal) : j_ (journal) - , m_view (view) - , m_view_cancel (view_cancel) - , m_book (book) - , m_when (when) - , m_tip (view, book) + , view_ (view) + , cancelView_ (cancelView) + , book_ (book) + , expire_ (when) + , tip_ (view, book_) , config_ (config) { } @@ -44,24 +45,24 @@ OfferStream::erase (ApplyView& view) // correctly remove the directory if its the last entry. // Unfortunately this is a protocol breaking change. - auto p = view.peek (keylet::page(m_tip.dir())); + auto p = view.peek (keylet::page(tip_.dir())); if (p == nullptr) { - if (j_.error) j_.error << - "Missing directory " << m_tip.dir() << - " for offer " << m_tip.index(); + JLOG(j_.error) << + "Missing directory " << tip_.dir() << + " for offer " << tip_.index(); return; } auto v (p->getFieldV256 (sfIndexes)); - auto it (std::find (v.begin(), v.end(), m_tip.index())); + auto it (std::find (v.begin(), v.end(), tip_.index())); if (it == v.end()) { - if (j_.error) j_.error << - "Missing offer " << m_tip.index() << - " for directory " << m_tip.dir(); + JLOG(j_.error) << + "Missing offer " << tip_.index() << + " for directory " << tip_.dir(); return; } @@ -69,9 +70,9 @@ OfferStream::erase (ApplyView& view) p->setFieldV256 (sfIndexes, v); view.update (p); - if (j_.trace) j_.trace << - "Missing offer " << m_tip.index() << - " removed from directory " << m_tip.dir(); + JLOG(j_.trace) << + "Missing offer " << tip_.index() << + " removed from directory " << tip_.dir(); } bool @@ -84,52 +85,48 @@ OfferStream::step () { // BookTip::step deletes the current offer from the view before // advancing to the next (unless the ledger entry is missing). - if (! m_tip.step()) + if (! tip_.step()) return false; - std::shared_ptr entry = m_tip.entry(); + std::shared_ptr entry = tip_.entry(); // Remove if missing if (! entry) { - erase (view()); - erase (view_cancel()); + erase (view_); + erase (cancelView_); continue; } // Remove if expired if (entry->isFieldPresent (sfExpiration) && - entry->getFieldU32 (sfExpiration) <= m_when) + entry->getFieldU32 (sfExpiration) <= expire_) { - if (j_.trace) j_.trace << + JLOG(j_.trace) << "Removing expired offer " << entry->getIndex(); - offerDelete (view_cancel(), - view_cancel().peek( - keylet::offer(entry->key()))); + offerDelete (cancelView_, + cancelView_.peek(keylet::offer(entry->key()))); continue; } - m_offer = Offer (entry, m_tip.quality()); + offer_ = Offer (entry, tip_.quality()); - Amounts const amount (m_offer.amount()); + Amounts const amount (offer_.amount()); // Remove if either amount is zero if (amount.empty()) { - if (j_.warning) j_.warning << + JLOG(j_.warning) << "Removing bad offer " << entry->getIndex(); - offerDelete (view_cancel(), - view_cancel().peek( - keylet::offer(entry->key()))); - m_offer = Offer{}; + offerDelete (cancelView_, + cancelView_.peek(keylet::offer(entry->key()))); + offer_ = Offer{}; continue; } // Calculate owner funds - // NIKB NOTE The calling code also checks the funds, how expensive is - // looking up the funds twice? - auto const owner_funds = accountFunds(view(), - m_offer.owner(), amount.out, fhZERO_IF_FROZEN, + auto const owner_funds = accountFunds(view_, + offer_.owner(), amount.out, fhZERO_IF_FROZEN, config_); // Check for unfunded offer @@ -138,23 +135,23 @@ OfferStream::step () // If the owner's balance in the pristine view is the same, // we haven't modified the balance and therefore the // offer is "found unfunded" versus "became unfunded" - auto const original_funds = accountFunds(view_cancel(), - m_offer.owner(), amount.out, fhZERO_IF_FROZEN, + auto const original_funds = accountFunds(cancelView_, + offer_.owner(), amount.out, fhZERO_IF_FROZEN, config_); if (original_funds == owner_funds) { - offerDelete (view_cancel(), view_cancel().peek( + offerDelete (cancelView_, cancelView_.peek( keylet::offer(entry->key()))); - if (j_.trace) j_.trace << + JLOG(j_.trace) << "Removing unfunded offer " << entry->getIndex(); } else { - if (j_.trace) j_.trace << + JLOG(j_.trace) << "Removing became unfunded offer " << entry->getIndex(); } - m_offer = Offer{}; + offer_ = Offer{}; continue; } diff --git a/src/ripple/app/tx/impl/OfferStream.h b/src/ripple/app/tx/impl/OfferStream.h index 027f896a4..132ce6891 100644 --- a/src/ripple/app/tx/impl/OfferStream.h +++ b/src/ripple/app/tx/impl/OfferStream.h @@ -21,12 +21,11 @@ #define RIPPLE_APP_BOOK_OFFERSTREAM_H_INCLUDED #include -#include +#include #include #include #include #include -#include namespace ripple { @@ -51,41 +50,23 @@ class OfferStream { private: beast::Journal j_; - std::reference_wrapper m_view; - std::reference_wrapper m_view_cancel; - Book m_book; - Clock::time_point m_when; - BookTip m_tip; - Offer m_offer; + ApplyView& view_; + ApplyView& cancelView_; + Book book_; + Clock::time_point const expire_; + BookTip tip_; + Offer offer_; Config const& config_; void erase (ApplyView& view); public: - OfferStream (ApplyView& view, ApplyView& view_cancel, + OfferStream (ApplyView& view, ApplyView& cancelView, BookRef book, Clock::time_point when, Config const& config, beast::Journal journal); - ApplyView& - view () noexcept - { - return m_view; - } - - ApplyView& - view_cancel () noexcept - { - return m_view_cancel; - } - - Book const& - book () const noexcept - { - return m_book; - } - /** Returns the offer at the tip of the order book. Offers are always presented in decreasing quality. Only valid if step() returned `true`. @@ -93,7 +74,7 @@ public: Offer const& tip () const { - return m_offer; + return offer_; } /** Advance to the next valid offer.