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

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

Description

Credits of this pr belong to @vncoelho . This pr duplicate changes in #3612 and added more corresponding update.

Changes in this pr are:

decrease the block time to 3 seconds.
include consensus time into block time.
update the gas being minted per block with hardfork
corresponding limitation configs.

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 updat

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 <[email protected]>

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)]
        public BigInteger GetGasPerBlock(DataCache snapshot)
        {
            return GetSortedGasRecords(snapshot, Ledger.CurrentIndex(snapshot) + 1).First().GasPerBlock;
        }

+        [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

vncoelho

There are changes not necessary for this PR.
Some of them were extensive explored in the past with strong experiments. Thus, for their current implementation REAL tests and a MINIMAL care with running a real scenario is surely necessary.

Jim8y · 2025-02-12T01:19:07Z

There are changes not necessary for this PR. Some of them were extensive explored in the past with strong experiments. Thus, for their current implementation REAL tests and a MINIMAL care with running a real scenario is surely necessary.

This pr is already being running in real scenario for 2 weeks, stress test included， and of course it will also be tested in the testnet for weeks, but that will be after the merge of this pr.

vncoelho · 2025-02-12T01:30:33Z

@Jim8y
Where is it running and which setup ?

There are some problems with this change when we have change views, and more normally when the multiplied by factor is applied. It also have a cascade effect when a node has delays and did not still processed last commit, but, the one who processed and is the Speaker would already have sent next Prepare Request.

Maybe other components on Neo Core improved, which is surely true. But in the past we where not able to run this scenario efficiently. I am not saying it is not possible, but my suggestion would surely be to merge these changes apart of the time change, which does not necessary need this in the first moment.

vncoelho · 2025-02-12T01:33:53Z

I will try to run this in an abnormal scenario and check results.

Jim8y · 2025-02-12T01:36:40Z

@Jim8y Where is it running and which setup ?

There are some problems with this change when we have change views, and more normally when the multiplied by factor is applied. It also have a cascade effect when a node has delays and did not still processed last commit, but, the one who processed and is the Speaker would already have sent next Prepare Request.

Maybe other components on Neo Core improved, which is surely true. But in the past we where not able to run this scenario efficiently. I am not saying it is not possible, but my suggestion would surely be to merge these changes apart of the time change, which does not necessary need this in the first moment.

Sorry the testnet was setup and run for weeks with all information and result being shared in the core dev chatting group and TC chatting group. While the testnet was running, I encouraged everyone to run tests over the testnet. The testnet was shutted down after running for a few weeks due to high server cost.

And before we bring changes in this pr to the mainnet, of course we will have a testnet for our newly released core version, so if you have any concern with the changes, you can still do some test in that testnet.

vncoelho · 2025-02-12T01:39:23Z

I remember to see some results, was @superboyiii in charge of the simulation scenario?

Jim8y · 2025-02-12T01:42:15Z

I remember to see some results, was @superboyiii in charge of the simulation scenario?

Yes, @superboyiii setup a 7 nodes testnet on the cloud servers, for a while when the stress test was running, one of the node is running on my local mac machine. That testnet ran for weeks and smoothly generate new blocks with 3-seconds consensus. Such test is very expensive,,, so not likely to do it again if there is no changes that can directly affect this pr.

I finished tests on a high latency and selective distributed scenario where CN can not reach each other easily (sometime even not possible), and also with 1,5s delay in a 0,5s block generation period.

Change views occurs as expected and system keeps its stability.

vncoelho · 2025-02-12T15:29:45Z

Congratulations to the core devs for the general work, the consensus is much more stable as well as the P2P, my last simulation environment surprised myself.

Regarding the PR itself, all changes looks good to me, but some modification are still needed before approval.

src/Neo/ProtocolSettings.cs

roman-khimov · 2025-02-25T13:46:32Z

src/Plugins/DBFTPlugin/Consensus/ConsensusService.cs

@@ -35,6 +35,8 @@ private class Timer { public uint Height; public byte ViewNumber; }
        private readonly IActorRef blockchain;
        private ICancelable timer_token;
        private DateTime block_received_time;


I think this can be dropped now.

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

obsolete

belane · 2025-03-03T17:08:55Z

As previously discussed, modifying the blocktime could introduce risks to some projects. Specifically, projects that rely on the assumption of a fixed lock time for time-based calculations (vestings, timelocks, rewards, ...).

For a smart contract to be impacted by this change, it must be aware of the current time in some way. I think this can only happen through one of the following methods (if there are other ways, suggestions are welcome):

Use GetTime.
Retrieve the current block and extract the block timestamp.
Retrieve the current block hash, query the block by hash, and extract the timestamp.

A quick search for these patterns reveals some contracts that could be affected by this change. It would be advisable that we contact each project and evaluate the impact on a case-by-case basis.

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 <[email protected]>

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

Open

Merge branch 'master' into 3s-consensus

1570990

cschuchardt88 added the Work in Progress label Feb 7, 2025

Jim8y 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

Jim8y 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

Update tests/Neo.Plugins.OracleService.Tests/config.json

3664890

vncoelho previously requested changes Feb 12, 2025

View reviewed changes

Merge branch 'master' into 3s-consensus

fde030c

Merge branch 'master' into 3s-consensus

ae6594c

Jim8y added 2 commits February 13, 2025 17:41

Merge branch 'master' into 3s-consensus

2a39868

Merge branch 'master' into 3s-consensus

593e573

shargon reviewed Feb 19, 2025

View reviewed changes

src/Neo/ProtocolSettings.cs Show resolved Hide resolved

Merge branch 'master' into 3s-consensus

430f346

cschuchardt88 assigned Jim8y and vncoelho Feb 25, 2025

cschuchardt88 approved these changes Feb 25, 2025

View reviewed changes

Jim8y requested review from shargon, nan01ab, roman-khimov, vncoelho and AnnaShaleva February 25, 2025 12:42

roman-khimov reviewed Feb 25, 2025

View reviewed changes

vncoelho and others added 2 commits February 25, 2025 21:34

Merge branch 'master' into 3s-consensus

89891b7

Merge branch 'master' into 3s-consensus

e81cd4c

shargon added this to the v3.8.0 milestone Mar 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Jim8y commented Dec 12, 2024 •

edited

Loading

Jim8y Dec 12, 2024

vncoelho Feb 12, 2025

cschuchardt88 Feb 12, 2025

Jim8y Feb 12, 2025 •

edited

Loading

vncoelho Feb 12, 2025

Jim8y Feb 11, 2025

shargon Feb 11, 2025

shargon Feb 11, 2025

shargon Feb 11, 2025

Jim8y Feb 11, 2025

Jim8y Feb 11, 2025

Jim8y Feb 11, 2025

nan01ab Feb 12, 2025

roman-khimov left a comment

vncoelho left a comment

Jim8y commented Feb 12, 2025

vncoelho commented Feb 12, 2025

vncoelho commented Feb 12, 2025

Jim8y commented Feb 12, 2025 •

edited

Loading

vncoelho commented Feb 12, 2025

Jim8y commented Feb 12, 2025 •

edited

Loading

vncoelho commented Feb 12, 2025 •

edited

Loading

roman-khimov Feb 25, 2025

belane commented Mar 3, 2025

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

Are you sure you want to change the base?

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

Conversation

Jim8y commented Dec 12, 2024 • edited Loading

Description

Type of change

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-khimov left a comment

Choose a reason for hiding this comment

vncoelho left a comment

Choose a reason for hiding this comment

Jim8y commented Feb 12, 2025

vncoelho commented Feb 12, 2025

vncoelho commented Feb 12, 2025

Jim8y commented Feb 12, 2025 • edited Loading

vncoelho commented Feb 12, 2025

Jim8y commented Feb 12, 2025 • edited Loading

vncoelho commented Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

belane commented Mar 3, 2025

Jim8y commented Dec 12, 2024 •

edited

Loading

Jim8y Feb 12, 2025 •

edited

Loading

Jim8y commented Feb 12, 2025 •

edited

Loading

Jim8y commented Feb 12, 2025 •

edited

Loading

vncoelho commented Feb 12, 2025 •

edited

Loading