Implement shell quorum balancing #845

AndrejMitrovic · 2020-05-15T07:07:07Z

Requires:
~~#800~~, ~~#840~~, ~~#810~~, ~~#841~~, ~~#881~~

source/agora/node/Validator.d

@@ -112,7 +174,8 @@ public class Validator : FullNode, API

            log.info("Doing network discovery..");
            this.network.discover(required_peer_keys);
-            this.network.startPeriodicCatchup(this.ledger, &this.nominator.isNominating);
+            this.network.startPeriodicCatchup(this.ledger,


codecov · 2020-05-18T07:08:17Z

Codecov Report

Merging #845 into v0.x.x will increase coverage by 0.03%.
The diff coverage is 90.71%.

@@            Coverage Diff             @@
##           v0.x.x     #845      +/-   ##
==========================================
+ Coverage   90.40%   90.43%   +0.03%     
==========================================
  Files          71       73       +2     
  Lines        5795     5868      +73     
==========================================
+ Hits         5239     5307      +68     
- Misses        556      561       +5

Flag	Coverage Δ
#integration	`55.29% <71.42%> (+0.77%)`	⬆️
#unittests	`89.18% <90.71%> (+0.05%)`	⬆️

Impacted Files	Coverage Δ
source/agora/common/Config.d	`91.52% <ø> (-0.09%)`	⬇️
source/agora/test/Simple.d	`81.81% <ø> (+0.33%)`	⬆️
source/agora/node/Validator.d	`76.31% <75.00%> (+2.12%)`	⬆️
source/agora/test/Quorum.d	`88.88% <88.88%> (ø)`
source/agora/consensus/Quorum.d	`90.47% <90.47%> (ø)`
source/agora/test/EnrollmentManager.d	`89.47% <90.62%> (-7.41%)`	⬇️
source/agora/consensus/protocol/Nominator.d	`88.70% <100.00%> (+0.73%)`	⬆️
source/agora/node/FullNode.d	`89.39% <100.00%> (+4.54%)`	⬆️
source/agora/node/Ledger.d	`97.51% <100.00%> (+0.17%)`	⬆️
source/agora/test/Base.d	`86.64% <100.00%> (-0.16%)`	⬇️
... and 6 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 d2931dd...023698e. Read the comment docs.

source/agora/consensus/Quorum.d

AndrejMitrovic · 2020-05-19T04:06:20Z

It seems the test-suite times out (30 minutes). I'm trying to bisect it.

AndrejMitrovic · 2020-05-19T08:04:47Z

source/agora/test/Base.d


    Returns:
        The genesis block

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

-private immutable(Block) makeGenesisBlock (in NodeConfig[] node_configs)
+private immutable(Block) makeGenesisBlock (in KeyPair[] key_pairs)


This change should be moved to the previous commit where the function was introduced.

AndrejMitrovic · 2020-05-20T05:32:25Z

I've discovered that what is causing these failures is actually the code coverage mode. The test-suite works fine without it. But with it there are timeouts. Either the code coverage is significantly slowing down the tests, or there is a bug in the coverage generator code in DMD front-end.

AndrejMitrovic · 2020-06-04T02:12:59Z

The feature I discovered in Geod24/localrest#81 came in useful. I rewrote one test.

AndrejMitrovic · 2020-06-04T04:54:25Z

I further simplified the changeset. The diff is much smaller now.

The generator currently takes the set of enrollments and generates the same quorum set for all nodes.

In the same way that we remove transactions from the pool only after they're externalized into a block, we should also remove enrollments at this stage.

The quorum set is now derived from the active set of enrollments in the blockchain. After each block has been externalized, the active validator set is checked. If the set of validators has changed, then the quorum set for any Validators is reconfigured according to the quorum generator algorithm. Fixes bosagora#785

AndrejMitrovic · 2020-06-05T07:13:49Z

@bpfkorea/core please review when you have the time.

Geod24 · 2020-06-05T07:49:24Z

source/agora/node/Ledger.d

@@ -228,6 +228,9 @@ public class Ledger
        this.enroll_man.clearExpiredValidators(block.header.height);

        foreach (idx, ref enrollment; block.header.enrollments)
+        {
+            this.enroll_man.pool.remove(enrollment.utxo_key);


https://github.com/bpfkorea/agora/blob/d2931dd0d0f552997bc80e799a5275f5c3cecff5/source/agora/consensus/EnrollmentManager.d#L214

Ah, right. I think in my fixes for #836 I will remove the call within addValidator because that function can fail.

TrustHenry

Overall, it looks Great.

TrustHenry · 2020-06-05T07:32:53Z

source/agora/node/Validator.d

+        return .buildQuorumConfig(this.config.node.key_pair.address,
+            enrollments, this.utxo_set.getUTXOFinder());


agora.consensus.Quorum.buildQuorumConfig ( const ref PublicKey node_key,
in Enrollment[] enrolls, in UTXOFinder finder )
I think it's pointing here.
There's no problem. But The code is difficult to read.

TrustHenry · 2020-06-05T07:48:08Z

source/agora/node/Ledger.d

+            this.enroll_man.pool.remove(enrollment.utxo_key);
+
            if (auto r = this.enroll_man.addValidator(enrollment,
                block.header.height, this.utxo_set.getUTXOFinder()))


this.enroll_man.pool.remove
The remove is run twice here.

Where do you see it?

#845 (comment)

Geod24 · 2020-06-05T07:55:50Z

source/agora/test/Quorum.d

+    // at block height 1007 the freeze tx's are available
+    nodes.enumerate.each!((idx, node) =>
+        retryFor(node.getBlockHeight() == 1007, 5.seconds,
+            format("Node %s has block height %s. Expected: %s",
+                idx, node.getBlockHeight(), 1007)));
+
+    // now we can create enrollments
+    Enrollment enroll_0 = nodes[$ - 2].createEnrollmentData();
+    Enrollment enroll_1 = nodes[$ - 1].createEnrollmentData();
+    nodes[2].enrollValidator(enroll_0);
+    nodes[3].enrollValidator(enroll_1);


Question about this: Should we add the ability to enroll as soon as we freeze ?
Pro:

Genesis does it;

It allows to potentially solve the censorship issue if paired with a rule that more validators => Higher priority;
Cons:

Attack vector ?

Let's file an issue for this. And yes, I wanted this feature too.

Geod24 · 2020-06-05T07:56:17Z

I don't think the double clear should block the merge.

MichaelKim20 · 2020-06-05T07:54:00Z

source/agora/consensus/Quorum.d

+
+    const bool ExtraChecks = true;
+    const(char)* fail_reason;
+    enforce(isQuorumSetSane(scp_quorum, ExtraChecks, &fail_reason),


I think it would be nice to have a test code for isQuorumSetSane.

MichaelKim20 · 2020-06-05T07:54:11Z

source/agora/consensus/Quorum.d

+        format("Quorum %s fails sanity check before normalization: %s",
+                quorum, fail_reason.fromStringz));
+
+    normalizeQSet(scp_quorum);


I think it would be nice to have a test code for normalizeQSet .

TrustHenry · 2020-06-05T10:49:09Z

source/agora/consensus/Quorum.d

+    // test up to 1024 nodes
+    foreach (num_nodes; iota(1, 10).map!(n => 2 ^^ n))


// test up to 1022 nodes

unittest { import std.algorithm; import std.stdio; import std.range; uint sum; foreach (num_nodes; iota(1, 10).map!(n => 2 ^^ n)) sum+=num_nodes; writeln(sum); }

Result : 1022

It's not testing all node count configurations. That would be too expensive (especially once the algorithm is changed).

What I am testing here is this:

num_nodes : 2 num_nodes : 4 num_nodes : 8 num_nodes : 16 num_nodes : 32 num_nodes : 64 num_nodes : 128 num_nodes : 256 num_nodes : 512 num_nodes : 1024

See?

Well actually it's not inclusive as I forgot to use 11 in the iota call. +1 points to you.

AndrejMitrovic added status-blocked Another issue/PR/external dependency is blocking completion of this PR/issue type-feature An addition to the system introducing new functionalities labels May 15, 2020

AndrejMitrovic force-pushed the shell-quorum branch from d58bfe0 to 93e707b Compare May 15, 2020 07:13

This comment has been minimized.

Sign in to view

AndrejMitrovic commented May 15, 2020

View reviewed changes

AndrejMitrovic removed the status-blocked Another issue/PR/external dependency is blocking completion of this PR/issue label May 18, 2020

AndrejMitrovic force-pushed the shell-quorum branch from 93e707b to b1efd87 Compare May 18, 2020 05:12

This comment has been minimized.

Sign in to view

AndrejMitrovic mentioned this pull request May 18, 2020

Fixup gen block parsing & enhance retryFor #853

Merged

AndrejMitrovic force-pushed the shell-quorum branch from b1efd87 to fe95bcd Compare May 18, 2020 06:59

AndrejMitrovic force-pushed the shell-quorum branch 3 times, most recently from caefd1e to 052786b Compare May 18, 2020 07:37

AndrejMitrovic mentioned this pull request May 18, 2020

Remove parsing the quorum configuration from the config file #511

Closed

AndrejMitrovic force-pushed the shell-quorum branch from 73b7bb7 to f36abe2 Compare May 18, 2020 08:16

Geod24 reviewed May 18, 2020

View reviewed changes

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

AndrejMitrovic force-pushed the shell-quorum branch 3 times, most recently from d51c4e8 to fa23de1 Compare May 19, 2020 07:46

AndrejMitrovic commented May 19, 2020

View reviewed changes

AndrejMitrovic force-pushed the shell-quorum branch 4 times, most recently from 6f65b13 to 523d12a Compare May 20, 2020 05:10

AndrejMitrovic force-pushed the shell-quorum branch from 5e3c654 to 3938b9e Compare May 20, 2020 05:58

AndrejMitrovic mentioned this pull request May 20, 2020

Increase default timeout in tests due to code coverage slow down #858

Merged

AndrejMitrovic force-pushed the shell-quorum branch from 3938b9e to b01471c Compare May 20, 2020 06:25

AndrejMitrovic force-pushed the shell-quorum branch from b9fb70e to 393f26f Compare June 4, 2020 02:12

AndrejMitrovic force-pushed the shell-quorum branch 5 times, most recently from ecc45cc to 1ab1b1e Compare June 4, 2020 04:53

AndrejMitrovic force-pushed the shell-quorum branch 4 times, most recently from 1c66d18 to 00996a3 Compare June 5, 2020 01:41

AndrejMitrovic added 4 commits June 5, 2020 10:58

Comment out a flakey test

967382f

Introduce a Quorum generator module

b484f04

The generator currently takes the set of enrollments and generates the same quorum set for all nodes.

Clear enrollments after they're externalized

21e2241

In the same way that we remove transactions from the pool only after they're externalized into a block, we should also remove enrollments at this stage.

AndrejMitrovic force-pushed the shell-quorum branch from 00996a3 to 023698e Compare June 5, 2020 01:58

Geod24 reviewed Jun 5, 2020

View reviewed changes

TrustHenry reviewed Jun 5, 2020

View reviewed changes

Geod24 reviewed Jun 5, 2020

View reviewed changes

Geod24 approved these changes Jun 5, 2020

View reviewed changes

Geod24 merged commit c27172a into bosagora:v0.x.x Jun 5, 2020

MichaelKim20 reviewed Jun 5, 2020

View reviewed changes

AndrejMitrovic deleted the shell-quorum branch June 5, 2020 09:36

TrustHenry reviewed Jun 5, 2020

View reviewed changes

This was referenced Jun 5, 2020

[Draft] Implement a quorum balancing algorithm #684

Closed

Should we add the ability to enroll as soon as we freeze ? #935

Closed

AndrejMitrovic mentioned this pull request Jun 15, 2020

Design and implement quorum balancing #240

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement shell quorum balancing #845

Implement shell quorum balancing #845

AndrejMitrovic commented May 15, 2020 •

edited

Loading

This comment has been minimized.

This comment was marked as resolved.

This comment has been minimized.

codecov bot commented May 18, 2020 •

edited

Loading

AndrejMitrovic commented May 19, 2020

AndrejMitrovic May 19, 2020

AndrejMitrovic commented May 20, 2020

AndrejMitrovic commented Jun 4, 2020

AndrejMitrovic commented Jun 4, 2020

AndrejMitrovic commented Jun 5, 2020 •

edited

Loading

Geod24 Jun 5, 2020

AndrejMitrovic Jun 5, 2020

TrustHenry left a comment

TrustHenry Jun 5, 2020

TrustHenry Jun 5, 2020

AndrejMitrovic Jun 5, 2020

Geod24 Jun 5, 2020

Geod24 Jun 5, 2020

AndrejMitrovic Jun 5, 2020

Geod24 commented Jun 5, 2020

MichaelKim20 Jun 5, 2020

MichaelKim20 Jun 5, 2020

TrustHenry Jun 5, 2020

AndrejMitrovic Jun 5, 2020

AndrejMitrovic Jun 5, 2020

		return .buildQuorumConfig(this.config.node.key_pair.address,
		enrollments, this.utxo_set.getUTXOFinder());

		// test up to 1024 nodes
		foreach (num_nodes; iota(1, 10).map!(n => 2 ^^ n))

Implement shell quorum balancing #845

Implement shell quorum balancing #845

Conversation

AndrejMitrovic commented May 15, 2020 • edited Loading

This comment has been minimized.

This comment was marked as resolved.

This comment has been minimized.

codecov bot commented May 18, 2020 • edited Loading

Codecov Report

AndrejMitrovic commented May 19, 2020

Choose a reason for hiding this comment

AndrejMitrovic commented May 20, 2020

AndrejMitrovic commented Jun 4, 2020

AndrejMitrovic commented Jun 4, 2020

AndrejMitrovic commented Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TrustHenry 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 commented Jun 5, 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

AndrejMitrovic commented May 15, 2020 •

edited

Loading

codecov bot commented May 18, 2020 •

edited

Loading

AndrejMitrovic commented Jun 5, 2020 •

edited

Loading