Reveal a pre-image periodically #563

linked0 · 2020-02-19T07:40:50Z

A validator reveals a pre-image periodically.

Every 1 hour, or 6 blocks.
The revelation period could become configurable in another issue.
The pre-image has the data for double the period because it make the validator to be able to skip to next revelation timing In case of a failure of the revelation.
For unittest, the updateEnrolledHeight function is called manually because the PR Write Enrollments data to the block. #504 is being reviewed now. The PR has the code setting an enrolled height for an enrollment.

codecov · 2020-02-19T07:47:05Z

Codecov Report

Merging #563 into v0.x.x will increase coverage by 0.09%.
The diff coverage is 94.36%.

@@            Coverage Diff             @@
##           v0.x.x     #563      +/-   ##
==========================================
+ Coverage   88.52%   88.62%   +0.09%     
==========================================
  Files          58       58              
  Lines        4306     4377      +71     
==========================================
+ Hits         3812     3879      +67     
- Misses        494      498       +4

Flag	Coverage Δ
#unittests	`88.62% <94.36%> (+0.09%)`	⬆️

Impacted Files	Coverage Δ
source/agora/node/Node.d	`90.36% <100%> (+0.61%)`	⬆️
source/agora/test/Base.d	`88.42% <100%> (+0.06%)`	⬆️
source/agora/consensus/EnrollmentManager.d	`86.62% <90%> (+0.37%)`	⬆️
source/agora/test/EnrollmentManager.d	`96.77% <97.14%> (+0.47%)`	⬆️

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 34ffcfa...2c1ab5c. Read the comment docs.

TrustHenry · 2020-02-19T07:57:26Z

source/agora/node/GossipProtocol.d

@@ -80,6 +80,16 @@ public class GossipProtocol
            this.network.sendTransaction(tx);
            this.ledger.tryNominateTXSet();
        }
+
+        if (this.enroll_man.needRevealPreimage(this.ledger.getBlockHeight()))


The needRevealPreimage check at that location causes unnecessary calls.
I think it would be better to check when the height of the block changes as a timing for checking next_reveal_height

linked0 · 2020-02-20T04:07:03Z

Ready for review.
All builds and tests have passed.

source/agora/consensus/EnrollmentManager.d

- needRevealPreimage: function for checking if a certain height is revelation timing - increaseNextRevealHeight: function for setting next block height to reveal pre-image

A validator reveals a pre-image every 1 hour, or 6 blocks and the pre-image has the data for double the period because it make the validator to be able to skip to next revelation timing in case of a failure of the revelation. The revelation period could be configurable sometime.

Geod24 · 2020-02-23T20:30:00Z

Rebased on v0.x.x and fixed

AndrejMitrovic

Post-merge review because I didn't have the chance to review this before.

AndrejMitrovic · 2020-02-28T04:17:07Z

source/agora/consensus/EnrollmentManager.d

@@ -120,6 +127,9 @@ public class EnrollmentManager

        if (!results.empty)
            this.enroll_key = results.oneValue!(ubyte[]).deserializeFull!(Hash);
+
+        // load next height for preimage revelation
+        this.next_reveal_height = this.getNextRevealHeight();


How come setNextRevealHeight is not called here?

At first, the next_reveal_height is set by calling setNextRevealHeight in updateEnrolledHeight which is called in addValidatedBlock of the ledger, which is being reviewed in the PR #504

AndrejMitrovic · 2020-02-28T04:21:40Z

source/agora/consensus/EnrollmentManager.d

@@ -309,6 +319,11 @@ public class EnrollmentManager
                enroll_hash, ex);
            return false;
        }
+
+        // set next height for revealing a pre-image
+        if (enroll_hash == this.enroll_key)


In the future we should separate these two functionalities of the EnrollmentManager:

Managing enrollments for other nodes

Managing enrollments for this node

Otherwise the implementation will become complex and full of state, like it has started to show.

In the future we should separate these two functionalities of the EnrollmentManager

I totally agree with you. I have a plan to split the current EnrollmentManager into EnrollmentManager and ValidatorSet

AndrejMitrovic · 2020-02-28T04:23:43Z

source/agora/consensus/EnrollmentManager.d

+
+    public bool getNextPreimage (out PreimageInfo preimage) @safe
+    {
+        auto height = this.next_reveal_height + PreimageRevealPeriod * 2;


Why the * 2 part? So will this actually reveal preimages for the next 12 blocks (2 hours), rather than 6 blocks (1 hour)?

That was discussed during the whiteboarding session. Currently it's a bit convoluted, as you mentioned, we should use a sliding windows instead.

Thanks, I missed the first part of the whiteboarding session when this happened.

AndrejMitrovic · 2020-02-28T04:24:37Z

source/agora/consensus/EnrollmentManager.d

+    {
+        auto height = this.next_reveal_height + PreimageRevealPeriod * 2;
+        if (height > ValidatorCycle - 1)
+            height = ValidatorCycle - 1;


This looks completely wrong? If the block height is 1009 then we will simply not reveal the preimage?

If the block height is 1009 then we will simply not reveal the preimage?

As previously mentioned, the next_reveal_height is set based on a current block height by calling setNextRevealHeight in updateEnrolledHeight which is called in addValidatedBlock of the ledger which is being reviewed in the PR #504

AndrejMitrovic · 2020-02-28T04:25:39Z

source/agora/consensus/EnrollmentManager.d

+
+    ***************************************************************************/
+
+    public bool getNextPreimage (out PreimageInfo preimage) @safe


Where are the unittests for this function?

Where are the unittests for this function?

I missed it. I will add it in a related PR.

AndrejMitrovic · 2020-02-28T04:33:13Z

source/agora/consensus/EnrollmentManager.d

+    public void increaseNextRevealHeight () @safe nothrow
+    {
+        ulong next_height = this.getNextRevealHeight();
+        if (this.next_reveal_height < ulong.max)


So if there is a database error then the node will silently stop emitting preimages? The only way the user will know this is if they're actively monitoring the logs.

In the meantime the node stops emitting preimages, it gets slashed for not following the protocol, and the node owner loses funds. This is not good.

If there is an exceptional error that makes the node misbehave then we have to treat it as an error, not as a diagnostical issue. If the node can't reveal its preimages because of a database error then the node should shutdown with an error. The node operator can get notified when its node shuts down. That way the node operator gets the chance to fix the error (faulty disk or whatever else is causing the issue).

The node operator can get notified when its node shuts down.

Right. There needs a way to notify an abnormal status of a node to the operator through client UI.

The UI is a separate concern. Agora doesn't have to run with any UI at all. It just has to exit with a process code != 0, and then whichever initialization system is in use (systemd or otherwise) can notify the operator - whether it's through e-mail alerts, push notifications, etc. All of that stuff is external to the Agora node. But we must notify the operating system that we failed, and that means shutting down with an exit code.

I don't think the operators of nodes need to be familiar with command line environment. That's why I said about UI. Adding the code of assert or something in will be worked on one related PR or one issue.

AndrejMitrovic · 2020-02-28T04:33:56Z

source/agora/consensus/EnrollmentManager.d

+        }
+        catch (Exception ex)
+        {
+            log.error("Database operation error {}", ex);


As mentioned before, this is a showstopper error. The node cannot reveal preimages, it should be shut down.

As mentioned before, this is a showstopper error. The node cannot reveal preimages, it should be shut down

There needs to add the code of assert(0) in a related PR.

AndrejMitrovic · 2020-02-28T04:34:29Z

source/agora/consensus/EnrollmentManager.d

+        }
+        catch (Exception ex)
+        {
+            log.error("Database operation error {}", ex);


Same problem here. We are logging errors and then proceeding as if there was no error.

Same problem here. We are logging errors and then proceeding as if there was no error.

There needs to add the code of assert(0) in a related PR.

AndrejMitrovic · 2020-02-28T04:38:34Z

source/agora/node/Node.d

@@ -248,6 +248,16 @@ public class Node : API
            this.network.sendTransaction(tx);
            this.ledger.tryNominateTXSet();
        }
+
+        if (this.enroll_man.needRevealPreimage(this.ledger.getBlockHeight()))


This is not the right place to put this. Why is it being triggered when we receive a Transaction? There might not be enough transactions in the pool to start a nomination. It should be triggered when the ledger's state changes (new block added / externalize was called), and only if we're not catching up on outdated blocks.

TrustHenry reviewed Feb 19, 2020

View reviewed changes

linked0 requested review from Geod24 and AndrejMitrovic February 19, 2020 08:04

linked0 force-pushed the periodic-preimage branch from 9e4e5d0 to 25b8445 Compare February 20, 2020 04:14

Geod24 approved these changes Feb 23, 2020

View reviewed changes

source/agora/consensus/EnrollmentManager.d Outdated Show resolved Hide resolved

Add functions for processing pre-image revelation timing

457d9e4

- needRevealPreimage: function for checking if a certain height is revelation timing - increaseNextRevealHeight: function for setting next block height to reveal pre-image

Geod24 force-pushed the periodic-preimage branch from 25b8445 to 78950c2 Compare February 23, 2020 20:19

Geod24 force-pushed the periodic-preimage branch from 78950c2 to 2c1ab5c Compare February 23, 2020 20:29

Geod24 merged commit 0cdc928 into bosagora:v0.x.x Feb 24, 2020

AndrejMitrovic reviewed Feb 28, 2020

View reviewed changes

AndrejMitrovic mentioned this pull request Feb 28, 2020

Make the validator set restore validators' data from the chain on startup #605

Closed

1 task

linked0 deleted the periodic-preimage branch July 6, 2020 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reveal a pre-image periodically #563

Reveal a pre-image periodically #563

linked0 commented Feb 19, 2020

codecov bot commented Feb 19, 2020 •

edited

Loading

TrustHenry Feb 19, 2020

linked0 commented Feb 20, 2020 •

edited

Loading

Geod24 commented Feb 23, 2020

AndrejMitrovic left a comment

AndrejMitrovic Feb 28, 2020

linked0 Feb 28, 2020 •

edited

Loading

AndrejMitrovic Feb 28, 2020

linked0 Feb 28, 2020 •

edited

Loading

AndrejMitrovic Feb 28, 2020

Geod24 Feb 28, 2020

AndrejMitrovic Feb 28, 2020

AndrejMitrovic Feb 28, 2020

linked0 Feb 28, 2020

AndrejMitrovic Feb 28, 2020

linked0 Feb 28, 2020 •

edited

Loading

AndrejMitrovic Feb 28, 2020

linked0 Feb 28, 2020

AndrejMitrovic Feb 28, 2020

linked0 Feb 28, 2020 •

edited

Loading

AndrejMitrovic Feb 28, 2020

linked0 Feb 28, 2020

AndrejMitrovic Feb 28, 2020

linked0 Feb 28, 2020

AndrejMitrovic Feb 28, 2020

Geod24 Feb 28, 2020


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

		public bool getNextPreimage (out PreimageInfo preimage) @safe

Reveal a pre-image periodically #563

Reveal a pre-image periodically #563

Conversation

linked0 commented Feb 19, 2020

codecov bot commented Feb 19, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

linked0 commented Feb 20, 2020 • edited Loading

Geod24 commented Feb 23, 2020

AndrejMitrovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linked0 Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linked0 Feb 28, 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

linked0 Feb 28, 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

linked0 Feb 28, 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

codecov bot commented Feb 19, 2020 •

edited

Loading

linked0 commented Feb 20, 2020 •

edited

Loading

linked0 Feb 28, 2020 •

edited

Loading

linked0 Feb 28, 2020 •

edited

Loading

linked0 Feb 28, 2020 •

edited

Loading

linked0 Feb 28, 2020 •

edited

Loading