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

feat(rln-relay): The window of acceptable roots should change per block, not per event #1266

Closed
1 task
Tracked by #1268
rymnc opened this issue Oct 14, 2022 · 3 comments · Fixed by #1349
Closed
1 task
Tracked by #1268
Assignees
Labels
track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications

Comments

@rymnc
Copy link
Contributor

rymnc commented Oct 14, 2022

Problem

Currently, the window of acceptable roots changes per event that is processed. With the current acceptable root window size (5), this would mean that if a block has more than 5 registrations, the previous acceptable window of roots would be invalidated, and a poorly connected node would not be able to send messages till it catches up.

Suggested solution

The acceptable window of roots must be updated only after all the events in the block are processed. This would also ensure that there would not be any inconsistency while processing multiple events at the same time (MemberRegistrations, MemberDeletions). This would then allow poorly connected nodes to still participate in messaging, since they still hold a valid root.

Alternatives considered

We could expand the size of the acceptable window of roots dynamically depending on the count of tree changing events emitted per block.

Additional context

None

Acceptance criteria

  • After all events are processed in a block, update the window of roots
@rymnc rymnc added the track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications label Oct 14, 2022
@s1fr0
Copy link
Contributor

s1fr0 commented Oct 14, 2022

I agree, root should change and update the window not per public key added but on multiple (burst) registrations. In principle should happen at every epoch, since it's the time frame where spamming could happen, but it is a too short window and probably better to process registrations present in a block (and allow spam for the time we need to process new roots).

On top of this, I would also probably wait transactions to be confirmed, i.e. wait ~6 blocks confirmations. If a block containing registrations is not confirmed, we have no way to revert except restarting nodes. Waiting would be beneficial for confirming the registrations and to allow a bit more time to poorly connected nodes to recover the block.

So if we are at block X, we process registrations at block X-n with n between 5 and 10. this cannot work otherwise new members won't be able to chat. However 5 blocks confirmation should happen within a minute, a time which might be reasonable considering that the user -in order to generate valid proofs- needs also to fetch all commitments from the contract and update its local tree (that is, on average, 2^19 commitments).

@rymnc
Copy link
Contributor Author

rymnc commented Oct 14, 2022

On top of this, I would also probably wait transactions to be confirmed, i.e. wait ~6 blocks confirmations. If a block containing registrations is not confirmed, we have no way to revert except restarting nodes. Waiting would be beneficial for confirming the registrations and to allow a bit more time to poorly connected nodes to recover the block.

I don't think we should wait for 6 confirmations, we should have additional logic to handle forks.

@s1fr0
Copy link
Contributor

s1fr0 commented Oct 14, 2022

I don't think we should wait for 6 confirmations, we should have additional logic to handle forks.

If the window consists of 5 elements, and e.g. a non-confirmed block drops the last 3-4 blocks, most of the window is gone and you'll have the original problem of this issue.

Note that changing the order of registrations will affect the root too, so even if we can speculate that the newly registered commitments will be the same, we cannot know the definitive order in case of forks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants