3s consensus - With consensus time included into consensus intervals. by Jim8y · Pull Request #3622 · neo-project/neo

Jim8y · 2024-12-12T15:01:42Z

Description

Credits of this pr belong to @AnnaShaleva. Changes included into this PR:

Move MSPerBlock protocol configuration to native Policy contract.
Take into account PrepareRequest receival time during dBFT timer reset (close [Consensus] Include the consensus time into block interval. #3627).

Type of change

Enhancement.

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

Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>

Jim8y · 2024-12-12T15:03:34Z

src/Plugins/DBFTPlugin/Consensus/ConsensusService.cs

-                    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;
+                        // Include the consensus time into the consensus intervals.
+                        var diff = TimeProvider.Current.UtcNow - onPrepareReceivedTime;


This is the major change to this pr

This should be in another PR and not here, if that is the case.

And for your notice, as I said in our meetings, this configuration has been tested in the past.

I think that you need more experimental tests before publishing things to quick and even writing new standards (that are not even necessary).
I am not saying that the change is terrible, @Jim8y, but there are some consequences, I think that you did not analyse the entire scenario before pushing this and creating standards with it.

I will surely be against this change here in this PR for a series of reasons.
Try this in a decentralized scenario or inserting delays (you can deal with that on docker) or create on AWS/AZURE in different locations.

this configuration has been tested in the past.

Lots have changed in code since then. Would that affect anything like this? if so, we have to run these tests again to ensure its trueful.

This should be in another PR and not here, if that is the case.

And for your notice, as I said in our meetings, this configuration has been tested in the past.

I think that you need more experimental tests before publishing things to quick and even writing new standards (that are not even necessary). I am not saying that the change is terrible, @Jim8y, but there are some consequences, I think that you did not analyse the entire scenario before pushing this and creating standards with it.

I will surely be against this change here in this PR for a series of reasons. Try this in a decentralized scenario or inserting delays (you can deal with that on docker) or create on AWS/AZURE in different locations.

We have setup a real testnet to carry out stress test and stability test for weeks. The network works fine and no issue. As you said should be in another pr thing, all changes in this pr now is for the service of and only serve the 3-seconds consensus, it is meaningless to add any of the prs and test any of the prs that only contain part of the changes in this pr. Casue non of the code in this pr means a thing without rest part.

We encourage small prs for the intention of easier review and test, its not like everything must be as small as possible.
If there is no review issue and no real logic problem, we should not split a complete and already tested pr.

Lots have changed in code since then. Would that affect anything like this? if so, we have to run these tests again to ensure its trueful.

You are right @cschuchardt88 , I was also saying that in my last message. Let's see

Jim8y · 2025-02-11T07:45:17Z

src/Neo/SmartContract/Native/NeoToken.cs

        }

+        [ContractMethod(CpuFee = 1 << 15, RequiredCallFlags = CallFlags.ReadStates | CallFlags.WriteStates)]
+        public BigInteger GetGasPerBlock(ApplicationEngine engine, DataCache snapshot)


@shargon @roman-khimov please check this, i am try to update the gasperblock without really update it casue it should be a council thing.

You can change the arguments without hardfork, because both arguments are native ones

But we should not write into storages here... should be done by council

Maybe CN can detect when this is made by council and start the new time?

Hi shargon, i think my latest commit has addressed your concern:

reuse existing GetGasPerBlock

avoid write to the store but directly calculate a value

ensures the council can still update the gasperblock, and the new value can be used immediately.

UT updated accordingly.

I intentially have two return currentgasperblolck to make the logic more clear as they are different situations. Code can be optimized if the logic passes review if necessary.

The ContractMethod cannot have ApplicationEngine and DataCache parameters at the same time.

roman-khimov

Most of the changes in this PR are just not needed at all (the only relevant is timer shift in fact).
No hardfork is needed to change the block time in the current structure. It's just a matter of restarting consensus nodes with appropriate configuration.
No native contract changes are required to change the GAS per block value. It's just a matter of sending a committee transaction. This one is really serious, btw, we do have a governance model and your changes just break it.
Give 2 and 3 we can just schedule 3s block time to some post-Echidna date, create and send a transaction, restart CNs. This won't and can't be perfectly aligned, true, but I doubt it's a problem in practice. Changing governance assumptions behind the scenes is much bigger problem.
But this still doesn't answer the time lock problem we can have with mainnet.

src/Neo/ProtocolSettings.cs

src/Neo/SmartContract/Native/NeoToken.cs

Jim8y · 2025-04-10T09:23:39Z

@Jim8y Conflicts, I need more tests..

@shargon, it looks like I'm the maintainer of this PR now, so conflicts are fixed, please take a look one more time. I'll add a unit test for Policy changes later. @vncoelho, we also need your review.

This is in the C# code and is of course a c# implementation documentation and is not an NEP. The NEP that i want to have on 3s is under the proposal repo. If the name is misleading

@Jim8y, regarding this NEP: it's not only about the name is misleading, it's more about the fact that content of this file diverges from the current implementation. Since you haven't pushed any fixes, I removed this file from the current PR in order not to delay 3-s consensus changes. Later, when you have time, you may create a separate PR to update the documentation part, and we'll review it.

Let me remove the document first, then add another version after the release of v3.8, looks like you already did, great.

src/Neo/SmartContract/Native/PolicyContract.cs

src/Plugins/StateService/Verification/VerificationService.cs

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

Fix neo-project#3622 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva · 2025-04-10T09:50:27Z

I'll add a unit test

Added.

No functional changes, just remove useless changes and adjust documentation. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

vncoelho · 2025-04-10T10:21:55Z

@Jim8y Conflicts, I need more tests..

@shargon, it looks like I'm the maintainer of this PR now, so conflicts are fixed, please take a look one more time. I'll add a unit test for Policy changes later. @vncoelho, we also need your review.

This is in the C# code and is of course a c# implementation documentation and is not an NEP. The NEP that i want to have on 3s is under the proposal repo. If the name is misleading

@Jim8y, regarding this NEP: it's not only about the name is misleading, it's more about the fact that content of this file diverges from the current implementation. Since you haven't pushed any fixes, I removed this file from the current PR in order not to delay 3-s consensus changes. Later, when you have time, you may create a separate PR to update the documentation part, and we'll review it.

Sure, give me until tomorrow

shargon · 2025-04-10T10:24:57Z

https://github.com/neo-project/neo/pull/3622/files#r2037019862 (conversion is collapsed)

src/Neo/Extensions/NeoSystemExtensions.cs

shargon · 2025-04-10T19:05:50Z

src/Neo/ProtocolSettings.cs

+        /// For code that needs the block generation time based on blockchain state, 
+        /// use NeoSystemExtensions.GetBlockGenTime extension method instead.
        /// </summary>
        public uint MillisecondsPerBlock { get; init; }


We can use obsolete to avoid the use, only in the new method

But this value is actually not obsolete, it's still used until Echidna is enabled. And it's used to initialize Policy at Echidna height, so this value will never be removed from the node's configuration.

shargon · 2025-04-10T19:06:47Z

src/Neo/SmartContract/Native/PolicyContract.cs

+        public uint GetMSPerBlock(IReadOnlyStore snapshot)
+        {
+            // this should not be null since this method is only available after the HF_Echidna
+            snapshot.TryGet(_blockGenTime, out var item);


If ! throw an exception instead of get null exception should be good for debug

We should just use simple index expression here, like for any other Policy getter method. Will fix.

shargon · 2025-04-10T19:08:20Z

src/Plugins/DBFTPlugin/Consensus/ConsensusService.Check.cs

            {
                block_received_index = context.Block.Index;
-                block_received_time = TimeProvider.Current.UtcNow;
                Block block = context.CreateBlock();


What's happend with this variable?

It's removed because it's not used after we implement #3622 (comment).

shargon · 2025-04-10T19:09:14Z

src/Plugins/DBFTPlugin/Consensus/ConsensusService.cs

-                    TimeSpan span = neoSystem.Settings.TimePerBlock;
-                    if (block_received_index + 1 == context.Block.Index)
+                    TimeSpan span = context.BlockGenTime;
+                    if (block_received_index + 1 == context.Block.Index && onPrepareBlockIndex + 1 == context.Block.Index)


Extra condition?

I did not notice that! Good eyes @shargon.

I will dismiss my approval until this is clarified.

Yes, extra condition, because @Jim8y wants to include #3637 into this PR. So this PR contains not only MSPerBlock Policy extension, but also dBFT improvement (#3627), that's why I asked review from @vncoelho.

If you wish, I can split this PR into two PRs to decouple Policy update from dBFT timer logic change. That was my original intention in #3622 (comment).

OK, I see #3622 (comment), let's use two separate PRs. I'll create them soon.

@AnnaShaleva, in fact I knew about the change in how to process block time.

Initially I was against changing it here directly because it is off course not the focus of this PR, but @Jim8y insisted and said @superboyiii was testing on testnet for long time.
So, we did also our experiments with this PR and the behaviours were quite good, we introduced adversaries delays, blocked communication and etc...The results were better than before.

But, when I say better than before, I mean the overall neo-core itself, not the change separately.
In this sense, I still believe it should be merged in another PR, let's say a minor patch after release. That is my opinion.

Indeed I double checked here again this line.

I was just confused...aheuaheuahea

In fact, in the past, when we studied this exactly modification that @Jim8y is pushing forward, we did not add this line.
This line is important and makes the difference for avoiding some problems on the node relative time in the new rule. It was a good catch by @Jim8y .

For me this PR can be merged.

On the other hand, fell free to split the PR and merge in two different commits, that also seems to be a better way to proceed.

Let's use #3895, it's more clean and contains a single logical change, it's easier to review for everyone. So @vncoelho, @cschuchardt88 if you agree with this PR then please approve also #3895.

Changes were made.

cschuchardt88

LGTM

AnnaShaleva · 2025-04-11T11:59:59Z

As agreed in #3622 (comment), this PR will be replaced by two PRs to split the logic: native Policy update (#3895) and dBFT timer adjustment (will create it after the merge of #3895).

…ect#3861) * Native: move MaxValidUntilBlockIncrement to native Policy The problem is described in nspcc-dev/neo-go#3841. In short words, the chain's state is dependent on MaxValidUntilBlockIncrement setting, whereas this setting depends on the block time. Since block generation time is a part of native Policy (neo-project#3622), MaxValidUntilBlockIncrement should follow, we need the same value to be applied at all CNs simultaneously. Port nspcc-dev/neo-go#3849. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Extensions: improve formatting No functional changes. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: set maximum constraint for MaxValidUntilBlockIncrement It's set to be a day of 1-second blocks, that should be enough, ref. neo-project#3861 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Will <201105916+Wi1l-B0t@users.noreply.github.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: Jimmy <jinghui@wayne.edu>

vncoelho and others added 13 commits December 5, 2024 15:51

First commit and no format

b5fd4ec

Start adjusting param but devcontainer not working

91c4bd2

Merge branch 'master' into decreaseblocktime-3s

9ba85d8

Merge branch 'master' into decreaseblocktime-3s

a095496

Update docs/config.json.md

3329c54

Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>

Merge branch 'master' into decreaseblocktime-3s

774471a

format

739beaf

Merge branch 'master' into decreaseblocktime-3s

af42801

Merge branch 'master' into decreaseblocktime-3s

9d677c5

Update src/Neo.CLI/config.fs.mainnet.json

cd91440

Update src/Neo.CLI/config.json

26b1e58

Update src/Neo.CLI/config.mainnet.json

3b52a3c

include the consensus time into block intervals.

f016575

Jim8y marked this pull request as draft December 12, 2024 15:01

Jim8y commented Dec 12, 2024

View reviewed changes

Jim8y mentioned this pull request Dec 16, 2024

[Consensus] Include the consensus time into block interval. #3627

Closed

Merge branch 'master' into 3s-consensus

1570990

cschuchardt88 added the Work in Progress label Feb 7, 2025

Jimmy added 4 commits February 8, 2025 18:40

Merge branch 'master' into 3s-consensus

6fe0abf

fix ut

19d0850

update gasperblock, and ut is expected to fail due to hardfork settings.

18cd445

update the code and unit tests

169a7e7

Jim8y marked this pull request as ready for review February 10, 2025 06:33

Merge branch 'master' into 3s-consensus

8b47891

Jim8y added Waiting for Review and removed Work in Progress labels Feb 10, 2025

Jim8y commented Feb 11, 2025

View reviewed changes

Jimmy added 2 commits February 11, 2025 20:55

Merge branch 'master' into 3s-consensus

45eb20d

apply shargon's suggestion and avoid change the storage.

ded7002

roman-khimov previously requested changes Feb 11, 2025

View reviewed changes

src/Neo/ProtocolSettings.cs Outdated Show resolved Hide resolved

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

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

shargon reviewed Apr 10, 2025

View reviewed changes

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

src/Plugins/StateService/Verification/VerificationService.cs Outdated Show resolved Hide resolved

AnnaShaleva added 2 commits April 10, 2025 12:41

Native: add unit test for Policy's [set,get]MSPerBlock

6d4edcb

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

StateService: adapt dynamic MSPerBlock to verification timeot

321406a

Fix neo-project#3622 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva requested a review from shargon April 10, 2025 09:50

*: format updates

dba4916

No functional changes, just remove useless changes and adjust documentation. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

vncoelho reviewed Apr 10, 2025

View reviewed changes

src/Neo/Extensions/NeoSystemExtensions.cs Show resolved Hide resolved

AnnaShaleva requested a review from vncoelho April 10, 2025 11:40

vncoelho previously approved these changes Apr 10, 2025

View reviewed changes

Merge branch 'master' into 3s-consensus

7da2823

shargon reviewed Apr 10, 2025

View reviewed changes

cschuchardt88 previously approved these changes Apr 11, 2025

View reviewed changes

AnnaShaleva requested review from cschuchardt88, shargon and vncoelho April 11, 2025 10:20

cschuchardt88 approved these changes Apr 11, 2025

View reviewed changes

AnnaShaleva closed this Apr 11, 2025

vncoelho approved these changes Apr 11, 2025

View reviewed changes

This was referenced Apr 11, 2025

Native: move MillisecondsPerBlock setting to native Policy #3895

Merged

DBFTPlugin: include consensus time into block interval #3900

Merged

Conversation

Jim8y commented Dec 12, 2024 • edited by AnnaShaleva Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jim8y Feb 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roman-khimov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AnnaShaleva commented Apr 10, 2025

Uh oh!

vncoelho commented Apr 10, 2025

Uh oh!

shargon commented Apr 10, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Apr 11, 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!

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

Jim8y commented Dec 12, 2024 •

edited by AnnaShaleva

Loading

Jim8y Feb 12, 2025 •

edited

Loading

Jim8y commented Apr 10, 2025 •

edited

Loading

AnnaShaleva Apr 11, 2025 •

edited

Loading

vncoelho Apr 11, 2025 •

edited

Loading

AnnaShaleva Apr 11, 2025 •

edited

Loading