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

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

Closed
abitmore opened this issue Mar 25, 2018 · 39 comments

Comments

Projects
7 participants
@abitmore
Copy link
Member

commented Mar 25, 2018

Change those FC_ASSERT( limit <= 100 ) limitations to configurable. Inspired by #781 (comment).

  • change them to configurable
  • add API to fetch the configured values (#626)
  • 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.

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
    • Assigned: @manikey123
      • 1. Change them to configurable
        • Research and development (for point 1,2) 2 hr
        • Review 1478 - fixes , testcases, node start 5.5 hr
        • Review 1513 - rewrite testcase and revision 2 hr
        • get_account_history - testcase and code 1.5 hr
        • get_grouped_limit_orders - API research , testcase and implement 2.5 hr
        • get_relative_account_history testcase and code 1 hr
      • 2. Add API to fetch the configured values (#626)
        • get_key_references should not subscribe to related accounts( (Issue 1472) #1499) 3 hr
        • get_account_history_by_operations 1 hr
        • get_asset_holders 2 hr
        • review 1513 again for above Point2 1 hr
      • 3. Add restricted API for service providers to change the values on the fly
        • Research 3.5 hrs
        • Run 6 test cases for restarting configfile 1 hr
        • Debugged main program and code map 3 hr
        • Partial completion, on hold pending decision #782 (comment)
      • Estimated:
        • 1. 12 hours
        • 2. 9 hours
        • 3. 13.5 hours
      • Remittance:
        • 1. 14.5 hours - Week 52
        • 2. 7 hours - Week 5
        • 3. 7.5 hours - Week 5
  • Perform QA/Testing
  • Update Documentation
@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2018

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.

@ryanRfox ryanRfox added this to New -Awaiting Core Team Evaluation in Project Backlog via automation May 24, 2018

@ryanRfox ryanRfox moved this from New -Awaiting Core Team Evaluation to Unassigned - Features & Enhancements in Project Backlog May 24, 2018

@manikey123

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2018

I’m going to claim this @ryanRfox

@ryanRfox ryanRfox removed this from Unassigned - Features & Enhancements in Project Backlog Nov 7, 2018

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

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

@manikey123

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

-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

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

any feedback on above approach ?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

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

  • I shall first focus on building typical case

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.

  • Will explore shared_ptr approach

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

  • ok

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

  • ok
@manikey123

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

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

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

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

@manikey123

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

ok. thanks.

@manikey123

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

bitshares account id = betas-11

@manikey123

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

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::valueboost::filesystem::path()->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.M_start)@1
$1 = {{px = 0x561dfcf15d60, pn = {pi
= warning: RTTI symbol not found for class 'boost::detail::sp_counted_impl_pboost::program_options::option_description'
0x561dfcee58d0}}}
(gdb) p $1.px.get()
Couldn't find method boost::shared_ptrboost::program_options::option_description::element_type::get
(gdb) p *$1.px.get()
Couldn't find method boost::shared_ptrboost::program_options::option_description::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 M_ptr.
(gdb) p $1
$2 = {{px = 0x561dfcf15d60, pn = {pi
= warning: RTTI symbol not found for class 'boost::detail::sp_counted_impl_pboost::program_options::option_description'
0x561dfcee58d0}}}

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

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

Uploaded code for review : #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

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 manikey123 referenced this issue Dec 21, 2018

Closed

782 part1 #1478

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

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

@ryanRfox ryanRfox referenced this issue Feb 4, 2019

Open

Server Info API #626

2 of 3 tasks complete
@bitfag

This comment has been minimized.

Copy link

commented Feb 5, 2019

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

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

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

@ryanRfox ryanRfox moved this from To do to In development in Feature Release (3.1.0) Feb 12, 2019

@manikey123

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

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

  • Are there any more new changes to the config options that need to be factored based on code merge/integration ?
  • To prevent recurrence, recommendation is to update entire code in #783 and merge immediately
    Any better suggestions, let me know
@manikey123

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

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 added a commit that referenced this issue Mar 20, 2019

Merge pull request #1513 from manikey123/782_test_p1
Fix #782: Change hard-coded limitations in API's to configurable
@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

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

This comment has been minimized.

Copy link

commented Mar 20, 2019

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

@cedar-book

This comment has been minimized.

Copy link

commented Mar 27, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Thanks!

@pmconrad pmconrad closed this Mar 27, 2019

Feature Release (3.1.0) automation moved this from In development to Done Mar 27, 2019

@pmconrad pmconrad removed this from In Testing in Community Claims Apr 29, 2019

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.