Save and load all preimages on the database #495

linked0 · 2020-01-23T07:41:45Z

A validator must periodically reveal the preimages.
For that requirement, the preimages should be stored to database for a cycle.

This implementation is the first definition of done of the issue #492

codecov · 2020-01-23T07:47:02Z

Codecov Report

Merging #495 into v0.x.x will decrease coverage by 1.11%.
The diff coverage is 86.11%.

@@            Coverage Diff             @@
##           v0.x.x     #495      +/-   ##
==========================================
- Coverage   88.82%   87.71%   -1.12%     
==========================================
  Files          58       57       -1     
  Lines        4143     4151       +8     
==========================================
- Hits         3680     3641      -39     
- Misses        463      510      +47

Flag	Coverage Δ
#integration	`?`
#unittests	`87.71% <86.11%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
source/agora/common/Serializer.d	`98.66% <100%> (+0.05%)`	⬆️
source/agora/common/Deserializer.d	`91.66% <100%> (+0.52%)`	⬆️
source/agora/consensus/EnrollmentManager.d	`86.26% <82.14%> (-0.92%)`	⬇️
source/agora/common/Task.d	`0% <0%> (-100%)`	⬇️
source/agora/common/Config.d	`72.72% <0%> (-17.49%)`	⬇️
source/agora/node/Node.d	`85.93% <0%> (-10.99%)`	⬇️
source/agora/network/NetworkManager.d	`70.21% <0%> (-4.97%)`	⬇️
source/agora/common/crypto/Key.d	`84.25% <0%> (-4.63%)`	⬇️
source/agora/consensus/protocol/Nominator.d	`77.92% <0%> (-1.3%)`	⬇️
source/agora/node/BlockStorage.d	`67.87% <0%> (-0.73%)`	⬇️
... and 1 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 f9fa2e1...e494213. Read the comment docs.

linked0 · 2020-01-23T08:40:44Z

Ready for review

AndrejMitrovic · 2020-01-28T02:22:27Z

source/agora/consensus/EnrollmentManager.d

+public struct PreimageStore
+{
+    /// Preimages of hashes of random value
+    public Hash[] data;


We could avoid writing all this code if we added serialization support to Hash[] in Serializer.d and Deserializer.d.

@Geod24 can you think of any drawbacks?

After adding serialization support to Hash[] in this PR, I will request a review again.

@AndrejMitrovic : Actually for the binary protocol, I wrote a way to serialize ranges... Guess it's time to PR it.

linked0 · 2020-01-29T07:30:13Z

Ready for review.

AndrejMitrovic · 2020-01-29T07:34:21Z

source/agora/consensus/EnrollmentManager.d

+        results = this.db.execute("SELECT val FROM node_enroll_data " ~
+            "WHERE key = ?", "preimages");
+
+        foreach (row; results)


I think maybe this would work too:

if (!results.empty) this.preimages = results.oneValue!(ubyte[]).deserializeFull!(Hash[]);

Could be worth trying.

AndrejMitrovic · 2020-01-29T07:35:43Z

source/agora/consensus/EnrollmentManager.d

        foreach (i; 0 .. this.data.cycle_length-1)
-            this.data.random_seed = hashFull(this.data.random_seed);
+            this.preimages ~= hashFull(this.preimages[i]);
+        this.data.random_seed = this.preimages[this.data.cycle_length-1];


Why not just this.preimages[$ - 1];? Is it not the same?

AndrejMitrovic · 2020-01-29T07:37:04Z

source/agora/consensus/EnrollmentManager.d

@@ -453,6 +506,38 @@ public class EnrollmentManager
    }
 }

+/// test for serialization and deserialization of PreimageStore


Slightly outdated comment, as PreimageStore doesn't exist anymore.

AndrejMitrovic

small issues

linked0 · 2020-01-29T09:01:18Z

Ready for review. Thanks.

AndrejMitrovic · 2020-01-29T10:27:05Z

source/agora/consensus/EnrollmentManager.d

@@ -453,6 +502,38 @@ public class EnrollmentManager
    }
 }

+/// test for serialization and deserialization of a Hash array
+unittest


This seems to just test serialization? It's not testing EnrollmentManager.

No, It's not. I will make the unittest with EnrollmentManager.

linked0 · 2020-01-30T03:41:30Z

Ready for review.

AndrejMitrovic · 2020-01-30T04:06:12Z

source/agora/consensus/EnrollmentManager.d

@@ -587,4 +672,13 @@ unittest
    man.getUnregisteredEnrollments(enrolls);
    assert(enrolls.length == 3);
    assert(enrolls.isStrictlyMonotonic!("a.utxo_key < b.utxo_key"));
+
+    // test serialization/deserializetion for pre-images
+    auto preimages_1 = man.getPreimages();


You can access the field directly, because private allows access from the same module.

So just auto preimages_1 = man.preimages;.

AndrejMitrovic · 2020-01-30T04:08:58Z

source/agora/consensus/EnrollmentManager.d

+    auto preimages_1 = man.getPreimages();
+    auto preimages_2 = man.loadPreimages();
+    int true_count;
+    foreach (i; 0 .. 1008)


These few lines can be replaced with assert(preimages_1[] == preimages_2[]);. This will check the length too.

AndrejMitrovic · 2020-01-30T04:31:17Z

source/agora/consensus/EnrollmentManager.d

+    foreach (i; 0 .. 1008)
+        if (preimages_1[i] == preimages_2[i])
+            true_count++;
+    assert(true_count == 1008);


There is a missing check that the preimages are actually preimages. You can use this:

import std.algorithm; assert(equal!((a, b) => a.hashFull() == b)(preimages_1[0 .. $ - 1], preimages_1[1 .. $]));

linked0 · 2020-01-30T08:10:00Z

Ready for review after applying the suggestions of @AndrejMitrovic .

AndrejMitrovic

LGTM.

A validator must periodically reveal the pre-images. For that requirement, the pre-images should be stored to database for a cycle.

linked0 · 2020-01-31T00:07:49Z

Ping @Geod24

linked0 force-pushed the save-preimage branch 2 times, most recently from 5841a1f to 7c89d29 Compare January 23, 2020 07:46

linked0 force-pushed the save-preimage branch from 4f150d6 to a8f9f2a Compare January 23, 2020 08:13

AndrejMitrovic reviewed Jan 28, 2020

View reviewed changes

linked0 force-pushed the save-preimage branch from a8f9f2a to 06bdb9f Compare January 29, 2020 06:38

AndrejMitrovic reviewed Jan 29, 2020

View reviewed changes

AndrejMitrovic suggested changes Jan 29, 2020

View reviewed changes

linked0 force-pushed the save-preimage branch from 06bdb9f to d2a0372 Compare January 29, 2020 07:49

AndrejMitrovic reviewed Jan 29, 2020

View reviewed changes

linked0 force-pushed the save-preimage branch 2 times, most recently from 6d55c73 to 97e99c4 Compare January 30, 2020 02:39

AndrejMitrovic suggested changes Jan 30, 2020

View reviewed changes

linked0 force-pushed the save-preimage branch from 97e99c4 to a88d8f6 Compare January 30, 2020 07:34

linked0 requested review from AndrejMitrovic and Geod24 January 30, 2020 08:38

AndrejMitrovic approved these changes Jan 30, 2020

View reviewed changes

linked0 added 2 commits January 30, 2020 18:43

Add serialization/deserialization support to Hash[]

c701470

Save and load all pre-images on the database

e494213

A validator must periodically reveal the pre-images. For that requirement, the pre-images should be stored to database for a cycle.

linked0 force-pushed the save-preimage branch from a88d8f6 to e494213 Compare January 30, 2020 09:44

Geod24 approved these changes Jan 31, 2020

View reviewed changes

Geod24 merged commit 3f4bd69 into bosagora:v0.x.x Jan 31, 2020

linked0 deleted the save-preimage branch March 29, 2020 02:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save and load all preimages on the database #495

Save and load all preimages on the database #495

linked0 commented Jan 23, 2020 •

edited

codecov bot commented Jan 23, 2020 •

edited

linked0 commented Jan 23, 2020

AndrejMitrovic Jan 28, 2020

linked0 Jan 29, 2020

Geod24 Jan 29, 2020

linked0 commented Jan 29, 2020

AndrejMitrovic Jan 29, 2020

AndrejMitrovic Jan 29, 2020

AndrejMitrovic Jan 29, 2020

AndrejMitrovic left a comment

linked0 commented Jan 29, 2020

AndrejMitrovic Jan 29, 2020

linked0 Jan 29, 2020

linked0 commented Jan 30, 2020

AndrejMitrovic Jan 30, 2020

AndrejMitrovic Jan 30, 2020

AndrejMitrovic Jan 30, 2020

linked0 commented Jan 30, 2020

AndrejMitrovic left a comment

linked0 commented Jan 31, 2020

Save and load all preimages on the database #495

Save and load all preimages on the database #495

Conversation

linked0 commented Jan 23, 2020 • edited

codecov bot commented Jan 23, 2020 • edited

Codecov Report

linked0 commented Jan 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linked0 commented Jan 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrejMitrovic left a comment

Choose a reason for hiding this comment

linked0 commented Jan 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linked0 commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linked0 commented Jan 30, 2020

AndrejMitrovic left a comment

Choose a reason for hiding this comment

linked0 commented Jan 31, 2020

linked0 commented Jan 23, 2020 •

edited

codecov bot commented Jan 23, 2020 •

edited