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

soundness bug: enabled selector returns a zero Expression #325

Closed
themighty1 opened this issue May 13, 2024 · 2 comments
Closed

soundness bug: enabled selector returns a zero Expression #325

themighty1 opened this issue May 13, 2024 · 2 comments

Comments

@themighty1
Copy link

themighty1 commented May 13, 2024

Update: confirming that this was fixed in main by 54a8fd8

I have a fixed lookup table with 4 items [0,1,2,3]. I'm trying to lookup the value 123, expecting the lookup to fail.
However, the lookup succeeds.

Apparently the lookup selector even though it is .enable()d, returns a zero Expression. Since my lookup table has a 0, this causes the lookup to always succeed.

Here is a link to a minimal repro: https://github.com/themighty1/authdecode_lookup/tree/bug_repro
You can reproduce by running:

git clone https://github.com/themighty1/authdecode_lookup -b bug_repro
cd authdecode_lookup/
cargo build --release
./target/release/authdecode_lookup

You will see "Proof verifier" when in fact it should fail.

@themighty1
Copy link
Author

Feel free to close this since it has been fixed.
Maybe if you had a test which asserts that the wrong item is not present in the lookup table, you could have caught the bug earlier?

@ed255
Copy link
Member

ed255 commented May 14, 2024

I'm sorry you encountered this bug! As you have already seen, it was fixed via #322

Maybe if you had a test which asserts that the wrong item is not present in the lookup table, you could have caught the bug earlier?

In the PR where we fixed the bug we also added a test that checks the behavior of compression of selectors as enabled/disabled and also adds some tests that should fail. But you're right that we need to improve our unit tests to avoid facing issues like this in the future.

I'll close this issue and create a new one to improve unit testing, mentioning your suggestion of the lookup table test.

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

No branches or pull requests

2 participants