Implement broadcasting a pre-image #530

linked0 · 2020-02-07T05:57:22Z

This PR is about broadcasting a pre-image and saving it into the validator set table.

sendPreimage endpoint in Validator API

codecov · 2020-02-07T06:10:13Z

Codecov Report

Merging #530 into v0.x.x will decrease coverage by 1%.
The diff coverage is 84.28%.

@@            Coverage Diff             @@
##           v0.x.x     #530      +/-   ##
==========================================
- Coverage   89.14%   88.14%   -1.01%     
==========================================
  Files          59       58       -1     
  Lines        4285     4320      +35     
==========================================
- Hits         3820     3808      -12     
- Misses        465      512      +47

Flag	Coverage Δ
#integration	`?`
#unittests	`88.14% <84.28%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
source/agora/test/EnrollmentManager.d	`96.29% <100%> (+0.64%)`	⬆️
source/agora/network/NetworkClient.d	`92.1% <100%> (+0.43%)`	⬆️
source/agora/node/GossipProtocol.d	`100% <100%> (ø)`	⬆️
source/agora/node/Node.d	`86.56% <100%> (-10.36%)`	⬇️
source/agora/test/Base.d	`87.7% <100%> (+0.2%)`	⬆️
source/agora/network/NetworkManager.d	`69.86% <75%> (-4.79%)`	⬇️
source/agora/consensus/EnrollmentManager.d	`86.24% <80.76%> (-1.32%)`	⬇️
source/agora/common/Task.d	`0% <0%> (-100%)`	⬇️
source/agora/common/Config.d	`72.72% <0%> (-17.49%)`	⬇️
... and 7 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 6584fcf...99f3c12. Read the comment docs.

Geod24 · 2020-02-07T06:12:35Z

source/agora/consensus/EnrollmentManager.d

+{
+    public Hash utxo_key;
+    public Hash hash;
+    public uint round;


That's an interesting approach. When we were originally talking about it, I had in mind that this would by block height, not round. Because the issue is that if you re-use the same utxo_key (as you would), then you can do replay attacks.
Of course validation would fail, but you could potentially pretend a node sent you the wrong preimage data. We didn't factor any slashing rules that could be triggered by this tho.

A downside of using the block_height is that it's harder to pre-emptively broadcast preimage as you don't know whe height at which it'll be included. But that can be dealt with when the enrollment is received.

In any case, this could use some documentation

I'd like accept your thought because it's more convenient to recognize the block to which the preimage is related. So, I could add the variable named start_block_height into the Enrollment structure which is the block height where a validator is supposed to participate in validating. Then, The variable, round of the PreimageInfo structure could be replaced with a variable like round_block_height. How about this?

start_block_height

Minor stylistic note: "height" is pretty unambiguous in blockchain code, so you can just use start_height (because there's no other height that block height).

which is the block height where a validator is supposed to participate in validating

I'll have to double check, the exact flow, but bear in mind that if we go this way, it is an implicit value. What I mean is that, when you create an Enrollment structure and sign it, you cannot know which start_height you will be assigned, because it has to be accepted by the validators. Same goes for transactions, when you create a transaction, you cannot know at which height it will be.

Does that make sense ? Also, I think both approaches have their merits, so maybe let's have a whiteboarding session about it ?

I understand what you're considering. After a bit of thought, I can use the enrolled_height variable which is implemented by Henry. The value of enrolled_height + 1 will have the same value as what I want to have with the start_block_height variable. It's good to have a whiteboarding session.

Geod24 · 2020-02-07T06:19:35Z

source/agora/consensus/EnrollmentManager.d

+    PreimageInfo preimage;
+    auto round = 1000;
+    assert(man.getPreimage(1000, preimage));
+    assert(preimage.hash == man.preimages[round-1]);


Should also test with 0 and > ValidatorCycleLength

Geod24 · 2020-02-07T06:20:02Z

source/agora/consensus/EnrollmentManager.d

+{
+    public Hash utxo_key;
+    public Hash hash;
+    public uint round;


In any case, this could use some documentation

Geod24 · 2020-02-07T06:21:11Z

source/agora/consensus/Validation.d

+///
+unittest
+{
+    uint CycleLength = 1008;


Use const if it isn't going to be modified

Also, since you made it a constant in an earlier commit, can we just use it ?

Geod24 · 2020-02-07T06:22:36Z

source/agora/consensus/Validation.d

+*******************************************************************************/
+
+public string isInvalidPreimageReason (const ref Hash preimage, const uint round,
+    const ref Hash init_seed, immutable uint cycle_length) nothrow @safe


For value type, const / immutable does not make sense. So just use uint.

Geod24 · 2020-02-07T06:23:38Z

source/agora/consensus/Validation.d

+
+    // check the pre-image has a right hash value
+    Hash temp_hash = preimage;
+    for (int i = round; i < cycle_length; i++)


Suggested change

for (int i = round; i < cycle_length; i++)

for (uint i = round; i < cycle_length; i++)

Geod24 · 2020-02-07T06:29:32Z

source/agora/network/NetworkClient.d

+
+    ***************************************************************************/
+
+    public void sendPreimage (PreimageInfo preimage) @trusted


Suggested change

public void sendPreimage (PreimageInfo preimage) @trusted

public void sendPreimage (ref PreimageInfo preimage) @trusted

Geod24 · 2020-02-07T06:30:53Z

source/agora/consensus/EnrollmentManager.d

+
+*******************************************************************************/
+
+public struct PreimageInfo


This should probably be in consensus.data

Geod24 · 2020-02-07T06:31:41Z

source/agora/test/Base.d

@@ -433,6 +433,12 @@ public interface TestAPI : API

    ///
    public abstract Enrollment createEnrollmentData();
+
+    ///
+    public abstract PreimageInfo getNodePreimage(uint round);


Suggested change

public abstract PreimageInfo getNodePreimage(uint round);

public abstract PreimageInfo getNodePreimage (uint round);

Geod24 · 2020-02-07T06:35:39Z

source/agora/api/Validator.d

+
+    ***************************************************************************/
+
+    public void sendPreimage (PreimageInfo preimage);


Naming wise, we have receiveEnvelope but sendPreimage. I don't mind either way but we should be consistent.

Do you mean changing the function name into a name like receivePreimage? If you do, changing the name is not a problem for me. I mean I can change the neme.

Geod24 · 2020-02-07T06:36:54Z

source/agora/test/EnrollmentManager.d

+    preimage_1 = node_1.getNodePreimage(1000);
+    node_1.sendPreimage(preimage_1);
+    auto preimage_2 = node_2.getValidatorPreimage(preimage_1.enroll_key);
+    assert(preimage_1 == preimage_2);


This is not black box testing. Adding 2 special methods means you aren't really testing the API.
A better test would be to send a pre-image and check if the (X) next block contains it.
Additionally you could filter the sendPreimage function of a block to see if it propagates correctly even in the absence of the preimage in one block (but that depends on Henry's PR)/

My original approach is not to save pre-images into a block. It's stored only in the validator set table. A pre-image can be restored from another nodes if the database has been crashed. In that case, another API endpoint like getPreimage is needed with the getNodePreimage and getValidatorPreimage removed. Anyway the two functions could be confusing.

There is another approach about saving pre-images. It's to store the source value of the random seed encrypted with a node's private key into the Enrollment structure. With that, if the database crashed, the source value of the random seed used for enrollment can be restored from the chain.

Geod24 · 2020-02-11T04:30:07Z

source/agora/consensus/EnrollmentManager.d

+    ***************************************************************************/
+
+    public bool getPreimage (uint round, out PreimageInfo preimage)
+        @trusted nothrow


Why @trusted ?

linked0 · 2020-02-12T01:08:42Z

I will make a smaller PR including two commits of this PR and apply reviewer's suggestion in the new PR. And then, I will handle with the remaining commits of this PR which is not inluded in the new PR.

linked0 · 2020-02-17T01:39:36Z

Ready for review.
In order to make smaller PRs, I am splitting this PR to three or four PRs inclusive of this PR. So In this PR, I only put the code about the sendPreimage endpoint in Validator API.

source/agora/consensus/EnrollmentManager.d

AndrejMitrovic

Not done reviewing, github won't let me continue because of a push.

source/agora/consensus/EnrollmentManager.d

AndrejMitrovic

Reviewed first commit.

source/agora/consensus/EnrollmentManager.d

AndrejMitrovic · 2020-02-17T05:44:52Z

source/agora/consensus/EnrollmentManager.d

+        {
+            () @trusted {
+                this.db.execute("UPDATE validator_set SET preimage = ? " ~
+                    "WHERE key = ?", buffer, preimage.enroll_key.toString());


Same comment about memory allocation, toString() allocates. Can use sformat to write the entire string to a buffer before calling execute.

source/agora/consensus/EnrollmentManager.d

source/agora/network/NetworkClient.d

source/agora/node/GossipProtocol.d

source/agora/test/Base.d

AndrejMitrovic · 2020-02-17T05:57:45Z

source/agora/test/EnrollmentManager.d

+
+    // tests for revealing a pre-image
+    auto preimage = node_1.getPreimage(1000);
+    node_1.receivePreimage(preimage);


This node should already know about its own preimages. It is odd that it would need to send the preimage to itself in order to know about it, no?

AndrejMitrovic · 2020-02-17T06:02:32Z

Done with review.

The EnrollmentManger class must provide functions to add a piece of pre-image information and check existence of it for broadcasting preimage.

A Node reveals a pre-image information into the network. The broadcasting of a pre-image is processed through the gossip protocol.

Geod24

LGTM, let's move forward with this.

Geod24 · 2020-02-14T08:51:06Z

source/agora/api/Validator.d

@@ -43,6 +43,8 @@
 module agora.api.Validator;

 import agora.common.crypto.Key;
+import agora.common.Hash;


Suggested change

import agora.common.Hash;

import agora.common.Types;

Addressed

Geod24 reviewed Feb 7, 2020

View reviewed changes

Geod24 suggested changes Feb 7, 2020

View reviewed changes

linked0 force-pushed the send-preimage branch 2 times, most recently from 029981c to 2939554 Compare February 11, 2020 00:43

Geod24 reviewed Feb 11, 2020

View reviewed changes

linked0 mentioned this pull request Feb 12, 2020

Implement getting node's pre-image based on block height #536

Merged

linked0 force-pushed the send-preimage branch 4 times, most recently from 4ae42f3 to f56a3a2 Compare February 17, 2020 00:43

TrustHenry previously requested changes Feb 17, 2020

View reviewed changes

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

linked0 force-pushed the send-preimage branch 2 times, most recently from c739564 to 0bde88c Compare February 17, 2020 05:42

AndrejMitrovic suggested changes Feb 17, 2020

View reviewed changes

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

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

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

AndrejMitrovic suggested changes Feb 17, 2020

View reviewed changes

source/agora/network/NetworkClient.d Outdated Show resolved Hide resolved

source/agora/node/GossipProtocol.d Outdated Show resolved Hide resolved

source/agora/node/GossipProtocol.d Outdated Show resolved Hide resolved

AndrejMitrovic previously requested changes Feb 17, 2020

View reviewed changes

linked0 force-pushed the send-preimage branch 2 times, most recently from 9c943dc to 74c5006 Compare February 18, 2020 05:39

linked0 added 3 commits February 18, 2020 15:13

Add addPreimage and hasPreimage functions to EnrollmentManager

2c72cdd

The EnrollmentManger class must provide functions to add a piece of pre-image information and check existence of it for broadcasting preimage.

Add receivePreimage function into the Validator API

d64693b

A Node reveals a pre-image information into the network. The broadcasting of a pre-image is processed through the gossip protocol.

Add unittest code for broadcasting a preimage through the network

99f3c12

linked0 force-pushed the send-preimage branch from 74c5006 to 99f3c12 Compare February 18, 2020 06:19

Geod24 approved these changes Feb 18, 2020

View reviewed changes

Geod24 merged commit b6c859f into bosagora:v0.x.x Feb 18, 2020

linked0 deleted the send-preimage branch March 29, 2020 02:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement broadcasting a pre-image #530

Implement broadcasting a pre-image #530

linked0 commented Feb 7, 2020 •

edited

codecov bot commented Feb 7, 2020 •

edited

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

linked0 Feb 9, 2020

Geod24 Feb 10, 2020

linked0 Feb 10, 2020

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

Geod24 Feb 7, 2020

linked0 Feb 10, 2020

Geod24 Feb 7, 2020

linked0 Feb 10, 2020

linked0 Feb 10, 2020

Geod24 Feb 11, 2020

linked0 commented Feb 12, 2020

linked0 commented Feb 17, 2020 •

edited

AndrejMitrovic left a comment

AndrejMitrovic left a comment

AndrejMitrovic Feb 17, 2020

AndrejMitrovic Feb 17, 2020

AndrejMitrovic commented Feb 17, 2020

Geod24 left a comment

Geod24 Feb 14, 2020

	for (int i = round; i < cycle_length; i++)
	for (uint i = round; i < cycle_length; i++)


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

		public void sendPreimage (PreimageInfo preimage) @trusted

	public void sendPreimage (PreimageInfo preimage) @trusted
	public void sendPreimage (ref PreimageInfo preimage) @trusted


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

		public struct PreimageInfo

	public abstract PreimageInfo getNodePreimage(uint round);
	public abstract PreimageInfo getNodePreimage (uint round);

Implement broadcasting a pre-image #530

Implement broadcasting a pre-image #530

Conversation

linked0 commented Feb 7, 2020 • edited

codecov bot commented Feb 7, 2020 • edited

Codecov Report

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

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

linked0 commented Feb 12, 2020

linked0 commented Feb 17, 2020 • edited

AndrejMitrovic left a comment

Choose a reason for hiding this comment

AndrejMitrovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrejMitrovic commented Feb 17, 2020

Geod24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linked0 commented Feb 7, 2020 •

edited

codecov bot commented Feb 7, 2020 •

edited

linked0 commented Feb 17, 2020 •

edited