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 16 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
11 changes: 11 additions & 0 deletions src/invariant/Invariant.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,16 @@ 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
19 changes: 19 additions & 0 deletions src/invariant/InvariantManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,4 +227,23 @@ 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
237 changes: 237 additions & 0 deletions src/invariant/OrderBookIsNotCrossed.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
#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"

namespace stellar
{

std::vector<std::pair<Asset, Asset>>
extractAssetPairs(Operation const& op, LedgerTxnDelta const& ltxd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just check the asset-pair for every offer that appears in ltxd, similar to what you did for ALLOW_TRUST but incorporating created offers as well? I think this would be simpler and more reliable than handling each operation type specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This simplification is great, and at this point it may seem like I do not utilize operations at all now, which is true. However, I do not believe I should remove the operation inclusion code from the tests because future order book invariants, specifically the one I have locally, wip, BestOffersTaken, relies on the operation for certain parts of its check.

{
switch (op.body.type())
{
case MANAGE_BUY_OFFER:
{
auto const& offer = op.body.manageBuyOfferOp();
return {std::make_pair(offer.selling, offer.buying)};
}
case MANAGE_SELL_OFFER:
{
auto const& offer = op.body.manageSellOfferOp();
return {std::make_pair(offer.selling, offer.buying)};
}
case CREATE_PASSIVE_SELL_OFFER:
{
auto const& offer = op.body.createPassiveSellOfferOp();
return {std::make_pair(offer.selling, offer.buying)};
}
case PATH_PAYMENT:
{
auto const& pp = op.body.pathPaymentOp();

// if no path, only have a pair between send and dest
if (pp.path.size() == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: typically written like pp.path.empty()

{
return {std::make_pair(pp.sendAsset, pp.destAsset)};
}

// For send, dest, {A, B} we get: {send, A}, {A, B}, {B, dest}
std::vector<std::pair<Asset, Asset>> assets;

// beginning: send -> A
assets.emplace_back(pp.sendAsset, pp.path.front());
for (int i = 1; i < pp.path.size(); ++i)
{
// middle: A -> B
assets.emplace_back(pp.path[i - 1], pp.path[i]);
}
// end: B -> dest
assets.emplace_back(pp.path.back(), pp.destAsset);

return assets;
}
case ALLOW_TRUST:
{
auto const& at = op.body.allowTrustOp();

// if revoke auth, all offers for that user against that asset are
// deleted
if (!at.authorize)
{
std::vector<std::pair<Asset, Asset>> assets;
// since we only get one side of the asset pair from the operation,
// we derive the rest of the information from the
// LedgerTxnDelta entries
for (auto const& entry : ltxd.entry)
{
if (entry.second.previous &&
entry.second.previous->data.type() == OFFER)
{
auto const& offer = entry.second.previous->data.offer();
assets.emplace_back(offer.selling, offer.buying);
}
}

return assets;
Copy link
Contributor

Choose a reason for hiding this comment

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

assets contains duplicates, which will be wasteful when checking crossed later. (I believe this is also possible for PATH_PAYMENT)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of an above change, I loop through Offers and only grab unique asset pairs.

}

return {};
}
default:
return {};
}
}

double
Copy link
Contributor

Choose a reason for hiding this comment

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

These free functions should be marked static in order to get internal linkage.

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

double
getMinOfferPrice(Orders const& orders)
{
return orders.cbegin() == orders.cend() ? __DBL_MAX__
: price(*orders.cbegin());
}

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
if (orderBook.find(a) == orderBook.end() ||
orderBook.find(b) == orderBook.end() ||
orderBook.at(a).find(b) == orderBook.at(a).end() ||
orderBook.at(b).find(a) == orderBook.at(b).end())
{
return {};
}

auto const& asks = orderBook.at(a).at(b);
auto const& bids = orderBook.at(b).at(a);

auto lowestAsk = getMinOfferPrice(asks);
auto highestBid = 1.0 / getMinOfferPrice(bids);

if (highestBid >= lowestAsk)
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 really think this is an elegant approach. I had to convince myself that it's impossible for this condition to be satisfied if either asks or bids is empty. I think a more readable approach would be something like

if (!asks.empty() && !bids.empty() && price(asks.front()) < 1 / price(bids.front()))

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.

Ok makes sense. The above should read lowestAsk <= highestBid

Copy link
Contributor

@jonjove jonjove Aug 29, 2019

Choose a reason for hiding this comment

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

Yes you're right, I missed equality in the last condition. Good catch.

{
auto assetToString = [](Asset const& asset) {
auto r = std::string{};
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: while perfectly valid code, I really dislike this pattern. It's strictly longer than std::string r and has a negative impact on readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with xdr::xdr_to_string as recommended below.

switch (asset.type())
{
case stellar::ASSET_TYPE_NATIVE:
r = std::string{"XLM"};
break;
case stellar::ASSET_TYPE_CREDIT_ALPHANUM4:
assetCodeToStr(asset.alphaNum4().assetCode, r);
break;
case stellar::ASSET_TYPE_CREDIT_ALPHANUM12:
assetCodeToStr(asset.alphaNum12().assetCode, r);
break;
}
return r;
};
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 {} >= {}!",
assetToString(a), assetToString(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::vector<std::pair<Asset, Asset>> assetPairs)
{
for (auto const& assetPair : assetPairs)
{
auto const& checkCrossedResult =
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks dangerous: taking a reference to a stack-allocated value from a different function

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(operation, 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
Loading