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

move --plugins from application to binary #1427 #1437

Merged
merged 16 commits into from Jan 20, 2019

Conversation

Projects
5 participants
@oxarbitrage
Copy link
Member

commented Nov 15, 2018

I am sorry to have to open a new pull request for this ...

Related pulls and history are in: #1427 and #1431

In this one i used the commits from @nathanhourt but moved the plugin option to the executable as suggested by #1431 (comment)

In witness_node/main.cpp all plugins need to be registered: https://github.com/oxarbitrage/bitshares-core/blob/f5031bde96c5f3e20fc71c1d34a519bb417e850d/programs/witness_node/main.cpp#L74-L82 but only the ones from the command line will be enabled: https://github.com/oxarbitrage/bitshares-core/blob/f5031bde96c5f3e20fc71c1d34a519bb417e850d/programs/witness_node/main.cpp#L110 or the default ones: https://github.com/oxarbitrage/bitshares-core/blob/f5031bde96c5f3e20fc71c1d34a519bb417e850d/programs/witness_node/main.cpp#L115-L118

This will at least remove the hardcoded plugins from the application.

I was not able to use register_plugin<>(true) here as they all need to be registered before parsing the command line but only some enabled after it, however i did not removed the option as there is probably a workaround for my code there that can make it simpler by the use of that.

Another thing changed is enable_plugin() member from private to public to be called by the executable.

Let me know what do you guys think as if we go with this approach i will need to do something similar in delayed node.

A few things i also noticed while working on this and related are, probably to consider as new issues:

  • Some plugins(snapshot, witness) add some log msg to the console when they start so you can know they are loaded, others don't do this so you actually dont know from the console if they were loaded or not. I think we should do something for that.
  • The options you add to the command line are ignored in the config file created, for example if you call the node with witness_node --someoption true, then someoption should be the only line not commented in the config file however this is not the case, it loads the default options. Same behaviour without this pull request(not related to this changes).

nathanhourt and others added some commits Nov 7, 2018

Allow required plugins
As far as I can tell, there was no way previously for an application
to register a plugin and ensure that plugin got loaded -- it would
be necessary to manually edit the config and specify the plugin be
loaded.

This is suboptimal; if third party code wishes to track third party
extensions on the blockchain, the correct way to do this is with a
plugin, and this third party build should be able to load these
required plugins regardless of whether the config lists them or not.

This commit adds a boolean parameter to application::register_plugin
which defaults to false for backwards compatibility; however, if
set to true, the plugin will automatically be enabled when the app
initializes.
Clean up plugin loader
That code was nasty and... kinda wrong. So fix it up all shiny-like.

But I also removed the super annoying default "wanted" plugins list,
which only causes problems for third parties like me, and in general is
just poor form. In my opinion, 5220425
provides a much cleaner way to do this, in a way that is friendly rather
than hostile to third parties.

Would Be Nice: A generalized plugin conflict system added at the
abstract_plugin level, so, for example, the elasticsearch plugin can
conflict account_history and we deal with this in a general fashion
rather than having this dirty special case check here.
@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

Found usage to Nathan true flag to fix the cli_test: adef6a2

}
});
}
else {

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 17, 2018

Contributor

Perhaps remove this else block and set a default value in line 70 instead?

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Nov 17, 2018

Author Member

nice one, added at ce35a79

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2018

So the cli_test crashes were caused by API calls on uninitialized plugins?
We should make sure that such APIs are not accessible.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2018

The options you add to the command line are ignored in the config file created, for example if you call the node with witness_node --someoption true, then someoption should be the only line not commented in the config file however this is not the case, it loads the default options. Same behaviour without this pull request(not related to this changes).

IMO the current behaviour is reasonable. The config file contains the usual defaults, and the option given on the command line overrides the config file.

}

if (options.count("plugins")) {

This comment has been minimized.

Copy link
@pmconrad

pmconrad Nov 18, 2018

Contributor

Hm, isn't this always true now that the option has a default value?

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Nov 18, 2018

Author Member

sorry about that, you are right, fixed at 58ad814

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

Looks good. @nathanhourt what do you think? Does that satisfy your requirements?

@nathanhourt

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

Yep, this looks reasonable to me.

This was referenced Nov 27, 2018

@abitmore

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

Please update title of this PR to clearer. It's annoying to have to click to another issue/PR to see what this PR is about.

@oxarbitrage oxarbitrage changed the title Another one for pull #1427 move --plugins from application to binary #1427 Nov 28, 2018

@abitmore

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

This will break the standalone delayed_node binary, no?

@abitmore you're right.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2018

delayed_node compiles and runs but doesn't activate the delayed_node plugin. :-/

@oxarbitrage please adapt delayed_node/main.cpp in the same way.

auto delayed_plug = node.register_plugin<delayed_node::delayed_node_plugin>();
auto history_plug = node.register_plugin<account_history::account_history_plugin>();
auto market_history_plug = node.register_plugin<market_history::market_history_plugin>();
auto delayed_plug = node->register_plugin<delayed_node::delayed_node_plugin>();

This comment has been minimized.

Copy link
@nathanhourt

nathanhourt Dec 4, 2018

Contributor

Perhaps auto delayed_plug = node->register_plugin<delayed_node::delayed_node_plugin>(true);?

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Dec 5, 2018

Author Member

make sense in this context, added. thanks.

@@ -60,7 +60,7 @@ fc::optional<fc::logging_config> load_logging_config_from_ini_file(const fc::pat

int main(int argc, char** argv) {
try {
app::application node;
app::application* node = new app::application();

This comment has been minimized.

Copy link
@pmconrad

pmconrad Dec 14, 2018

Contributor

Why? Now you have to delete it when you're done.

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Dec 14, 2018

Author Member

i actually changed that because of this:

(gdb) r
Starting program: /home/alfredo/CLionProjects/pull1427_2/programs/delayed_node/delayed_node 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
2133844ms th_a       main.cpp:109                  main                 ] Writing new config file at /home/alfredo/CLionProjects/pull1427_2/delayed_node_data_dir/config.ini

Program received signal SIGSEGV, Segmentation fault.
0x0000555557889fdd in graphene::chain::database::close (this=0x55555b83ab70, rewind=true) at /home/alfredo/CLionProjects/pull1427_2/libraries/chain/db_management.cpp:222
222	         uint32_t cutoff = get_dynamic_global_properties().last_irreversible_block_num;
(gdb) bt
#0  0x0000555557889fdd in graphene::chain::database::close (this=0x55555b83ab70, rewind=true) at /home/alfredo/CLionProjects/pull1427_2/libraries/chain/db_management.cpp:222
#1  0x0000555557240b11 in graphene::app::application::~application (this=0x7fffffffda00, __in_chrg=<optimized out>) at /home/alfredo/CLionProjects/pull1427_2/libraries/app/application.cpp:926
#2  0x0000555557003387 in main (argc=1, argv=0x7fffffffde48) at /home/alfredo/CLionProjects/pull1427_2/programs/delayed_node/main.cpp:63
(gdb) 

This comment has been minimized.

Copy link
@pmconrad

pmconrad Dec 15, 2018

Contributor

Hm, the destructor generates a SIGSEGV and the solution is to avoid calling the destructor? That's evil.
Does the crash also happen without your other changes? If so, this should be kept as-is and fixed properly in another issue. If not you should find the root cause and fix it properly now.

@@ -160,26 +167,24 @@ int main(int argc, char** argv) {
elog("Error parsing configuration file: ${e}", ("e", e.what()));
return 1;
}
if( !options.count("plugins") )

This comment has been minimized.

Copy link
@pmconrad

pmconrad Dec 14, 2018

Contributor

You should keep the "plugins" option. We don't want to change the behaviour of the executable.

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Dec 28, 2018

Author Member

this is done 1326a5c

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2018

In a branch i have handy here based on hardfork without any of the changes:

alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1324_3$ ./programs/delayed_node/delayed_node 
Segmentation fault (core dumped)
alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1324_3$ 

I didn't know the issue was unrelated to the changes done here until now.

So i did it the way the witness_node binary is done but without deleting. What do you think if we add:

int signal = exit_promise->wait();
ilog("Exiting from signal ${n}", ("n", signal));
node->shutdown_plugins();
node->shutdown();
delete node;
return 0;
} catch( const fc::exception& e ) {
// deleting the node can yield, so do this outside the exception handler
unhandled_exception = e;
}
if (unhandled_exception)
{
elog("Exiting with error:\n${e}", ("e", unhandled_exception->to_detail_string()));
node->shutdown();
delete node;
return 1;
}
in this pull to fix it ?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2018

Yes, if that works.
Note that shutdown may be called twice here (same is true in witness_node). But that's an unrelated issue.

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

@pmconrad by going more into this i decided to reverse most of the changes done in the delayed node and just make the delayed node plugin to be loaded correctly into the binary. a8020a7

The delayed node haves several issues that are unrelated to the scope of this pull request, for example:

  • plugins option will not work:

in this patch output will be:

alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1427_2$ ./programs/delayed_node/delayed_node --plugins "whatever"
Error parsing command line: unrecognised option '--plugins'
Segmentation fault (core dumped)
alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1427_2$ 

in unpatched node:

alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1324_3$ ./programs/delayed_node/delayed_node --plugins "whatever"
Segmentation fault (core dumped)
alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1324_3$
  • when required trusted node option is needed:

with this pull:

alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1427_2$ ./programs/delayed_node/delayed_node
Segmentation fault (core dumped)
alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1427_2$ 

without it:

alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1324_3$ ./programs/delayed_node/delayed_node
Segmentation fault (core dumped)
alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1324_3$ 
  • this will work in both versions, this is what really matters:
alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1427_2$ ./programs/delayed_node/delayed_node --trusted-node "127.0.0.1:8090"
3331840ms th_a       main.cpp:109                  main                 ] Writing new config file at /home/alfredo/CLionProjects/pull1427_2/delayed_node_data_dir/config.ini
3331845ms th_a       db_management.cpp:174         open                 ] Wiping object_database due to missing or wrong version
3331845ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
3331845ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.
3331845ms th_a       object_database.cpp:106       open                 ] Opening object database from /home/alfredo/CLionProjects/pull1427_2/delayed_node_data_dir/blockchain ...
3331845ms th_a       object_database.cpp:111       open                 ] Done opening object database.
3331847ms th_a       application.cpp:325           operator()           ] Initializing database...
...

version without patch:

alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1324_3$ ./programs/delayed_node/delayed_node --trusted-node "127.0.0.1:8090"
3278444ms th_a       db_management.cpp:174         open                 ] Wiping object_database due to missing or wrong version
3278444ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
3278444ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.
3278445ms th_a       object_database.cpp:106       open                 ] Opening object database from /home/alfredo/CLionProjects/pull1324_3/delayed_node_data_dir/blockchain ...
3278453ms th_a       object_database.cpp:111       open                 ] Done opening object database.
3278454ms th_a       application.cpp:325           operator()           ] Initializing database...

  • delayed node uses old config functions that are now in config_utils.cpp.
  • etc.

Much of this is unrelated to the scope of this pull(which is already pretty big) so we better treat them somewhere else as you suggested, the important thing to me is that now the delayed node binary of this pull works the same(with the same problems) as the delayed node before.

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

There is another case where the unpatched version was working ok while the patched failing.

It is the case when --plugins option is used with 1 of the 3 allowed plugins from the delayed binary(delayed_node account_history market_history).

With 1326a5c both versions will do the same. For example user will want to load delayed node and market history plugin leaving account history out, he can do that now in the patched version.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2018

These changes look good. I see one comment above that I am unsure that it has been handled. Perhaps it has, but I do not see it.

We should make sure that such APIs are not accessible.

If it has been handled, great. If not, what would be the expected behavior? An exception stating that the function is not available? Or perhaps you cannot even get a pointer to that API to be able to call that function?

std::set<std::string> plugins;
boost::split(plugins, options.at("plugins").as<std::string>(), [](char c){return c == ' ';});

if(plugins.count("account_history") && plugins.count("elasticsearch")) {

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jan 2, 2019

Contributor

This check is useless. Elasticsearch is not available in delayed_node.

oxarbitrage added some commits Jan 16, 2019

@@ -58,7 +58,7 @@ delayed_node_plugin::~delayed_node_plugin()
void delayed_node_plugin::plugin_set_program_options(bpo::options_description& cli, bpo::options_description& cfg)
{
cli.add_options()
("trusted-node", boost::program_options::value<std::string>(), "RPC endpoint of a trusted validating node (required for delayed_node)")
("trusted-node", boost::program_options::value<std::string>()->default_value("127.0.0.1:8090"), "RPC endpoint of a trusted validating node (required for delayed_node)")

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jan 17, 2019

Contributor

This is probably a bad default because that's usually the port where the node itself will be listening. IMO it's better to leave this option without a default. Users should have to actively choose which node they trust.

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Jan 17, 2019

Author Member

I changed it back at e198552

I added it because the binary seg faults when starting with no option:

alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1427_2$ ./programs/delayed_node/delayed_node 
Segmentation fault (core dumped)
alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1427_2$ 

It need to be started with trusted node option:

alfredo@alfredo-System-Product-Name:~/CLionProjects/pull1427_2$ ./programs/delayed_node/delayed_node --trusted-node "127.0.0.1:8090"
2348175ms th_a       main.cpp:111                  main                 ] Writing new config file at /home/alfredo/CLionProjects/pull1427_2/delayed_node_data_dir/config.ini
2348180ms th_a       db_management.cpp:174         open                 ] Wiping object_database due to missing or wrong version
2348180ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
2348180ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.

This problem is independent from the changes in this pull request. Another example of segfault in the delayed_node binary can be observed at the end of the output from ./delayed_node --help

As a workaround i tried: app::application* node = new app::application(); before but was later discarded.

I think we can address this issue in a separate pull request in order to dont complicate things too much further.

Let me know what do you think, thanks again.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jan 20, 2019

Contributor

The segfault will be fixed with #1529. It happens because the delayed_node plugin complains about the missing option and then tries to close a database that has never been opened.

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Jan 20, 2019

Author Member

great, thanks.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2019

Good to go now IMO, thanks!

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2019

thanks @pmconrad merging now.

@oxarbitrage oxarbitrage merged commit bbcbed0 into bitshares:develop Jan 20, 2019

1 of 2 checks passed

ci/dockercloud Your tests failed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

@abitmore abitmore added this to In progress in Feature Release (201902) via automation Jan 20, 2019

@abitmore abitmore moved this from In progress to Done in Feature Release (201902) Jan 20, 2019

@pmconrad pmconrad referenced this pull request Mar 7, 2019

Closed

"plugins" option in config.ini is ignored #1637

1 of 17 tasks complete
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.