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.

Sign up for GitHub

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

Jump to bottom

feat(rln-relay-v2): rln-keystore-generator updates #2392

Merged
merged 7 commits into from
Feb 9, 2024
Merged

feat(rln-relay-v2): rln-keystore-generator updates #2392

merged 7 commits into from
Feb 9, 2024

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Feb 2, 2024 •
edited

Description

Updates the contract listening and registration process, and inserts rateCommitments into the tree instead of idCommitments

A follow up PR will be created to:

  • accept the arguments into WakuRlnConfig, and allowing wakunode2 to be setup with rln-v2 credentials.
  • incrementing nonce per epoch

Changes

  • adds additional config option --rln-relay-user-message-limit to be set while running the keystore generator, which is included in the key creation in the keyfile
  • inserts rateCommitments into the tree instead of idCommitments

How to test

  1. Build wakunode2 RLN_V2=true make -j16 wakunode2
  2. Register to the membership set
./build/wakunode2 generateRlnKeystore \
--rln-relay-cred-path:./rlnKeystore.json \
--rln-relay-eth-client-address:https://<secret> \
--rln-relay-cred-password:sup3rsecure \
--rln-relay-eth-contract-address:0xABd93f37833107eDbAb58633ad5463aDEa1F95F4 \
--rln-relay-eth-private-key:<secret> \
--rln-relay-user-message-limit:5 \
--execute

which yields the below output:

libbacktrace error: no debugging symbols available. Compile with '--debugger:native'.
execution error: metadata does not exist
WRN 2024-02-07 16:23:50.728+05:30 could not initialize with persisted rln metadata topics="waku rln_relay onchain_group_manager" tid=704058 file=group_manager.nim:660
INF 2024-02-07 16:24:01.276+05:30 Your membership has been registered on-chain. topics="rln_keystore_generator" tid=704058 file=rln_keystore_generator.nim:76 chainId=0xAA36A7 contractAddress=0xABd93f37833107eDbAb58633ad5463aDEa1F95F4 membershipIndex=254
INF 2024-02-07 16:24:01.276+05:30 Your user message limit is                 topics="rln_keystore_generator" tid=704058 file=rln_keystore_generator.nim:80 userMessageLimit=5
INF 2024-02-07 16:24:03.245+05:30 credentials persisted                      topics="rln_keystore_generator" tid=704058 file=rln_keystore_generator.nim:111 path=./rlnKeystore.json

Issue

addresses a part of #2345

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 2, 2024

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.

@rymnc rymnc changed the title wip: rln-v2 in rln-keystore-generator feat(rln-relay-v2): rln-keystore-generator updates Feb 7, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 7, 2024 •
edited

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2392

Built from ec10d52

@rymnc rymnc marked this pull request as ready for review February 7, 2024 11:03
@rymnc rymnc requested review from Ivansete-status and alrevuelta and removed request for Ivansete-status February 7, 2024 11:04
Ivansete-status
Ivansete-status approved these changes Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM even though I don't have a profound knowledge on the topic!
Thanks! 🥳

apps/wakunode2/external_config.nim Outdated
@@ -79,6 +79,11 @@ type
desc: "Private key for broadcasting transactions",
defaultValue: "",
name: "rln-relay-eth-private-key" }: string

rlnRelayUserMessageLimit* {.
desc: "Set a user message limit for the rln membership registration.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick comment: shall we add the units to make it clearer?

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 3be31f0

waku/waku_rln_relay/group_manager/group_manager_base.nim Outdated
Comment on lines 211 to 217
let proofGenRes = proofGen(rlnInstance = g.rlnInstance,
data = data,
memKeys = g.idCredentials.get(),
memIndex = g.membershipIndex.get(),
epoch = epoch)
if proofGenRes.isErr():
return err("proof generation failed: " & $proofGenRes.error())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated to this PR but the valueOr and isOkOr result statements are very interesting

Suggested change
let proofGenRes = proofGen(rlnInstance = g.rlnInstance,
data = data,
memKeys = g.idCredentials.get(),
memIndex = g.membershipIndex.get(),
epoch = epoch)
if proofGenRes.isErr():
return err("proof generation failed: " & $proofGenRes.error())
let proofGen = proofGen(rlnInstance = g.rlnInstance,
data = data,
memKeys = g.idCredentials.get(),
memIndex = g.membershipIndex.get(),
epoch = epoch).valueOr:
return err("proof generation failed: " & $error)

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 ea8dd11

waku/waku_rln_relay/group_manager/static/group_manager.nim Outdated

let groupKeys = g.groupKeys

for i in 0..<groupKeys.len():
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick again:)

Suggested change
for i in 0..<groupKeys.len():
for i in 0..<groupKeys.len:

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 69bd411

waku/waku_rln_relay/group_manager/static/group_manager.nim
memberSeq.add(Membership(rateCommitment: rateCommitments[i], index: g.latestIndex + MembershipIndex(i) + 1))
await g.registerCb.get()(memberSeq)

discard g.slideRootQueue()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for further PRs we'd need to tackle all the discard statements that we have in rln code base.

waku/waku_rln_relay/group_manager/static/group_manager.nim Outdated

if g.registerCb.isSome():
var memberSeq = newSeq[Membership]()
for i in 0..<rateCommitments.len():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nitpick comment

Suggested change
for i in 0..<rateCommitments.len():
for i in 0..<rateCommitments.len:

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 69bd411

waku/waku_rln_relay/group_manager/static/group_manager.nim
let leavesRes = rateCommitments.toLeaves()
if not leavesRes.isOk():
raise newException(ValueError, "Failed to convert rate commitments to leaves")
let leaves = cast[seq[seq[byte]]](leavesRes.get())
Copy link
Collaborator

Choose a reason for hiding this comment

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

May it be more explicit?

Suggested change
let leaves = cast[seq[seq[byte]]](leavesRes.get())
let leaves = cast[seq[IDCommitment]](leavesRes.get())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this may cause confusion about converting the rateCommitment into an IDCommitment, when that is not the case. here we have used seq[byte] to discern it from an IDCommitment. I don't see the value in creating a type for it i.e SerializedRateCommitment yet.

waku/waku_rln_relay/group_manager/static/group_manager.nim Outdated

if g.registerCb.isSome():
var memberSeq = newSeq[Membership]()
for i in 0..<idCommitments.len():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for i in 0..<idCommitments.len():
for i in 0..<idCommitments.len:

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 69bd411

waku/waku_rln_relay/group_manager/static/group_manager.nim Outdated

discard g.slideRootQueue()

g.latestIndex += MembershipIndex(idCommitments.len())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
g.latestIndex += MembershipIndex(idCommitments.len())
g.latestIndex += MembershipIndex(idCommitments.len)

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 69bd411

alrevuelta
alrevuelta approved these changes Feb 9, 2024
View reviewed changes
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.

Out of curiosity, rlnRelayUserMessageLimit over which epoch size is applied? The default one of 1 sec?

@rymnc rymnc merged commit 2d46c35 into master Feb 9, 2024
9 of 10 checks passed
@rymnc rymnc deleted the update-rln-keystore-generator-v2 branch February 9, 2024 11:01
@rymnc rymnc mentioned this pull request Feb 9, 2024
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alrevuelta alrevuelta alrevuelta approved these changes

@Ivansete-status Ivansete-status Ivansete-status approved these changes

Assignees

@rymnc rymnc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rymnc @alrevuelta @Ivansete-status