INDY-1846: Change pool state root hash for BLS-signature in Commit messages #980

Toktar · 2018-11-16T12:27:06Z

- Add the current uncommitted pool state root hash to "POOL_STATE_ROOT_HASH" to PrePrepare message (Put it before PLUGIN_FIELDS for correct plugins work. Plugins add this field in the end of PrePrepare message)
- Extend validation of received PrePreapares to check, that POOL_STATE_ROOT_HASH is equal to the node's uncommitted state root hash. If they are not equal - raise a suspicious error.
- Change commit BLS signature creating. Now it should use pool state root hash from PrePrepare message instead committed state root hash. We can do it because pool state root hash from PrePrepare has already been verified in PrePrepare validation.
- Change validation of a commit BLS signature. Now it should use pool state root hash from PrePrepare message instead committed state root hash.
- Change logging message in case with incorrect BLS signature for commit message. Add information about an incorrect Commit message
- Change using BLS key in creating signature from committed state to using key from uncommitted state.

Add unit tests:

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

Changes: - Add the current uncommitted pool state root hash to "POOL_STATE_ROOT_HASH" to PrePrepare message - Extend validation of received PrePrepares Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

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

Changes: - Change commit BLS signature creating - Change validation of a commit BLS signature - Add tests Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

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

Changes: - Add tests - Move pool_state_root_hash in create3PCBatch() - Back to use in signatures committed state Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

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

ashcherbakov · 2018-11-21T13:31:05Z

plenum/bls/bls_bft_replica_plenum.py

@@ -183,10 +188,12 @@ def _validate_multi_sig(self, multi_sig: MultiSignature):
                                                                  public_keys)

    def _sign_state(self, pre_prepare: PrePrepare):
-        pool_root_hash_ser = self.state_root_serializer.serialize(
-            bytes(self._bls_bft.bls_key_register.get_pool_root_hash_committed()))
+        pool_root_hash = pre_prepare.poolStateRootHash \


I think it would be better to create a helper method (in this class) to get pool_root_hash with PrePrepare as an input

Added BlsBftReplicaPlenum._get_pool_root_hash()

ashcherbakov · 2018-11-21T13:33:39Z

plenum/common/messages/node_messages.py

@@ -152,11 +151,14 @@ class PrePrepare(MessageBase):
        (f.TXN_ROOT.nm, MerkleRootField(nullable=True)),
        (f.SUB_SEQ_NO.nm, NonNegativeNumberField()),
        (f.FINAL.nm, BooleanField()),
+        (f.POOL_STATE_ROOT_HASH.nm, MerkleRootField(optional=True,


It needs to be after BLS_MULTI_SIG

Please add a test checking that a Primary sending POOL_STATE_ROOT_HASH is understood by nodes which are not aware of this field

POOL_STATE_ROOT_HASH needs to be before BLS_MULTI_SIG because PrePrepare can be not contains multi signature.
But will add tests for parsing different variants of PrePrepare messages.

add test_process_pre_prepare_validation_old_schema (for all variants of PrePrepare)

change test_process_pre_prepare_validation (for all variants of PrePrepare)

ashcherbakov · 2018-11-21T13:46:38Z

plenum/test/bls/test_bls_bft_replica.py

@@ -295,6 +315,19 @@ def test_validate_commit_incorrect_sig(bls_bft_replicas, pre_prepare_with_bls):
            assert status == BlsBftReplica.CM_BLS_SIG_WRONG


+def test_validate_commit_incorrect_signed_without_pool_state_root(bls_bft_replicas, multi_signature):


Why the signature is incorrect in this case?

Signature is correct in this case. Changed wrong method name to test_validate_commit_signature_without_pool_state_root

ashcherbakov · 2018-11-21T13:49:54Z

plenum/test/replica/test_api.py

+
+
+def test_process_pre_prepare_with_pool_state_root(replica):


Should this test be named test_process_pre_prepare_with_incorrect_pool_state_root?

ashcherbakov · 2018-11-21T13:53:29Z

plenum/test/bls/test_bls_bft_replica.py

+                                                        sender_bls_bft.node_id,
+                                                        pre_prepare)
+
+


I think we need more unit tests when pool_state_root_hash is incorrect

What can we add to addition the test where pool_state_root_hash in PrePrepare is incorrect and message will be discarded?

ashcherbakov · 2018-11-21T13:55:50Z

plenum/test/bls/test_commit_signature_validation_integration.py

+    Nodes 1, 2 ordered txn1 and nodes 3, 4 did not.
+    All nodes  receive PrePrepare2(txn2 for domain_ledger)
+    Nodes 3, 4 receive commits from nodes 1, 2
+    Nodes 1, 2 ordered txn1


Should it be: Nodes 3, 4 ordered txn1?

Yes, thank you. Fixed it.

ashcherbakov · 2018-11-21T14:06:10Z

plenum/test/bls/test_commit_signature_validation_integration.py

+        looper.run(eventually(check_fast_nodes_ordered_request))
+
+        request2 = sdk_send_random_request(looper, sdk_pool_handle, sdk_wallet_client)
+        looper.run(eventually(check_nodes_receive_pp, first_ordered[0], first_ordered[1] + 2))


I think we should also check, that slow nodes receive commits for the second request (that is they validate it while txn1 is still not ordered)

Added check.

Changes: - refactoring test_process_pre_prepare_with_pool_state_root - refactoring test_commit_signature_validation_integration - refactoring test_validate_commit_incorrect_signed_without_pool_state_root - refactoring test_validate_commit_correct_sig_second_time - add _get_pool_root_hash to BlsBftReplicaPlenum Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

ashcherbakov · 2018-11-22T06:47:58Z

plenum/bls/bls_bft_replica_plenum.py

        sender_node = self.get_node_name(sender)
        pk = self._bls_bft.bls_key_register.get_key_by_name(sender_node, pool_root_hash)
        if not pk:
            return False
+        pool_root_hash_ser = self._get_pool_root_hash(pre_prepare, serialize=False)


I think serialized value needs to be passed here (serialize=True).
In general, I would suggest to move calculation of pool_root_hash_ser to _create_multi_sig_value_for_pre_prepare method.

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

ashcherbakov · 2018-11-23T08:20:01Z

plenum/test/replica/test_api.py

-    replica.processPrePrepare(pp, replica.primaryName)
+    fake_replica.node.reportSuspiciousNodeEx = reportSuspiciousNodeEx
+
+    fake_replica.processPrePrepare(pre_prepare, fake_replica.primaryName)


This test doesn't make sense, since schema is not called here, we already pass pre_prepare object.
We need to create PrePrepare as a dict, patch PrePrepare schema, and create PrePrepare instance from this dict.
see test_strict_schema.py for example

ashcherbakov · 2018-11-23T08:21:15Z

plenum/test/bls/test_bls_bft_replica.py

@@ -236,8 +236,11 @@ def test_validate_pre_prepare_no_sigs(bls_bft_replicas, pre_prepare_no_bls):
 def test_validate_pre_prepare_correct_multi_sig(bls_bft_replicas, pre_prepare_with_bls):
    for sender_bls_bft_replica in bls_bft_replicas:
        for verifier_bls_bft_replica in bls_bft_replicas:
+            committed_root_hash = verifier_bls_bft_replica._bls_bft.bls_key_register.get_pool_root_hash_committed


Please create a separate test test_validate_pre_prepare_does_not_use_committed_pool_state

ashcherbakov · 2018-11-23T08:22:00Z

plenum/test/bls/test_bls_bft_replica.py

@@ -298,9 +301,12 @@ def test_validate_commit_correct_sig_second_time(bls_bft_replicas, pre_prepare_w
    for sender_bls_bft in bls_bft_replicas:
        commit = create_commit_bls_sig(sender_bls_bft, key, pre_prepare_with_bls)
        for verifier_bls_bft in bls_bft_replicas:
+            committed_root_hash = verifier_bls_bft._bls_bft.bls_key_register.get_pool_root_hash_committed


Please create a separate test test_validate_commit_does_not_use_committed_pool_state

ashcherbakov · 2018-11-23T08:23:34Z

plenum/test/bls/test_bls_bft_replica.py

@@ -236,8 +236,11 @@ def test_validate_pre_prepare_no_sigs(bls_bft_replicas, pre_prepare_no_bls):
 def test_validate_pre_prepare_correct_multi_sig(bls_bft_replicas, pre_prepare_with_bls):
    for sender_bls_bft_replica in bls_bft_replicas:
        for verifier_bls_bft_replica in bls_bft_replicas:
+            committed_root_hash = verifier_bls_bft_replica._bls_bft.bls_key_register.get_pool_root_hash_committed


Can we patch the methods like this? Should we use monkeypatch?

Changed to using monkeypatch.

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

Toktar added 3 commits November 16, 2018 15:09

INDY-1846: Add integration test

4f9d2d5

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

INDY-1846: Add POOL_STATE_ROOT_HASH to PrePrepare

7f09a1a

Changes: - Add the current uncommitted pool state root hash to "POOL_STATE_ROOT_HASH" to PrePrepare message - Extend validation of received PrePrepares Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1846: Add tests for POOL_STATE_ROOT_HASH in PrePrepare

479e60d

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

Toktar changed the title ~~[WIP][ci skip][INDY-1846] Change pool state root hash for BLS-signature in Commit messages~~ [WIP][INDY-1846] Change pool state root hash for BLS-signature in Commit messages Nov 19, 2018

Toktar added 4 commits November 20, 2018 14:16

INDY-1846: Change commit BLS signature creating

6a75f26

Changes: - Change commit BLS signature creating - Change validation of a commit BLS signature - Add tests Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1846: code style

48e7b69

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

INDY-1846: bugfix create3PCBatch()

45e2cee

Changes: - Add tests - Move pool_state_root_hash in create3PCBatch() - Back to use in signatures committed state Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1846: get hash to bls-key from pre-prepare for commit validation

fea9e20

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

Toktar changed the title ~~[WIP][INDY-1846] Change pool state root hash for BLS-signature in Commit messages~~ INDY-1846: Change pool state root hash for BLS-signature in Commit messages Nov 20, 2018

Toktar added 3 commits November 20, 2018 18:11

INDY-1846: fix test test_create_3pc_batch_with_empty_requests()

826af7e

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

INDY-1846: fix checkPropagated in tests

fdab037

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

INDY-1846: update test_has_expected_fields

9f445bd

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

ashcherbakov reviewed Nov 21, 2018

View reviewed changes

ashcherbakov reviewed Nov 22, 2018

View reviewed changes

Toktar added 2 commits November 22, 2018 13:03

INDY-1846: add tests to test_api.py and fix to _validate_signature()

246dc55

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

INDY-1846: expansion tests for commit and pre-prepare validation

6dc0dfa

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

ashcherbakov reviewed Nov 23, 2018

View reviewed changes

INDY-1846: refactoring tests

bc8296c

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

ashcherbakov approved these changes Nov 23, 2018

View reviewed changes

ashcherbakov merged commit 2e4fb6c into hyperledger:master Nov 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INDY-1846: Change pool state root hash for BLS-signature in Commit messages #980

INDY-1846: Change pool state root hash for BLS-signature in Commit messages #980

Toktar commented Nov 16, 2018 •

edited

ashcherbakov Nov 21, 2018

Toktar Nov 21, 2018

ashcherbakov Nov 21, 2018

ashcherbakov Nov 21, 2018

Toktar Nov 21, 2018

Toktar Nov 22, 2018

ashcherbakov Nov 21, 2018

Toktar Nov 21, 2018 •

edited

ashcherbakov Nov 21, 2018

Toktar Nov 21, 2018

ashcherbakov Nov 21, 2018

Toktar Nov 21, 2018

ashcherbakov Nov 21, 2018

Toktar Nov 21, 2018

ashcherbakov Nov 21, 2018

Toktar Nov 21, 2018

ashcherbakov Nov 22, 2018

Toktar Nov 22, 2018

ashcherbakov Nov 23, 2018

Toktar Nov 23, 2018

ashcherbakov Nov 23, 2018

Toktar Nov 23, 2018

ashcherbakov Nov 23, 2018

Toktar Nov 23, 2018

ashcherbakov Nov 23, 2018

Toktar Nov 23, 2018

		@@ -295,6 +315,19 @@ def test_validate_commit_incorrect_sig(bls_bft_replicas, pre_prepare_with_bls):
		assert status == BlsBftReplica.CM_BLS_SIG_WRONG


		def test_validate_commit_incorrect_signed_without_pool_state_root(bls_bft_replicas, multi_signature):

INDY-1846: Change pool state root hash for BLS-signature in Commit messages #980

INDY-1846: Change pool state root hash for BLS-signature in Commit messages #980

Conversation

Toktar commented Nov 16, 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

Choose a reason for hiding this comment

Toktar Nov 21, 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

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

Toktar commented Nov 16, 2018 •

edited

Toktar Nov 21, 2018 •

edited