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

Patricia trie prefix proof support #694

Merged
merged 18 commits into from
May 25, 2018

Conversation

lovesh
Copy link
Contributor

@lovesh lovesh commented May 22, 2018

No description provided.

lovesh and others added 16 commits March 26, 2018 12:07
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
part = key_prfx

if node_type == NODE_TYPE_LEAF:
if starts_with(full, part):
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a LEAF and len(key_prfx) > len(curr_key) and curr_key is a part of key_prfx, then we will return this node. But should we return BLANK_NODE in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@@ -389,6 +387,49 @@ def _get(self, node, key):
else:
return BLANK_NODE

def _get_last_node_for_prfx(self, node, key_prfx):
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 unit tests for this method

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

proof.pop()
return o

def produce_spv_proof_for_key_prfx(self, key_prfx, root=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

So, we are returning state proof for the common part (root till prefix) plus sub-proofs for all prefix's sub-nodes.
But the method name may be a bit confusing since it looks like we return just a common proof (from root till prefix).
Please add the comment for the method. Also maybe we can rename it to produce_spv_proof_for_key_prfx_subnodes?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

try:
new_trie.root_hash = root
except Exception as e:
print(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not good to print exception here. Maybe throw some exception that verification failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am following the same pattern as is present in similar methods

add_prefix_nodes_and_verify(state, prefix)


def test_state_proof_for_key_prefix_4(state):
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 negative test case where verification fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method calls an underlying method. The negative test cases are present for the underlying method in test_proof_multiple_prefix_nodes

Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
Signed-off-by: Lovesh Harchandani <lovesh.bond@gmail.com>
@ashcherbakov ashcherbakov merged commit c889d86 into hyperledger:master May 25, 2018
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