Improve transitive quorum set logic, persist it into the database #2045

MonsieurNicolas · 2019-04-10T01:47:16Z

Description

This PR makes a couple changes to how we deal with the transitive quorum:

it simplified the quorum management code by maintaining a quorum set, attempting to expand it over time as quorum sets are discovered
- this avoids recomputing the transitive set all the time
- this also avoids merging the transitive set between slots
it saves the mapping between nodes and quorum information in the database
- this in turn allows to recover information about nodes, even if those nodes are down, or if the local node restarts

vogel · 2019-04-10T15:19:57Z

src/herder/test/QuorumTrackerTests.cpp

+
+    auto vv = makeValue(1);
+
+    auto checkInQuorum = [&](bool checkSelf, std::set<int> ids) {


checkSelf is always true

ah good catch, fixed

graydon

Handful of clarification comments and a couple perf/correctness bits. Overall looks very good though! I'm happy to be killing off the existing calculation of this! It's a serious bottleneck in the SCP-receive path.

graydon · 2019-04-10T19:18:09Z

src/herder/PendingEnvelopes.cpp

-        res = (r != SCP::TB_FALSE);
-
-        mNodesInQuorum.put(node, res);
+        mQuorumTracker.rebuild([&](NodeID const& id) {


Might be nice for readability to put a return type on this lambda and a comment on the call to rebuild summarizing the logic in the lambda (I can read it off but its purpose and correctness takes a few seconds).

graydon · 2019-04-10T19:40:29Z

src/herder/QuorumTracker.h

+class QuorumTracker : public NonMovableOrCopyable
+{
+    SCP& mSCP;
+    std::unordered_map<NodeID, SCPQuorumSetPtr> mQuorum;


Add comment here: "mQuorum encodes our best current knowledge about the transitive closure of the local node's quorum set (qset). The keys of mQuorum are the nodes that we think are in that transitive closure, and the associated values are either that node's own qset or else nullptr if we do not know / have not heard the definition of that qset yet. The map is incrementally expanded as we hear new messages containing references to new qsets. If a node ever changes its qset -- that is, if we hear a message from a node referencing a qset that is different from the qset for that node stored in mQuorum -- we flush and rebuild mQuorum."

graydon · 2019-04-10T19:41:10Z

src/herder/QuorumTracker.h

+    bool isNodeInQuorum(NodeID const& id);
+
+    // attempts to expand quorum at node `id`
+    // returns true if expansion succeeded


define success and failure of expansion here, and obligation to call rebuild when expansion fails.

graydon · 2019-04-10T19:41:50Z

src/herder/QuorumTracker.cpp

+}
+
+bool
+QuorumTracker::expand(NodeID const& id, SCPQuorumSetPtr qSet)


consider flushing cache in this method also? failure to expand is a "this data structure is invalid and needs to be rebuilt" condition, AIUI

I am letting this up to the caller (added clarification to the contract of expand)

graydon · 2019-04-10T19:46:50Z

src/herder/QuorumTracker.cpp

+                    *qSet, [&](NodeID const& id) { backlog.insert(id); });
+            }
+            mQuorum[n] = qSet;
+        }


There's a third condition you're not handling here, where it != mQuorum.end(), and it->second != nullptr. Implicitly I get that that's a no-op case if it->second == qSet (as I think it generally ought to be here?), but if it's not equal to qSet, you've got a problem. Either a logic error in lookup or maybe a data inconsistency somewhere? Suggest asserting in an else here.

I want to return false in this case (which is already the case):
at this layer, it's possible that we would go from "this node has a quorum X" but we're told that from now on "node has quorum Y" (Y !=X and Y null or not, doesn't matter), I already deal with the case where X == Y explicitely.

Resolution for caller is to call rebuild when this happens

graydon · 2019-04-10T19:55:36Z

src/herder/PendingEnvelopes.h

@@ -139,6 +139,8 @@ class PendingEnvelopes
    // returns true if we think that the node is in quorum
    bool isNodeInQuorum(NodeID const& node);

+    std::unordered_map<NodeID, SCPQuorumSetPtr> const& getCurrentQuorum() const;


As with comment on QuorumTracker's mQuorum member, should comment this map to explain its contents (what the keys mean, the presence of nullptr values in it and so forth). Or make a utility type in QuorumTracker that encapsulates this representation. Or something.

graydon · 2019-04-10T20:00:18Z

src/herder/HerderPersistenceImpl.cpp

+        std::string qSetHHex(binToHex(qSetH));
+
+        auto prep = db.getPreparedStatement(
+            "UPDATE quoruminfo SET qsethash = :h WHERE nodeid = :id");


We're on a version of sql that supports upserts (INSERT ... ON CONFLICT SET ...) now, if you want to simplify this a bit.

I am not too worried about this one yet, and sqlite doesn't support it

graydon · 2019-04-10T20:02:58Z

src/herder/HerderPersistenceImpl.cpp

+}
+
+SCPQuorumSetPtr
+HerderPersistence::getQuorumSet(Database& db, soci::session& sess,


From a separate-commits-compile-on-their-own perspective, I think you are calling this function in the commit before this one that defines it. I don't care too much but if you want to clean up / rebase-edit, might want to fix that.

graydon · 2019-04-10T20:05:48Z

src/herder/HerderPersistenceImpl.cpp

+        std::vector<uint8_t> qSetBytes;
+        decoder::decode_b64(qset64, qSetBytes);
+
+        xdr::xdr_get g1(&qSetBytes.front(), &qSetBytes.back() + 1);


I thought xdr_{to,from}_opaque should handle this a little less-clunkily?

yes, this was from the old code
fixed

graydon · 2019-04-10T20:09:36Z

src/herder/PendingEnvelopes.cpp

+                if (res == nullptr)
+                {
+                    // see if we had some information for that node
+                    auto& db = mApp.getDatabase();


This seems to me like it risks doing a SQL query on every SCP message (including those from strangers), even when they're returning null. I think we probably need a little more caching of the results here, both positive and negative.

no it won't:
rebuild is only happening when we update quorum information which only happens when we actually process an SCP message, and we only process SCP messages for nodes in the known transitive quorum.

ok, so only known people -- but is it still potentially an awful lot of messages?

MonsieurNicolas · 2019-04-11T03:25:14Z

r+ 1dba743ada250e0fc7205ce4c64a56663fb6bd1b

MonsieurNicolas · 2019-04-11T05:25:38Z

had to fix some tests that have some hacky setup: worked around it for now by adding a safeguard when we don't know the local node's quorum set

MonsieurNicolas · 2019-04-11T05:25:44Z

r+ 24f14e1

MonsieurNicolas · 2019-04-11T16:40:43Z

missed a few more tests that had bogus setup

MonsieurNicolas · 2019-04-11T16:40:51Z

missed a few more tests that had bogus setup

MonsieurNicolas · 2019-04-11T16:41:56Z

missed a few more tests that had bogus setup

MonsieurNicolas · 2019-04-11T16:42:12Z

r+ 55d9b18

Improve transitive quorum set logic, persist it into the database Reviewed-by: MonsieurNicolas

MonsieurNicolas added this to To do in v11.0.0 via automation Apr 10, 2019

vogel reviewed Apr 10, 2019

View reviewed changes

graydon requested changes Apr 10, 2019

View reviewed changes

MonsieurNicolas force-pushed the improveTransitiveQSet branch from 142616b to 66962b0 Compare April 10, 2019 22:56

MonsieurNicolas moved this from To do to In progress in v11.0.0 Apr 10, 2019

MonsieurNicolas force-pushed the improveTransitiveQSet branch from 66962b0 to 1dba743 Compare April 11, 2019 03:24

MonsieurNicolas force-pushed the improveTransitiveQSet branch from 1dba743 to 24f14e1 Compare April 11, 2019 05:24

MonsieurNicolas added 12 commits April 11, 2019 09:37

Windows: fix link error

efa021f

warning police

52f8ddc

SCP: add "getLatestMessage" helper function

0f97931

Herder: rework quorum membership code

bb7b074

SCP: remove obsolete "isNodeInQuorum"

c33050e

herder: expose current quorum information

1b94c55

add table to persist quorum information

622ed6d

herder: add quorum/quorumset loading code

ea800bb

herder: persist quorum information in database

ee2e1f8

herder: hook up quorum/quorumset lookup functions with database

cf13457

Herder: ask random peers for SCP messages when out of sync

b8496a6

fix pending envelope test

55d9b18

MonsieurNicolas force-pushed the improveTransitiveQSet branch from 24f14e1 to 55d9b18 Compare April 11, 2019 16:40

latobarita added a commit that referenced this pull request Apr 11, 2019

Merge pull request #2045 from MonsieurNicolas/improveTransitiveQSet

922e366

Improve transitive quorum set logic, persist it into the database Reviewed-by: MonsieurNicolas

latobarita merged commit 55d9b18 into stellar:master Apr 11, 2019

v11.0.0 automation moved this from In progress to Done Apr 11, 2019

MonsieurNicolas mentioned this pull request Apr 18, 2019

tidy up master #2060

Merged

MonsieurNicolas added enhancement performance labels Apr 18, 2019

theaeolianmachine assigned MonsieurNicolas Apr 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve transitive quorum set logic, persist it into the database #2045

Improve transitive quorum set logic, persist it into the database #2045

MonsieurNicolas commented Apr 10, 2019

vogel Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon left a comment

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas Apr 10, 2019

graydon Apr 10, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019


		auto vv = makeValue(1);

		auto checkInQuorum = [&](bool checkSelf, std::set<int> ids) {

Improve transitive quorum set logic, persist it into the database #2045

Improve transitive quorum set logic, persist it into the database #2045

Conversation

MonsieurNicolas commented Apr 10, 2019

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graydon 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

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

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019

MonsieurNicolas commented Apr 11, 2019