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

Generate inclusion proof with blob index instead of hash #649

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

ian-shim
Copy link
Contributor

Why are these changes needed?

Previously, it was using the blob header hash to identify the index of the leaf, which is problematic when there are duplicate leaves.
Generating proofs with the leaf index solves this issue.

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@ian-shim ian-shim force-pushed the fix-inclusion-proof branch from 7b9375d to 10b5851 Compare July 19, 2024 18:55
@ian-shim ian-shim marked this pull request as ready for review July 19, 2024 19:08
continue
}
merkleProof, err := batchData.merkleTree.GenerateProof(blobHeaderHash[:], 0)
merkleProof, err := batchData.merkleTree.GenerateProofWithIndex(uint64(blobIndex), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any compatibility issue with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't be. It won't affect blobs that had been dispersed already. It will only affect blobs to be dispersed after the deploy by populating correct inclusion proofs in the confirmation info

Copy link
Contributor

Choose a reason for hiding this comment

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

What about the consumer of this inclusion proof? Will this break anything on that side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the consumer of the inclusion proof reads it from DB without re-generating it

@ian-shim ian-shim merged commit 3285f41 into Layr-Labs:master Jul 22, 2024
6 checks passed
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.

3 participants