Implement a time-based block creation algorithm #1100

AndrejMitrovic · 2020-08-05T11:00:12Z

This implements one part of #1004.

There is no synchronization of clocks between nodes - but this is something I'm working on and might file as a separate issue.

Note that I'm using shared(time_t) rather than a TestNode API endpoint to get & set a custom clock time because I want to avoid request overheads when setting the time (especially because these requests may be delayed).

codecov · 2020-08-05T11:09:45Z

Codecov Report

Merging #1100 into v0.x.x will increase coverage by 0.18%.
The diff coverage is 95.39%.

@@            Coverage Diff             @@
##           v0.x.x    #1100      +/-   ##
==========================================
+ Coverage   78.76%   78.95%   +0.18%     
==========================================
  Files          86       89       +3     
  Lines        8203     8309     +106     
==========================================
+ Hits         6461     6560      +99     
- Misses       1742     1749       +7

Flag	Coverage Δ
#integration	`41.15% <75.47%> (+0.11%)`	⬆️
#unittests	`77.33% <93.42%> (+0.22%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
source/agora/common/Config.d	`91.19% <66.66%> (-0.48%)`	⬇️
source/agora/test/TimeDrift.d	`92.00% <92.00%> (ø)`
source/agora/consensus/protocol/Nominator.d	`90.90% <94.11%> (+0.28%)`	⬆️
source/agora/test/Base.d	`86.51% <96.15%> (+0.89%)`	⬆️
source/agora/test/TimeBlockInterval.d	`96.15% <96.15%> (ø)`
source/agora/consensus/data/ConsensusParams.d	`100.00% <100.00%> (ø)`
source/agora/network/Clock.d	`100.00% <100.00%> (ø)`
source/agora/network/NetworkManager.d	`77.31% <100.00%> (-0.24%)`	⬇️
source/agora/node/FullNode.d	`84.00% <100.00%> (+0.21%)`	⬆️
source/agora/node/Validator.d	`95.38% <100.00%> (+0.14%)`	⬆️
... and 12 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 edcd16f...c7ac8cc. Read the comment docs.

AndrejMitrovic · 2020-08-05T13:03:26Z

Of all the targets to fail, it had to be Windows. 🤦‍♂️😄

AndrejMitrovic · 2020-08-05T16:02:34Z

source\agora\test\Base.d(553,16): Error: cannot implicitly convert expression cast(ulong)this.genesis_start_time + height.value * cast(ulong)this.test_conf.block_interval_sec of type ulong to int

Huh, why is time_t 32-bit on Windows? That is not at all what the docs say it should be: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/time-time32-time64?view=vs-2019

time is a wrapper for _time64 and time_t is, by default, equivalent to __time64_t. If you need to force the compiler to interpret time_t as the old 32-bit time_t, you can define _USE_32BIT_TIME_T

But apparently in druntime it's different:

C:\dev> rdmd --eval="import core.stdc.time; writeln(time_t.sizeof);"
4

AndrejMitrovic · 2020-08-05T16:11:09Z

Edit: also with -m64. I think DMD defaults to -m32 on Windows if I'm not mistaken. I'll ask in the D forums.

AndrejMitrovic · 2020-08-06T07:15:07Z

Update: Added a clock-drift test-case and it works exactly as expected.

Geod24

This needs a rebase.

Geod24 · 2020-08-09T21:16:43Z

source/agora/common/Config.d

@@ -138,6 +138,9 @@ public struct NodeConfig

    /// Maximum number of nodes to include in an autogenerated quorum set
    public uint max_quorum_nodes = 7;
+
+    /// Threshold to use in the autogenerated quorum
+    public double quorum_threshold = 0.80;


That feels like going back to QuorumConfig, just with a global value.
Do you envision this as a temporary solution, or do you think it's going to be permanent ?

It's only for the unittests. In this case it makes it significantly easier to write a simple test-case - otherwise I would have to instantiate many more nodes and I prefer to have tests which are simple to understand.

I don't actually want this to be in the node configs. But there was a commit which removed the ConsensusParams from the FullNode ctor parameter list and ConsensusParams was instead instantiated directly in the constructor body using the node config parameters.

Geod24 · 2020-08-09T21:19:35Z

source/agora/network/Clock.d

@@ -0,0 +1,39 @@
+/*******************************************************************************
+
+    Contains a Clock implementation.


I love this commit <3

Geod24 · 2020-08-09T21:21:47Z

source/agora/common/Config.d

@@ -102,6 +102,13 @@ public struct NodeConfig
    /// in place of the built-in genesis block as defined by CoinNet
    public string genesis_block;

+    /// The Unix timestamp of the Genesis block. Blocks are created at a
+    /// regular interval starting from this time.
+    public time_t genesis_start_time = 1596179709;


Ooooohhhh didn't think of this. Right. I would say, if we parse it from the config file, let's make it parse an ISO8601 string. Also, mention the time is UTC.

Yep. Also I'm not sure whether we need or don't need a timestamp in the block header yet. It might be possible to omit it for now since the estimated block timestamp is derived from its height.

Geod24 · 2020-08-09T21:23:33Z

source/agora/consensus/protocol/Nominator.d

@@ -181,7 +229,8 @@ extern(D):

    ***************************************************************************/

-    protected bool prepareNominatingSet (out ConsensusData data) @safe
+    protected bool prepareNominatingSet (out ConsensusData data,
+        in time_t cur_time) @safe


I see what you did here

Geod24 · 2020-08-09T21:24:43Z

source/agora/consensus/protocol/Nominator.d

+        if (cur_time < this.params.GenesisStartTime)
+        {
+            log.fatal("Clock is out of sync. " ~
+                "Current time: {}. Genesis time: {}", cur_time,
+                this.params.GenesisStartTime);
+            return;
+        }
+


Maybe we should just check it at startup and assert here. I can see a lot of headache otherwise.

Check it at startup: Needs to be user-friendly for the user in regular build, and dev friendly in UT

I'll add another check at start-up, however I will leave this here as well. There is another PR coming which implements synchronization of the clock across the network, together with a lot of documentation about clock time drift and so on..

Geod24 · 2020-08-09T21:26:08Z

source/agora/node/FullNode.d

            this.network.gossipTransaction(tx);
-            this.onAcceptedTransaction();
-        }
+


That's a sin

Geod24 · 2020-08-09T21:27:08Z

source/agora/node/Validator.d

+    /// Workaround: onRegenerateQuorums() is called from within the ctor,
+    /// but LocalScheduler is not instantiated yet.
+    private bool started;


How can the timer be caller if the scheduler isn't running ? Can I have an ELIMM (Explain Like It's Monday Morning) ?

We cannot use setTimer until LocalScheduler is instantiated, and it is instantiated after the constructor finishes. In the constructor of the node onRegenerateQuorums is called. But onRegenerateQuorums is also called when a block is externalized.

Geod24 · 2020-08-09T21:28:36Z

source/agora/test/Base.d

+
+    /// the adjustable local clock time for this node.
+    /// This does not affect request timeouts and is only
+    /// used in the Nomination protocol.
+    private shared(time_t)* cur_time;
+
+    /// Get the current clock time
+    @property time_t time () @trusted @nogc nothrow
+    {
+        return atomicLoad(*this.cur_time);
+    }
+
+    /// Set the new time
+    @property void time (time_t new_time) @trusted @nogc nothrow
+    {
+        atomicStore(*this.cur_time, new_time);
+    }


Why not in the TestAPI ?

It's explained in the PR description.

Ah, missed that. The overhead is negligible TBH (we have a resolution of 1 second and the overhead is usually 10ms, 400ms is the highest I've seen it). But I'm fine if we leave it this way for the moment.

I forgot to reply here. There is also the issue of filtering - I want to circumvent active filters on the node but still have the ability to change its time.

AndrejMitrovic · 2020-08-10T01:19:02Z

This needs a rebase.

Ok.

Geod24 · 2020-08-23T23:56:24Z

This has looooooads of conflicts. Feel free to submit things piece-meal if that's easier.

AndrejMitrovic · 2020-09-01T03:29:21Z

This can be reviewed.

Note that synchronization of clocks across nodes will be done in a separate PR.

AndrejMitrovic · 2020-09-01T05:16:12Z

source/agora/consensus/protocol/Nominator.d

+
+        This routine should be called after every externalize event and
+        after any valid block is received from the network (getBlocksFrom()).
+


Missing Params:

AndrejMitrovic · 2020-09-01T05:17:37Z

source/agora/consensus/protocol/Nominator.d

@@ -168,7 +244,8 @@ extern(D):

    ***************************************************************************/

-    protected bool prepareNominatingSet (out ConsensusData data) @safe
+    protected bool prepareNominatingSet (out ConsensusData data,


Needs Params docs. cur_time is used by derived classes, and will also be used here but by subsequent PRs.

AndrejMitrovic · 2020-09-01T05:17:52Z

source/agora/consensus/protocol/Nominator.d

@@ -187,31 +264,61 @@ extern(D):

    /***************************************************************************

-        Try to begin a nomination round.
+        Gets the expected block nomination time for for the provided height.
+


Missing Params

AndrejMitrovic · 2020-09-01T05:18:20Z

source/agora/consensus/protocol/Nominator.d

+
+    ***************************************************************************/
+
+    private ulong getExpectedBlockTime (Height height) @safe @nogc nothrow pure


We should change all time_t to ulong since time_t doesn't seem to be portable on Windows.

AndrejMitrovic · 2020-09-01T05:18:43Z

source/agora/consensus/protocol/Nominator.d


    ***************************************************************************/

-    public void tryNominate () @safe
+    public void checkNominate () @safe


This function is called periodically by the nominating timer.

Should this be private then?

TrustHenry · 2020-09-04T06:46:11Z

source/agora/consensus/data/ConsensusParams.d


    ***************************************************************************/

    public this (immutable(Block) genesis, uint validator_cycle = 1008,
                 uint max_quorum_nodes = 7, uint quorum_threshold = 80,
-                 uint quorum_shuffle_interval = 30)
+                 uint quorum_shuffle_interval = 30,
+                 time_t genesis_start_time = 1596179709,


Suggested change

time_t genesis_start_time = 1596179709,

time_t genesis_start_time = 1596153600,

assert(SysTime(DateTime.fromISOString("20200731T000000"), UTC()).toUnixTime == 1596153600);

According to the time declared in config value, the unix timestamp value is as above.

Ah yes. In any case these are just placeholder values. We need to change them to the actual time of when mainnet will be deployed. But we don't know this yet.

This code will later be supplemented by network-synchronization features, therefore it will live in the networking package.

It will need them to control the nominating behavior.

- genesis_start_time => The Unix timestamp of the Genesis block. Blocks are created at `block_interval` intervals starting from `genesis_start_time` - block_interval => How frequently blocks are created. It is assumed that block #1 was created at timestamp: `genesis_start_time` + `block_interval`

Fixes bosagora#1138

The two config options `genesis_start_time` and `block_interval` dictate when and how often a Validator will nominate blocks. `genesis_start_time` is the expected timestamp of when the UnitNet/TestNet/CointNet was deployed, whereas `block_interval` dictates how often new blocks should be nominated. It is therefore expected that block height #1 should be nominated at the time: genesis_start_time + (block_inteval * 1) Note that this is a preliminary implementation and lacks network clock synchronization primitives.

Geod24 · 2020-09-09T06:57:46Z

source/agora/consensus/protocol/Nominator.d

+    public this (Clock clock,
+        NetworkManager network, KeyPair key_pair, Ledger ledger,


You have weird life choices

Oh you mean the order of parameters. I kind of tried to order them by importance, but this is purely subjective. I'll add params at the end from now on.

Geod24 · 2020-09-09T06:58:06Z

source/agora/network/Clock.d

+
+mixin AddLogger!();
+
+/// The current implementation uses NTP


Oh well outdated comment. But under the hood the OS uses NTP. Interestingly enough Ethereum (or was it Bitcoin?) tried to roll out their own NTP code and realized it was worse than what the OS already implements, so they rolled back.

AndrejMitrovic force-pushed the timer-blocks branch 4 times, most recently from 2514abe to 26438e4 Compare August 6, 2020 04:06

AndrejMitrovic changed the title ~~[WIP] Implement timer-based block creation~~ [WIP] Implement time-based block creation Aug 6, 2020

AndrejMitrovic force-pushed the timer-blocks branch 2 times, most recently from 9e19b2a to 2ec626b Compare August 6, 2020 07:14

AndrejMitrovic force-pushed the timer-blocks branch from 2ec626b to 5e38b35 Compare August 6, 2020 07:25

AndrejMitrovic mentioned this pull request Aug 6, 2020

Add quorum threshold to consensus params & improve ensureConsistency() #1102

Merged

AndrejMitrovic changed the title ~~[WIP] Implement time-based block creation~~ Implement time-based block creation Aug 7, 2020

AndrejMitrovic marked this pull request as ready for review August 7, 2020 05:48

Geod24 reviewed Aug 10, 2020

View reviewed changes

AndrejMitrovic force-pushed the timer-blocks branch 5 times, most recently from ba5e317 to c59dc60 Compare August 24, 2020 07:45

AndrejMitrovic mentioned this pull request Aug 24, 2020

Node will reject catch-up blocks if it happens to be in the nominating stage #1138

Closed

AndrejMitrovic force-pushed the timer-blocks branch 3 times, most recently from baf140d to 6bf49a9 Compare August 25, 2020 05:25

AndrejMitrovic changed the title ~~Implement time-based block creation~~ Implement a time-based block creation algorithm Aug 25, 2020

AndrejMitrovic force-pushed the timer-blocks branch from 8620e52 to 556ead7 Compare August 25, 2020 07:33

AndrejMitrovic force-pushed the timer-blocks branch from 3a37c8b to caf3f0d Compare August 25, 2020 07:50

AndrejMitrovic mentioned this pull request Aug 25, 2020

Disable ManyValidators test as it frequently fails #1144

Merged

AndrejMitrovic force-pushed the timer-blocks branch 5 times, most recently from 9131899 to 9dfc863 Compare August 28, 2020 03:19

AndrejMitrovic force-pushed the timer-blocks branch 3 times, most recently from 066c050 to 34aa299 Compare September 1, 2020 05:13

AndrejMitrovic commented Sep 1, 2020

View reviewed changes

AndrejMitrovic force-pushed the timer-blocks branch from 34aa299 to f750e23 Compare September 1, 2020 09:09

TrustHenry reviewed Sep 4, 2020

View reviewed changes

AndrejMitrovic mentioned this pull request Sep 8, 2020

Implement synchronization of clocks across the network #1174

Closed

AndrejMitrovic added 5 commits September 11, 2020 16:43

Add a simple Clock implementation

f4a7729

This code will later be supplemented by network-synchronization features, therefore it will live in the networking package.

Pass ConsensusParams to the Nominator

4713019

It will need them to control the nominating behavior.

Allow accepting blocks in the middle of nomination

7fbebf5

Fixes bosagora#1138

AndrejMitrovic force-pushed the timer-blocks branch 2 times, most recently from 9c12b49 to c7ac8cc Compare September 11, 2020 07:48

AndrejMitrovic added a commit to AndrejMitrovic/agora that referenced this pull request Sep 11, 2020

[Merge bosagora#1100]

c1a61c0

AndrejMitrovic mentioned this pull request Sep 11, 2020

Implement network clock time synchronization #1194

Merged

Geod24 approved these changes Sep 14, 2020

View reviewed changes

Geod24 merged commit 0330491 into bosagora:v0.x.x Sep 14, 2020

		@@ -0,0 +1,39 @@
		/*******************************************************************************

		Contains a Clock implementation.


		This routine should be called after every externalize event and
		after any valid block is received from the network (getBlocksFrom()).


		***************************************************************************/

		private ulong getExpectedBlockTime (Height height) @safe @nogc nothrow pure

	time_t genesis_start_time = 1596179709,
	time_t genesis_start_time = 1596153600,

		public this (Clock clock,
		NetworkManager network, KeyPair key_pair, Ledger ledger,

Implement a time-based block creation algorithm #1100

Implement a time-based block creation algorithm #1100

Conversation

AndrejMitrovic commented Aug 5, 2020 • edited Loading

codecov bot commented Aug 5, 2020 • edited Loading

Codecov Report

AndrejMitrovic commented Aug 5, 2020

AndrejMitrovic commented Aug 5, 2020

AndrejMitrovic commented Aug 5, 2020

AndrejMitrovic commented Aug 6, 2020

Geod24 left a 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

AndrejMitrovic Aug 10, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrejMitrovic commented Aug 10, 2020

Geod24 commented Aug 23, 2020

AndrejMitrovic commented Sep 1, 2020

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

Choose a reason for hiding this comment

AndrejMitrovic commented Aug 5, 2020 •

edited

Loading

codecov bot commented Aug 5, 2020 •

edited

Loading

AndrejMitrovic Aug 10, 2020 •

edited

Loading