Native: move MillisecondsPerBlock setting to native Policy by AnnaShaleva · Pull Request #3895 · neo-project/neo

AnnaShaleva · 2025-04-11T11:57:12Z

Description

Move MillisecondsPerBlock protocol configuration setting to native Policy contract.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Check_SetMSPerBlock unit-test

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

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva · 2025-04-11T12:02:48Z

@shargon, your latest reviewcomments from #3622 are fixed/answered. Please, review one more time and raise conversations in this PR if something is not correct.

src/Neo/SmartContract/Native/PolicyContract.cs

Wi1l-B0t · 2025-04-12T01:53:36Z

src/Neo/SmartContract/Native/PolicyContract.cs

+        /// <param name="snapshot">The snapshot used to read data.</param>
+        /// <returns>The block generation time in milliseconds.</returns>
+        [ContractMethod(Hardfork.HF_Echidna, CpuFee = 1 << 15, RequiredCallFlags = CallFlags.ReadStates)]
+        public uint GetMSPerBlock(IReadOnlyStore snapshot)


GetMillisecondsPerBlock and SetMillisecondsPerBlock are better.
Abbreviation is not good idea.

Or getTimePerBlock

Or getTimePerBlock

The problem with getTimePerBlock is that it returns an integer and this integer is not the number of seconds (which is a default unit when we're talking about the time). So it's not clear to the end user what is the unit of the time.

That's why we changed it from GetTimePerBlock to GetMSPerBlock in nspcc-dev/neo-go#3835 (comment).

GetMillisecondsPerBlock and SetMillisecondsPerBlock are better.

From the code prospective I agree, but it's the native contract method, and originally I picked shorter name because it will cost less GAS to invoke GetMSPerBlock (just shorter transaction script).

So given these arguments, do you still want to change this name?

OK, there's not a large diff in GAS price, let's use GetMillisecondsPerBlock and SetMillisecondsPerBlock.

src/Neo/ProtocolSettings.cs

src/Neo/SmartContract/Native/PolicyContract.cs

vncoelho · 2025-04-12T12:41:56Z

src/Neo/SmartContract/Native/PolicyContract.cs

+        /// <summary>
+        /// The event name for the block generation time changed.
+        /// </summary>
+        private const string MSPerBlockChangedEventName = "MSPerBlockChanged";


Also change here, "TimePerBlockChanged" or even better "ConsensusTimePerBlockChanged"

This name must be kept in accordance with the getter/setter name, so we firstly need to agree on #3895 (comment).

src/Plugins/DBFTPlugin/Consensus/ConsensusService.cs

vncoelho · 2025-04-12T12:45:36Z

src/Plugins/DBFTPlugin/Consensus/ConsensusService.cs

        {
            TimeSpan nextDelay = expected_delay - (TimeProvider.Current.UtcNow - clock_started)
-                + TimeSpan.FromMilliseconds(maxDelayInBlockTimes * neoSystem.Settings.MillisecondsPerBlock / (double)context.M);
+                + TimeSpan.FromMilliseconds(maxDelayInBlockTimes * context.TimePerBlock.TotalMilliseconds / (double)context.M);


Later we need to improve this formula as @superboyiii said.

This requires larger extend by factor when block times are slower because the NETWORK P2P delay did not change (it is the same for both cases).
Here we have a parameter that was initially set based on the network delay, that was calibrated for 15s block.

Let's keep this in mind @shargon @AnnaShaleva @Jim8y @roman-khimov

It's a separate issue that needs careful evaluation, so it won't be done in the current PR.

vncoelho

looks all good! Just some minor changes and ready to go

vncoelho · 2025-04-12T13:45:06Z

@Jim8y pay attention to my previous comments, I have just reviewed O.o

shargon · 2025-04-13T11:55:12Z

#3895 (comment) please check it

No functional changes, fix #3895 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva · 2025-04-14T10:39:44Z

@shargon, @vncoelho @Wi1l-B0t please, check conversations and review one more time.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

vncoelho · 2025-04-14T21:40:29Z

@AnnaShaleva good that you fixed RPC, nice catch @shargon

I think it is ready

Wi1l-B0t · 2025-04-15T13:11:05Z

LGTM

* master: DBFTPlugin: include consensus time into block interval (neo-project#3900) Isolate unit tests (neo-project#3904) Use DateTime.UtcNow (neo-project#3902) Update workflow (neo-project#3901) optimize: show state more readable (neo-project#3899) Optimize KeyedCollectionSlim (neo-project#3877) Native: move MillisecondsPerBlock setting to native Policy (neo-project#3895) optimize: history command support (neo-project#3892) Ensure that view interops can't be changed outside (neo-project#3812)

…ct#3895) * docs: add missing hardforks to the node configuration doc Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: move MillisecondsPerBlock setting to native Policy Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: rename [get,set]MSPerBlock to [get,set]MillisecondsPerBlock No functional changes, fix neo-project#3895 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * RpcServer: include dynamic block time into getversion response Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

AnnaShaleva added 2 commits April 11, 2025 14:00

docs: add missing hardforks to the node configuration doc

50e545c

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Native: move MillisecondsPerBlock setting to native Policy

3951a9f

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva added Waiting for Review Hardfork Core labels Apr 11, 2025

AnnaShaleva added this to the v3.8.0 milestone Apr 11, 2025

AnnaShaleva requested review from Jim8y, Wi1l-B0t, cschuchardt88, roman-khimov, shargon and vncoelho April 11, 2025 11:57

AnnaShaleva mentioned this pull request Apr 11, 2025

3s consensus - With consensus time included into consensus intervals. #3622

Closed

9 tasks

cschuchardt88 previously approved these changes Apr 11, 2025

View reviewed changes

shargon reviewed Apr 11, 2025

View reviewed changes

src/Neo/SmartContract/Native/PolicyContract.cs Outdated Show resolved Hide resolved

AnnaShaleva requested a review from shargon April 11, 2025 19:18

Merge branch 'master' into policy-blocktime

7480d77

Jim8y previously approved these changes Apr 12, 2025

View reviewed changes

Wi1l-B0t reviewed Apr 12, 2025

View reviewed changes

shargon reviewed Apr 12, 2025

View reviewed changes

src/Neo/ProtocolSettings.cs Show resolved Hide resolved

Merge branch 'master' into policy-blocktime

f792e43

Jim8y requested a review from shargon April 12, 2025 11:26

vncoelho reviewed Apr 12, 2025

View reviewed changes

src/Neo/SmartContract/Native/PolicyContract.cs Outdated Show resolved Hide resolved

vncoelho reviewed Apr 12, 2025

View reviewed changes

src/Plugins/DBFTPlugin/Consensus/ConsensusService.cs Show resolved Hide resolved

vncoelho reviewed Apr 12, 2025

View reviewed changes

Jim8y requested review from Wi1l-B0t and vncoelho April 12, 2025 13:41

Jim8y added the Critical Issues (bugs) that need to be fixed ASAP label Apr 12, 2025

Native: rename [get,set]MSPerBlock to [get,set]MillisecondsPerBlock

8e5e213

No functional changes, fix #3895 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva dismissed stale reviews from cschuchardt88 and Jim8y via 8e5e213 April 14, 2025 10:35

AnnaShaleva requested review from Jim8y and cschuchardt88 April 14, 2025 10:58

RpcServer: include dynamic block time into getversion response

ea04f2e

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

shargon approved these changes Apr 14, 2025

View reviewed changes

shargon added Ready to Merge and removed Waiting for Review labels Apr 14, 2025

vncoelho approved these changes Apr 14, 2025

View reviewed changes

NGDAdmin added 2 commits April 15, 2025 13:55

Merge branch 'master' into policy-blocktime

7fe779d

Merge branch 'master' into policy-blocktime

c76d28f

Wi1l-B0t approved these changes Apr 15, 2025

View reviewed changes

cschuchardt88 approved these changes Apr 15, 2025

View reviewed changes

NGDAdmin merged commit 66717c9 into master Apr 16, 2025
7 checks passed

NGDAdmin deleted the policy-blocktime branch April 16, 2025 03:08

This was referenced Apr 16, 2025

DBFTPlugin: include consensus time into block interval #3900

Merged

Decrease block time to 3 seconds on N3 networks nspcc-dev/neo-go#3745

Closed

mialbu mentioned this pull request Apr 28, 2025

[Neo 3.8] Move MillisecondsPerBlock to native Policy contract neow3j/neow3j#1112

Closed

Conversation

AnnaShaleva commented Apr 11, 2025

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

AnnaShaleva commented Apr 11, 2025

Uh oh!

Uh oh!

Wi1l-B0t Apr 12, 2025

Choose a reason for hiding this comment

Uh oh!

shargon Apr 12, 2025

Choose a reason for hiding this comment

Uh oh!

vncoelho Apr 12, 2025

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vncoelho Apr 12, 2025

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vncoelho Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

vncoelho left a comment

Choose a reason for hiding this comment

Uh oh!

vncoelho commented Apr 12, 2025

Uh oh!

shargon commented Apr 13, 2025

Uh oh!

AnnaShaleva commented Apr 14, 2025

Uh oh!

vncoelho commented Apr 14, 2025

Uh oh!

Wi1l-B0t commented Apr 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AnnaShaleva Apr 14, 2025 •

edited

Loading

vncoelho Apr 12, 2025 •

edited

Loading