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

get_key_references performance issue? #1472

Closed
abitmore opened this issue Dec 18, 2018 · 13 comments

Comments

Projects
5 participants
@abitmore
Copy link
Member

commented Dec 18, 2018

Bug Description
If a key is in authorities of many accounts, get_key_references API will return all of them and will subscribe to all of them, this could lead to potential performance issues. This API is used by GUI to get related accounts when importing a key.

bitshares/bitshares-ui#2367 is related.

Update:
due to a bug, (I believe) the API is not subscribing to the returned accounts, so we can

  • completely remove the code about subscribing to accounts

The result can still be too large, E.G. containing thousands of accounts, thus can be problematic.

  • cap size of returned data?

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
@xqcool

This comment has been minimized.

Copy link

commented Dec 19, 2018

IMO,it is api design issue,unless you can limit the searching depth and get the right result.

@clockworkgr

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

wasn't this always standard behaviour? has something changed?

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Dec 25, 2018

Are you worried about the performance of querying for all of them? Or are you worried about the performance of all those subscriptions? Or both?

If we limit the subscriptions, how would we communicate to the user that may be expecting us to subscribe to all of them?

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Dec 25, 2018

This issue is related to UI issue bitshares/bitshares-ui#2367. Please check there for more info.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Dec 25, 2018

get_full_accounts API has a limitation when subscribing to accounts, but get_key_references doesn't. IMHO we should apply same limitation to the two APIs.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2018

i is a vector<graphene::chain::account_id_type>, which according to subscribe_to_item gets fc::raw::pack'd, then inserted into the _subscribe_filter bloom filter.

I would be surprised if that works, as you are subscribing to a vector, and not individual accounts. Is there some magic happening here, or is my surprise warranted?

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

... you are subscribing to a vector ...

LOL. I don't think it will work. So, due to this, we effectively didn't subscribe to anything? As discussed in the pull request, let's remove the code completely.

@abitmore abitmore added this to the 201901 - Feature Release milestone Jan 1, 2019

@abitmore abitmore added this to To do in Feature Release (201902) via automation Jan 1, 2019

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

I think it's still problematic if the returned nested vector is too large. OP updated.

@clockworkgr

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

thing is, UI still does a get_full_accounts on them after received...so it effectively/eventually does subscribe to all of them.

we've had a discussion on UI group about this and it seems this will need some major refactoring (to fix other performance issues as well)

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

Every account ID in returned vector is around 16 bytes, so if the JSON-RPC interface is able to return data packets greater than 1M bytes, that means 64K accounts, so it is a bit hard to attack although possible.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

This has been fixed by PR #1499

Note: There are related outstanding PRs at #782 #1513

@jmjatlanta jmjatlanta closed this Jan 7, 2019

Feature Release (201902) automation moved this from To do to Done Jan 7, 2019

@manikey123

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

@jmjatlanta @pmconrad is this hard coded part to be changed and added to the config file?
53feeff

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

@jmjatlanta @pmconrad is this hard coded part to be changed and added to the config file?
53feeff

Yes. We hard-coded that the user should be limited to passing no more than 100 keys. We would like that to be configurable, so that a node can easily customize the quantity their client can request.

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.