search

bitshares / bitshares-core

BitShares Blockchain node and command-line wallet
https://bitshares.github.io/
Other
1.17k stars 648 forks source link

Change hard-coded limitations in API's to configurable #782

Closed abitmore closed 5 years ago

abitmore commented 6 years ago

Change those FC_ASSERT( limit <= 100 ) limitations to configurable. Inspired by https://github.com/bitshares/bitshares-core/issues/781#issuecomment-375962114.

CORE TEAM TASK LIST

jmjatlanta commented 6 years ago

If rewriting config.ini while the node is running is scary, you may consider writing such settings to a new file, and doing a merge on the next startup.

manikey123 commented 5 years ago

I’m going to claim this @ryanRfox

ryanRfox commented 5 years ago

Glad to have you take this on @manikey123. May I request you begin by offering an estimation for an implementation? It looks like this Issue may have at least one child Issue. You may consider investigating and claiming #626 and #1407 as well. I just need an estimation for each issue you claim. Thanks in advance for your assistance.

manikey123 commented 5 years ago

By going through the code, the config.init file is used by the running node. So do not think it can be modified as it would be in read only mode. Considering non-file based approach and researching that. 2 hours estimated.

manikey123 commented 5 years ago

When bitshares local node starts, plan is to push the config file into memory location say temp table part of database ( for example sql lite ) when node is running, if some one needs to edit the config file , currently readonly, instead the values is pushed into the database tables when node is running to update configuration, use shell scripts that has update query having new information Question : is there need to save the temporary table as when node stops the temp table gets dropped as per sqllite If configuration needs to exist, i can make normal table Let me know feedback so as to proceed implementation.

pmconrad commented 5 years ago

An additional database is overkill for this issue. Please find a simpler solution.

manikey123 commented 5 years ago

-Part1 Assume original config file is parsed by validator function successfully. Hence local node up and running Need Mutex for parsing to avoid race condition

-Part2 Values of config file go into stack memory. Advantage of using stack memory over heap memory is that it would stay alive as long as node is running Timestamp of config file is also stored into stack memory Referencing memory values will help us not access the config file, hence user can edit it when running

-Part3 Config file values can be updated and or new insertions take place To identify these changes, we can use cross-platform libraries such as: https://code.google.com/archive/p/simplefilewatcher/ https://github.com/emcrisostomo/fswatch

-Part4A - New Config values pass Happy path. Repeat Part1 Question : old config values are available as long as node is running. Do we need to archive these ?

-Part4B - New Config values fail Two options : Log failure result Restore original configuration and Discard the change

Feel free to enhance or suggest on above

manikey123 commented 5 years ago

any feedback on above approach ?

pmconrad commented 5 years ago

1: Please optimize for the typical case, i. e. the config hasn't changed. Not sure if a mutex is the best choice.

2: I'd think that having the active config in heap and accessing through shared_ptr is simpler - that way you don't need to keep track of who is still using the old config after an update. The shared_ptr takes care of that. But feel free to implement as you see fit.

3: No additional 3rd-party-libraries please. This task is too simple to justify the additional burden.

4A: no, when new config takes effect the old config can be discarded

4B: Both - log the failure and continue to use old config

manikey123 commented 5 years ago

1: Please optimize for the typical case, i. e. the config hasn't changed. Not sure if a mutex is the best choice.

2: I'd think that having the active config in heap and accessing through shared_ptr is simpler - that way you don't need to keep track of who is still using the old config after an update. The shared_ptr takes care of that. But feel free to implement as you see fit.

3: No additional 3rd-party-libraries please. This task is too simple to justify the additional burden.

4A: no, when new config takes effect the old config can be discarded

4B: Both - log the failure and continue to use old config

manikey123 commented 5 years ago

@pmconrad @ryanRfox : let me know if I can start implementation

ryanRfox commented 5 years ago

@manikey123 Please comment with your effort hours estimates for each Task in your list. You included them to me directly elsewhere, but we need them herein for transparency and for @pmconrad to review and provide additional feedback to me.

I'm going to increase the research estimate to 3.5 hours (from 2) based on suggestions @pmconrad provided you and the adaptations you have considered. Thank you.

manikey123 commented 5 years ago

High level implementation estimate (not started) -Part1 - typical case - 3 hours Assume original config file is parsed by validator function successfully. Hence local node up and running Need Mutex for parsing to avoid race condition

-Part2 - shared_ptr approach - 4 hours Values of config file go into stack memory. Advantage of using stack memory over heap memory is that it would stay alive as long as node is running Timestamp of config file is also stored into stack memory Referencing memory values will help us not access the config file, hence user can edit it when running

-Part3 - tracking timestamp wrapper -2 hours Config file values can be updated and or new insertions take place To identify these changes, we can use cross-platform libraries such as: https://code.google.com/archive/p/simplefilewatcher/ https://github.com/emcrisostomo/fswatch

-Part4B - New Config values fail - log the failure and continue to use old config -1 hour Two options : Log failure result Restore original configuration and Discard the change

pmconrad commented 5 years ago

No additional libraries please. You can use std::filesystem::last_write_time to detect changes.

manikey123 commented 5 years ago

ok. thanks.

manikey123 commented 5 years ago

bitshares account id = betas-11

manikey123 commented 5 years ago

getting familiar and going through code that interacts with config ini ran 6 test cases across the week for the app_test and debugged main program

manikey123 commented 5 years ago

1.Program Flow for First load -> configuration options are stored in memory. 1A> programs/witness_node/main.cpp image

1B> At line 73 - Plugins set the remaining 35 options image

1C> set program options sets the other 15 options total there are 50 configurations image

1D> Check is made if Config.ini exists or not. As this is first load, config.ini does not exist --> config.ini is created from configuration option --> sharepointers values are loaded and on successful validation, node is established

image

2> Second load --> detected based on change in config.ini. Here comparison is made of share pointer values of first and second load. now second load items are considered and if validated successfully, this is considered the new config.ini

3>Clarifications / Inputs Needed:

  1. second load : is testing needed on new sharepointer changed values ? is that in scope or out of scope ?
  2. I seek to add timestamp as configuration option for reading and tracking purpose. Now when the new config file changes, the timestamp changes via std::filesystem::last_write_time . What is recommended place to add this time tracker loop which checks for timestamp change , there I will again call the load_logging_config_file function
  3. Seek to debug share_pointer in debugger. I get error message → RTII share pointer not found

Need to debug share ptr values part of issue 782

In main.cpp, line 65 bitshares-core/programs/witness_node/main.cpp

app_options.add_options() ("help,h", "Print this help message and exit.") ("data-dir,d", bpo::value()->default_value("witness_node_data_dir"), "Directory containing databases, configuration file, etc.") ("version,v", "Display version information") ;

I tried approach gdb and I get the below error (gdb) print (app_options.m_options._M_impl._Mstart)@1 $1 = {{px = 0x561dfcf15d60, pn = {pi = warning: RTTI symbol not found for class 'boost::detail::sp_counted_impl_p' 0x561dfcee58d0}}} (gdb) p $1.px.get() Couldn't find method boost::shared_ptr::element_type::get (gdb) p $1.px.get() Couldn't find method boost::shared_ptr::element_type::get (gdb) p $1.*px.get() No symbol "px" in current context. (gdb) p $1.px._M_ptr There is no member named _Mptr. (gdb) p $1 $2 = {{px = 0x561dfcf15d60, pn = {pi = warning: RTTI symbol not found for class 'boost::detail::sp_counted_impl_p' 0x561dfcee58d0}}}

Can anyone help me debug the above approach or suggest a better approach

pmconrad commented 5 years ago

second load : is testing needed on new sharepointer changed values ? is that in scope or out of scope ?

My idea would be to encapsulate all the config handling and reloading in a class of its own. One instance of that is created on startup and handed over to plugins and application. Existing code should simply continue to use the configuration values from application start, i. e. making all these reloadable is out of scope. You only need to add the testing that you require for the API limits.

What is recommended place to add this time tracker loop which checks for timestamp change

You can create an async task that executes the check and re-adds itself as its last step. See https://github.com/bitshares/bitshares-core/blob/2.0.181127/libraries/net/node.cpp#L4118-L4127 for examples.

It looks like your trying to debug Boost internal data structures. I've never tried that. You may be looking in the wrong place. What are you trying to find?

manikey123 commented 5 years ago

Uploaded code for review : https://github.com/bitshares/bitshares-core/pull/1478

Focusing on these two areas for now in the code:

  1. change them to configurable
  2. add API to fetch the configured values (#626)

Code development covers below scenario: Create a max account history operations limit. This link is configuration. Default value is 100. It can be changed, I have changed this to 300 by changing value in config file.

Clarifications: 1. On running function get_account_history_operations located in file history_api_test.cpp, config.init file is not loaded , can you suggest any approach to test the API ? image

  1. I am using ilog function for debugging the shared_pointers as I face BOOST error for the RTTI symbols not found. In my company the team uses CLION IDE with GDB and found it to be more time-saving than ilog. I am open for any better way to debug and open to connect with any technical member for the same.

manikey123 commented 5 years ago

Added the review comments in request #1478 and test case for the api validation and created a new pull request #1513

This is wrt issue #782 Part A (Completed): get_account_history_operations -made FC_ASSERT limit configurable and wrote the testcase for the same

Testcase Scenarios (Completed for PartA) :

created 1 account with higher return limits(200) created 2 account with higher return limits(200) No asset_create op larger than id1 with higher return limits(200) Limited 1 returns 1 result alice has 1 op returns 1 result create a bunch of accounts and return set the limit to 125 and the max operation size to higher return limits(125)-- operation is limited by the max-operation-limit tickets:

1490

Part B: get_account_history - Do you want me to proceed with Part2 for get_account_history to make its FC_ASSSERT configurable? Test cases already present

get_account_history Part C: Below apis have FC_ASSERT should they be made configurable? Currently test cases not present. If they need to be implemented would need to know the scenarios

get_grouped_limit_orders get_asset_holders_count get_asset_holders get_account_history_by_operations get_relative_account_history get_fill_order_history Let me know any feedback @pmconrad @jmjatlanta

manikey123 commented 5 years ago

Request is to divide 782 into two tickets to manage the code. One is existing 782 and considering creating new ticket for below. Let me know.

Scope of existing 782 ticket :

  1. change them to configurable
  2. add API to fetch the configured values (#626)
  3. get_key_references should not subscribe to related accounts (Issue 1472) #1499

Request to create new ticket for below as it is independent in nature 1.add restricted API for service providers to change the values on the fly (without restarting the server) maybe remember changed value. Not easy since we're using (almost) read-only config.ini and startup parameters right now. @ryanRfox @pmconrad @jmjatlanta

pmconrad commented 5 years ago

Agree to handle config-changing API in a separate ticket. Not sure if this is a necessary feature though - node restart should be fast with the recent performance improvements.

Does anyone have info how long a restart (without replay) takes for a node with some history?

manikey123 commented 5 years ago

Agree to handle config-changing API in a separate ticket. Not sure if this is a necessary feature though - node restart should be fast with the recent performance improvements.

Does anyone have info how long a restart (without replay) takes for a node with some history?

From my side, I tried using the database approach that digital lucifier had provided on telegram group. this took more than two hours for syncing the ledger. this was not even 10% completed. This was for node without replay. This was about 2 weeks back.

pmconrad commented 5 years ago

My question was meant to be about restarting a fully synched node, in order to determine how much restarting for a config change hurts. One answer from telegram channel was 4 minutes I think.

Several telegram members agreed that an API for config changes without a restart would be "nice to have but not very important". It was a very small samle though. Nothing to base a decision on.

manikey123 commented 5 years ago

Updating below the task details with time spent.

Requirement :

  1. change them to configurable
  2. add API to fetch the configured values (#626)

Tasks Details Performed with time spent ( Requirement 1,2): a. Research and development (for point 1,2) 2 hr b. #review 1478 - fixes , testcases, nodestart 5.5 hr c. #review 1513 - rewrite testcase and revision 2 hr d. get_account_history - testcase and code 1.5 hr
e. get_grouped_limit_orders - API research , testcase and implement 2.5 hr f. get_relative_account_history testcase and code 1 hr SubTotal 14.5 hr

Requirement :

  1. add restricted API for service providers to change the values on the fly

Tasks Details Performed with time spent (december): run 6 test cases for restarting configfile 1 hr debugged main program and code map 3 hr SubTotal 4 hr

From Requirement1,2 (remaining items) : get_account_history_by_operations
get_asset_holders
get_key_references should not subscribe to related accounts

manikey123 commented 5 years ago

Completed get_account_history_by_operations get_asset_holders

Uploaded code for review PR: #1513

Remaining item yet to start: get_key_references should not subscribe to related accounts( (Issue 1472) #1499)

manikey123 commented 5 years ago

@jmjatlanta @pmconrad is this hard coded part to be changed and added to the config file? https://github.com/bitshares/bitshares-core/pull/1499/commits/53feeff43874a85bddb052f5d32befe9700002ae

jmjatlanta commented 5 years ago

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

@manikey123 Yes. We hard-coded that the client can pass a maximum of 100 keys to this method call. It would be better if the node administrator could configure that value to what they wish.

ryanRfox commented 5 years ago

Added this to our (next) Feature Release to get some eyes on this.

bitphage commented 5 years ago

What about get_order_book API call? It has a hardcoded limit FC_ASSERT( limit <= 50 );

ryanRfox commented 5 years ago

@bitfag I added get_order_book to #783 for review/implementation.

manikey123 commented 5 years ago

Estimated 3 hrs to fix the code breakage caused due to code merge/ integration Already spent 2 hrs

Made unsigned int changes on the test cases below: api_limit_get_account_history_operations api_limit_get_account_history api_limit_get_relative_account_history api_limit_get_account_history_by_operations api_limit_get_key_references

Below are the breakages due to plugin updates: api_limit_get_grouped_limit_orders

Below api input parameters are changed api_limit_get_asset_holders

manikey123 commented 5 years ago

Integration issue identified and resolved for API get_grouped_limit_orders Solution : Use _app.enable_plugin("grouped_orders"); Clarification needed for the solution : What place should the solution line be placed at ? In test case or API ? preference is for test case as I believe the plugin needs to be checked in the api call. Do suggest.. image

@pmconrad @jmjatlanta

pmconrad commented 5 years ago

Please enable in database_fixture for the specific test case, similar to https://github.com/bitshares/bitshares-core/blob/test-2.0.190212/tests/common/database_fixture.cpp#L146

pmconrad commented 5 years ago

Implemented via #1513 - @cedar-book please check if the config file documentation needs to be updated with the new parameters. https://github.com/manikey123/bitshares-core/blob/78cb2c7c7d4c417602ba51c0d429e3e4af408e97/libraries/app/application.cpp#L1006-L1019

cedar-book commented 5 years ago

@pmconrad, Okay. Yes, I will check into that.

cedar-book commented 5 years ago

@pmconrad, updated: config example information

pmconrad commented 5 years ago

Thanks!