Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call price is inconsistent when MCR changed #1270

Closed
abitmore opened this issue Aug 21, 2018 · 5 comments

Comments

@abitmore
Copy link
Member

commented Aug 21, 2018

Bug Description
According to discussion in https://bitsharestalk.org/index.php?topic=26496.0

... in current system, we don't refresh the "call price" cache when MCR changed, that said, existing short positions' call prices can be inconsistent with latest MCR. In the meanwhile, call prices of new positions or updated positions will be always up-to-date as long as MCR didn't change. This may lead to unintended margin calls when MCR decreased, or unintended inability to call when MCR increased.

To solve the cache inconsistency issue, we have options:

  1. totally drop the caching mechanism, calculate call prices on the fly (may impact order matching performance)

  2. update "call price" cache immediately when MCR changed (may impact performance since possibly need to update lots of data at a time)

  3. update "call price" cache at maintenance interval since maintenance interval is meant to be used to process mass calculation

3.1 if we don't change MCR update behavior, aka update "current effective" MCR directly when feed changed, then the call price cache will be still inconsistent before maintenance interval;

3.2 we can defer MCR update to maintenance interval, that said, when detected a MCR change due to feed change or asset option e.g. feed lifetime change, don't apply the MCR update immediately, but apply it in next maintenance interval. With this approach, the call price data will be always consistent to "current effective" MCR.

#941 is an attempt to implement 3.2.

Later discussion suggests it's better to go with option 1, and perhaps can use a cache on median price feed to improve performance (need test).

Impacts
Describe which portion(s) of BitShares Core may be impacted by this bug. Please tick at least one box.

  • API (the application programming interface)
  • Build (the build process or something prior to compiled code)
  • CLI (the command line wallet)
  • Deployment (the deployment process after building such as Docker, Travis, etc.)
  • DEX (the Decentralized EXchange, market engine, etc.)
  • P2P (the peer-to-peer network for transaction/block propagation)
  • Performance (system or user efficiency, etc.)
  • Protocol (the blockchain logic, consensus, validation, etc.)
  • Security (the security of system or user data, etc.)
  • UX (the User Experience)
  • Other (please add below)

CORE TEAM TASK LIST

  • Evaluate / Prioritize Bug Report
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation

@abitmore abitmore added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Aug 21, 2018

@abitmore abitmore self-assigned this Aug 31, 2018

abitmore added a commit to abitmore/bitshares-core that referenced this issue Sep 1, 2018

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2018

Note: #460 makes the situation a bit more complicated, that said, MCR is now affecting PMs as well although it shouldn't. In principle we keep the behavior unchanged, but for better code maintenance-ability, it's better to still use same code for both PMs and non-PM bitAssets (that means we may change the behavior of PMs a bit).

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2018

I think it's better to modify the median calculation a bit.

Given:

MCP  = settlement_price * MCR;
MSSP = settlement_price * MSSR;
feed = { settlement_price, MCR, MSSR, CER,
         (MCP), (MSSP) } // using `()` here because they are derivations

Explanation: for call orders, define CP = collateral_amount/debt_amount for call orders. When CP is smaller than MCP, the order enters margin call territory (will get margin called if can match a limit order on the opposite position). The worst price it will sell its collateral is MSSP.

Currently,

median_feed = { median(settlement_price), median(MCR), median(MSSR), median(CER),
                median(settlement_price) * median(MCR),
                median(settlement_price) * median(MSSR) }

When MCR and MSSR are static, curve of median MCP and MSSR would be smooth, and result would be expected; when MCR or MSSR is dynamic, we may get unexpected result. I think the formula below is better:

median_feed = { median(settlement_price), median(MCR), median(MSSR), median(CER),
                median(settlement_price * MCR),
                median(settlement_price * MSSR) }

For example, given this data set:

feed1 = { price=1.0, MCR=2.5 }
feed2 = { price=2.0, MCR=1.5 }
feed3 = { price=3.0, MCR=2.0 }

Result of current formula would be:

median_MCP = median(1.0, 2.0, 3.0) * median(2.5, 1.5, 2.0) = 2.0 * 2.0 = 4.0

Result of the revised formula would be:

median_MCP = median(1.0*2.5, 2.0*1.5, 3.0*2.0) = median(2.5, 3.0, 6.0) = 3.0

Thoughts?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

I think it makes more sense to view SP, MCR and MSSR as independent values, and it is easier for market participants to think in terms of "I need 1.75 times collateral" and "on margin call I will lose 10%". With the new formula, behaviour would be much less predictable and consequently much more difficult to follow.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2018

@pmconrad I disagree. MCR and MSSR are subjective parameters, while settlement price was objective before BSIP42. For objective data, it makes sense to get the median input; but for subjective data, IMHO it's better to get the input as a whole to achieve combined effects. Combining subjective input from different sources will likely lead to unexpected consequences.

For better UX, we need to keep the numbers consistent, that said, if we use this formula:

median_feed = { median(settlement_price), median(MCR), median(MSSR), median(CER),
                median(settlement_price * MCR),
                median(settlement_price * MSSR) }

It's possible that median_feed.(MCP) != median_feed.settlement_price * median_feed.MCR.

To solve this inconsistency, we may change the algorithm to:

median_MCP = median((MCP)) = median(settlement_price * MCR);
accepted_feed = the corresponding feed whose (MCP) has been assigned to median_MCP;
median_feed = accepted_feed;

Note: it's possible that median(settlement_price * MCR) and median(settlement_price * MSSR) are from different witnesses. For consistency, we may need to use all data from one witness. However, if the other data is not median, it means it can be far off thus possibly lead to serious issues.

Thoughts?

abitmore added a commit that referenced this issue Sep 22, 2018

@ryanRfox ryanRfox referenced this issue Nov 8, 2018

Closed

Test Cases for PR 1270 (MCR Bugfix) #1423

6 of 10 tasks complete

@ryanRfox ryanRfox added this to To do in Protocol Upgrade Release (4.0.0) via automation Feb 1, 2019

@ryanRfox ryanRfox removed this from In Development in Protocol Upgrade Release (3.0.0) Feb 1, 2019

@ryanRfox ryanRfox added this to To Do in Protocol Upgrade Release (3.0.0) via automation Feb 1, 2019

@ryanRfox ryanRfox moved this from To Do to In Development in Protocol Upgrade Release (3.0.0) Feb 1, 2019

@pmconrad pmconrad moved this from In Development to In Review in Protocol Upgrade Release (3.0.0) Feb 5, 2019

abitmore added a commit that referenced this issue Feb 5, 2019

Merge pull request #1324 from bitshares/1270-fix-call-price-cache
Fix #1270 Call price is inconsistent when MCR changed
@abitmore

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

Fixed by #1324.

@abitmore abitmore closed this Feb 5, 2019

Protocol Upgrade Release (3.0.0) automation moved this from In Review to Done Feb 5, 2019

abitmore added a commit that referenced this issue Mar 24, 2019

Rewrite globally_settle() with a template function
The distinction before/after hardfork #1270 (Call price inconsistent when MCR changed) now looks clearer.
#1669

@abitmore abitmore referenced this issue Apr 23, 2019

Closed

[3] 3.0.0 call_price field #2634

4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.