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 block generation to the witness plugin #2861

Merged
merged 23 commits into from
Oct 17, 2018

Conversation

sgerbino
Copy link
Contributor

Closes #2717.

@mvandeberg
Copy link
Contributor

You re-created generate_block in the witness plugin, but did not remove it from database

@sgerbino sgerbino changed the title WIP: Move block generation to the witness plugin Move block generation to the witness plugin Sep 5, 2018
@mvandeberg
Copy link
Contributor

Let's add a method in chain_plugin called, register_block_generator that accepts a function pointer/lambda and we'll call that on a block generate write request. This does not remove the ping pong between the chain and witness plugin on the call stack, but it will remove the source dependency between them. This is the pattern APIs and the jsonrpc plugin use so that the jsonrpc plugin can call APIs without needing to include any information about them during compilation.

…within the witness plugin, emit a warning when overriding the block generator #2717
@theoreticalbts
Copy link
Contributor

There's a race condition here. The write processing thread is started by chain_plugin::plugin_startup(), which AFAIK because plugins execute in dependency order, occurs before witness_plugin::plugin_startup(). The write processing thread reads the block_generator field once when it starts, it never reads the block generator field again.

I suggest handling this by the following strategy:

  • Get rid of the block generator fields in chain_plugin, have it only exist on the write_request_visitor object which is local to the write processing thread
  • To get the block generator into the write_request_visitor field, send it as a queue item (i.e., write_request_ptr member type)

It is also tricky to keep races from occurring during shutdown. Can a block production request be in the queue when the witness plugin is destroyed? If it could, then when executing the block production request, it would try to reference a non-existing block_producer, since the block_producer is "owned" by the witness plugin.

I don't think this shutdown race can occur if we only call chain_plugin::generate_block() from witness_plugin::generate_block() . However, I don't think it's good to rely on that.

Fortunately there's a straightforward refactor which will prevent this shutdown race:

  • Replace the block_producer_func typedef with a class which has a generate_block() abstract virtual method with the same signature as block_generator_func. Let's call this class abstract_block_producer.
  • Instead of passing around block_generator_func*, instead use shared_ptr< abstract_block_producer >.
  • Since abstract_block_producer subclasses can be destroyed through a pointer to the base class, abstract_block_producer must define a virtual destructor.

@theoreticalbts
Copy link
Contributor

theoreticalbts commented Sep 11, 2018

Turns out the startup race can't occur because the block producer's registered in plugin_initialize, and the thread is started in plugin_startup.

Copy link
Contributor

@mvandeberg mvandeberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not merging yet, but lgtm

@mvandeberg mvandeberg merged commit c0b3837 into master Oct 17, 2018
@mvandeberg mvandeberg deleted the 2717-move-block-generation-to-witness-plugin branch October 17, 2018 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants