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.

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

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 55aae58

With respect to the TODO at market_engine.cpp#L379-L382 -- this block of code was created in two places by 14d47ab, one was quickly removed in d076871. 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 7f9a60c

[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 f93723d 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 #1315 (comment)

[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.