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

chore(rln-relay): remove websocket from OnchainGroupManager #2364

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Jan 22, 2024

Description

Removes dependence on eth_subscribe since websocket connections can be flaky.

Changes

  • Removes eth_subscribe call and replaces it with block polling every 1 second. default can be changed

How to test

  1. Pass in a http rpc provider instead of ws while running nwaku / make -j16 test
  2. Generate an rln keystore using
./build/wakunode2 generateRlnKeystore \
  --rln-relay-cred-path:./rlnKeystore.json \
  --rln-relay-eth-client-address:https:<> \
  --rln-relay-cred-password:sup3rsecure \
  --rln-relay-eth-contract-address:0xF471d71E9b1455bBF4b85d475afb9BB0954A29c4 \
  --rln-relay-eth-private-key:<> \
  --execute

insert your https rpc provider as well as private key in the required positions

Issue

addresses part of #2345

@rymnc rymnc self-assigned this Jan 22, 2024
@rymnc rymnc marked this pull request as draft January 22, 2024 10:57
Copy link

github-actions bot commented Jan 22, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2364

Built from d9e245b

@rymnc rymnc marked this pull request as ready for review January 22, 2024 13:51
@rymnc rymnc requested review from alrevuelta and Ivansete-status and removed request for alrevuelta January 22, 2024 13:52
@rymnc rymnc added the track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications label Jan 22, 2024
@rymnc rymnc mentioned this pull request Jan 22, 2024
17 tasks
@rymnc rymnc requested a review from gabrielmer January 23, 2024 09:10
waku/waku_rln_relay/constants.nim Show resolved Hide resolved
Comment on lines 321 to +323
proc getAndHandleEvents(g: OnchainGroupManager,
fromBlock: BlockNumber,
toBlock: BlockNumber): Future[void] {.async: (raises: [Exception]).} =
toBlock: BlockNumber): Future[bool] {.async: (raises: [Exception]).} =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this proc either returns true or raises an exception.

Am I missing something or is it supposed to be like that? If so, what's the point of returning bool if the return value is always true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's for compatibility with the retryWrapper template, it expects a return value to assign the result of an async operation, just set to true here since we don't use it (discarded)

Copy link

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

@gabrielmer gabrielmer added the release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions label Jan 23, 2024
@gabrielmer gabrielmer self-requested a review January 23, 2024 11:36
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.

LGTM! Thanks so much! 🤩

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

lgtm thanks! minor comment regarding DefaultBlockPollRate


const DefaultKeyStorePath* = "rlnKeystore.json"
const DefaultKeyStorePassword* = "password"

const DefaultBlockPollRate* = 1.seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this is too often? Would suggest 6s (half-block time). Thinking here of people that may be using freemium providers. Once per second is 86.400 requests a day (and that just for knowing the current block).

On the other hand, since we consider as valid multiple roots, we could relax this further (thinking of >30sec). But lets leave this by now, as it may open up some edge cases. 6s should be ok by now.

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 dbd6cc3

@rymnc rymnc merged commit efdc524 into master Jan 23, 2024
9 of 10 checks passed
@rymnc rymnc deleted the convert-rln-to-http branch January 23, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants