Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

INDY-1583: Fix bls signature checking #890

Merged
merged 6 commits into from
Aug 24, 2018

Conversation

ArtObr
Copy link
Contributor

@ArtObr ArtObr commented Aug 23, 2018

Signed-off-by: ArtObr artemobruchnikov@gmail.com

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>
@@ -76,6 +76,11 @@ def get_for_root_hash(self, root_hash, key: bytes) -> Optional[bytes]:
if val:
return self.get_decoded(val)

def get_all_leaves_for_root_hash(self, root_hash):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

del node.nodeReg[node_name]

# Still we can validate Preprepare
assert node.master_replica._bls_bft_replica._bls_bft.bls_key_register.get_key_by_name(node_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure that _load_keys_for_root is called.
Can we write a test for BlsKeyRegisterPoolManager._load_keys_for_root where we take some old pool_state_root_hash?
Actually we need to check the initial case: demote the node, and call _load_keys_for_root for a pool state before demotion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>
@ArtObr ArtObr changed the title INDY-1583: Fix bls signature checking INDY-1583: Fix bls signature checking [skip ci] [ci skip] Aug 23, 2018
Signed-off-by: ArtObr <artemobruchnikov@gmail.com>
@@ -261,6 +261,10 @@ def test_get_for_old_root(state):
assert state.get_for_root_hash(head_hash2, b'k2') == b'v2'
assert state.get_for_root_hash(head_hash2, b'k3') == b'v3'

assert len(state.get_all_leaves_for_root_hash(head_hash1)) == 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a specific test for get_all_leaves_for_root_hash and check the content as well, not only length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

def get_current_bls_keys(node):
return node.master_replica._bls_bft_replica._bls_bft.bls_key_register._current_bls_keys

restarted_node.master_replica._bls_bft_replica._bls_bft.bls_key_register._load_keys_for_root(state_root_hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to call it? It's assumed already be called when processing last requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

node_to_demote = txnPoolNodeSet[4]
txnPoolNodeSet.remove(node_to_demote)

last_pre_prepare = \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines are not used and can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

assert node.master_replica._bls_bft_replica._bls_bft.bls_key_register.get_key_by_name(node_name)


def test_bls_not_depend_on_node_reg_demotion(looper, txnPoolNodeSet,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better name for the test: test_order_after_demote_and_restart

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

from plenum.common.config_helper import PNodeConfigHelper
from plenum.common.types import f

nodeCount = 7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make nodeCount = 4 to make sue that we can order after demotion and restart

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

# We've removed one of the nodes from another node's log
del node.nodeReg[node_name]

# Still we can validate Preprepare
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please call _load_keys_for_root here

Copy link
Contributor Author

@ArtObr ArtObr Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>
@ArtObr ArtObr changed the title INDY-1583: Fix bls signature checking [skip ci] [ci skip] INDY-1583: Fix bls signature checking Aug 24, 2018
@ArtObr
Copy link
Contributor Author

ArtObr commented Aug 24, 2018

test this please

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>
@ArtObr
Copy link
Contributor Author

ArtObr commented Aug 24, 2018

test this please

Signed-off-by: ArtObr <artemobruchnikov@gmail.com>
@ashcherbakov ashcherbakov merged commit 03b2f54 into hyperledger:master Aug 24, 2018
@ArtObr ArtObr deleted the indy_1583_1 branch August 27, 2018 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants