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 check_call_orders not only when settlement_price changed #935

Closed
abitmore opened this issue May 19, 2018 · 8 comments
Closed

Call check_call_orders not only when settlement_price changed #935

abitmore opened this issue May 19, 2018 · 8 comments

Comments

@abitmore
Copy link
Member

Actually changing either of these fields in price_feed struct should trigger check_call_orders:

  • settlement_price
  • maintenance_collateral_ratio

Another field is maximum_short_squeeze_ratio, seems no need to call check_call_orders after it changed, but it's safe to check as well.

@abitmore abitmore added this to the 201805 - Consensus Changing Release milestone May 19, 2018
@abitmore
Copy link
Member Author

Specifically, these checks are NOT enough:

const auto old_feed_price = bdo.current_feed.settlement_price;
bdo.update_median_feeds( db.head_block_time() );
// We need to call check_call_orders if the settlement price changes after hardfork core-868-890
return after_hf_core_868_890 && old_feed_price != bdo.current_feed.settlement_price;

bool median_changed = ( old_price != bitasset_data.current_feed.settlement_price );

@abitmore
Copy link
Member Author

Need to change to check like this:

if( !(old_feed == bad.current_feed) )

@abitmore abitmore added the bug label May 21, 2018
@abitmore abitmore self-assigned this May 21, 2018
@abitmore
Copy link
Member Author

Question: is it better to fix this directly, or fix with a new hard fork (actually only for testnet)?

@abitmore
Copy link
Member Author

@pmconrad @jmjatlanta please comment.

@jmjatlanta
Copy link
Contributor

jmjatlanta commented May 21, 2018

What we have added in the latest hardfork is an improvement. I agree that these additional cases should trigger a check_call_orders.

If I'm understanding your question correctly:

A new hardfork check gives us more flexibility at low cost IMHO, and as always, can be removed if nothing triggers.

I defer to you and @pmconrad as to deciding where in the release timeline this fix should be placed.

@abitmore
Copy link
Member Author

Actually this can cause more serious issues, since it may incorrectly skip black swan checks, when combined with changes for BSIP31-34, we won't check for black swan when placing new limit orders after hf (probably a bad decision, too much performance related consideration led to poorer error tolerance), so when a new limit order is placed, collateral ratio of matching call order may be less than 100%... I'm not going to reason the final consequences, anyway, we'll fix this issue before production release, but it's less important (also perhaps no enough time) to make it perfect for testnet (where bad things may or may not happen). @pmconrad what's your opinion?

@pmconrad
Copy link
Contributor

Agree to include the fix now.

@abitmore
Copy link
Member Author

Fixed with #965.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants