-
Notifications
You must be signed in to change notification settings - Fork 57
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): Verify proofs based on bandwidth usage #1844
Changes from 1 commit
dbb8144
a633f30
654b874
906d150
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ else: | |
|
||
import | ||
std/[algorithm, sequtils, strutils, tables, times, os, deques], | ||
chronicles, options, chronos, stint, | ||
chronicles, options, chronos, chronos/ratelimit, stint, | ||
confutils, | ||
web3, json, | ||
web3/ethtypes, | ||
|
@@ -23,7 +23,8 @@ import | |
./protocol_metrics | ||
import | ||
../waku_core, | ||
../waku_keystore | ||
../waku_keystore, | ||
../utils/collector | ||
|
||
logScope: | ||
topics = "waku rln_relay" | ||
|
@@ -41,6 +42,7 @@ type WakuRlnConfig* = object | |
rlnRelayCredPath*: string | ||
rlnRelayCredentialsPassword*: string | ||
rlnRelayTreePath*: string | ||
rlnRelayBandwidthThreshold*: int | ||
|
||
proc createMembershipList*(rln: ptr RLN, n: int): RlnRelayResult[( | ||
seq[RawMembershipCredentials], string | ||
|
@@ -86,6 +88,7 @@ type WakuRLNRelay* = ref object of RootObj | |
nullifierLog*: Table[Epoch, seq[ProofMetadata]] | ||
lastEpoch*: Epoch # the epoch of the last published rln message | ||
groupManager*: GroupManager | ||
messageBucket*: Option[TokenBucket] | ||
|
||
proc hasDuplicate*(rlnPeer: WakuRLNRelay, | ||
proofMetadata: ProofMetadata): RlnRelayResult[bool] = | ||
|
@@ -160,14 +163,16 @@ proc absDiff*(e1, e2: Epoch): uint64 = | |
else: | ||
return epoch2 - epoch1 | ||
|
||
proc validateMessage*(rlnPeer: WakuRLNRelay, msg: WakuMessage, | ||
timeOption: Option[float64] = none(float64)): MessageValidationResult = | ||
proc validateMessage*(rlnPeer: WakuRLNRelay, | ||
msg: WakuMessage, | ||
timeOption = none(float64)): MessageValidationResult = | ||
## validate the supplied `msg` based on the waku-rln-relay routing protocol i.e., | ||
## the `msg`'s epoch is within MaxEpochGap of the current epoch | ||
## the `msg` has valid rate limit proof | ||
## the `msg` does not violate the rate limit | ||
## `timeOption` indicates Unix epoch time (fractional part holds sub-seconds) | ||
## if `timeOption` is supplied, then the current epoch is calculated based on that | ||
|
||
let decodeRes = RateLimitProof.init(msg.proof) | ||
if decodeRes.isErr(): | ||
return MessageValidationResult.Invalid | ||
|
@@ -286,6 +291,18 @@ proc generateRlnValidator*(wakuRlnRelay: WakuRLNRelay, | |
let contentTopic = wakuRlnRelay.contentTopic | ||
proc validator(topic: string, message: messages.Message): Future[pubsub.ValidationResult] {.async.} = | ||
trace "rln-relay topic validator is called" | ||
|
||
## Check if enough tokens can be consumed from the message bucket | ||
try: | ||
if wakuRlnRelay.messageBucket.isSome() and | ||
wakuRlnRelay.messageBucket.get().tryConsume(message.data.len): | ||
return pubsub.ValidationResult.Accept | ||
else: | ||
info "message bandwidth limit exceeded, running rate limit proof validation" | ||
except OverflowDefect: # not a problem | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ehh that shouldn't be necessary, and catching defects is not great, what happened? Probably a bug to fix in the rate limiter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, if you pass in 0 to the TokenBucket, it underflows. sorry for not creating an issue! |
||
debug "not enough bandwidth, running rate limit proof validation" | ||
|
||
|
||
let decodeRes = WakuMessage.decode(message.data) | ||
if decodeRes.isOk(): | ||
let | ||
|
@@ -377,9 +394,14 @@ proc mount(conf: WakuRlnConfig, | |
# Start the group sync | ||
await groupManager.startGroupSync() | ||
|
||
let messageBucket = if conf.rlnRelayBandwidthThreshold > 0: | ||
some(TokenBucket.new(conf.rlnRelayBandwidthThreshold)) | ||
else: none(TokenBucket) | ||
|
||
return WakuRLNRelay(pubsubTopic: conf.rlnRelayPubsubTopic, | ||
contentTopic: conf.rlnRelayContentTopic, | ||
groupManager: groupManager) | ||
groupManager: groupManager, | ||
messageBucket: messageBucket) | ||
|
||
|
||
proc new*(T: type WakuRlnRelay, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a user perspective, how could I set the proper value of this setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it's mentioned bytes/sec, so it should be according to their personal bandwidth constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im fine with having it as a cli flag, but imho this parameter should be fixed for the network and not "according to their personal bandwidth constraints". wouldn't this be dangerous?
if different nodes in the same network set different thresholds, then message validity (valid/invalid) wont be an objective truth, but something that will depend on each node (each node will have its own view on wether the message is valid or invalid). and for waku as a network, i thinnk this can cause troubles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this problem can be solved if we broadcast capabilities like waku_relay, however if you'd like to set it to a global value, it'll be a matter of prioritising resource restrained nodes vs an overall global stable network. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to ensure we are on the same page:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the network will be stable if we set a fixed bandwidth threshold for all nodes, however, some nodes which have a bandwidth lesser than that will blindly forward messages to other nodes. and if we implement peer scoring for rln, this opens another issue where we need to adjust peer score based on the other's bandwidth threshold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be the case not matter what with this PR, since the order in which the messages are received will impact if they are valid or invalid.
As long as the messages are not counted as
Invalid
, this shouldn't cause issue at the GS level, but it will of course means that the global view of the messages won't be.. global(just to clarify: for GS, the valid/ignore distinction doesn't need to be network-wide, but
Invalid
does need to be global)