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

fix(rln-relay): clear nullifier log only if length is over max epoch gap #2836

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Jun 21, 2024

Description

Fixes an edge case where the nullifier log was cleared when maxEpochGap == nullifierLog.len

Changes

  • Adds unit tests for varied epoch sizes
  • fixes edge case

@rymnc rymnc marked this pull request as draft June 21, 2024 12:30
@rymnc rymnc changed the base branch from master to release/v0.30 June 21, 2024 12:30
@rymnc rymnc changed the base branch from release/v0.30 to master June 21, 2024 12:31
Copy link

github-actions bot commented Jun 21, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2836

Built from 6cc5fce

@rymnc rymnc changed the base branch from master to release/v0.30 June 21, 2024 12:36
@alrevuelta
Copy link
Contributor

Thanks for this. Would it be possible to add a unit test to catch this edge case?

@rymnc
Copy link
Contributor Author

rymnc commented Jun 24, 2024

Thanks for this. Would it be possible to add a unit test to catch this edge case?

added in ef242e7, previously we did not test for multiple epoch sizes, this test ensures it for 1,5,10,30, and 60.


check: wakuRlnRelay.nullifierLog.len().uint == rlnMaxEpochGap

# append one more now
Copy link
Contributor

Choose a reason for hiding this comment

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

two things:

wondering if all this repeated code can be avoided by reusing the for i in 0..<rlnMaxEpochGap:. unsure if removing the < may help? afaik the intention here is to add rlnMaxEpochGap + 1 nullifier and see that clearNullifierLog doesn't clean them.

and for completeness, perhaps we should check that clearNullifierLog does clean the log when expected? meaning i keep adding and len()keeps beingrlnMaxEpochGap`?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 7a9ee74

@rymnc rymnc marked this pull request as ready for review June 24, 2024 10:48
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Thank you!

@Ivansete-status Ivansete-status merged commit c567853 into release/v0.30 Jun 24, 2024
11 of 12 checks passed
@Ivansete-status Ivansete-status deleted the fix-epoch-log-clearing branch June 24, 2024 11:29
Ivansete-status added a commit that referenced this pull request Jul 2, 2024
* CHANGELOG.md add info for v0.30.0
* fix(rln-relay): clear nullifier log only if length is over max epoch gap (#2836)
* chore: add TWN parameters for RLNv2 (#2843)
* fix(rln): nullifierlog vulnerability (#2855)
* chore(rln-relay): add chain-id flag to wakunode and restrict usage if mismatches rpc provider (#2858)

---------

Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>
Co-authored-by: Alvaro Revuelta <alvrevuelta@gmail.com>
Ivansete-status added a commit that referenced this pull request Jul 2, 2024
chore: Update master from release v0.30 (#2866)

* CHANGELOG.md add info for v0.30.0
* fix(rln-relay): clear nullifier log only if length is over max epoch gap (#2836)
* chore: add TWN parameters for RLNv2 (#2843)
* fix(rln): nullifierlog vulnerability (#2855)
* chore(rln-relay): add chain-id flag to wakunode and restrict usage if mismatches rpc provider (#2858)
    
---------

Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>
Co-authored-by: Alvaro Revuelta <alvrevuelta@gmail.com>
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.

4 participants