feat(c-bindings): rln relay #2544
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
You can find the image built from this PR at Built from 9afbf17 |
richard-ramos
force-pushed
the
bindings/rln
branch
2 times, most recently
from
444a599 to
2a06e55
Compare
|
Oops! I forgot to append the proof to the RLN message. Will add that in a separate commit EDIT: added in 964d7ab |
richard-ramos
force-pushed
the
node-init
branch
from
32297d9 to
0f30ea7
Compare
richard-ramos
force-pushed
the
bindings/rln
branch
from
964d7ab to
4939a5a
Compare
|
Done! PR rebased! |
Ivansete-status
approved these changes
Mar 25, 2024
library/waku_thread/config.nim
Outdated
|
|
||
| if jsonNode.contains("clusterId"): | ||
| if jsonNode["clusterId"].kind != JsonNodeKind.JInt: | ||
| errorResp = "The clusterId config param should be a int" |
There was a problem hiding this comment.
Tiny typo :)
Suggested change
| errorResp = "The clusterId config param should be a int" | |
| errorResp = "The clusterId config param should be an int" |
library/waku_thread/config.nim
Outdated
| @@ -149,6 +165,29 @@ proc parseTopics(jsonNode: JsonNode, conf: var WakuNodeConf) = | |||
| else: | |||
| conf.topics = @["/waku/2/default-waku/proto"] | |||
|
|
|||
| proc parseRLNRelay(jsonNode: JsonNode, conf: var WakuNodeConf, errorResp: var string): bool = | |||
| if not jsonNode.contains("rlnRelay"): | |||
There was a problem hiding this comment.
I wonder if we should use the same style as the wakunode has. Hyphen-separated terms.
Suggested change
| if not jsonNode.contains("rlnRelay"): | |
| if not jsonNode.contains("rln-relay"): |
library/waku_thread/config.nim
Outdated
Comment on lines
178
to
187
| conf.rlnRelayCredPath = jsonNode{"credentialPassword"}.getStr() | ||
| conf.rlnRelayEthClientAddress = EthRpcUrl.parseCmdArg(jsonNode{"ethClientAddress"}.getStr("http://localhost:8540")) | ||
| conf.rlnRelayEthContractAddress = jsonNode{"contractAddress"}.getStr() | ||
| conf.rlnRelayCredPassword = jsonNode{"credentialPassword"}.getStr() | ||
| conf.rlnRelayUserMessageLimit = uint64(jsonNode{"userMessageLimit"}.getInt(1)) | ||
| conf.rlnEpochSizeSec = uint64(jsonNode{"epochSizeSec"}.getInt(1)) | ||
| conf.rlnRelayCredIndex = some(uint(jsonNode{"membershipIndex"}.getInt())) | ||
| conf.rlnRelayDynamic = jsonNode{"dynamic"}.getBool() | ||
| conf.rlnRelayTreePath = jsonNode{"treePath"}.getStr() | ||
| conf.rlnRelayBandwidthThreshold = jsonNode{"bandwidthThreshold"}.getInt() |
There was a problem hiding this comment.
Analogue to my previous comment, I suggest using the same names as in wakunode app:
$ ./build/wakunode2 --help | grep rln
--rln-relay-cred-path The path for peristing rln-relay credential.
--rln-relay-eth-client-address HTTP address of an Ethereum testnet client e.g., http://localhost:8540/
--rln-relay-eth-contract-address Address of membership contract on an Ethereum testnet.
--rln-relay-cred-password Password for encrypting RLN credentials.
--rln-relay-eth-private-key Private key for broadcasting transactions.
--rln-relay-user-message-limit Set a user message limit for the rln membership registration. Must be a positive
--rln-relay-epoch-sec Epoch size in seconds used to rate limit RLN memberships. Default is 1 second.
--rln-relay Enable spam protection through rln-relay: true|false [=false].
--rln-relay-membership-index the index of the onchain commitment to use.
--rln-relay-dynamic Enable waku-rln-relay with on-chain dynamic group management: true|false
--rln-relay-id-key Rln relay identity secret key as a Hex string.
--rln-relay-id-commitment-key Rln relay identity commitment key as a Hex string.
--rln-relay-tree-path Path to the RLN merkle tree sled db (https://github.com/spacejam/sled).
--rln-relay-bandwidth-threshold Message rate in bytes/sec after which verification of proofs should happen [=0].
--rln-relay-tree-path Path to the RLN merkle tree sled db (https://github.com/spacejam/sled).
warning: could not open directory 'postgres-data/': Permission denied
There was a problem hiding this comment.
I can use the hyphens to separate the words but adding the rln prefix I'm not sure, because it currently looks like this:
{
"port": 123,
"host": "0.0.0.0",
"relay": true,
"rlnRelay": {
"enabled": "true",
"ethClientAddress": "http://localhost:8545",
"membershipIndex": 2,
...
}
}And seems to me that adding the rln prefix would be redundant, as the values are already part of a rlnRelay section. A possibility could be to put everything a single level deep (i.e., move all the items from rlnRelay into the root json, but seems to me that it's not benefiting from the possibilities of json allowing more logical groups.
Would something like this work? (i.e. having the rln-relay prefix be instead the key that holds all the rln related configs):
{
"port": 123,
"host": "0.0.0.0",
"relay": true,
"rln-relay": {
"enabled": "true",
"eth-client-address": "http://localhost:8545",
"membership-index": 2,
"bandwidth-threshold": 1,
...
}
}There was a problem hiding this comment.
Yes, I think this is the best balanced option (simplicity vs consistency)
Comment on lines
+71
to
+89
| if conf.clusterId == 1: | ||
| let twnClusterConf = ClusterConf.TheWakuNetworkConf() | ||
| if len(conf.shards) != 0: | ||
| conf.pubsubTopics = conf.shards.mapIt(twnClusterConf.pubsubTopics[it.uint16]) | ||
| else: | ||
| conf.pubsubTopics = twnClusterConf.pubsubTopics | ||
|
|
||
| # Override configuration | ||
| conf.maxMessageSize = twnClusterConf.maxMessageSize | ||
| conf.clusterId = twnClusterConf.clusterId | ||
| conf.rlnRelay = twnClusterConf.rlnRelay | ||
| conf.rlnRelayEthContractAddress = twnClusterConf.rlnRelayEthContractAddress | ||
| conf.rlnRelayDynamic = twnClusterConf.rlnRelayDynamic | ||
| conf.rlnRelayBandwidthThreshold = twnClusterConf.rlnRelayBandwidthThreshold | ||
| conf.discv5Discovery = twnClusterConf.discv5Discovery | ||
| conf.discv5BootstrapNodes = | ||
| conf.discv5BootstrapNodes & twnClusterConf.discv5BootstrapNodes | ||
| conf.rlnEpochSizeSec = twnClusterConf.rlnEpochSizeSec | ||
| conf.rlnRelayUserMessageLimit = twnClusterConf.rlnRelayUserMessageLimit |
There was a problem hiding this comment.
We have a similar structure in
nwaku/apps/wakunode2/wakunode2.nim
Lines 91 to 109 in 18a0535
I wonder if we could encapsulate that somehow. Maybe in a separate PR
SionoiS
approved these changes
Mar 27, 2024
|
richard-ramos
force-pushed
the
bindings/rln
branch
from
4939a5a to
ed35360
Compare
richard-ramos
force-pushed
the
bindings/rln
branch
from
ed35360 to
a9757fe
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Exposes the rln relay configuration to the bindings.
Issue
closes #2454