Bitshares-core: Margin call order fills at price of matching limit_order but not at a price related to itself or settlement_price

[alexpmorris]

just wanted to share this issue I came across. I outlined it here, with a possible solution (or at least, a better understanding of what the problem is likely to be)...

https://steemit.com/bitshares/@alexpmorris/beware-crossing-the-offer-with-your-bid-on-the-bitshares-dex-or-it-might-cost-you-a-bundle

[pmconrad]

I don't think the described behaviour is a bug. Did you ever receive less payment than you requested?

[oxarbitrage]

We discussed the issue today in the telegram chat, we are getting aligned on a possible blockchain patch.

the issue will be discussed formally in this thread:

https://bitsharestalk.org/index.php/topic,24715.0.html

by now i will be after trying to replicate the problem in the testnet and/or make a unit test for the problem. in the meantime we will be discussing consensus on hot to best fix it.

@alexpmorris thank you very much for bringing this issue here.

[abitmore]

@oxarbitrage try https://openledger.io/market/BTS_GOLD

[abitmore]

Among these lines in https://github.com/bitshares/bitshares-core/blob/2.0.170710/libraries/chain/db_market.cpp#L503-L507 (quoted below),

       margin_called = true;

       auto usd_to_buy   = call_itr->get_debt();

       if( usd_to_buy * match_price > call_itr->get_collateral() )

we can adjust match_price to min_price before the if, and need to be guarded by a hard fork check, of course, so something like this:

       margin_called = true;

       if( head_block_time() >= HF_CORE_ISSUE_338_TIME )
          match_price = min_price;

       auto usd_to_buy   = call_itr->get_debt();

       if( usd_to_buy * match_price > call_itr->get_collateral() )

//Update: not this simple.

[abitmore]

By the way, there seems to be something wrong with the code.

https://github.com/bitshares/bitshares-core/blob/2.0.170710/libraries/chain/db_market.cpp#L464

bool filled_limit = false;

should be defined inside the loop, because check_call_orders is not only be called when a new limit_order is created, but will also be called when there is a price feed change, in this case there may be multiple limit_order objects as well as multiple call_order objects be processed. I guess this issue has happened in production for many times, fixing this would require a hard fork as well.

Another minor issue, this line
https://github.com/bitshares/bitshares-core/blob/2.0.170710/libraries/chain/db_market.cpp#L541

       auto old_limit_itr = filled_limit ? limit_itr++ : limit_itr;
       fill_order(*old_limit_itr, order_pays, order_receives, true);

should be changed to something like this for better performance

       auto old_limit_itr = limit_itr;
       if( filled_limit ) ++limit_itr;
       fill_order(*old_limit_itr, order_pays, order_receives, true);

[pmconrad]

Nice find.

[abitmore]

@oxarbitrage GOLD has been global settled. Try SILVER.

[abitmore]

Another issue indicated in the post is related to rounding, which is actually a very old & known issue. For example:

The order was selling 0.185 BTS, but apparently it's not enough to get 21 satoshi of HERO, so finally it got 20. However, to get 20 satoshi of HERO, only need to pay 0.1838 BTS.

132 and #184 are probably related. Or perhaps best to open a new ticket.

//Edit:
Current implementation is in favor of the maker. Should we change it to in favor of the taker, or anything that is fairer? At least we need a clearer feature definition.

[abitmore]

By the way, there is another feature about call orders which I think is debatable: for example, in BTS:bitCNY market (price shows as X bitcny per BTS), when there are bids above the settlement price, even when there is a call order with insufficient collateral, they won't match.

[abitmore]

As I posted in forum, The fix I proposed above is not ideal:

there are at least two scenarios, they should be treated differently:

1. There are old bids on the book, when a new price feed is published, a short position get margin called. In this case, the call order should walk through the bids until MSSP, it's best to use the bid prices.
2. There are already margin call orders waiting to be filled, then a new bid is created against it. In this case, it's best to use MSSP.

In short, in favor of the taker.

[abitmore]

Moving discussion of rounding issue to #342.

[abitmore]

Created a new ticket (#453) for the multiple order matching issue described in this ticket.

[abitmore]

Done with #829.