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

Refactor/separate block production and running #7012

Merged
merged 14 commits into from
May 15, 2024

Conversation

asdacap
Copy link
Contributor

@asdacap asdacap commented May 13, 2024

  • Separate the building of block and the triggering of the building of the block.
  • Make it easy to identify if a plugin is replacing only the building of the block or signalling of the block production, which does not change a lot actually.
  • Remove the need of IManualBlockProductionTrigger and just call IBlockProducer directly, except of evm_mine which may want to trigger event which is used to put the block in main chain.

Changes

  • Created IBlockProducerRunner which have the interface of the current IBlockProducer.
  • The interface of IBlockProducer is now the same as IManualBlockProductionTrigger.
  • Move any use of IManualBlockProductionTrigger to just calling IBlockProducer directly.

Types of changes

What types of changes does your code introduce?

  • Refactoring

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Nodes on testing

@asdacap asdacap marked this pull request as draft May 13, 2024 05:19
@asdacap asdacap marked this pull request as ready for review May 13, 2024 07:23
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Mixed feelings: It doesn't seem to make things simpler? Also am I right that we are loosing easily triggering block production from a simple plugin as we are not exposing trigger?

event EventHandler<BlockEventArgs> BlockProduced;
Task<Block?> BuildBlock(
BlockHeader? parentHeader = null,
CancellationToken? cancellationToken = null,
Copy link
Member

Choose a reason for hiding this comment

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

minor: convention puts cancellationToken often as last parameter to separate it from more logic-oriented parameters.

Comment on lines 12 to 13
Task Start();
Task StopAsync();
Copy link
Member

Choose a reason for hiding this comment

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

We don't have truly async implementations of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only clique for stop

using Nethermind.Evm.Tracing;

namespace Nethermind.Consensus.Producers
{
public interface IBlockProducerInfo
{
IBlockProducer BlockProducer { get; }
IManualBlockProductionTrigger BlockProductionTrigger { get; }
Func<BlockHeader, bool> Condition { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Cryptic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to custom interface.

@asdacap
Copy link
Contributor Author

asdacap commented May 13, 2024

Yea, it does not look like it help much unfortunately. It does expose IConsensusPlugin.CreateBlockProducerRunner for the purpose of the plugin orchestrating when to produce block. IMO it make it easier for the plugin to arbitrarily create block without creating side effect by calling IBlockProducer.BuildBlock.

@@ -9,7 +9,7 @@ namespace Nethermind.Consensus;

public interface IBlockProducerRunner
{
Task Start();
void Start();
Copy link
Member

Choose a reason for hiding this comment

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

or we could make it a ValueTask if we wanted flexibility

@asdacap asdacap merged commit ff5da7b into master May 15, 2024
67 checks passed
@asdacap asdacap deleted the refactor/separate-block-production-and-running branch May 15, 2024 09:02
@kamilchodola kamilchodola restored the refactor/separate-block-production-and-running branch May 16, 2024 16:24
@kamilchodola kamilchodola deleted the refactor/separate-block-production-and-running branch May 16, 2024 16:34
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.

None yet

2 participants