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.

Sign up for GitHub

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

Jump to bottom

[Core Plugin DBFT] include consensus time into block interval #3637

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Core Plugin DBFT] include consensus time into block interval #3637

wants to merge 1 commit into from
+8 −2

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Dec 20, 2024 •
edited
Loading

Description

this pr implements issue #3627 to include consensus time into block interval.

Fixes # (issue)

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@roman-khimov roman-khimov mentioned this pull request Dec 20, 2024
Open
shargon
shargon reviewed Dec 20, 2024
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

Just to know

src/Plugins/DBFTPlugin/Consensus/ConsensusService.cs
@@ -95,9 +97,10 @@ private void InitializeConsensus(byte viewNumber)
else
{
TimeSpan span = neoSystem.Settings.TimePerBlock;
if (block_received_index + 1 == context.Block.Index)
if (block_received_index + 1 == context.Block.Index && onPrepareBlockIndex + 1 == context.Block.Index)
Copy link
Member

Choose a reason for hiding this comment

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

This is required? seems a fix for a different thing

vncoelho
vncoelho requested changes Dec 20, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

From my perspective this change is not essential right at this moment.

I am not against the idea of the change itself, but I am not in favor of merging this without strong tests, as we already did in the past.
This requires a setup with delay and a design of experiments that simulate mainnet.
Otherwise, setup some nodes on cloud and test it.

src/Plugins/DBFTPlugin/Consensus/ConsensusService.cs
{
var diff = TimeProvider.Current.UtcNow - block_received_time;
// Include the consensus time into the consensus intervals.
var diff = TimeProvider.Current.UtcNow - onPrepareReceivedTime;
Copy link
Member

Choose a reason for hiding this comment

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

I believe this methodology can trigger strange behaviors in a real scenario, after change views, delays and etc...

@vncoelho
Copy link
Member

My suggestion is that each core dev interested in the PR should setup a node.

I am available for that. Just set a date and my node will be ready to join our Core Testnet.
We can share any needed details internally to make a quick setup.

@Jim8y
Copy link
Contributor Author

Jim8y commented Dec 20, 2024 •
edited
Loading

From my perspective this change is not essential right at this moment.

I am not against the idea of the change itself, but I am not in favor of merging this without strong tests, as we already did in the past.
This requires a setup with delay and a design of experiments that simulate mainnet.
Otherwise, setup some nodes on cloud and test it.

Tests will be done only after this one is properly reviewed, otherwise if this pr keeps changing, comments and suggestions keeps coming, it's meaningless to carry out any real world tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shargon shargon shargon left review comments

@vncoelho vncoelho vncoelho requested changes

@cschuchardt88 cschuchardt88 Awaiting requested review from cschuchardt88

@nan01ab nan01ab Awaiting requested review from nan01ab

@roman-khimov roman-khimov Awaiting requested review from roman-khimov

@AnnaShaleva AnnaShaleva Awaiting requested review from AnnaShaleva

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Waiting for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Jim8y @vncoelho @shargon