Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OrderBookIsNotCrossed invariant #2210

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
e2544ff
Application: register OrderBookIsNotCrossed invariant
robertDurst Jul 30, 2019
cace409
InvariantTestUtils: extract generateOffer from LiabilitiesMatchOffers…
robertDurst Jul 30, 2019
7315125
InvariantTestUtils: add operation pointer as param to store method
robertDurst Jul 30, 2019
6f2fe76
Invariant: add resetForFuzzer method behind build flag
robertDurst Jul 30, 2019
3ac55ed
OrderBookIsNotCrossed: invariant implementation
robertDurst Jul 30, 2019
16b6db4
OrderBookIsNotCrossed: tests for invariant
robertDurst Jul 30, 2019
faeecec
[pr fix]: conditional compile everything behind FUZZING_BUILD_MODE_UN…
robertDurst Jul 31, 2019
08af91f
[pr fix]: remove XNOR, bool #define, and 2nd test sections and simpli…
robertDurst Jul 31, 2019
78e2055
[pr fix]: simplify in-memory orderbook tests, removing sections among…
robertDurst Jul 31, 2019
626bd07
[pr fix]: header changes - remove comments, make member functions, co…
robertDurst Jul 31, 2019
83b1203
[pr fix]: combine update and check, don't us operation, extract min f…
robertDurst Aug 9, 2019
5ecb2af
[pr fix]: const things and undo change to InvariantTestUtils
robertDurst Aug 9, 2019
2290445
[pr fix]: use hash instead of string for AssetId
robertDurst Aug 9, 2019
bccd74b
[substantial changes] account for allowTrust & pathPayment, order off…
robertDurst Aug 14, 2019
fef0439
[pr fix] utilize make_pair and simplify path asset extraction
robertDurst Aug 20, 2019
c0c09c0
properly reset in-memory orderbook, snapshotting before tx and reset …
robertDurst Aug 28, 2019
6ee9d56
[pr fix] small spacing nit
robertDurst Aug 29, 2019
6cad472
[pr fix] snapshot and reset invariant for fuzzer
robertDurst Aug 29, 2019
1d1059c
[pr fix] move Asset std::hash to LedgerHashUtils
robertDurst Aug 29, 2019
814ec60
[pr fix] cleanup OfferEtryCmp
robertDurst Aug 29, 2019
2be048e
[pr fix] simplify extractAssetPairs
robertDurst Aug 29, 2019
9a7b3ae
[pr fix] simplify checkCrossed
robertDurst Aug 29, 2019
72a269b
[pr fix] add const refs in OrderBookIsNotCrossedTests
robertDurst Aug 29, 2019
cfd2846
[pr fix] simplify checkCrossed and simplify extractAsset
robertDurst Aug 29, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/invariant/Invariant.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,17 @@ class Invariant
{
return std::string{};
}

#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
virtual void
snapshotForFuzzer()
{
}

virtual void
jonjove marked this conversation as resolved.
Show resolved Hide resolved
resetForFuzzer()
{
}
#endif // FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
};
}
5 changes: 5 additions & 0 deletions src/invariant/InvariantManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ class InvariantManager

virtual void enableInvariant(std::string const& name) = 0;

#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
virtual void snapshotForFuzzer() = 0;
virtual void resetForFuzzer() = 0;
#endif // FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION

template <typename T, typename... Args>
std::shared_ptr<T>
registerInvariant(Args&&... args)
Expand Down
20 changes: 20 additions & 0 deletions src/invariant/InvariantManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,4 +227,24 @@ InvariantManagerImpl::handleInvariantFailure(
CLOG(ERROR, "Invariant") << REPORT_INTERNAL_BUG;
}
}

#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
void
InvariantManagerImpl::snapshotForFuzzer()
{
for (auto const& invariant : mEnabled)
{
invariant->snapshotForFuzzer();
}
}

void
jonjove marked this conversation as resolved.
Show resolved Hide resolved
InvariantManagerImpl::resetForFuzzer()
{
for (auto const& invariant : mEnabled)
{
invariant->resetForFuzzer();
}
}
#endif // FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
}
5 changes: 5 additions & 0 deletions src/invariant/InvariantManagerImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ class InvariantManagerImpl : public InvariantManager

virtual void enableInvariant(std::string const& name) override;

#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, I don't think you are ever actually using these functions?

Copy link
Contributor Author

@robertDurst robertDurst Aug 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebased so now I can add it to the appropriate place in the fuzzing code, before and after:

// attempt to apply transaction
txFramePtr->attemptApplication(*mApp, ltx);

void snapshotForFuzzer() override;
void resetForFuzzer() override;
#endif // FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION

private:
void onInvariantFailure(std::shared_ptr<Invariant> invariant,
std::string const& message, uint32_t ledger);
Expand Down
174 changes: 174 additions & 0 deletions src/invariant/OrderBookIsNotCrossed.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
// Copyright 2019 Stellar Development Foundation and contributors. Licensed
// under the Apache License, Version 2.0. See the COPYING file at the root
// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0

#include "invariant/OrderBookIsNotCrossed.h"
#include "invariant/InvariantManager.h"
#include "ledger/LedgerTxn.h"
#include "lib/util/format.h"
#include "main/Application.h"
#include "xdr/Stellar-ledger-entries.h"
#include <xdrpp/printer.h>

namespace stellar
{

static std::set<std::pair<Asset, Asset>>
extractAssetPairs(LedgerTxnDelta const& ltxd)
{

std::set<std::pair<Asset, Asset>> assets;
for (auto const& entry : ltxd.entry)
{
if (entry.first.type() == OFFER && entry.second.current)
{
auto const& offer = entry.second.current->data.offer();

auto const& assetPair = std::make_pair(offer.selling, offer.buying);
auto const& oppositeAssetPair =
std::make_pair(offer.buying, offer.selling);
if (assets.find(oppositeAssetPair) == assets.end())
{
assets.insert(assetPair);
}
}
}

return assets;
}

static double
price(OfferEntry const& offer)
{
return double(offer.price.n) / double(offer.price.d);
}

static std::string
checkCrossed(Asset const& a, Asset const& b, OrderBook const& orderBook)
{
// if either side of order book for asset pair empty or does not yet exist,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block (109:120) is a bit wasteful since you are repeating look ups you've already done (by my count, this takes 12 look ups when it can be done with 4). I would store the results as you get them, something like

auto iterA1 = orderBook.find(a);
auto iterB1 = orderBook.find(b);
if (iterA1 == orderBook.end() || iterB1 == orderBook.end())
{
    return {};
}

// ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to fill in the rest of the code starting from // ... (I only gave you the first part of it). Should need to make some changes through auto const& bids = ....

// order book cannot be crossed
auto iterA = orderBook.find(a);
auto iterB = orderBook.find(b);
if (iterA == orderBook.end() || iterB == orderBook.end())
{
return {};
}

auto const& iterAB = orderBook.at(a).find(b);
auto const& iterBA = orderBook.at(b).find(a);
if (iterAB == (*iterA).second.end() || iterBA == (*iterB).second.end())
{
return {};
}

auto const& asks = orderBook.at(a).at(b);
auto const& bids = orderBook.at(b).at(a);
if (asks.empty() || bids.empty())
{
return {};
}

// check if crossed
auto lowestAsk = price(*asks.cbegin());
auto highestBid = 1 / price(*bids.cbegin());
if (lowestAsk <= highestBid)
{
return fmt::format(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to use xdr::xdr_to_string instead of your custom assetCodeToStr lambda. One advantage, for example, is that it would include both the assetCode and the issuer.

"Order book is in a crossed state for {} - {} "
"asset pair.\nTop of the book is:\n\tAsk price: {}\n\tBid "
"price: {}\n\nWhere {} >= {}!",
xdr::xdr_to_string(a), xdr::xdr_to_string(b), lowestAsk, highestBid,
highestBid, lowestAsk);
}
return {};
}

void
OrderBookIsNotCrossed::deleteFromOrderBook(OfferEntry const& oe)
{
mOrderBook[oe.selling][oe.buying].erase(oe);
}

void
OrderBookIsNotCrossed::addToOrderBook(OfferEntry const& oe)
{
mOrderBook[oe.selling][oe.buying].emplace(oe);
}

void
OrderBookIsNotCrossed::updateOrderBook(LedgerTxnDelta const& ltxd)
{
for (auto const& entry : ltxd.entry)
{
// there are three possible "deltas" for an offer:
// CREATED: (nil) -> LedgerKey
// MODIFIED: LedgerKey -> LedgerKey
// DELETED: LedgerKey -> (nil)
if (entry.first.type() == OFFER)
{
if (entry.second.previous)
{
deleteFromOrderBook(entry.second.previous->data.offer());
}
if (entry.second.current)
{
addToOrderBook(entry.second.current->data.offer());
}
}
}
}

std::string
OrderBookIsNotCrossed::check(
std::set<std::pair<Asset, Asset>> const& assetPairs)
{
for (auto const& assetPair : assetPairs)
{
auto checkCrossedResult =
checkCrossed(assetPair.first, assetPair.second, mOrderBook);
if (!checkCrossedResult.empty())
{
return checkCrossedResult;
}
}

return std::string{};
}

std::shared_ptr<Invariant>
OrderBookIsNotCrossed::registerInvariant(Application& app)
{
return app.getInvariantManager().registerInvariant<OrderBookIsNotCrossed>();
}

std::string
jonjove marked this conversation as resolved.
Show resolved Hide resolved
OrderBookIsNotCrossed::getName() const
{
return "OrderBookIsNotCrossed";
}

std::string
OrderBookIsNotCrossed::checkOnOperationApply(Operation const& operation,
OperationResult const& result,
LedgerTxnDelta const& ltxDelta)
{
updateOrderBook(ltxDelta);
auto assetPairs = extractAssetPairs(ltxDelta);
return assetPairs.size() > 0 ? check(assetPairs) : std::string{};
}

void
OrderBookIsNotCrossed::snapshotForFuzzer()
{
mRolledBackOrderBook = mOrderBook;
};

void
OrderBookIsNotCrossed::resetForFuzzer()
{
mOrderBook = mRolledBackOrderBook;
};
}
#endif // FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
89 changes: 89 additions & 0 deletions src/invariant/OrderBookIsNotCrossed.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
#pragma once

// Copyright 2019 Stellar Development Foundation and contributors. Licensed
// under the Apache License, Version 2.0. See the COPYING file at the root
// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0

#include "crypto/ByteSliceHasher.h"
#include "invariant/Invariant.h"
#include "ledger/LedgerHashUtils.h"
#include "xdr/Stellar-ledger.h"

#include <set>
#include <unordered_map>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this include

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I do. Invariant.h only includes:

#include <memory>
#include <string>


namespace stellar
{

class Application;
struct LedgerTxnDelta;
struct OfferEntry;

// compare two OfferEntry's by price
struct OfferEntryCmp
{
bool
operator()(OfferEntry const& a, OfferEntry const& b) const
{
double priceA = double(a.price.n) / double(a.price.d);
double priceB = double(b.price.n) / double(b.price.d);

if (priceA < priceB)
{
return true;
}
else if (priceA == priceB)
{
return a.offerID < b.offerID;
}
else
{
return false;
}
}
};

using Orders = std::set<OfferEntry, OfferEntryCmp>;
using AssetOrders = std::unordered_map<Asset, Orders>;
using OrderBook = std::unordered_map<Asset, AssetOrders>;

class OrderBookIsNotCrossed : public Invariant
{
public:
explicit OrderBookIsNotCrossed() : Invariant(true)
{
}

static std::shared_ptr<Invariant> registerInvariant(Application& app);

virtual std::string getName() const override;

virtual std::string
checkOnOperationApply(Operation const& operation,
OperationResult const& result,
LedgerTxnDelta const& ltxDelta) override;

OrderBook const&
getOrderBook() const
{
return mOrderBook;
}

void snapshotForFuzzer() override;
void resetForFuzzer() override;

private:
// as of right now, since this is only used in fuzzing, the mOrderBook will
robertDurst marked this conversation as resolved.
Show resolved Hide resolved
// be empty to start. If used in production, since invraiants are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: invraiants -> invariants

// configurable, it is likely that this invariant will be enabled with
// pre-existing ledger and thus the mOrderBook will need a way to sync
OrderBook mOrderBook;
OrderBook mRolledBackOrderBook;
void deleteFromOrderBook(OfferEntry const& oe);
void addToOrderBook(OfferEntry const& oe);
void updateOrderBook(LedgerTxnDelta const& ltxd);
std::string check(std::set<std::pair<Asset, Asset>> const& assetPairs);
};
}
#endif // FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
35 changes: 32 additions & 3 deletions src/invariant/test/InvariantTestUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,30 @@ generateRandomAccount(uint32_t ledgerSeq)
return le;
}

LedgerEntry
generateOffer(Asset const& selling, Asset const& buying, int64_t amount,
Price price)
{
REQUIRE(!(selling == buying));
REQUIRE(amount >= 1);

LedgerEntry le;
le.lastModifiedLedgerSeq = 2;
le.data.type(OFFER);

auto offer = LedgerTestUtils::generateValidOfferEntry();
offer.amount = amount;
offer.price = price;
offer.selling = selling;
offer.buying = buying;

le.data.offer() = offer;
return le;
}

bool
store(Application& app, UpdateList const& apply, AbstractLedgerTxn* ltxPtr,
OperationResult const* resPtr)
OperationResult const* resPtr, Operation const* opPtr)
{
bool shouldCommit = !ltxPtr;
std::unique_ptr<LedgerTxn> ltxStore;
Expand Down Expand Up @@ -86,8 +107,16 @@ store(Application& app, UpdateList const& apply, AbstractLedgerTxn* ltxPtr,
bool doInvariantsHold = true;
try
{
app.getInvariantManager().checkOnOperationApply({}, *resPtr,
ltxPtr->getDelta());
if (opPtr == nullptr)
robertDurst marked this conversation as resolved.
Show resolved Hide resolved
{
app.getInvariantManager().checkOnOperationApply({}, *resPtr,
ltxPtr->getDelta());
}
else
{
app.getInvariantManager().checkOnOperationApply(*opPtr, *resPtr,
ltxPtr->getDelta());
}
}
catch (InvariantDoesNotHold&)
{
Expand Down
Loading