ST-623 -- Init Commit multi-sig process #1325

KitHat · 2019-09-05T16:12:36Z

Signed-off-by: Nikita Khateev [email protected]

Signed-off-by: Nikita Khateev <[email protected]>

plenum/common/messages/node_messages.py

plenum/bls/bls_bft_replica_plenum.py

ashcherbakov · 2019-09-06T07:56:06Z

plenum/bls/bls_bft_replica_plenum.py

@@ -92,6 +97,36 @@ def update_commit(self, commit_params, pre_prepare: PrePrepare):
        logger.debug("{}{} signed COMMIT {} for state {} with sig {}"
                     .format(BLS_PREFIX, self, commit_params, state_root_hash, bls_signature))
        commit_params.append(bls_signature)
+
+        audit_ledger = self._database_manager.get_ledger(AUDIT_LEDGER_ID)


Do we have unit tests for the new functionality?

Signed-off-by: Nikita Khateev <[email protected]>

Signed-off-by: Nikita Khateev <[email protected]> # Conflicts: # plenum/common/messages/node_messages.py

Signed-off-by: Nikita Khateev <[email protected]>

KitHat · 2019-09-10T14:11:01Z

test me please

lampkin-diet · 2019-09-10T14:13:03Z

test this please

ashcherbakov · 2019-09-11T08:37:36Z

plenum/bls/bls_bft_replica_plenum.py

@@ -48,6 +62,10 @@ def validate_prepare(self, prepare: Prepare, sender):

    @measure_time(MetricsName.BLS_VALIDATE_COMMIT_TIME)
    def validate_commit(self, commit: Commit, sender, pre_prepare: PrePrepare):
+        if f.BLS_MULTI_SIGS.nm in commit:
+            if not list(filter(lambda sig: self._validate_signature(sender, sig, pre_prepare), commit.blsSigs)):


filter here returns all signatures for which validation succeeded, so if it's empty then either none of them succeeded, or there were no sigs at all. So, according to existing logic:

We return CM_BLS_SIG_WRONG if all sigs are valid

We return CM_BLS_SIG_WRONG if there are no sigs

We don't return CM_BLS_SIG_WRONG if some but not all sigs are invalid

We don't return CM_BLS_SIG_WRONG if all sigs are invalid

ashcherbakov · 2019-09-11T09:02:09Z

plenum/bls/bls_bft_replica_plenum.py

@@ -68,6 +86,8 @@ def update_pre_prepare(self, pre_prepare_params, ledger_id):
        pre_prepare_params.append(self._bls_latest_multi_sig.as_list())
        self._bls_latest_multi_sig = None
        # Send signature in COMMITs only
+        if self._all_bls_latest_multi_sigs is not None:


Can it be that _bls_latest_multi_sig is False/None (so that we call return), but _all_bls_latest_multi_sigs is not None?

ashcherbakov · 2019-09-11T09:03:52Z

plenum/bls/bls_bft_replica_plenum.py

@@ -48,6 +62,10 @@ def validate_prepare(self, prepare: Prepare, sender):

    @measure_time(MetricsName.BLS_VALIDATE_COMMIT_TIME)
    def validate_commit(self, commit: Commit, sender, pre_prepare: PrePrepare):
+        if f.BLS_MULTI_SIGS.nm in commit:
+            if not list(filter(lambda sig: self._validate_signature(sender, sig, pre_prepare), commit.blsSigs)):
+                return


We can not call return here since f.BLS_SIG.nm needs to be set latter

ashcherbakov · 2019-09-11T09:06:30Z

plenum/bls/bls_bft_replica_plenum.py

@@ -115,6 +153,28 @@ def process_commit(self, commit: Commit, sender):
            self._signatures[key_3PC] = {}
        self._signatures[key_3PC][self.get_node_name(sender)] = commit.blsSig

+        if f.BLS_SIGS.nm not in commit:


Can it be that commit.blsSig is None (so we called return), but commit.blsSigs is not None ?

ashcherbakov · 2019-09-11T09:10:27Z

plenum/bls/bls_bft_replica_plenum.py

        self._signatures = {}
+        self._all_signatures = {}


Do we really need self._all_signatures and self._signatures? (self._all_bls_latest_multi_sigs and self._bls_latest_multi_sig)?
Can we make self._signatures (self._bls_latest_multi_sig) to be a special case in self._all_signatures (self._all_bls_latest_multi_sigs)?

I have thought about it. This way we will always need to fetch the correct ledger from _all_signatures according to PrePrepare. On the other hand we have some code duplication -- but it is pretty obvious and we will remove it with the next release and there will be no problems.

ashcherbakov · 2019-09-11T09:18:52Z

plenum/bls/bls_bft_replica_plenum.py

+                )
+            )
+            if sigs_invalid:
+                self._all_signatures.pop(key_3PC, None)


Method name can_do_something needs to do only the checks, not modifications. So, code like self._all_signatures.pop looks really suspicious here.

ashcherbakov · 2019-09-11T09:22:49Z

plenum/bls/bls_bft_replica_plenum.py

+            sigs_for_request = self._all_signatures[key_3PC]
+            sigs_invalid = list(
+                filter(
+                    lambda item: not quorums.bls_signatures.is_reached(len(list(item[1].values()))),


Maybe the following is more clear:

lambda ledger_id, sigs: not quorums.bls_signatures.is_reached(len(sigs))

ashcherbakov · 2019-09-11T09:23:52Z

plenum/bls/bls_bft_replica_plenum.py

+                    sigs_for_request.items()
+                )
+            )
+            if sigs_invalid:


What if we have quorum for some ledgers, not for all? Currently False is returned, but maybe we should be able to calculate multi-sig for the sigs we have quorum for?

ashcherbakov · 2019-09-11T09:39:25Z

plenum/test/bls/test_bls_bft_replica.py

@@ -339,6 +372,81 @@ def test_process_order(bls_bft_replicas, pre_prepare_no_bls, quorums):
                              pre_prepare_no_bls)


+# ------ MULTIPLE MULTI_SIGS ------


Do we have tests where we have a mix of multiple and one sigs?

ashcherbakov · 2019-09-11T10:10:07Z

plenum/test/bls/test_bls_bft_replica.py

+                                                          create_audit_txn_with_multiple_ledgers):
+    for sender_bls_bft_replica in bls_bft_replicas:
+        for verifier_bls_bft_replica in bls_bft_replicas:
+            assert not verifier_bls_bft_replica.validate_pre_prepare(pre_prepare_with_bls_multi,


Please add more tests for validation. For example, when we have multiple sigs, and some of them valid while some are not.

ashcherbakov · 2019-09-11T10:11:41Z

plenum/test/bls/test_bls_bft_replica.py

+    key = (0, 0)
+    for sender_bls_bft in bls_bft_replicas:
+        fake_sig = base58.b58encode(b"somefakesignaturesomefakesignaturesomefakesignature").decode("utf-8")
+        commit = create_commit_with_bls_sigs(key, fake_sig)


Do we have multiple sigs here?
Please add more tests for validation:

when we really have more than 1 sig

when some of this sigs are valid and some are not

when there is multi-sig field, but it's empty

ashcherbakov · 2019-09-11T10:13:28Z

plenum/test/helper.py

@@ -1222,7 +1222,8 @@ def create_pre_prepare_params(state_root,
                              pp_seq_no=0,
                              inst_id=0,
                              audit_txn_root=None,
-                              reqs=None):
+                              reqs=None,
+                              bls_multi_sigs=[]):


I think we need to include bls_multi_sigs into all test helpers for creation of PrePrepare. If it's not specified, a defualt can be added (like [bls_sig]).

ashcherbakov · 2019-09-11T10:13:39Z

plenum/test/helper.py

@@ -1271,6 +1274,14 @@ def create_commit_with_bls_sig(req_key, bls_sig):
    return Commit(*params)


+def create_commit_with_bls_sigs(req_key, bls_sig):


I think we need to include bls_multi_sigs into all test helpers for creation of Commit. If it's not specified, a defualt can be added (like [bls_sig]).

Signed-off-by: Nikita Khateev <[email protected]>

ashcherbakov · 2019-09-12T08:18:43Z

plenum/bls/bls_bft_replica_plenum.py

+                multi_sig = MultiSignature.from_list(*sig)
+                if not self._validate_multi_sig(multi_sig):
+                    return BlsBftReplica.PPR_BLS_MULTISIG_WRONG
+            return


I think return here needs to be removed sine we may have both BLS_MULTI_SIGS and BLS_MULTI_SIG

ashcherbakov · 2019-09-12T08:20:38Z

plenum/bls/bls_bft_replica_plenum.py

@@ -68,6 +95,8 @@ def update_pre_prepare(self, pre_prepare_params, ledger_id):
        pre_prepare_params.append(self._bls_latest_multi_sig.as_list())
        self._bls_latest_multi_sig = None
        # Send signature in COMMITs only
+        if self._all_bls_latest_multi_sigs is not None:
+            pre_prepare_params.append([val.as_list() for val in self._all_bls_latest_multi_sigs])


Should we set _all_bls_latest_multi_sigs to None like we do for _bls_latest_multi_sig ?

ashcherbakov · 2019-09-12T09:05:51Z

plenum/test/helper.py

@@ -1241,6 +1242,8 @@ def create_pre_prepare_params(state_root,
              audit_txn_root or generate_state_root()]
    if bls_multi_sig:
        params.append(bls_multi_sig.as_list())
+    if bls_multi_sigs:


If bls_multi_sigs=[] (default), then this code will not be called.

Fixed this as well

Signed-off-by: Nikita Khateev <[email protected]>

ashcherbakov · 2019-09-12T12:59:42Z

plenum/test/helper.py

@@ -1244,6 +1244,8 @@ def create_pre_prepare_params(state_root,
        params.append(bls_multi_sig.as_list())
    if bls_multi_sigs:


I believe this needs to be

create_pre_prepare_params(....., bls_multi_sigs=None) ...... if bls_multi_sigs is not None: params.append(bls_multi_sig.as_list()) elif bls_multi_sig: params.append([bls_multi_sig.as_list()])

Signed-off-by: Nikita Khateev <[email protected]>

ashcherbakov · 2019-09-12T16:21:15Z

plenum/bls/bls_bft_replica_plenum.py

@@ -62,7 +62,7 @@ def validate_prepare(self, prepare: Prepare, sender):
    @measure_time(MetricsName.BLS_VALIDATE_COMMIT_TIME)
    def validate_commit(self, commit: Commit, sender, pre_prepare: PrePrepare):
        if f.BLS_SIGS.nm in commit:
-            audit_txn = self._get_correct_audit_transaction(pre_prepare.ledgerId, pre_prepare.stateRootHash)
+            audit_txn = self._get_correct_audit_transaction(pre_prepare)


Should we check that audit_txn contains the same number of changes (length of AUDIT_TXN_STATE_ROOT) as blsSigs? Otherwise a KeyError may be raised here.

Signed-off-by: Nikita Khateev <[email protected]>

KitHat · 2019-09-13T05:37:17Z

test this please

ST-623 -- Init Commit multi-sig process

df147fe

Signed-off-by: Nikita Khateev <[email protected]>

KitHat requested review from lampkin-diet, ashcherbakov, skhoroshavin and Toktar as code owners September 5, 2019 16:12

ashcherbakov reviewed Sep 6, 2019

View reviewed changes

Nikita Khateev and others added 8 commits September 9, 2019 19:31

ST-623 -- Making more tests, fixing bugs

6a0f792

Signed-off-by: Nikita Khateev <[email protected]>

Add more unit tests

693cb32

Signed-off-by: Nikita Khateev <[email protected]>

ST-623 -- Finish unit-tests

f71bc07

Signed-off-by: Nikita Khateev <[email protected]>

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

59fa9e0

Signed-off-by: Nikita Khateev <[email protected]> # Conflicts: # plenum/common/messages/node_messages.py

Fix order of fields

ad122a8

Signed-off-by: Nikita Khateev <[email protected]>

Fix order of fields

a276234

Signed-off-by: Nikita Khateev <[email protected]>

ST-623 -- Make fixture yield and fix

722c942

Signed-off-by: Nikita Khateev <[email protected]>

ST-623 -- Fix Yield

f62ce3f

Signed-off-by: Nikita Khateev <[email protected]>

ashcherbakov reviewed Sep 11, 2019

View reviewed changes

Nikita Khateev added 2 commits September 11, 2019 16:21

ST-623 -- Fix some validation

cbecb24

Signed-off-by: Nikita Khateev <[email protected]>

ST-623 -- Add release validation test

3985050

Signed-off-by: Nikita Khateev <[email protected]>

ashcherbakov reviewed Sep 12, 2019

View reviewed changes

Nikita Khateev added 3 commits September 12, 2019 12:31

ST-623 -- Fix some tests

f16dc42

Signed-off-by: Nikita Khateev <[email protected]>

ST-623 -- Fix flake8

9ad4f0f

Signed-off-by: Nikita Khateev <[email protected]>

ST-623 -- Fix some tests

1293fda

Signed-off-by: Nikita Khateev <[email protected]>

ashcherbakov reviewed Sep 12, 2019

View reviewed changes

Nikita Khateev added 2 commits September 12, 2019 17:39

ST-623 -- Fix audit txn search

121e6b0

Signed-off-by: Nikita Khateev <[email protected]>

ST-623 -- Fix audit txn search

f5c4a52

Signed-off-by: Nikita Khateev <[email protected]>

ashcherbakov reviewed Sep 12, 2019

View reviewed changes

ST-623 -- Fix possible malicious node

b5cb01b

Signed-off-by: Nikita Khateev <[email protected]>

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

645b812

ashcherbakov approved these changes Sep 13, 2019

View reviewed changes

ashcherbakov merged commit 06089ae into hyperledger:master Sep 13, 2019

		@@ -339,6 +372,81 @@ def test_process_order(bls_bft_replicas, pre_prepare_no_bls, quorums):
		pre_prepare_no_bls)


		# ------ MULTIPLE MULTI_SIGS ------

		@@ -1271,6 +1274,14 @@ def create_commit_with_bls_sig(req_key, bls_sig):
		return Commit(*params)


		def create_commit_with_bls_sigs(req_key, bls_sig):

		@@ -1244,6 +1244,8 @@ def create_pre_prepare_params(state_root,
		params.append(bls_multi_sig.as_list())
		if bls_multi_sigs:

ST-623 -- Init Commit multi-sig process #1325

ST-623 -- Init Commit multi-sig process #1325

Uh oh!

Conversation

KitHat commented Sep 5, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KitHat commented Sep 10, 2019

Uh oh!

lampkin-diet commented Sep 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KitHat commented Sep 13, 2019

Uh oh!

Uh oh!