Chore(rln-relay): better documentation for rln nim bindings #1016

staheri14 · 2022-06-22T16:37:57Z

This is a minimal PR merely to add more detailed descriptions to the rln nim bindings to ease its replacement with the new zero-kit.

oskarth

LGTM

@s1fr0 should approve PR before merged

s1fr0 · 2022-06-23T22:28:03Z

waku/v2/protocol/waku_rln_relay/rln.nim

-
+## creates an instance of rln object as defined by the rln lib https://github.com/kilic/rln/blob/7ac74183f8b69b399e3bc96c1ae8ab61c026dc43/src/public.rs#L48
+## merkle_depth represent the depth of the Merkle tree
+## parameters_buffer holds prover and verifier keys


To be precise, parameters_buffer here corresponds to the user's proving key. The proving key, in turn, contains a copy of the verifying key.

Curious on how this distinction would matter? In my view, it generates CRS that embodies both the proving keys and the verification keys.

It doesn't matter indeed, it is just a clarification: the proving key just contains the verification key. IMO a RLN-relay node doesn't need to generate proofs but only verify them, so in some circumstance (optimization?) it might make sense to separate the two (proving keys are bigger than verification keys).

Proof generation is needed as part of message publishing, it contains both capabilities i.e., relaying and publishing.

s1fr0 · 2022-06-23T22:34:12Z

waku/v2/protocol/waku_rln_relay/rln.nim

 proc key_gen*(ctx: RLN[Bn256], keypair_buffer: ptr Buffer): bool {.importc: "key_gen".}
+## generates id key and id commitment key serialized inside keypair_buffer as | id_key <32 bytes>| id_commitment_key <32 bytes> |
+## the return bool value indicates the success or failure of the operation

 proc generate_proof*(ctx: RLN[Bn256],
                    input_buffer: ptr Buffer,
                    output_buffer: ptr Buffer): bool {.importc: "generate_proof".}
 ## input_buffer serialized as  [ id_key<32> | id_index<8> | epoch<32> | signal_len<8> | signal<var> ]


Why id_index is 8 bytes long? Aren't 4 enough? (32 bytes * 2^(8*4) = 128GiB of leaves).
I guess its based on the typical 8-bytes usize for most 64bits architecture, but over 32bits architectures such length won't make sense.

uint8 is the smallest among the integer types supported by Rust (and typically by other languages), which also appeared to be large enough for the leaf index. Of course, smaller than that also works but requires a custom type definition.

uint8 is 1 byte, while here id_index is 8 bytes (at least in comment). Did you mean something else, e.g. uint32/uint64?

Right, misinterpreted the sizes. Right, the 32 depth of the tree seems reasonable and large enough, and perhaps uint32 would also do the job. But considering that this data is not being transmitted as part of the proof on the wire, it does not cost bandwidth. Would it be of any help to change it to uint32? if yes, then we can do it.

The problem here is not about bandwidth but the architecture: on 32 bits machines usize is 4 bytes and you then need a safe conversion from u64 to usize in order to pass this to the RLN API. The other way around, i.e. when serializing the input, keeping index at 8 bytes, requires casting to a non-native type (u64) before serializing it. This extra overhead and extra checks don't make sense here if 4 bytes out of 8 will always be 0. We can address this in a later PR.

I see, if the concern is about interaction with the Rust lib API, then the rln lib and the nim code will run and be compiled on the same machine with the same architecture, right? then my understanding is that uint will be rendered with an identical size both by the nim code and by the underlying lib, so no conversion would be needed. Or are you referring to some other issue?

The issue will be then in the deserialization: right now you read a fixed amount of 8 bytes (the common usize byte-size over 64 bits architectures); to make doable what you say you need to first get the size of usize (4 or 8, but not necessarily) and then read that amount of bytes to deserialize the index. To me makes sense to just work with u32.

s1fr0 · 2022-06-23T22:35:08Z

waku/v2/protocol/waku_rln_relay/rln.nim


 proc delete_member*(ctx: RLN[Bn256], index: uint): bool {.importc: "delete_member".}
+## index is the position of the id commitment key to be deleted from the tree


I would clarify that delete overwrite the leaf with the default 0 leaf and that other leaves indexes are kept unchanged.

Done in efdfad7

s1fr0 · 2022-06-23T22:36:19Z

waku/v2/protocol/waku_rln_relay/rln.nim

 proc key_gen*(ctx: RLN[Bn256], keypair_buffer: ptr Buffer): bool {.importc: "key_gen".}
+## generates id key and id commitment key serialized inside keypair_buffer as | id_key <32 bytes>| id_commitment_key <32 bytes> |


You may add that id_commitment_key is the poseidon hash of id_key?

Sure, done in efdfad7

s1fr0 · 2022-06-23T22:39:10Z

waku/v2/protocol/waku_rln_relay/rln.nim

 proc key_gen*(ctx: RLN[Bn256], keypair_buffer: ptr Buffer): bool {.importc: "key_gen".}
+## generates id key and id commitment key serialized inside keypair_buffer as | id_key <32 bytes>| id_commitment_key <32 bytes> |
+## the return bool value indicates the success or failure of the operation

 proc generate_proof*(ctx: RLN[Bn256],
                    input_buffer: ptr Buffer,
                    output_buffer: ptr Buffer): bool {.importc: "generate_proof".}
 ## input_buffer serialized as  [ id_key<32> | id_index<8> | epoch<32> | signal_len<8> | signal<var> ]
 ## output_buffer holds the proof data and should be parsed as |proof<256>|root<32>|epoch<32>|share_x<32>|share_y<32>|nullifier<32>|


Unrelated to this PR, but the zk proof can be compressed to 128 bytes (only x coordinates are kept for G1,G2,G3 + 1 bit for the sign of y), so this will improve data communication.

Good point! but it comes with a trade-off where it would save us bandwidth and will add computation to the verifying peers.

The overhead is 3 square roots (i.e. 3 exponentiations) in Fr vs. 128 bytes space per proof per node. While the proof expansion can be done in parallel with other proofs, increased proof size will affect bandwidth and thus will slower propagation in the network of messages. It's very unclear how the two interplay though, so hard to give an opinion without proper benchmarks. We can just keep this in mind for the moment and do some tests later.

Sure, makes sense to consider it as a potential optimization. Though, I am not quite sure whether the extra bytes for the proof will cause any extra delay. But the extra computation will certainly add overhead to the verification time hence delaying message propagation.

The delay would be mainly in bandwidth/data transfer if you want it to scale: 1M messages exchanged x 128 extra bytes ~ 128MB ~ 6s at 20MB/s (optimistic or pessimistic?) per node. To be better, square roots if Fp have to take around 200ns each. Will see with the integration of zerokit if we can have some benchmark.

Thanks for the clarification, I understand your point, but that depends on the messaging rate of the application which in your example is 1 million per second which may or may not be the case for all the apps. Let's see what benchmarking tells us.

s1fr0 · 2022-06-23T22:42:10Z

waku/v2/protocol/waku_rln_relay/rln.nim

 proc verify*(ctx: RLN[Bn256],
            proof_buffer: ptr Buffer,
            result_ptr: ptr uint32): bool {.importc: "verify".}
 ## proof_buffer [ proof<256>| root<32>| epoch<32>| share_x<32>| share_y<32>| nullifier<32> | signal_len<8> | signal<var> ]
+## the return bool value indicates the success or failure of the verification


This sounds misleading: the return bool value indicates the success of the verify API call, not if the proof results successfully verified. To verify thatm you need to check result_ptr value (which seems that counterintuitively has to be set to 0 for the proof to be valid).

Thanks, clarified in efdfad7

s1fr0

Looks good! Left some comments: most relevant probably the one about proof verification.

staheri14 · 2022-06-24T00:42:49Z

Looks good! Left some comments: most relevant probably the one about proof verification.

Thanks @s1fr0 for the comments! they are addressed in https://github.com/status-im/nwaku/pull/1016/commits/efdfad78cf5d3001887ffd9fef145b610be753f5/. Please let me know if further edits are required.

s1fr0 · 2022-06-24T09:11:29Z

waku/v2/protocol/waku_rln_relay/rln.nim

 proc verify*(ctx: RLN[Bn256],
            proof_buffer: ptr Buffer,
            result_ptr: ptr uint32): bool {.importc: "verify".}
 ## proof_buffer [ proof<256>| root<32>| epoch<32>| share_x<32>| share_y<32>| nullifier<32> | signal_len<8> | signal<var> ]
+## the return bool value indicates the success or failure of the call to the verify function
+## the verification of the zk proof  is available in result_ptr, where 0 indicates success and 1 is failure


Suggested change

## the verification of the zk proof is available in result_ptr, where 0 indicates success and 1 is failure

## the result of the verification of the zk proof is stored in the value pointed by result_ptr: if 0, the proof is valid; if 1, it is not

s1fr0

Thanks @staheri14. LGTM except the 8 bytes assigned to the index_id.

Besides this, where do you use the Auth object? https://github.com/status-im/nwaku/blob/6f93b219283a489cfdac7219ae126e902549c04d/waku/v2/protocol/waku_rln_relay/rln.nim#L27
It's in kilic's code, but I don't think is used here.

status-im-auto · 2022-06-28T17:50:21Z

Jenkins Builds

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	`6f93b21`	#1	2022-06-28 17:50:20	~22 min	`macos`	📄`log`
❌	`6f93b21`	#1	2022-06-28 17:50:22	~22 min	`linux`	📄`log`

staheri14 · 2022-06-28T22:37:38Z

Thanks @staheri14. LGTM except the 8 bytes assigned to the index_id.

Besides this, where do you use the Auth object?

https://github.com/status-im/nwaku/blob/6f93b219283a489cfdac7219ae126e902549c04d/waku/v2/protocol/waku_rln_relay/rln.nim#L27

It's in kilic's code, but I don't think is used here.

In the early version of the lib, AuthObj was used to save the id key and the index of id commitment (and was part of the APIs), but it it was removed from the later versions.

staheri14 · 2022-06-28T22:45:07Z

@s1fr0 Please let me know if any further clarification is needed, otherwise, I'll merge the PR.

s1fr0

LGTM! Some details will be addressed in followup PRs when we will replace kilic's implementations with zerokit RLN.

annotates rln nim bindings for more clarity

d5509bc

staheri14 marked this pull request as ready for review June 22, 2022 16:38

staheri14 changed the title ~~Chore (rln-relay): better documentation for rln nim bindings~~ Chore(rln-relay): better documentation for rln nim bindings Jun 22, 2022

staheri14 added 2 commits June 22, 2022 09:39

adds pointer to the rln construct

75e88f3

revises further

a01dc0d

staheri14 requested review from jm-clius, kaiserd, oskarth and s1fr0 June 22, 2022 18:53

Merge branch 'master' into rln-relay/rln-nim-bindings-doc

037eac5

oskarth approved these changes Jun 23, 2022

View reviewed changes

Merge branch 'master' into rln-relay/rln-nim-bindings-doc

1946b35

jm-clius approved these changes Jun 23, 2022

View reviewed changes

staheri14 self-assigned this Jun 23, 2022

s1fr0 reviewed Jun 23, 2022

View reviewed changes

staheri14 added 2 commits June 23, 2022 17:39

addresses comments

efdfad7

Merge branch 'master' into rln-relay/rln-nim-bindings-doc

6f93b21

staheri14 requested a review from s1fr0 June 24, 2022 00:47

s1fr0 reviewed Jun 24, 2022

View reviewed changes

Merge branch 'master' into rln-relay/rln-nim-bindings-doc

14b777a

staheri14 added 2 commits June 28, 2022 15:41

revises proc description of verify

9161656

removes AuthObj

d549cb7

Merge branch 'master' into rln-relay/rln-nim-bindings-doc

a4d22a1

s1fr0 approved these changes Jun 29, 2022

View reviewed changes

staheri14 merged commit 5c7baf8 into master Jun 29, 2022

staheri14 deleted the rln-relay/rln-nim-bindings-doc branch June 29, 2022 20:45

s1fr0 mentioned this pull request Jul 6, 2022

Use Zerokit RLN module in nwaku #1030

Closed

s1fr0 mentioned this pull request Sep 2, 2022

serialization lengths should be u32 vacp2p/zerokit#39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore(rln-relay): better documentation for rln nim bindings #1016

Chore(rln-relay): better documentation for rln nim bindings #1016

staheri14 commented Jun 22, 2022

oskarth left a comment

s1fr0 Jun 23, 2022

staheri14 Jun 24, 2022

s1fr0 Jun 24, 2022

staheri14 Jun 29, 2022

s1fr0 Jun 23, 2022

staheri14 Jun 24, 2022

s1fr0 Jun 24, 2022

staheri14 Jun 29, 2022

s1fr0 Jun 29, 2022 •

edited

Loading

staheri14 Jun 29, 2022

s1fr0 Jun 29, 2022

s1fr0 Jun 23, 2022

staheri14 Jun 24, 2022

s1fr0 Jun 23, 2022

staheri14 Jun 24, 2022

s1fr0 Jun 23, 2022

staheri14 Jun 24, 2022 •

edited

Loading

s1fr0 Jun 24, 2022

staheri14 Jun 29, 2022

s1fr0 Jun 29, 2022

staheri14 Jun 29, 2022

s1fr0 Jun 23, 2022

staheri14 Jun 24, 2022

s1fr0 left a comment

staheri14 commented Jun 24, 2022

s1fr0 Jun 24, 2022 •

edited

Loading

s1fr0 left a comment

status-im-auto commented Jun 28, 2022 •

edited

Loading

staheri14 commented Jun 28, 2022 •

edited

Loading

staheri14 commented Jun 28, 2022

s1fr0 left a comment


		proc delete_member*(ctx: RLN[Bn256], index: uint): bool {.importc: "delete_member".}
		## index is the position of the id commitment key to be deleted from the tree

		proc key_gen*(ctx: RLN[Bn256], keypair_buffer: ptr Buffer): bool {.importc: "key_gen".}
		## generates id key and id commitment key serialized inside keypair_buffer as \| id_key <32 bytes>\| id_commitment_key <32 bytes> \|

	## the verification of the zk proof is available in result_ptr, where 0 indicates success and 1 is failure
	## the result of the verification of the zk proof is stored in the value pointed by result_ptr: if 0, the proof is valid; if 1, it is not

Chore(rln-relay): better documentation for rln nim bindings #1016

Chore(rln-relay): better documentation for rln nim bindings #1016

Conversation

staheri14 commented Jun 22, 2022

oskarth 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

s1fr0 Jun 29, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 Jun 24, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1fr0 left a comment

Choose a reason for hiding this comment

staheri14 commented Jun 24, 2022

s1fr0 Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

s1fr0 left a comment

Choose a reason for hiding this comment

status-im-auto commented Jun 28, 2022 • edited Loading

Jenkins Builds

staheri14 commented Jun 28, 2022 • edited Loading

staheri14 commented Jun 28, 2022

s1fr0 left a comment

Choose a reason for hiding this comment

s1fr0 Jun 29, 2022 •

edited

Loading

staheri14 Jun 24, 2022 •

edited

Loading

s1fr0 Jun 24, 2022 •

edited

Loading

status-im-auto commented Jun 28, 2022 •

edited

Loading

staheri14 commented Jun 28, 2022 •

edited

Loading