chore(rln-relay): Verify proofs based on bandwidth usage #1844
Conversation
rymnc
force-pushed
the
bandwidth-verification
branch
9 times, most recently
from
2bf7b48 to
36c80b0
Compare
There was a problem hiding this comment.
Nice feature!
@jm-clius, what is the bandwidth cutoff you'd like to be the default?
Mmm. Wondering if this shouldn't be 0 for now - someone enabling RLN probably expects it to work immediately and not just after a threshold. This is also backwards compatible with previous iterations and current docs. We can reconsider once we have a default design for the public network finished. Does it make sense to use/initialise the token bucket at all if threshold is 0? Perhaps make it an Option?
rymnc
force-pushed
the
bandwidth-verification
branch
from
36c80b0 to
9974535
Compare
rymnc
force-pushed
the
bandwidth-verification
branch
from
9974535 to
dbb8144
Compare
There was a problem hiding this comment.
great! lgtm.
perhaps we are missing a simple unit test? since a missconfoguration of this feature can totally "short circuit" rln, totally disabling it. Even if it works now, its always nice to protect the code from future changes.
on the other hand (no action required) i usually prefer when things always works or never works. this introduces a sometimes works.
|
Thanks @alrevuelta, addressed in a633f30 and 63fa8dd |
| @@ -211,6 +211,11 @@ type | |||
| defaultValue: "" | |||
| name: "rln-relay-tree-path" }: string | |||
|
|
|||
| rlnRelayBandwidthThreshold* {. | |||
There was a problem hiding this comment.
From a user perspective, how could I set the proper value of this setting?
There was a problem hiding this comment.
Hmm, it's mentioned bytes/sec, so it should be according to their personal bandwidth constraints.
There was a problem hiding this comment.
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.
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.
just to ensure we are on the same page:
- do we agree that these value has to be fixed for the whole network because of the above reasons?
- mind elaborating on "it'll be a matter of prioritising resource restrained nodes vs an overall global stable network"?
There was a problem hiding this comment.
- yes, agree that the value has to be fixed for the whole network
- follow up comment
There was a problem hiding this comment.
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.
then message validity (valid/invalid) wont be an objective truth
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)
rymnc
force-pushed
the
bandwidth-verification
branch
from
63fa8dd to
654b874
Compare
| 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.
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.
yeah, if you pass in 0 to the TokenBucket, it underflows. sorry for not creating an issue!
Description
Runs rln verification only if bandwidth exceeds 1mbps. @jm-clius, what is the bandwidth cutoff you'd like to be the default?
Changes
Issue
closes #1837