BSIP31-34 market engine improvements #829

abitmore · 2018-04-07T12:10:10Z

This is a subset of changes on the market engine, cherry-picked from #641, including changes related to #338 #343 #453 #606 #625 #649, aka

Please review.

For: * #338 Margin call order fills at price of matching limit_order * #343 Inconsistent sorting of call orders between matching against a limit order and a force settle order * #453 Multiple limit order and call order matching issue * #606 Undercollateralized short positions should be called regardless of asks

Be consistent with check_call_orders().

abitmore · 2018-04-07T13:47:48Z

libraries/chain/db_market.cpp

       {
-          wlog( "Cull_small issue occurred at block #${block}", ("block",head_block_num()) );
-          limit_itr = next_limit_itr;
+          if( filled_limit ) ++limit_itr;


Note:

the code block here, including ++limit_itr here and below, has been refactored in a later commit, we'll need to review again.

there are test cases about the cull_small issue after hard fork in commits for BSIP35.

pmconrad · 2018-04-08T10:13:46Z

libraries/chain/db_maint.cpp

+      const asset_object& a = *itr;
+      ++itr;
+      // be here, next_maintenance_time should have been updated already
+      db.check_call_orders( a, true, false ); // allow black swan, and call orders are taker


Perhaps skip over PM's here?

Whether to skip over PM's here, the only logic difference might be the check_black_swan() call in check_call_orders().

Since we didn't fix #460 in this release (no BSIP for it so far), PM's will still suffer black swan events when there is a price feed change or matching logic change.

Although we didn't update call_prices of PM's, but we changed logic in check_black_swan() to check with MSSP, which is possible to trigger a black swan event. So the outcome is different if skip over PM's here.

I'd suggest we trigger black swan events here (if there would be some) to be consistent (rather than triggering on a new but same price feed).

Perhaps keep the old logic for PM's, don't trigger black swan event when there is no matching limit order?

I think we agreed that #460 is a bug and doesn't need a BSIP.
We could fix #460 quickly by adding a HF-protected check in check_for_black_swan, then don't need to skip here.

My opinion on #460 is here: #460 (comment). Aka not really a bug and not a quick fix to me.

So my recommendation in this release is to let PMs (still) have same behavior (on black swan events) as other MPAs, that said, we changed behavior of other MPAs, which will affect PMs as well.

pmconrad · 2018-04-08T19:19:28Z

libraries/chain/db_update.cpp

-       assert( settle_price.base.asset_id == limit_itr->sell_price.base.asset_id );
-       highest = std::max( limit_itr->sell_price, settle_price );
+       FC_ASSERT( highest.base.asset_id == limit_itr->sell_price.base.asset_id );
+       highest = std::max( limit_itr->sell_price, highest );


Is that still correct after HF_CORE_338?
The call will always be matched at the squeeze price, even if the limit order offers a better price. A call could have sufficient collateral to fill the limit order at its price, but insufficient collateral to fill at MSQP.

Important for understanding: check_for_blackswan is only called from check_call_orders.
check_call_orders currently always uses limit order price for filling call. That shouldn't be the case, though, if I understand BSIP-32 correctly.

Answer to previous remark:
After HF_625, check_call_orders is only ever called with the maker flag set to false. IOW after HF 338, check_call_orders is always taker, which means using the limit order price is correct, assuming that HF 338 and HF 625 happen simultaneously.

In short, this change is correct.

pmconrad · 2018-04-08T19:20:52Z

libraries/chain/db_update.cpp

-       globally_settle_asset(mia, ~least_collateral );
+       if( maint_time > HARDFORK_CORE_338_TIME && ~least_collateral <= settle_price )
+          // global settle at feed price if possible
+          globally_settle_asset(mia, settle_price );


Still struggling with the legitimacy of this, but tending to think it's ok.

pmconrad · 2018-04-08T19:57:36Z

libraries/chain/db_market.cpp

 *  3 - both were filled
 */
-template<typename OrderType>
-int database::match( const limit_order_object& usd, const OrderType& core, const price& match_price )
+int database::match( const limit_order_object& usd, const limit_order_object& core, const price& match_price )
 {
   assert( usd.sell_price.quote.asset_id == core.sell_price.base.asset_id );


Better turn these assert()s into FC_ASSERT()s as well?

Actually we need to carefully review all assert()s and FC_ASSERT()s as mentioned in #679 and #511 (comment).

For this one, if it would cause a chain halt, most probably would be caused by FBA buyback.

You replaced the template method with two specific implementations. The implementation for limit_order uses assert(), as did the template, but the implementation for call_order uses FC_ASSERT, which is different from the original. IMO we should stay consistent, either change both or none.

pmconrad · 2018-04-08T19:58:20Z

libraries/chain/db_market.cpp

-                                                                                   // the first param is a new order, thus taker
-   result |= fill_order( core, core_pays, core_receives, true, match_price, true ) << 1; // the second param is maker
+   result |= fill_limit_order( usd, usd_pays, usd_receives, false, match_price, false ); // the first param is taker
+   result |= fill_limit_order( core, core_pays, core_receives, true, match_price, true ) << 1; // the second param is maker
   assert( result != 0 );


assert() -> FC_ASSERT()

pmconrad · 2018-04-08T21:06:12Z

libraries/chain/db_market.cpp

@@ -643,8 +800,11 @@ bool database::check_call_orders(const asset_object& mia, bool enable_black_swan
    bool filled_limit = false;


Please add an FC_ASSERT to check_call_orders that ensures that for_new_limit_order is only true before HF 338 / 625 (see my remark on check_for_blackswan).

Similar as commented above, actually we need to carefully review all assert()s and FC_ASSERT()s as mentioned in #679 and #511 (comment), also be careful when adding new FC_ASSERT()s. Actually I want to get rid of all FC_ASSERT()s or catch them properly when processing post-transaction jobs in _apply_block().

For this one, if we add a FC_ASSERT() here then caused a chain halt, probably would be caused by clear_expired_feeds or FBA buyback.

After HF 388, check_call_orders / check_for_blackswan only operate correctly if the maker flag is false. Adding the suggested FC_ASSERT would enforce that this condition holds even if a future change mistakenly calls check_call_orders with the flag set to true.

General question (not rhetorical): what is worse, a chain halt or a chain operating with undefined and/or wrong behaviour?

IMHO at most times a chain halt is worse, but negative impact of undefined/wrong behavior is unbounded and sometimes is even worse. So I don't have a clear answer. I slightly tend to avoid chain halt.

I think the opposite, especially because wrong behaviour can go unnoticed for a long time.

We need more opinions. :-) @xeroc @oxarbitrage @ryanRfox

pmconrad · 2018-04-08T21:48:17Z

libraries/chain/db_maint.cpp

+      }
+      if( !abd || abd->is_prediction_market ) // nothing to do with PM's; check !abd just to be safe
+         continue;
+      db.modify( call_obj, [&]( call_order_object& call ) {


Please capture only abd

pmconrad · 2018-04-09T12:48:54Z

Please add/modify a test to cover this code path: https://sonarcloud.io/component?id=BitShares-develop%3Adevelop%3Alibraries%2Fchain%2Fdb_market.cpp&line=561

(Create a large limit order that is only partially filled by a margin call.)

abitmore · 2018-04-09T19:57:19Z

@pmconrad done changes for this:

(Create a large limit order that is only partially filled by a margin call.)

//Update:
Ideally we need several test cases for different scenarios when placing a new limit order.

abitmore · 2018-04-12T20:42:26Z

@pmconrad I've changed the assert()s to FC_ASSERT()s and added new FC_ASSERT() as requested. Please check again. WRT the PM black swan issue (#460), my opinion is we don't fix it this time.

abitmore added 24 commits April 7, 2018 06:50

Test case reproduces #338 #343 #453 #606 #625 #649

10298ca

Update tests to get around old feed expiration bug

ceb480a

Refactor and fix new limit order matching. #625

adebe1e

Minor tweak for fill_order(call_order_object)

c3d0acc

Change hard fork to occur at maintenance interval

ea12aa8

Update black swan event detection for #338

0b6bbca

Fix typo

8005788

Skip checking call orders on manually cancel #606

3f699c1

Skip checking calls on limit order expiration #606

29925b1

Rename fill_order funtions to indicate order types

462cf57

Remove unnecessary template match() funtion

3e3c457

Update comments, remove code that is commented out

dbb2dce

Apply_order(): also match calls with CR == MCR.

145b0ad

Be consistent with check_call_orders().

Remove cull_small_test for easier merging

c049b53

Test case for #338 #343 #453 #606 #625 #649 fixes

e5fc5d4

Remove unused CORE_338.hf file

82765cb

Update tests for hard fork on maintenance interval

8ea398d

Re-add CORE_338.hf

9ab327a

Add cross-hardfork tests #338 #343 #453 #606 #649

a8391d8

Update tests for global settlement price

98a2310

Add tests for multiple assets crossing hardfork

43d3795

Update tests about matching calls with CR == MCR

22a2ecc

Recover check_call_order_cull_small_test

62b3a7b

abitmore added the hardfork label Apr 7, 2018

abitmore added this to the 201805 - Consensus Changing Release milestone Apr 7, 2018

abitmore assigned pmconrad Apr 7, 2018

abitmore requested review from xeroc and jmjatlanta April 7, 2018 12:10

abitmore requested review from pmconrad, ryanRfox and oxarbitrage April 7, 2018 12:10

abitmore mentioned this pull request Apr 7, 2018

Market engine improvements #641

Closed

Avoid dereferencing invalidated iterator after hf

e311769

abitmore commented Apr 7, 2018

View reviewed changes

pmconrad suggested changes Apr 8, 2018

View reviewed changes

Minor code tweak

cd7bb4b

Add big limit order test case after hard fork #625

5eb43de

abitmore added 2 commits April 9, 2018 18:39

Minor logging format tweak

d6cdc89

Add FC_ASSERT and change some assert to FC_ASSERT

8dcd9ca

pmconrad approved these changes Apr 12, 2018

View reviewed changes

abitmore unassigned pmconrad Apr 12, 2018

abitmore merged commit 76eb437 into hardfork Apr 12, 2018

abitmore deleted the 201805-market-engine-change1 branch April 26, 2018 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BSIP31-34 market engine improvements #829

BSIP31-34 market engine improvements #829

abitmore commented Apr 7, 2018

abitmore Apr 7, 2018 •

edited

Loading

pmconrad Apr 8, 2018

abitmore Apr 9, 2018

abitmore Apr 9, 2018

pmconrad Apr 12, 2018

abitmore Apr 12, 2018

pmconrad Apr 12, 2018

pmconrad Apr 8, 2018

pmconrad Apr 8, 2018

abitmore Apr 9, 2018

pmconrad Apr 8, 2018

pmconrad Apr 8, 2018

abitmore Apr 9, 2018

pmconrad Apr 9, 2018

pmconrad Apr 8, 2018

pmconrad Apr 8, 2018 •

edited

Loading

abitmore Apr 9, 2018

pmconrad Apr 9, 2018

abitmore Apr 9, 2018

pmconrad Apr 12, 2018

pmconrad Apr 8, 2018

abitmore Apr 9, 2018

pmconrad commented Apr 9, 2018

abitmore commented Apr 9, 2018 •

edited

Loading

abitmore commented Apr 12, 2018

		@@ -643,8 +800,11 @@ bool database::check_call_orders(const asset_object& mia, bool enable_black_swan
		bool filled_limit = false;

BSIP31-34 market engine improvements #829

BSIP31-34 market engine improvements #829

Conversation

abitmore commented Apr 7, 2018

abitmore Apr 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmconrad Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmconrad commented Apr 9, 2018

abitmore commented Apr 9, 2018 • edited Loading

abitmore commented Apr 12, 2018

abitmore Apr 7, 2018 •

edited

Loading

pmconrad Apr 8, 2018 •

edited

Loading

abitmore commented Apr 9, 2018 •

edited

Loading