Collection of market issues

[theoreticalbts]

This is a collection of market tickets that are still open. Some of these are already resolved in develop branch.

• [x] #1061 Expired shorts without sufficient sell depth to cover should create a BUY WALL for BitUSD
• [x] #1273 margin call price is 150%, not 200%
• [ ] #1277 Test shorts to be forced to cover at up to 110% of price feed
• [ ] #1290 Implement wallet_market_sell
• [x] #1295 Verify BitUSD vs BitGLD in Unit Tests / Regression Tests
• [ ] #1298 Black swan resolution
• [x] #1315 Bug in market engine / matching cover_orders
• [ ] #1317 Expired short positions shouldn't accrue interest
• [x] #1320 Set market as dirty on block where cover order expires
• [x] #1343 Thoroughly specify and test behavior when feeds are out-of-date
• [x] #1354 Shorts below the price feed are mind-bending and possibly wrong
• [x] #1355 wallet_market_sell breaking API change
• [x] #1370 Test expired AND called cover
• [x] #1372 Test all pairs of order types against each other
• [x] #1438 Maximum APR on shorts is higher than it should be
• [ ] #1442 double cover (20033)
• [x] #1447 update_cover_operation
• [x] #1455 market.feature's short price limit test is failing (second one)
• [x] #1459 Potentially incorrect matching in block 2059269
• [x] #1464 Issue in bts/0.7.0 about market engine
• [x] #1465 feed price can change when round changes, even if nothing else has changed
• [x] #1495 Short orders lower than the highest margin call price can't be filled
• [x] #1499 The 5% margin call fee is gone in dvs/0.9.0 ?

[vikramrajkumar]

Good work so far and thanks for the writeup (https://github.com/BitShares/bitshares/blob/29064038e46db60241a2e3af02ec1c3a743a25f6/docs/market-0.8.md). Some questions:

• Do we need to address any of these TODOs:
  • https://github.com/BitShares/bitshares/blob/29064038e46db60241a2e3af02ec1c3a743a25f6/libraries/blockchain/market_engine.cpp#L57
  • https://github.com/BitShares/bitshares/blob/29064038e46db60241a2e3af02ec1c3a743a25f6/libraries/blockchain/market_engine.cpp#L138
  • https://github.com/BitShares/bitshares/blob/29064038e46db60241a2e3af02ec1c3a743a25f6/libraries/blockchain/market_engine.cpp#L381
• If no order can match only in the current pass, does this need to be a continue rather than a break: https://github.com/BitShares/bitshares/blob/29064038e46db60241a2e3af02ec1c3a743a25f6/libraries/blockchain/market_engine.cpp#L187-L198
• Please confirm that the extra short and feed indexing logic here is correct: https://github.com/BitShares/bitshares/blob/29064038e46db60241a2e3af02ec1c3a743a25f6/libraries/blockchain/chain_database.cpp#L2347-L2474
• Please confirm that this is the correct way to reload the extra short indexes at startup: https://github.com/BitShares/bitshares/blob/29064038e46db60241a2e3af02ec1c3a743a25f6/libraries/blockchain/chain_database.cpp#L208-L221

Please also close any of the issues in the OP whenever you consider them fully resolved (including marking them as wontfix, etc.). Note that I have added several to the list.

[theoreticalbts]

If no order can match only in the current pass, does this need to be a continue rather than a break

There are actually two loops. The inner loop is a while loop that executes once for each order, the outer loop is a for loop that iterates through the 3 passes.

At this point in the code we want to stop the current pass and go on to the next pass. So break drops out of the inner loop, but the outer loop will go on to the next pass.

The indentation is not so clear that there are two loops, mostly because I didn't want to muddy the waters on this commit with a bunch of whitespace changes to otherwise untouched lines.

[theoreticalbts]

The TODO at market_engine.cpp#L57 was a reminder to myself to review corner cases in the iterator logic. I'm replacing it with a comment containing my analysis, see f6d2d56f78b964c964e3025c21e563f562a26c2c

The TODO at market_engine.cpp#L138-142 was a note to myself that, while this termination check was not optional in the commit that introduced it, it would be made optional by a planned later commit. Indeed we can see that if limit_price < mtrx.ask_price then bid_price will be set to a value at least as small as limit_price, so if limit_price < ask_price then certainly bit_price < ask_price which will trigger the termination condition at market_engine.cpp#L194-198.

Actually on closer inspection, removing this check actually seems to fix a matching bug. With the check in place, ask_price might be high enough to trigger the check and terminate -- but in case of a called cover order, the code in the next if statement might "want" to update ask_price to force a match, but never run because of the check. End result, called cover orders would be unable to match against new shorts unless the short price actually overlapped the margin call price, contrary to the design intent. The commit that removes the check is 55aae58924374e85b413b25e4595cde27cd198e4

With respect to the TODO at market_engine.cpp#L379-L382 -- this block of code was created in two places by 14d47ab32509e6688cc4dd64d2181ce87876991c, one was quickly removed in d076871b8e74efae4e69a535b96da392f8ea73f3. It seems that the intent was to dispatch to the logic to update _shorts_at_feed where this check is, but _shorts_at_feed is updated in store_feed_record() instead. So dispatching to this logic from the market engine is no longer necessary. I'm deleting the if statement and TODO in 7f9a60ce0bd22a91144a9699dbe667117006e1f2

[theoreticalbts]

About flagging chain_database.cpp#L2347-L2474 as a potential trouble spot -- nice code reviewing skills. @valzav yesterday reported a failing acceptance_test which, upon investigation, I determined to be failing due to incorrect logic in this section. It is now fixed in f93723d57248635c663a46178f88309744c8f1de and everything else in this section seems to be correct.

[abitmore]

//EDIT: the comment below is incorrect. Just leave it here for reference.

will this always return an empty iterator?

• https://github.com/BitShares/bitshares/blob/7f9a60ce0bd22a91144a9699dbe667117006e1f2/libraries/blockchain/market_engine.cpp#L90-L93

Should it be something like std::make_pair( *_feed_price * 0.9, market_index_key( current_pair ))? same issue reported at https://github.com/BitShares/bitshares/issues/1315#issuecomment-80824265

[theoreticalbts]

@abitmore : _short_at_limit_itr is a reverse iterator so it starts at feed price and goes downwards (but excludes orders exactly at the feed price, those are covered by _short_at_feed_itr). When shorting into a margin call, what would happen is the cover price would be adjusted to consume more collateral at market_engine.cpp#L168-L184.

I think what you're missing here is that shorts can only execute on one side of the feed (which side this is depends on which flipped market you're thinking about). Shorts that are on the "wrong side" aren't inactivated, rather they are clamped to shorting at the feed value. Those clamped shorts are in _short_at_feed_itr. The whole point of these changes is that because these shorts have two modes of execution, they need to be indexed separately.

[abitmore]

@theoreticalbts Thanks for reply. I didn't know how the reverse iterator works (now I know). I guessed that it would start from the nearest short below feed_price and go down, but incorrectly assumed it would end at the parameter passed into the constructor of reverse_iterator, and misunderstood the rend() call. BTW I think I know how the shorts should match, just trying to understand the code.