Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

Fix proof direction and key bit order #22

Merged
merged 9 commits into from
Mar 10, 2021

Conversation

adlerjohn
Copy link
Member

Fixes #20 and conforms to celestiaorg/celestia-specs#129.

@adlerjohn
Copy link
Member Author

adlerjohn commented Mar 2, 2021

Looks like everything works as of c5a394b except for deletions. Looking into it now.

Edit: deleteWithSideNodes is actually incorrect, filed #25.

@adlerjohn adlerjohn marked this pull request as ready for review March 7, 2021 02:00
@adlerjohn adlerjohn requested review from musalbas and tzdybal March 9, 2021 16:26
@adlerjohn adlerjohn self-assigned this Mar 9, 2021
@adlerjohn adlerjohn added the bug Something isn't working label Mar 9, 2021
utils.go Show resolved Hide resolved
utils.go Show resolved Hide resolved
smt.go Outdated Show resolved Hide resolved
smt.go Outdated Show resolved Hide resolved
smt.go Outdated Show resolved Hide resolved
@adlerjohn adlerjohn requested a review from tzdybal March 10, 2021 13:53
@musalbas
Copy link
Member

Can we perhaps add tests to ensure that the keys are added with the correct key bit order?

@adlerjohn
Copy link
Member Author

Can we perhaps add tests to ensure that the keys are added with the correct key bit order?

Sure, but how specifically would we test for that? I guess we could add two leaves that are one bit apart and the proof needs to be 256 side nodes?

@musalbas
Copy link
Member

musalbas commented Mar 10, 2021

You could generate some trees, and compare it to the expected trees according to the spec.

@adlerjohn
Copy link
Member Author

@musalbas Updated the max depth test to manipulate the LSB directly instead of going through a function in 4c9f680.

@musalbas
Copy link
Member

How does that ensure that the keys are added with the correct key bit order?

@adlerjohn
Copy link
Member Author

adlerjohn commented Mar 10, 2021

How does that ensure that the keys are added with the correct key bit order?

It adds a check at the end that the proof is 256 nodes. If traversal isn't done in the correct bit order, the proof would be 256-8 = 248 nodes (maybe off-by-one, but you get the idea).

Copy link
Member

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

LGTM

smt.go Outdated Show resolved Hide resolved
Idiomatic

Co-authored-by: Tomasz Zdybał <[email protected]>
@adlerjohn adlerjohn merged commit 42bb10c into celestiaorg:master Mar 10, 2021
@adlerjohn adlerjohn deleted the adlerjohn-proof_direction branch March 10, 2021 21:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider changing key bit traversal order to be prefix-based
3 participants