[Core Plugin DBFT] include consensus time into block interval#3637
[Core Plugin DBFT] include consensus time into block interval#3637Jim8y wants to merge 1 commit intoneo-project:masterfrom
Conversation
| { | ||
| 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) |
There was a problem hiding this comment.
This is required? seems a fix for a different thing
There was a problem hiding this comment.
It's required. In fact, it was also required previously, because node can miss PrepareRequest from the previous round and get a block via P2P in which case using the onPrepareReceivedTime would be wrong.
vncoelho
left a comment
There was a problem hiding this comment.
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.
| { | ||
| var diff = TimeProvider.Current.UtcNow - block_received_time; | ||
| // Include the consensus time into the consensus intervals. | ||
| var diff = TimeProvider.Current.UtcNow - onPrepareReceivedTime; |
There was a problem hiding this comment.
I believe this methodology can trigger strange behaviors in a real scenario, after change views, delays and etc...
|
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. |
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. |
roman-khimov
left a comment
There was a problem hiding this comment.
See nspcc-dev/dbft#56 (comment) for my results, in general the approach is good and it can be done for the good.
Just don't expect this to give you <5% deviations on 3s mainnet, it's good, but not sufficient for that purpose.
| if (block_received_index + 1 == context.Block.Index) | ||
| if (block_received_index + 1 == context.Block.Index && onPrepareBlockIndex + 1 == context.Block.Index) | ||
| { | ||
| var diff = TimeProvider.Current.UtcNow - block_received_time; |
There was a problem hiding this comment.
Likely block_received_time is not needed now.
| { | ||
| 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) |
There was a problem hiding this comment.
It's required. In fact, it was also required previously, because node can miss PrepareRequest from the previous round and get a block via P2P in which case using the onPrepareReceivedTime would be wrong.
|
close as can be done in another pr |
) Close neo-project#3627. A port of neo-project#3637 with format updates. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> Co-authored-by: Vitor Nazário Coelho <vncoelho@gmail.com> Co-authored-by: Shargon <shargon@gmail.com>
Description
this pr implements issue #3627 to include consensus time into block interval.
Fixes # (issue)
Type of change
Checklist: