feat(rln-relay-v2): update C FFI api's and serde #2385

rymnc · 2024-01-31T05:42:51Z

feat(rln-relay-v2): integrate new ffi bindings, serde
chore: use ExtendedRateLimitProof to describe epoch + rlnIdentifier

Description

This PR introduces the changes required for serde, and creates a new type ExtendedRateLimitProof which contains all metadata required to verify a proof.

Changes

updates the c ffi api
updates serde to include userMessageLimit and messageId
new type ExtendedRateLimitProof which includes epoch and rlnIdentifier in calculation of the externalNullifier

Issue

closes #2377 and #2378

github-actions · 2024-01-31T06:28:26Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2385

Built from 4d72fa9

rymnc · 2024-01-31T07:33:38Z

waku/waku_rln_relay/constants.nim

+when defined(rln_v2):
+  const
+    # pre-processed "rln/waku-rln-relay/v2.0.0" to array[32, byte]
+    DefaultRlnIdentifier*: RlnIdentifier = [114, 108, 110, 47, 119, 97, 107, 117, 


imo this is the best way to have the DefaultRlnIdentifier stored - if we do decide to have it as a command line argument for rln-relay we can process it into an array of 32 bytes at that point.

rymnc · 2024-01-31T07:33:55Z

waku/waku_rln_relay/conversion_utils.nim

@@ -53,33 +53,60 @@ proc encodeLengthPrefix*(input: openArray[byte]): seq[byte] =
    output = concat(@len, @input)
  return output

-proc serialize*(idSecretHash: IdentitySecretHash, 
+when defined(rln_v2):


had to dupe the function because args are different

have the hunch that having rlnv2 behind a flag will end up being more problematic than using a branch (where we completely replace v1 with v2)

IMO, having a flag is much clearer and less error-prone than a separate branch.

rymnc · 2024-01-31T07:35:57Z

waku/waku_rln_relay/protocol_types.nim

+  when defined(rln_v2):
+    ## the external nullifier used for the generation of the `proof` (derived from poseidon([epoch, rln_identifier]))
+    externalNullifier*: ExternalNullifier
+  else:
+    ## the epoch used for the generation of the `proof`
+    epoch*: Epoch
+    ## Application specific RLN Identifier
+    rlnIdentifier*: RlnIdentifier
+
+when defined(rln_v2):
+  type ExtendedRateLimitProof* = ref object of RateLimitProof
+    ## epoch is the epoch for which the proof is generated
+    epoch*: Epoch
+    ## rlnIdentifier is the identifier of the RLN application
+    rlnIdentifier*: RlnIdentifier


The distinction between the two is made because now, RateLimitProof contains the proof with the public inputs/outputs. epoch and rlnIdentifier are no longer public inputs, and must not be confused this way. ExtendedRateLimitProof is what should be sent on the wire.

alrevuelta

lgtm

alrevuelta · 2024-01-31T09:34:16Z

waku/waku_rln_relay/protocol_types.nim

+when defined(rln_v2):
+  type ExtendedRateLimitProof* = ref object of RateLimitProof
+    ## epoch is the epoch for which the proof is generated
+    epoch*: Epoch


why is the epoch inside ExtendedRateLimitProof instead of RateLimitProof. I mean, both rln v1 and v2 should have this field, so? same for rlnIdentifier?

the difference between rln-v1 and v2 also includes that the epoch and rlnIdentifier is not passed into the circuit, instead the externalNullifier is directly passed in after computing it from the epoch and rlnIdentifier. the RateLimitProof struct properly defines this behaviour, and ExtendedRateLimitProof includes them so that a receiving node can ensure the epoch is not too old, and that the rlnIdentifier is the same as they one they're validating for.

I have the impression that the ExtendedRateLimitProof type is not needed. Could we only keep the RateLimitProof with the following?

## the epoch used for the generation of the `proof` epoch*: Epoch ## Application specific RLN Identifier rlnIdentifier*: RlnIdentifier when defined(rln_v2): ## the external nullifier used for the generation of the `proof` (derived from poseidon([epoch, rln_identifier])) externalNullifier*: ExternalNullifier

I think a good strategy to ease the future migration to rln_v2 is to keep a clear separation. Having such inheritance is some kind of messy IMO. Let me know if this is not feasible by another constraint that I'm missing.

I understand the concern, and I thought of this approach initially as well. however, RateLimitProof and ExtendedRateLimitProof serve 2 entirely different functions, and it's better to separate their concerns.

addressed in 561a967

alrevuelta · 2024-01-31T09:34:26Z

waku/waku_rln_relay/conversion_utils.nim

@@ -53,33 +53,60 @@ proc encodeLengthPrefix*(input: openArray[byte]): seq[byte] =
    output = concat(@len, @input)
  return output

-proc serialize*(idSecretHash: IdentitySecretHash, 
+when defined(rln_v2):


have the hunch that having rlnv2 behind a flag will end up being more problematic than using a branch (where we completely replace v1 with v2)

Ivansete-status

Thanks for this! I've added a few comments/questions that I hope you find useful :)

Ivansete-status · 2024-02-01T11:18:53Z

waku/waku_rln_relay/constants.nim

@@ -15,7 +18,7 @@ const
 # inputs of the membership contract constructor
 # TODO may be able to make these constants private and put them inside the waku_rln_relay_utils
 const
-  MembershipFee* = 1000000000000000.u256
+  MembershipFee* = 0.u256


Shall we indicate which units are being considered either in the const name or in a comment? Maybe wei?

addressed in 561a967

Ivansete-status · 2024-02-01T11:24:38Z

waku/waku_rln_relay/protocol_types.nim

 # Custom data types defined for waku rln relay -------------------------
-type RateLimitProof* = object
+type RateLimitProof* = object of RootObj


I think we don't need inheritance as per my comment below.

addressed in 561a967

Ivansete-status · 2024-02-01T11:31:21Z

waku/waku_rln_relay/conversion_utils.nim

@@ -53,33 +53,60 @@ proc encodeLengthPrefix*(input: openArray[byte]): seq[byte] =
    output = concat(@len, @input)
  return output

-proc serialize*(idSecretHash: IdentitySecretHash, 
+when defined(rln_v2):


IMO, having a flag is much clearer and less error-prone than a separate branch.

Ivansete-status · 2024-02-01T11:53:15Z

waku/waku_rln_relay/protocol_types.nim

+when defined(rln_v2):
+  type ExtendedRateLimitProof* = ref object of RateLimitProof
+    ## epoch is the epoch for which the proof is generated
+    epoch*: Epoch


I have the impression that the ExtendedRateLimitProof type is not needed. Could we only keep the RateLimitProof with the following?

## the epoch used for the generation of the `proof` epoch*: Epoch ## Application specific RLN Identifier rlnIdentifier*: RlnIdentifier when defined(rln_v2): ## the external nullifier used for the generation of the `proof` (derived from poseidon([epoch, rln_identifier])) externalNullifier*: ExternalNullifier

I think a good strategy to ease the future migration to rln_v2 is to keep a clear separation. Having such inheritance is some kind of messy IMO. Let me know if this is not feasible by another constraint that I'm missing.

Ivansete-status · 2024-02-01T12:06:01Z

waku/waku_rln_relay/protocol_types.nim


-  return output
+when defined(rln_v2):


Would it be possible to only have one encode implementation if we only had RateLimitProof?

addressed in 561a967

Ivansete-status · 2024-02-01T12:14:53Z

waku/waku_rln_relay/protocol_types.nim

-  rlnIdentifier*: RlnIdentifier
+  when defined(rln_v2):
+    ## the external nullifier used for the generation of the `proof` (derived from poseidon([epoch, rln_identifier]))
+    externalNullifier*: ExternalNullifier


Is is possible to use a different attribute name? externalNullifier is also part of the ProofMetadata and it is more difficult to follow its references.

it's the same field/value though

Ivansete-status · 2024-02-01T12:19:11Z

waku/waku_rln_relay/rln/rln_interface.nim

-## input_buffer is serialized as input_data as [ id_key<32> | path_elements<Vec<32>> | identity_path_index<Vec<1>> | x<32> | epoch<32> | rln_identifier<32> ]
+## input_buffer is serialized as input_data as [ identity_secret<32> | user_message_limit<32> | message_id<32> | path_elements<Vec<32>> | identity_path_index<Vec<1>> | x<32> | external_nullifier<32> ]


We are changing the param definition but this actually depends on the zerokit/rln implementation being used, right?

yes, I can add a note which mentions what is the serde pattern for v1 and v2 respectively if you'd like.

addressed in 561a967

Ivansete-status

LGTM! Thanks for it 🥳

rymnc force-pushed the update-rln-ffi-bindings branch 2 times, most recently from 4b327c1 to e0fd50d Compare January 31, 2024 06:23

rymnc linked an issue Jan 31, 2024 that may be closed by this pull request

rln-v2: Update serde for proof generation and commitment insertion #2378

Closed

feat(rln-relay-v2): integrate new ffi bindings, serde

7bfd461

rymnc force-pushed the update-rln-ffi-bindings branch from e0fd50d to 7bfd461 Compare January 31, 2024 07:00

rymnc commented Jan 31, 2024

View reviewed changes

rymnc marked this pull request as ready for review January 31, 2024 07:36

rymnc requested review from alrevuelta and Ivansete-status January 31, 2024 07:36

rymnc mentioned this pull request Jan 31, 2024

PoC: rln-v2 in waku #2345

Closed

17 tasks

alrevuelta approved these changes Jan 31, 2024

View reviewed changes

Ivansete-status assigned rymnc Feb 1, 2024

Ivansete-status reviewed Feb 1, 2024

View reviewed changes

chore: remove ExtendedRateLimitProof, add comments

561a967

rymnc force-pushed the update-rln-ffi-bindings branch from 15bd0b2 to 561a967 Compare February 1, 2024 12:55

rymnc added 2 commits February 1, 2024 18:27

Merge branch 'master' into update-rln-ffi-bindings

89c2456

fix: typo

1aa1204

Ivansete-status approved these changes Feb 1, 2024

View reviewed changes

rymnc merged commit b88facd into master Feb 1, 2024
9 of 10 checks passed

rymnc deleted the update-rln-ffi-bindings branch February 1, 2024 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rln-relay-v2): update C FFI api's and serde #2385

feat(rln-relay-v2): update C FFI api's and serde #2385

rymnc commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

rymnc Jan 31, 2024

rymnc Jan 31, 2024

alrevuelta Jan 31, 2024

Ivansete-status Feb 1, 2024

rymnc Jan 31, 2024

alrevuelta left a comment

alrevuelta Jan 31, 2024

rymnc Jan 31, 2024

Ivansete-status Feb 1, 2024

rymnc Feb 1, 2024

rymnc Feb 1, 2024

alrevuelta Jan 31, 2024

Ivansete-status left a comment

Ivansete-status Feb 1, 2024

rymnc Feb 1, 2024

Ivansete-status Feb 1, 2024

rymnc Feb 1, 2024

Ivansete-status Feb 1, 2024

Ivansete-status Feb 1, 2024

Ivansete-status Feb 1, 2024

rymnc Feb 1, 2024

Ivansete-status Feb 1, 2024

rymnc Feb 1, 2024

Ivansete-status Feb 1, 2024

rymnc Feb 1, 2024

rymnc Feb 1, 2024

Ivansete-status left a comment

		## input_buffer is serialized as input_data as [ id_key<32> \| path_elements<Vec<32>> \| identity_path_index<Vec<1>> \| x<32> \| epoch<32> \| rln_identifier<32> ]
		## input_buffer is serialized as input_data as [ identity_secret<32> \| user_message_limit<32> \| message_id<32> \| path_elements<Vec<32>> \| identity_path_index<Vec<1>> \| x<32> \| external_nullifier<32> ]

feat(rln-relay-v2): update C FFI api's and serde #2385

feat(rln-relay-v2): update C FFI api's and serde #2385

Conversation

rymnc commented Jan 31, 2024 • edited Loading

Description

Changes

Issue

github-actions bot commented Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrevuelta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

rymnc commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading