OrderBookIsNotCrossed invariant #2210
Conversation
| // Orders is a map of OfferId (int64_t) --> OfferEntry | ||
| using Orders = std::unordered_map<int64_t, OfferEntry>; | ||
| // AssetOrders, orders by asset, a map of AssetId --> map of Orders | ||
| using AssetOrders = std::unordered_map<AssetId, Orders>; |
There was a problem hiding this comment.
For both AssetOrders and OrderBook, serializing the key to string is not a C++ idiom. Write a template specialization of std::hash instead. See ledger/LedgerHashUtils.h for inspiration.
There was a problem hiding this comment.
The change you made is not what I meant, let's discuss further offline.
|
|
||
| // AssetId is defined as a concatenation of issuer and asset code: | ||
| // ASSET_CODE | '-' | ISSUER_ACCOUNT_ID | ||
| using AssetId = std::string; |
There was a problem hiding this comment.
Not needed (along with getAssetID) in accordance with the comment about specializing std::hash. As a general remark: if you are writing a comment to describe the specific structure that a string must have, then you would probably be better off using a struct instead.
|
|
||
| SECTION("Then modify offer") | ||
| { | ||
| auto offer2 = generateOffer(cur1, cur2, 2, Price{5, 3}); |
There was a problem hiding this comment.
Why not just copy offer1 and modify it directly?
| auto entry = ltxPtr->create(offer1); | ||
|
|
||
| invariant->checkOnOperationApply(op, opRes, ltxPtr->getDelta()); | ||
| auto orderbook = invariant->getOrderBook(); |
There was a problem hiding this comment.
You want auto const& here and many other places in this file.
| SECTION("Create offer") | ||
| { | ||
| // Offer 5: 7 A @ 8/5 (B/A) | ||
| auto offer5 = generateOffer(cur1, cur2, 7, Price{8, 5}); |
There was a problem hiding this comment.
Why not use genApplyCheckCreateOffer here? (This question applies in multiple places in this file)
| } | ||
|
|
||
| void | ||
| genApplyCheckModifyOffer(Asset& ask, Asset& bid, int64 amount, Price price, |
There was a problem hiding this comment.
Passing around the operations is kind of clunky. The invariant only depends on the assets in the operation, so you should be able to prepare them in these functions.
|
|
||
| SECTION("Not crossed when highest bid < lowest ask") | ||
| { | ||
| SECTION("Create offer") |
There was a problem hiding this comment.
Why put a section directly inside another section? (This applies in multiple places in this file)
| #include "invariant/Invariant.h" | ||
| #include "xdr/Stellar-ledger-entries.h" | ||
|
|
||
| #include <unordered_map> |
There was a problem hiding this comment.
I don't think you need this include
There was a problem hiding this comment.
I think I do. Invariant.h only includes:
#include <memory>
#include <string>
| #include "test/test.h" | ||
|
|
||
| using namespace stellar; | ||
| using namespace stellar::InvariantTestUtils; |
There was a problem hiding this comment.
I don't think you need this.
There was a problem hiding this comment.
As far as I can tell I do, won't compile without and it is also in most of the other invariant tests.
|
@marta-lokhova and @jonjove I have refactored per your feedback. Thanks -- I think this is quite a bit cleaner now! Since some of the refactors were more involved, the diffs may make review a bit harder. To address this I tried to sensibly organize my changes into a few commits labeled |
|
Updated for proper template specialization for |
robertDurst
changed the title
|
Changed header to WIP -- realize this does not properly account for |
|
Latest commit 86e3826 is a bit substantial (hence WIP in header). The most notable changes from the initial PR include:
|
robertDurst
changed the title
| case MANAGE_BUY_OFFER: | ||
| { | ||
| auto const& offer = op.body.manageBuyOfferOp(); | ||
| return {std::pair<Asset, Asset>(offer.selling, offer.buying)}; |
There was a problem hiding this comment.
std::make_pair can infer types for arguments, so until C++17 please use that
There was a problem hiding this comment.
Ok will do. Ah cool I see what you mean: https://www.codingame.com/playgrounds/2205/7-features-of-c17-that-will-simplify-your-code/template-argument-deduction-for-class-templates
| for (int i = 0; i < pp.path.size(); ++i) | ||
| { | ||
| // beginning: send -> A | ||
| if (i == 0) |
There was a problem hiding this comment.
is does not make sense for those conditions to be inside loop
you can loop from 1 to pp.path.size() - 1 and have those special cases outside of loop
There was a problem hiding this comment.
Good point. Fixed, simpler now.
|
While this works great for a persistent order book, the fuzzer does not want the order book to persist between fuzzing executions (not even between operations within a single transaction). So I will need to properly reset this. Have it reset properly on a local branch. Will push this soon. |
…SAFE_FOR_PRODUCTION
…fy genApplyCheck methods
… other small fixes
…nst things, and make strict invariant
…unction, const things
…ers using set, decouple some methods that did too much
|
|
||
| namespace std | ||
| { | ||
| template <> class hash<stellar::Asset> |
There was a problem hiding this comment.
Maybe this should be in ledger/LedgerHashUtils.h?
| bool | ||
| operator()(OfferEntry const& a, OfferEntry const& b) const | ||
| { | ||
| auto const& price = [](OfferEntry const& offer) { |
There was a problem hiding this comment.
Using a lambda here is unnecessary. I would just express it directly, as doing so would be shorter than the lambda. An alternative approach would be to remove the lambda, implement this in the .cpp file (leaving the declaration in the .h file), and use the static price method in that translation unit.
| { | ||
|
|
||
| std::vector<std::pair<Asset, Asset>> | ||
| extractAssetPairs(Operation const& op, LedgerTxnDelta const& ltxd) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| private: | ||
| // as of right now, since this is only used in fuzzing, the mOrderBook will | ||
| // be empty to start. If used in production, since invraiants are |
There was a problem hiding this comment.
Nitpick: invraiants -> invariants
| // dest: C | ||
| // path: {Bs} | ||
| // where Bs represents from 0 to 5 offers inclusive | ||
| Asset send = les.at(0).data.offer().selling; |
There was a problem hiding this comment.
Use front instead of at(0) and back instead of at(size()-1). Also these should be const references.
|
|
||
| // convert a single offer into an offer operation | ||
| Operation | ||
| opFromLedgerEntries(LedgerEntry le) |
There was a problem hiding this comment.
Nitpick: le and offer should be const references. This comment applies many places in this file, so I won't post it everywhere but it would be worthwhile for you to look over the file.
| } | ||
| } | ||
|
|
||
| return assets; |
There was a problem hiding this comment.
assets contains duplicates, which will be wasteful when checking crossed later. (I believe this is also possible for PATH_PAYMENT)
There was a problem hiding this comment.
As part of an above change, I loop through Offers and only grab unique asset pairs.
| @@ -50,6 +50,11 @@ class InvariantManagerImpl : public InvariantManager | |||
|
|
|||
| virtual void enableInvariant(std::string const& name) override; | |||
|
|
|||
| #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION | |||
There was a problem hiding this comment.
As far as I can tell, I don't think you are ever actually using these functions?
There was a problem hiding this comment.
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);
| auto assetPair = std::make_pair(offer.selling, offer.buying); | ||
| auto oppositeAssetPair = | ||
| std::make_pair(offer.buying, offer.selling); | ||
| if (assets.find(oppositeAssetPair) == assets.end() && |
There was a problem hiding this comment.
Technically, you only need to check oppositeAssetPair here. The insert on line 35 will do nothing if assetPair is already included.
There was a problem hiding this comment.
Good point -- fixed.
| // we derive the rest of the information from the | ||
| // LedgerTxnDelta entries | ||
| for (auto const& entry : ltxd.entry) | ||
| auto const& offer = entry.second.previous |
There was a problem hiding this comment.
Looking closely at this, I'm not sure that this makes sense as implemented. An important piece of context here: it is possible to modify the buying or selling assets on an offer. In those cases, previous and current won't have the same asset pairs.
But in any case, I'm wondering why we should ever care about the previous state? If an order book wasn't crossed and an order was deleted, then it still isn't crossed. If an order book wasn't crossed and an order was modified such that it has the same asset pair, then it will be gathered from the current state. If an order book wasn't crossed and an order was modified such that it doesn't have the same asset pair, then it still isn't crossed. In any of these cases, the previous asset pair isn't useful. Am I missing something here?
There was a problem hiding this comment.
Thanks, I did not realize you can change the asset, I thought you could only change the price and amount. Good catch.
Very good point -- if a set A of orders on an order book is not crossed, there is no subset B such that it may become crossed. The spread between the lowest ask and the highest bid for the subset B is >= the spread before the deletion of any order A.
There was a problem hiding this comment.
TLDR they can only be in a less or equally crossed state so if the state was not crossed before it is not crossed now.
| @@ -83,7 +83,7 @@ class OrderBookIsNotCrossed : public Invariant | |||
| void deleteFromOrderBook(OfferEntry const& oe); | |||
| void addToOrderBook(OfferEntry const& oe); | |||
| void updateOrderBook(LedgerTxnDelta const& ltxd); | |||
| std::string check(std::vector<std::pair<Asset, Asset>> assetPairs); | |||
| std::string check(std::set<std::pair<Asset, Asset>> assetPairs); | |||
There was a problem hiding this comment.
Nitpick: ... const& assetPairs
| 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, |
There was a problem hiding this comment.
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 = ....
|
will reopen later |
Description
This PR introduces a new invariant, named
OrderBookIsNotCrossed.Background:
Pre-protocol 10 was allowing in some cases the order book to be in a crossed state. The example from CAP-0004 reads:
Put very simply, a crossed state is one such that the top of the book contains a highest bid higher than the lowest ask.
Implementation Overview
This invariant, unlike the previous five, requires some state to determine the highest bid and the lowest ask. For this, I utilize a nested map data structure to simulate an order book, which is updated upon each call of
checkOnOperationApplybased on the offer entries inLedgerTxnDelta. Since this invariant will be used by the fuzzing code in #2182, which requires a reset of state between execution cycles, I have introduced aresetForFuzzermethod to the base invariant class.Open Questions
Checklist
clang-formatv5.0.0 (viamake formator the Visual Studio extension)