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

Forced exit support #204

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from
Open

Forced exit support #204

wants to merge 3 commits into from

Conversation

lok52
Copy link
Collaborator

@lok52 lok52 commented Sep 22, 2023

Closes #201

Changes:

  1. Create EventWatcher abstraction and refactor direct-deposit watcher
  2. Use EventWatcher for forced exits:
    2.1. New events are submitted to the forced-exit queue with a delay specified in the event
    2.2. After the delay relayer confirms that user indeed finalised the exit. If not, then nullifier is removed from local storage

@lok52 lok52 changed the title Feature/forced exit Forced exit support Sep 22, 2023
Copy link

@r0wdy1 r0wdy1 left a comment

Choose a reason for hiding this comment

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

lgtm

jobLogger.info('User has not finalized forced exit, removing it', { nullifier })
await pool.state.nullifiers.remove([nullifier])
} else {
jobLogger.info('User successfully finalized forced exit', { nullifier })

Choose a reason for hiding this comment

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

Suggested change
jobLogger.info('User successfully finalized forced exit', { nullifier })
jobLogger.info('Forced exit was outdated', { nullifier })

It's not pretty correct message. User also may cancel FE during 10-min buffer time interval in forced exit queue

@EvgenKor
Copy link

I'm not sure but it's seems the relayer will not restore watcher workers for active forced exits in case of restarting or failure. In that case the associated nullifiers remain in confirmed state and user cannot execute transaction after FE time interval will become expired. Could you please confirm or deny it?

Copy link

@EvgenKor EvgenKor left a comment

Choose a reason for hiding this comment

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

It seems that your solution doesn't include complete forced exit logic.

  1. You doesn't make distinguish between used nullifiers and committed ones for forced exit. It would be better to keep such nullifiers separately to make possible returns relevant errors (currently doublespend error will occurred if user will try to transact during forced exit timeframe)
  2. You cannot block user transactions efficiently while you pull events with some time interval. Imagine I sent forced exit commit transaction. Relayer will know about it within 10 minutes. During that time I can send regular transaction to the relayer and it will not blocked. And my forced exit will become invalid (because the nullifier will used). Such behaviour seems not valid. I think you should check committedForcedExits[nullifier] during transaction validation. But to make it correctly you also should take in mind when associated forced exit time window will close (because committedForcedExits[nullifier] won't removed before forced exit will become canceled by user)
  3. It would be better if you can check if user send transaction from the dead account during the transaction validation. It's needed to produce relevant error (for now the error message is doublespend detected in that case). To do that you need to check nullifier value (poolContract.nullifiers[nullifier]). The account considered to be dead if the nullifier value match pattern 0xdeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddead0000000000000000

Copy link

@EvgenKor EvgenKor left a comment

Choose a reason for hiding this comment

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

We have decided after the discussion the relayer behaviour is acceptable for the edge cases described above

Copy link

@k1rill-fedoseev k1rill-fedoseev left a comment

Choose a reason for hiding this comment

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

lgtm

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.

Support forced exits in the relayer
4 participants