INDY-1450: Request ledger statuses and consistency proofs in catchup #800

Toktar · 2018-07-09T09:13:15Z

Changes:

Send message request with ledger status if node did not recieve ledger status in some time.
Send ledger status for request consistency proof of last ordering if node have not f+1 consistensy proofs

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Changes: - added tests for catchup with lost consistency_proof - added resend ledger statuses for request first consistency proofs Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

…nto task-1450-lost-msgs-in-catchup

Toktar · 2018-07-09T13:25:29Z

test this please

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

ashcherbakov · 2018-07-09T15:54:41Z

plenum/common/ledger_manager.py

@@ -86,6 +88,20 @@ def addLedger(self, iD: int, ledger: Ledger,
            verifier=MerkleVerifier(ledger.hasher)
        )

+    def request_ledger_status_if_needed(self, ledger_id):
+        ledgerInfo = self.getLedgerInfoByType(ledger_id)
+        nodes = [node for node in self.owner.nodeReg if node not in ledgerInfo.ledgerStatusOk]


Do we need to add a check node.name != self.owner.name?

No, we don't need because request_ledger_status_from_nodes has this check.

ashcherbakov · 2018-07-09T16:13:27Z

plenum/test/node_catchup/test_node_catchup_with_connection_problem.py

+                                  ha=nodeHa, cliha=nodeCHa,
+                                  pluginPaths=allPluginsPath)
+    # patch canProcessConsistencyProof
+    monkeypatch.setattr(node_to_disconnect.ledgerManager,


Do we patch canProcessConsistencyProof or processConsistencyProof?

I'm sorry. Fixed it.

ashcherbakov · 2018-07-09T16:13:55Z

plenum/test/node_catchup/test_node_catchup_with_connection_problem.py

+    def unpatch_after_call(proof):
+        if node_to_disconnect.ledgerManager.spylog.count(
+                LedgerManager.processConsistencyProof) <= 2:
+            tmp_method(proof)


But tmp_method is for the old instance of disconnected node (before restart)

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

ashcherbakov · 2018-07-10T10:21:58Z

plenum/test/node_catchup/test_node_catchup_with_connection_problem.py

@@ -174,7 +174,24 @@ def unpatch_after_call(proof):
                                  config=tconf,
                                  ha=nodeHa, cliha=nodeCHa,
                                  pluginPaths=allPluginsPath)
-    # patch canProcessConsistencyProof
+
+    original_process_cp = node_to_disconnect.ledgerManager.canProcessConsistencyProof


This is still canProcessConsistencyProof but need to be processConsistencyProof, right?

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

…nto task-1450-lost-msgs-in-catchup

spivachuk · 2018-07-10T14:20:35Z

plenum/common/ledger_manager.py

+    def request_ledger_status_if_needed(self, ledger_id):
+        ledgerInfo = self.getLedgerInfoByType(ledger_id)
+        nodes = [node for node in self.owner.nodeReg if node not in ledgerInfo.ledgerStatusOk]
+        self.owner.request_ledger_status_from_nodes(ledger_id, nodes)


What will be if we do not gather sufficient count of LedgerStatuses this time too?

I suggest that we do this change in a separate task

spivachuk · 2018-07-10T15:08:21Z

plenum/common/ledger_manager.py

+        recvdConsProof = ledgerInfo.recvdConsistencyProofs
+        ledger_status = self.owner.build_ledger_status(ledgerId)
+        nodes = [frm for frm in self.owner.nodeReg if
+                 frm not in recvdConsProof and frm != self.owner.name]


We should ask only the nodes that are definitely ahead of us and did not sent us ConsistencyProofs yet. (Now we also ask the nodes that have not even sent us LedgerStatuses.) To do this we should track the set of nodes which have already been asked for ConsistencyProofs.

Ignore this comment for this PR since we decided to re-ask messages only once and so we should re-ask even those nodes which even potentially may be ahead of us.

spivachuk · 2018-07-10T16:39:28Z

plenum/common/ledger_manager.py

@@ -661,6 +687,11 @@ def canStartCatchUpProcess(self, ledgerId: int):
                adjustedQuorum.f + 1:
            # At least once correct node believes that this node is behind.

+            # Stop last consistensy proof requesting.
+            if ledgerId in self.consistency_proof_action_ids:


We should also cancel re-asking for LedgerStatuses at this moment.

spivachuk · 2018-07-10T16:44:05Z

plenum/common/ledger_manager.py

@@ -315,6 +338,9 @@ def processLedgerStatus(self, status: LedgerStatus, frm: str):
                            format(self, ledgerInfo.ledgerStatusOk, ledgerId))
                logger.info('{} found from ledger status {} that it does not need catchup'.
                            format(self, ledgerStatus))
+                if ledgerId in self.request_ledger_status_action_ids:
+                    self._cancel(self.request_ledger_status_action_ids[ledgerId])


We should also cancel re-asking for last ConsistencyProofs at this moment.

spivachuk

Please see comments.

spivachuk · 2018-07-10T17:30:11Z

plenum/test/node_catchup/test_node_catchup_with_connection_problem.py

+    consistency proof request (not for ledger status). In this case catchup node
+    has no quorum for proofing some size of transactions list. It need to request
+    CONSISTENCY_PROOFs again and finishes catchup. Test makes sure that the node
+    eventually finishes catchup'''


Currently this test does not verify what it must because all the ConsistencyProofs being received by the restarted node are the same. So when the restarted node gathers initial f+1 ConsistencyProofs, it starts catch-up, because these f+1 messages are the same.

Removed this test and added new with check cancel of schedule with requesting ledger statuses and consistency proofs.

Changes: - added cancel for request ledger statuses - added cancel for request consistency proofs - added test for check cancel scheduling ledger statuses and consistency proofs - remove test test_catchup_with_lost_last_consistency_proof Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

…nto task-1450-lost-msgs-in-catchup

spivachuk · 2018-07-11T09:32:51Z

plenum/common/ledger_manager.py

+    def _cancel_request_ledger_statuses_and_consistency_proofs(self, ledger_id):
+        if ledger_id in self.request_ledger_status_action_ids:
+            self._cancel(self.request_ledger_status_action_ids[ledger_id])
+            self.request_ledger_status_action_ids[ledger_id] = None


The way to remove a cancelled action from the map and the the condition being verified above mismatch. It will be uniform and simpler to remove an entry from the map rather than to use None value.

Do you mean something like this?

if ledger_id in self.request_ledger_status_action_ids: action = self.request_ledger_status_action_ids.pop(ledger_id) self._cancel(action)

spivachuk · 2018-07-11T09:33:18Z

plenum/common/ledger_manager.py

+
+        if ledger_id in self.consistency_proof_action_ids:
+            self._cancel(self.consistency_proof_action_ids[ledger_id])
+            self.consistency_proof_action_ids[ledger_id] = None


The same as in the previous comment.

spivachuk · 2018-07-11T09:37:17Z

plenum/common/ledger_manager.py

@@ -291,6 +316,13 @@ def processLedgerStatus(self, status: LedgerStatus, frm: str):
            if self.isLedgerOld(ledgerStatus):
                ledger_status = self.owner.build_ledger_status(ledgerId)
                self.sendTo(ledger_status, frm)
+                if ledgerId not in self.consistency_proof_action_ids or \
+                        ledgerId in self.consistency_proof_action_ids and \
+                        self.consistency_proof_action_ids[ledgerId] is None:


This condition will be simplified if _cancel_request_ledger_statuses_and_consistency_proofs is corrected (see the comment above).

spivachuk · 2018-07-11T09:46:10Z

plenum/common/ledger_manager.py

+            self._cancel(self.consistency_proof_action_ids[ledger_id])
+            self.consistency_proof_action_ids[ledger_id] = None
+
+    def request_ledger_status_if_needed(self, ledger_id):


The suffix _if_needed in the method name seems to be redundant because cancellation of the scheduled action is not a responsibility of this method itself.
It would be better to name this method reask_for_ledger_status.

Also at first this method should remove the triggered action from the map.

spivachuk · 2018-07-11T09:46:50Z

plenum/common/ledger_manager.py

+        nodes = [node for node in self.owner.nodeReg if node not in ledgerInfo.ledgerStatusOk]
+        self.owner.request_ledger_status_from_nodes(ledger_id, nodes)
+
+    def request_last_CP(self, ledgerId):


It would be better to name this method reask_for_last_CP.

Also at first this method should remove the triggered action from the map.

Renamed to reask_for_last_consistency_proof(). Is it ok?

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar added 4 commits July 6, 2018 11:44

INDY-1450: add test test_catchup_with_lost_ledger_status()

7c41d6b

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1450: add request ledger status to schedule

56a52c0

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1450: added request first consistency proofs

d60e890

Changes: - added tests for catchup with lost consistency_proof - added resend ledger statuses for request first consistency proofs Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

9a26019

…nto task-1450-lost-msgs-in-catchup

Toktar changed the title ~~[WIP][ci skip][INDY-1450] Request ledger statuses in catchup~~ INDY-1450: Request ledger statuses and consistency proofs in catchup Jul 9, 2018

Toktar added 2 commits July 9, 2018 16:27

INDY-1450: code style

8b5426c

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1450: code style

2aa41b4

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar force-pushed the task-1450-lost-msgs-in-catchup branch from af5a1e8 to 2aa41b4 Compare July 9, 2018 13:39

ashcherbakov reviewed Jul 9, 2018

View reviewed changes

INDY-1450: add comments to test_node_catchup_with_connection_problem.py

967e7c5

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar force-pushed the task-1450-lost-msgs-in-catchup branch from 1bf3bd0 to 967e7c5 Compare July 10, 2018 07:53

ashcherbakov reviewed Jul 10, 2018

View reviewed changes

Toktar added 2 commits July 10, 2018 13:27

INDY-1450: fix test test_catchup_with_lost_last_consistency_proof

5f030b3

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

e956197

…nto task-1450-lost-msgs-in-catchup

spivachuk reviewed Jul 10, 2018

View reviewed changes

ashcherbakov previously approved these changes Jul 10, 2018

View reviewed changes

spivachuk reviewed Jul 10, 2018

View reviewed changes

spivachuk suggested changes Jul 10, 2018

View reviewed changes

spivachuk reviewed Jul 10, 2018

View reviewed changes

Toktar added 2 commits July 11, 2018 12:05

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

44e61cb

…nto task-1450-lost-msgs-in-catchup

Toktar dismissed ashcherbakov’s stale review via 44e61cb July 11, 2018 09:06

spivachuk reviewed Jul 11, 2018

View reviewed changes

Toktar added 2 commits July 11, 2018 13:47

INDY-1450: refactoring ledger_manager.py

e50c910

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1450: fix test test_cancel_request_cp_and_ls_after_catchup()

1632ecf

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

spivachuk approved these changes Jul 11, 2018

View reviewed changes

ashcherbakov approved these changes Jul 11, 2018

View reviewed changes

ashcherbakov merged commit 4c506ac into hyperledger:master Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INDY-1450: Request ledger statuses and consistency proofs in catchup #800

INDY-1450: Request ledger statuses and consistency proofs in catchup #800

Toktar commented Jul 9, 2018 •

edited

Toktar commented Jul 9, 2018

ashcherbakov Jul 9, 2018

Toktar Jul 10, 2018

ashcherbakov Jul 9, 2018

Toktar Jul 10, 2018

ashcherbakov Jul 9, 2018

Toktar Jul 10, 2018

ashcherbakov Jul 10, 2018

spivachuk Jul 10, 2018

ashcherbakov Jul 10, 2018

spivachuk Jul 10, 2018 •

edited

spivachuk Jul 10, 2018

spivachuk Jul 10, 2018

Toktar Jul 11, 2018

spivachuk Jul 10, 2018

Toktar Jul 11, 2018

spivachuk left a comment

spivachuk Jul 10, 2018

Toktar Jul 11, 2018

spivachuk Jul 11, 2018

Toktar Jul 11, 2018

Toktar Jul 11, 2018

spivachuk Jul 11, 2018

Toktar Jul 11, 2018

spivachuk Jul 11, 2018

Toktar Jul 11, 2018

spivachuk Jul 11, 2018 •

edited

Toktar Jul 11, 2018

spivachuk Jul 11, 2018 •

edited

Toktar Jul 11, 2018

Toktar Jul 11, 2018

INDY-1450: Request ledger statuses and consistency proofs in catchup #800

INDY-1450: Request ledger statuses and consistency proofs in catchup #800

Conversation

Toktar commented Jul 9, 2018 • edited

Toktar commented Jul 9, 2018

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

spivachuk Jul 10, 2018 • edited

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

spivachuk 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

spivachuk Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spivachuk Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toktar commented Jul 9, 2018 •

edited

spivachuk Jul 10, 2018 •

edited

spivachuk Jul 11, 2018 •

edited

spivachuk Jul 11, 2018 •

edited