Seconds to milliseconds by vncoelho · Pull Request #918 · neo-project/neo

vncoelho · 2019-07-12T17:58:19Z

closes #651

Let's review this with careful.
I believe it will be a great chance, in particular, for very lightweight local neo privatenets.

There are some formulas and shift operations that probably needed to be adjusted.
- I went directly in the straight modification, but, perhaps, I surely missed some necessary adjustments.
There are also some functions that I believe are obsolete, maybe we can also remove them

vncoelho · 2019-07-12T18:08:04Z

neo/Network/P2P/Payloads/PingPayload.cs

            {
                LastBlockIndex = height,
-                Timestamp = DateTime.UtcNow.ToTimestamp(),
+                Timestamp = DateTime.UtcNow.ToTimestampMS(),


Maybe we can leave the Ping in seconds.

It is better to leave everything using the same "unit", what do you think?

vncoelho · 2019-07-12T18:08:19Z

neo/Network/P2P/Payloads/VersionPayload.cs

                Magic = ProtocolSettings.Default.Magic,
                Version = LocalNode.ProtocolVersion,
-                Timestamp = DateTime.Now.ToTimestamp(),
+                Timestamp = DateTime.Now.ToTimestampMS(),


Maybe we can leave the version payload in seconds as well.

neo/IO/Json/JNumber.cs

neo/Helper.cs

codecov-io · 2019-07-12T19:48:14Z

Codecov Report

Merging #918 into master will decrease coverage by 0.05%.
The diff coverage is 64.15%.

@@            Coverage Diff             @@
##           master     #918      +/-   ##
==========================================
- Coverage   45.53%   45.47%   -0.06%     
==========================================
  Files         178      178              
  Lines       12635    12639       +4     
==========================================
- Hits         5753     5748       -5     
- Misses       6882     6891       +9

Impacted Files	Coverage Δ
neo/IO/Json/JNumber.cs	`84.26% <ø> (+4.48%)`	⬆️
neo/Consensus/RecoveryRequest.cs	`45.45% <0%> (ø)`	⬆️
neo/ProtocolSettings.cs	`94% <100%> (ø)`	⬆️
neo/Consensus/PrepareRequest.cs	`95.23% <100%> (ø)`	⬆️
neo/Helper.cs	`51.74% <100%> (+2.23%)`	⬆️
neo/Consensus/ChangeView.cs	`93.33% <100%> (ø)`	⬆️
neo/Ledger/MemoryPool.cs	`77.94% <100%> (ø)`	⬆️
neo/Network/P2P/Payloads/BlockBase.cs	`72% <100%> (+2.88%)`	⬆️
neo/Consensus/ConsensusService.cs	`15.7% <12.5%> (ø)`	⬆️
neo/SmartContract/ApplicationEngine.cs	`68.47% <28.57%> (-20.16%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98e6d1c...6cc6b9b. Read the comment docs.

neo.UnitTests/protocol.json

Slower now...aheuahuea

vncoelho · 2019-07-13T16:18:22Z

neo.UnitTests/UT_Consensus.cs

            Console.WriteLine($"header {header} hash {header.Hash} timstamp {timestampVal}");

-            timestampVal.Should().Be(4244941696); //1968-06-01 00:00:00
+            timestampVal.Should().Be(1514007552); //1968-06-01 00:00:00


@igormcoelho, can you check this adjustment on these tests? I just copied the new generated timestamp.

This doesn't look right... it should increase, never decrease 😂 probably, it is overflowing some int32 size (should be int64 or long, at least).

kkkkkkkkkkk
I knew that....
Things need all be long for us. Ram never was a problem when things have vallgrind.

What is this date?
How did you guys manage to make 4244941696 be 1968-06-01 00:00:00? Seems weird to me. Maybe the tests were broken all this time?

vncoelho · 2019-07-13T16:19:54Z

neo/Consensus/ConsensusService.cs

        private void ExtendTimerByFactor(int maxDelayInBlockTimes)
        {
-            TimeSpan nextDelay = expected_delay - (TimeProvider.Current.UtcNow - clock_started) + TimeSpan.FromMilliseconds(maxDelayInBlockTimes * Blockchain.SecondsPerBlock * 1000.0 / context.M);
+            TimeSpan nextDelay = expected_delay - (TimeProvider.Current.UtcNow - clock_started) + TimeSpan.FromMilliseconds(maxDelayInBlockTimes * Blockchain.MillisecondsPerBlock * 1000 / context.M);


Does we need to include this .0 for forcing to float?

igormcoelho

TimePerBlock ?

neo/Helper.cs

lock9

Fix the dates - We should use 01/01/1970 00:00:00 instead of 01/06/1968 00:00
Use ulong instead of uint.

neo.UnitTests/TestUtils.cs

neo.UnitTests/UT_Block.cs

lock9 · 2019-07-15T16:08:51Z

neo.UnitTests/UT_Consensus.cs

            Console.WriteLine($"header {header} hash {header.Hash} timstamp {timestampVal}");

-            timestampVal.Should().Be(4244941696); //1968-06-01 00:00:00
+            timestampVal.Should().Be(1514007552); //1968-06-01 00:00:00


What is this date?
How did you guys manage to make 4244941696 be 1968-06-01 00:00:00? Seems weird to me. Maybe the tests were broken all this time?

neo/Consensus/ConsensusService.cs

neo/SmartContract/ApplicationEngine.cs

neo.UnitTests/UT_Header.cs

neo/Helper.cs

neo/IO/Json/JNumber.cs

lock9 · 2019-07-15T16:21:07Z

neo/Network/P2P/Payloads/PingPayload.cs

            {
                LastBlockIndex = height,
-                Timestamp = DateTime.UtcNow.ToTimestamp(),
+                Timestamp = DateTime.UtcNow.ToTimestampMS(),


It is better to leave everything using the same "unit", what do you think?

neo/ProtocolSettings.cs

…into seconds-to-milliseconds

vncoelho · 2019-07-17T22:52:24Z

Thanks, @lock9, I am thinking about still waiting a little bit more until @erikzhang and @shargon can give another last check.

neo/Helper.cs

shargon · 2019-07-18T07:44:24Z

neo/Ledger/Blockchain.cs


-                    if (blocksPersisted++ < blocksToPersistList.Count - (2 + Math.Max(0, (15 - SecondsPerBlock)))) continue;
+                    // 15000 is the default among of seconds per block, while MilliSecondsPerBlock is the current
+                    uint extraBlocks = (15000 - MillisecondsPerBlock) / 1000;


15000 is a hardcoded value, you should take this from the config

That is right, it has been like this. I just adjusted to ms.
We can do that in another PR, for avoiding other possible mistakes.

Furthermore, I am not good with this config until nowdays...

This, should take the value from the configuration

I don't understand this logic very well ... why it used this?

As faster the network is (privatenets with 1s) more blocks are rebroadcasted each time you Persist, limitted to 2+15.

shargon

Is possible to store it in 6 bytes instead of 8.

4 for seconds, and 2 more for milliseconds.

We can save 2 bytes.

vncoelho · 2019-07-18T20:15:33Z

It is possible, @shargon, however, the timestamp in seconds will also expire in 27 years...kkkkkkkk

I think that it is ok as ulong, it will be more clear.

Co-Authored-By: Shargon <shargon@gmail.com>

neo/Ledger/Blockchain.cs

shargon · 2019-07-19T22:40:56Z

i will review it tomorrow

This was already from milliseconds, thus, just removing the multiplier

neo/Helper.cs

vncoelho · 2019-07-22T17:22:25Z

🚀 aheuaheuaea
It needs tests, but here we go! When the client is ready we gonna be make it happen.

shargon · 2019-07-22T17:25:15Z

We should merge it when the tests come

vncoelho · 2019-07-22T17:34:32Z

The basic UTs were accomplished.
But if we do not merge this we will not merge anything, because all other PRs are also missing tests until we have the client, @shargon.

Furthermore, each time more we gonna have to work double because of conflicts between PRs, and after merged we can focused on the next idea.
In fact, we are in a hard situation. But I believe we are close to what we are looking for, my friend.

* Seconds to milliseconds * Sending unsaved files * fixing formula of consensus bonification * Refactoring milliseconds * Refactoring milliseconds from capeslock * Refactoring milliseconds from capeslock II * Adjusting UT * Adjusting UT II * Super fast protocol to 2s * Fixing timestamps to long and tests * Minor adjusts * Change view deserialization fix * Timestamp to ulong * Update neo/Helper.cs Co-Authored-By: Shargon <shargon@gmail.com> * Update JNumber.cs * Optimize and remove TODO * Update ApplicationEngine.cs * Fixing ExtendTimerByFactor This was already from milliseconds, thus, just removing the multiplier

Follow-up for 6604647 and 15a7927 to provide Neo 3 compatibility. Neo PR: neo-project/neo#918.

Seconds to milliseconds

71a48ba

vncoelho requested review from erikzhang, igormcoelho and shargon July 12, 2019 17:58

vncoelho added 4 commits July 12, 2019 15:00

Sending unsaved files

9b335da

fixing formula of consensus bonification

91bd341

Refactoring milliseconds

44b1f26

Refactoring milliseconds from capeslock

f9d437a

vncoelho commented Jul 12, 2019

View reviewed changes

vncoelho added this to the NEO 3.0 milestone Jul 12, 2019

vncoelho added 3 commits July 12, 2019 15:12

Refactoring milliseconds from capeslock II

3dee7a4

Adjusting UT

4c1ea7e

Adjusting UT II

fb614d0

vncoelho commented Jul 12, 2019

View reviewed changes

neo/IO/Json/JNumber.cs Outdated Show resolved Hide resolved

neo/Helper.cs Outdated Show resolved Hide resolved

neo/Helper.cs Outdated Show resolved Hide resolved

shargon previously requested changes Jul 12, 2019

View reviewed changes

neo.UnitTests/protocol.json Outdated Show resolved Hide resolved

vncoelho added 2 commits July 13, 2019 11:26

Super fast protocol to 2s

aff4d0c

Merge branch 'master' into seconds-to-milliseconds

6f64e6f

vncoelho marked this pull request as ready for review July 13, 2019 14:50

vncoelho requested a review from shargon July 13, 2019 15:03

vncoelho commented Jul 13, 2019

View reviewed changes

igormcoelho reviewed Jul 13, 2019

View reviewed changes

neo/Helper.cs Outdated Show resolved Hide resolved

lock9 suggested changes Jul 15, 2019

View reviewed changes

lock9 mentioned this pull request Jul 15, 2019

Add NEO SDK RPC module #850

Merged

vncoelho added 3 commits July 15, 2019 15:50

Fixing timestamps to long and tests

650efe5

Merge branch 'seconds-to-milliseconds' of github.com:neo-project/neo …

947694e

…into seconds-to-milliseconds

Minor adjusts

65c7e30

shargon reviewed Jul 18, 2019

View reviewed changes

neo/Helper.cs Outdated Show resolved Hide resolved

shargon reviewed Jul 18, 2019

View reviewed changes

Merge branch 'master' into seconds-to-milliseconds

3afa833

Update neo/Helper.cs

ab8542a

Co-Authored-By: Shargon <shargon@gmail.com>

vncoelho dismissed lock9’s stale review via ab8542a July 18, 2019 20:17

shargon reviewed Jul 18, 2019

View reviewed changes

neo/Ledger/Blockchain.cs Show resolved Hide resolved

shargon added 3 commits July 19, 2019 11:32

Update JNumber.cs

38428ec

Optimize and remove TODO

a029095

Update ApplicationEngine.cs

889dea1

lock9 previously approved these changes Jul 19, 2019

View reviewed changes

Merge branch 'master' into seconds-to-milliseconds

a4badf0

shargon self-requested a review July 19, 2019 22:41

Fixing ExtendTimerByFactor

dc6bd12

This was already from milliseconds, thus, just removing the multiplier

vncoelho dismissed lock9’s stale review via dc6bd12 July 22, 2019 01:44

Merge branch 'master' into seconds-to-milliseconds

6cc6b9b

shargon reviewed Jul 22, 2019

View reviewed changes

neo/Helper.cs Show resolved Hide resolved

shargon approved these changes Jul 22, 2019

View reviewed changes

vncoelho merged commit 00f9c27 into master Jul 22, 2019

vncoelho deleted the seconds-to-milliseconds branch July 22, 2019 17:21

Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020

minor updates (neo-project#918)

0874777

roman-khimov added a commit to nspcc-dev/dbft that referenced this pull request Jul 11, 2020

cv/recovery: change to 64-bit nanosecond timestamps

e77bac9

Follow-up for 6604647 and 15a7927 to provide Neo 3 compatibility. Neo PR: neo-project/neo#918.

roman-khimov added a commit to nspcc-dev/dbft that referenced this pull request Jul 11, 2020

cv/recovery: change to 64-bit nanosecond timestamps

a477cb6

Follow-up for 6604647 and 15a7927 to provide Neo 3 compatibility. Neo PR: neo-project/neo#918.

Conversation

vncoelho commented Jul 12, 2019

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!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

vncoelho Jul 13, 2019 • 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!

igormcoelho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lock9 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vncoelho commented Jul 17, 2019

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!

shargon left a comment • 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 commented Jul 18, 2019

Uh oh!

Uh oh!

shargon commented Jul 19, 2019

Uh oh!

Uh oh!

vncoelho commented Jul 22, 2019

Uh oh!

shargon commented Jul 22, 2019

Uh oh!

vncoelho commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

codecov-io commented Jul 12, 2019 •

edited

Loading

vncoelho Jul 13, 2019 •

edited

Loading

shargon left a comment •

edited

Loading

vncoelho commented Jul 22, 2019 •

edited

Loading