Bitshares-core: Review subscription API

[abitmore]

As mentioned in https://github.com/bitshares/bitshares-ui/issues/1338#issuecomment-375854295, the node is probably pushing unwanted data to the client.

I still get spammed with 2.8.x and 5.1.x and 5.2.x objects for example that I have no interest in.

OK, we do use the 2.9.x objects but none of the others are used by the GUI.

This ticket is a place holder for reviewing all the related issues. Please create new tickets for individual issues, and refer to this ticket.

issues found:

• [x] these APIs didn't actually subscribe to the objects queried:
  
  
  
  • get_assets
  
  
  • get_accounts
    
    Solution: subscribe.
  
  
• [x] lookup_accounts will always and only subscribe to the 2nd last account in the returned vector when limit > 1 and sizeof(returned vector)>=limit-1, but won't subscribe to any account when limit == 1
  Solution: subscribe only when limit == 1
• [x] get_key_references will subscribe to keys and addresses derived from keys, but the subscription is not used anywhere
  Solution: don't subscribe
• [x] get_balance_objects will subscribe to addresses, but the subscription is not used anywhere
  Solution: don't subscribe

CORE TEAM TASK LIST

• [ ] Evaluate / Prioritize Feature Request
• [ ] Refine User Stories / Requirements
• [ ] Design / Develop Solution
• [ ] Perform QA/Testing
• [ ] Update Documentation

[abitmore]

I guess we don't have enough time to finish this in next milestone. Moving to future milestone.

[jmjatlanta]

The following implementation details may help with the related tickets.

There are 3 collections that have to do with subscriptions, according to the database_api.cpp:

1. the _subscribe_filter which is a bloom filter
2. the _subscribed_accounts which is a collection of accounts
3. the _market_subscriptions which is a collection of markets (asset_id_type pairs)

_subscribe_filter is a filter to determine if you are subscribed to a certain event. Calling cancel_all_subscriptions() or set_subscribe_callback(func, bool) reset _subscribe_filter.
The following method calls add object ids to this filter:

• get_accounts(vector)
• get_assets(vector)
• get_balance_objects(vector)
• get_full_accounts(vector, bool) (you can opt-out via parameter)
• get_key_references(vector) which subscribes to 6 different items
• get_objects(vector) adds if the object_id_type is not (operation_history_object_type.protocol_ids or impl_account_transaction_history_object_type.implementation_ids)
• lookup_accounts(string, uint32_t)

The _subscribed_accounts collection can be added to by calling get_full_accounts(vector, bool). The boolean must be true to subscribe. To clear the collection, cancel_all_subscriptions() or set_subscribe_callback(func, bool) can be called.

The _market_subscriptions collection can be added to by calling subscribe_to_market(func, asset_id_type, asset_id_type). unsubscribe_from_market(asset_id_type, asset_id_type) will unsubscribe from a particular market. cancel_all_subscriptions() will remove all pairs from the collection.

[cogutvalera]

@ryanRfox @abitmore @jmjatlanta I want to claim this issue, is it possible ?

[abitmore]

Updated status to "gathering requirements". Not ready for development IMHO. @cogutvalera to claim this, need to work out the requirements first. Thanks.

[cogutvalera]

@abitmore Thanks ! OK will wait this issue later then :-)

[abitmore]

@cogutvalera you can help gather requirements as well.

[cogutvalera]

@abitmore yes sure ! Thanks !

[ryanRfox]

Assigning @cogutvalera to get us started. Perhaps this Issues becomes the Task List ticket and references three (3) new Task Issues, one for each collection @jmjatlanta described above. I'd like to see these new Issues contain specific details within. Also, easier to estimate smaller scopre of effort. Each are likely able to be merged separately to provide incremental value, rather than all at once in a single Issue/PR. I'm open to suggestions how best to represent this.

[abitmore]

@ryanRfox I've invited @cogutvalera to the organization. After he accepted, we can assign issues/PRs to him directly.

[cogutvalera]

@abitmore Thank you very much ! I've accepted your invitation ! Very appreciate it ! Thanks a lot !

[cogutvalera]

After some researches and according to the next code & comment:

https://github.com/bitshares/bitshares-core/blob/develop/libraries/app/include/graphene/app/database_api.hpp#L145-L154

/**
       * @brief Register a callback handle which then can be used to subscribe to object database changes
       * @param cb The callback handle to register
       * @param nofity_remove_create Whether subscribe to universal object creation and removal events.
       *        If this is set to true, the API server will notify all newly created objects and ID of all
       *        newly removed objects to the client, no matter whether client subscribed to the objects.
       *        By default, API servers don't allow subscribing to universal events, which can be changed
       *        on server startup.
       */
      void set_subscribe_callback( std::function<void(const variant&)> cb, bool notify_remove_create );

If this is set to true, the API server will notify all newly created objects and ID of all
newly removed objects to the client, no matter whether client subscribed to the objects.

As mentioned in bitshares/bitshares-ui#1338 (comment), the node is probably pushing unwanted data to the client:

OK, we do use the 2.9.x objects but none of the others are used by the GUI.
I've been doing quote a bit of testing with the boolean set to false and it works much better than I expected. Both new/changed margin positions show up correctly, and placing and cancelling orders works fine as well. I'm OK with us setting it to false in staging in the GUI for now, which will allow developers and testers to discover potential issues. Unless we discover something major we can then include it in the 0415 release.

So default behavior (as mentioned in code & comment above) of the node is to push unwanted data to the client no matter whether client subscribed to the objects (in event of flag is set to TRUE), otherwise with FALSE flag as mentioned by @svk31 all works fine enough.
Will continue my researches in more details ...

So guys should we change the default behavior ? If YES, then flag TRUE must work the same way as flag FALSE, no difference between both bool values ? Or there are any other requirements also ? Which logic behavior do we expect here ?

Thanks !

[cogutvalera]

Also according to @abitmore comment here:

https://github.com/bitshares/bitshares-core/pull/725#issuecomment-373529922

I guess nobody is using this flag except probably the block chain explorers and arbitrage bots. When it's set to true, witness node (API server) will push every new object and ID of every removed object to the client, no matter whether they're subscribed to, which IMHO can be used to DDoS public API servers.

IMHO we need to change this logic in event of flag is set to TRUE value in order to have some limitations against DDoS public API servers. Should we add time limitations or maybe some other limitations in this logic against DDoS attacks if we don't want to remove fully and completely current logic when flag is set to TRUE ? What are your thoughts guys ?

Thanks !

[abitmore]

@cogutvalera I believe your comments above will be addressed with #1049.

[ryanRfox]

Do we have any related Issues for the Task List https://github.com/bitshares/bitshares-core/issues/777#issuecomment-392164949 @jmjatlanta outlined above?

[sschiessl-bcp]

Added an overview here to keep track through bitsharesjs

https://github.com/bitshares/bitshares-ui/issues/2612

[sschiessl-bcp]

This issue is a real blocker for performance improvements on the reference UI.

I would like to request:

• all backend calls that do set subscriptions receive an optional flag to opt-out of subscription
• add a unsubsribe call that has a list of ids which in turn become unsubscribed if subscribed, otherwise nothing (no matter what the object is). Could optionally allow Wildcards (call with "1.2." to unsubscribe all accounts)

Sorry for jumping the gun here, how quick could this be supported?

@ryanRfox

[abitmore]

IIRC subscribed object IDs are stored in a bloom_filter which doesn't support delete, that means you can only add new IDs or clear all. bloom_filter was chosen (by BM) to save RAM. See https://github.com/bitshares/bitshares-core/issues/726

[sschiessl-bcp]

IIRC subscribed object IDs are stored in a bloom_filter which doesn't support delete, that means you can only add new IDs or clear all. bloom_filter was chosen (by BM) to save RAM. See #726

Ah, thank you!

Just the opt out then? Possible a set_subscribe_behavior that toggles the default subscription behavior (only subscribe if I add the optional parameter in every call with true)

[abitmore]

@sschiessl-bcp that's what we were discussing in #726

[sschiessl-bcp]

@sschiessl-bcp that's what we were discussing in #726

Sorry, should have read more details on it, that would be a big help. To ensure proper UX in the frontend we would need fine-control what subscribes and what not, thus the optional parameter in every call, also the stop_subscribe should come with a start_subscribe

[ryanRfox]

This is a priority issue identified by the UI Team. I’m looking for a Core Team Member to self assign this one. @MichelSantos is most up to date following conversations with @sschiessl-bcp and @startailcoon

[abitmore]

@ryanRfox this or #726?

[abitmore]

Found some bugs, added to OP, fixed in #1731.

A new API set_auto_subscription is added in #1731, which can be used to change auto-subscription behavior of follow-up API calls, as described in #726.

These topics haven't been addressed in #1731, since we haven't made a decision:

If we want to be able to optionally subscribe on every API call, we can't add a parameter to the old APIs (so far), instead, we need to add new APIs with new parameter list, otherwise it will break existing client applications. It would need some efforts to make APIs support variable number of arguments.

If we want to be able to unsubscribe to

• full accounts (subscribed with get_full_accounts), we can add a new API, which is not hard;
• single objects (subscribed with get_objects and etc), we need to replace the bloom_filter with something else, which would need some efforts (to research, benchmark and etc).

[nathanhourt]

If we want to be able to optionally subscribe on every API call, we can't add a parameter to the old APIs (so far), instead, we need to add new APIs with new parameter list, otherwise it will break existing client applications. It would need some efforts to make APIs support variable number of arguments.

OK, APIs now support variable number of arguments: https://github.com/bitshares/bitshares-fc/pull/126

[abitmore]

Merged PR #1731, closing this issue. Created issue #1819 for adding optional parameters to the APIs.