Skip to content
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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 7, 2023
Merged

chore(rln-relay): Verify proofs based on bandwidth usage #1844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dbb8144
chore(rln-relay): Verify proofs based on bandwidth usage
rymnc Jul 6, 2023
a633f30
fix(rln-relay): make default threshold 0, for backwards compat
rymnc Jul 7, 2023
654b874
fix(rln-relay): add unit test for bandwidth cutoff
rymnc Jul 7, 2023
906d150
fix(rln-relay): rlnRelayBandwidthTreshold option
rymnc Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/wakunode2/app.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,8 @@ proc setupProtocols(node: WakuNode,
rlnRelayEthAccountAddress: conf.rlnRelayEthAccountAddress,
rlnRelayCredPath: conf.rlnRelayCredPath,
rlnRelayCredentialsPassword: conf.rlnRelayCredentialsPassword,
rlnRelayTreePath: conf.rlnRelayTreePath
rlnRelayTreePath: conf.rlnRelayTreePath,
rlnRelayBandwidthThreshold: conf.rlnRelayBandwidthThreshold
)

try:
Expand Down
5 changes: 5 additions & 0 deletions apps/wakunode2/external_config.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,11 @@ type
defaultValue: ""
name: "rln-relay-tree-path" }: string

rlnRelayBandwidthThreshold* {.
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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:

  • 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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. yes, agree that the value has to be fixed for the whole network
  2. follow up comment

Copy link
Contributor Author

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

Copy link
Contributor

@Menduist Menduist Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

desc: "Message rate in bytes/sec after which verification of proofs should happen",
defaultValue: 0 # to maintain backwards compatibility
name: "rln-relay-bandwidth-threshold" }: int

staticnodes* {.
desc: "Peer multiaddr to directly connect with. Argument may be repeated."
name: "staticnode" }: seq[string]
Expand Down
60 changes: 59 additions & 1 deletion tests/v2/waku_rln_relay/test_waku_rln_relay.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,8 @@ suite "Waku rln relay":
let rlnConf = WakuRlnConfig(rlnRelayDynamic: false,
rlnRelayPubsubTopic: RlnRelayPubsubTopic,
rlnRelayContentTopic: RlnRelayContentTopic,
rlnRelayCredIndex: index.uint)
rlnRelayCredIndex: index.uint,
rlnRelayTreePath: genTempPath("rln_tree", "waku_rln_relay_2"))
let wakuRlnRelayRes = await WakuRlnRelay.new(rlnConf)
require:
wakuRlnRelayRes.isOk()
Expand Down Expand Up @@ -708,6 +709,63 @@ suite "Waku rln relay":
msgValidate2 == MessageValidationResult.Spam
msgValidate3 == MessageValidationResult.Valid
msgValidate4 == MessageValidationResult.Invalid

asyncTest "should validate invalid proofs if bandwidth is available":
let index = MembershipIndex(5)

let rlnConf = WakuRlnConfig(rlnRelayDynamic: false,
rlnRelayPubsubTopic: RlnRelayPubsubTopic,
rlnRelayContentTopic: RlnRelayContentTopic,
rlnRelayCredIndex: index.uint,
rlnRelayBandwidthThreshold: 4,
rlnRelayTreePath: genTempPath("rln_tree", "waku_rln_relay_3"))
let wakuRlnRelayRes = await WakuRlnRelay.new(rlnConf)
require:
wakuRlnRelayRes.isOk()
let wakuRlnRelay = wakuRlnRelayRes.get()

# get the current epoch time
let time = epochTime()

# create some messages from the same peer and append rln proof to them, except wm4
var
# this one will pass through the bandwidth threshold
wm1 = WakuMessage(payload: "Spam".toBytes())
# this message, will be over the bandwidth threshold, hence has to be verified
wm2 = WakuMessage(payload: "Valid message".toBytes())
# this message will be over the bandwidth threshold, hence has to be verified, will be false (since no proof)
wm3 = WakuMessage(payload: "Invalid message".toBytes())
wm4 = WakuMessage(payload: "Spam message".toBytes())

let
proofAdded1 = wakuRlnRelay.appendRLNProof(wm1, time)
proofAdded2 = wakuRlnRelay.appendRLNProof(wm2, time+EpochUnitSeconds)
proofAdded3 = wakuRlnRelay.appendRLNProof(wm4, time)

# ensure proofs are added
require:
proofAdded1
proofAdded2
proofAdded3

# validate messages
# validateMessage proc checks the validity of the message fields and adds it to the log (if valid)
let
# this should be no verification, Valid
msgValidate1 = wakuRlnRelay.validateMessage(wm1, some(time))
# this should be verification, Valid
msgValidate2 = wakuRlnRelay.validateMessage(wm2, some(time))
# this should be verification, Invalid
msgValidate3 = wakuRlnRelay.validateMessage(wm3, some(time))
# this should be verification, Spam
msgValidate4 = wakuRlnRelay.validateMessage(wm4, some(time))

check:
msgValidate1 == MessageValidationResult.Valid
msgValidate2 == MessageValidationResult.Valid
msgValidate3 == MessageValidationResult.Invalid
msgValidate4 == MessageValidationResult.Spam


test "toIDCommitment and toUInt256":
# create an instance of rln
Expand Down
6 changes: 6 additions & 0 deletions tests/v2/waku_rln_relay/test_wakunode_rln_relay.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ procSuite "WakuNode - RLN relay":
rlnRelayContentTopic: contentTopic,
rlnRelayCredIndex: 1.uint,
rlnRelayTreePath: genTempPath("rln_tree", "wakunode_4"),
rlnRelayBandwidthThreshold: 0,
))

await node1.start()
Expand All @@ -154,6 +155,7 @@ procSuite "WakuNode - RLN relay":
rlnRelayContentTopic: contentTopic,
rlnRelayCredIndex: 2.uint,
rlnRelayTreePath: genTempPath("rln_tree", "wakunode_5"),
rlnRelayBandwidthThreshold: 0,
))

await node2.start()
Expand All @@ -166,6 +168,7 @@ procSuite "WakuNode - RLN relay":
rlnRelayContentTopic: contentTopic,
rlnRelayCredIndex: 3.uint,
rlnRelayTreePath: genTempPath("rln_tree", "wakunode_6"),
rlnRelayBandwidthThreshold: 0,
))

await node3.start()
Expand Down Expand Up @@ -248,6 +251,7 @@ procSuite "WakuNode - RLN relay":
rlnRelayContentTopic: contentTopic,
rlnRelayCredIndex: 1.uint,
rlnRelayTreePath: genTempPath("rln_tree", "wakunode_7"),
rlnRelayBandwidthThreshold: 0,
))

await node1.start()
Expand All @@ -261,6 +265,7 @@ procSuite "WakuNode - RLN relay":
rlnRelayContentTopic: contentTopic,
rlnRelayCredIndex: 2.uint,
rlnRelayTreePath: genTempPath("rln_tree", "wakunode_8"),
rlnRelayBandwidthThreshold: 0,
))

await node2.start()
Expand All @@ -274,6 +279,7 @@ procSuite "WakuNode - RLN relay":
rlnRelayContentTopic: contentTopic,
rlnRelayCredIndex: 3.uint,
rlnRelayTreePath: genTempPath("rln_tree", "wakunode_9"),
rlnRelayBandwidthThreshold: 0,
))

await node3.start()
Expand Down
32 changes: 27 additions & 5 deletions waku/v2/waku_rln_relay/rln_relay.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -23,7 +23,8 @@ import
./protocol_metrics
import
../waku_core,
../waku_keystore
../waku_keystore,
../utils/collector

logScope:
topics = "waku rln_relay"
Expand All @@ -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
Expand Down Expand Up @@ -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] =
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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,
Expand Down