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

remove redundant nibble traversal for extensions when verifying a proof #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thecodingshrimp
Copy link

Problem

The verify function of the MerklePatriciaProof.sol smart contract does not process extensions correctly.

Problem Specifics

In lines 62-65 it checks if there is an extension at pathPtr. But, since pathPtr was already incremented in line 53, it can never detect an extension.

Solution

Remove lines 62-65 since we just traverse an extension and want to check for the next nibble.

@thecodingshrimp
Copy link
Author

@sammayo @zmitton let me know what you think!

@zmitton
Copy link
Contributor

zmitton commented Feb 21, 2023

Cool. It looks like we did write tests. Were you able to get them to run? If so, could you create a test case that currently fails on an extension and succeeds with the changes?

@thecodingshrimp
Copy link
Author

thecodingshrimp commented Feb 23, 2023

Cool. It looks like we did write tests. Were you able to get them to run? If so, could you create a test case that currently fails on an extension and succeeds with the changes?

I only used this one contract from your repo, so will try to get them to run. An initial try failed since a lot of packages are outdated now and don't run with a recent node version.

Will update the packages and get it to run and then write a test case. The ladder isn't too complicated as I have already written one for my project.

I am in a region with bad internet right now. Will work on it in the middle of march!

@thecodingshrimp
Copy link
Author

Build a small test suite and found a bug in the RLP smart contract which needs to be fixed as well. Some packages also need to be updated.

Should I create a separate PR for the RLP bug and the updating of the packages or can I do everything in one?

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.

2 participants