chore(rln-relay): gracefully handle chain forks #1623
Conversation
| proc removeMembers*(rlnInstance: ptr RLN, indices: seq[MembershipIndex]): bool = | ||
| for index in indices: | ||
| let deletion_success = delete_member(rlnInstance, index) | ||
| if not deletion_success: | ||
| return false | ||
| return true |
There was a problem hiding this comment.
This will be an atomic operation in zerokit soon
There was a problem hiding this comment.
LGTM. I think some procs that do a lot of different things (e.g. all the processing that happens per event when calling getEvents) is getting quite complex to follow. It may not be trivial, but I'd consider in future being very explicit about each processing step and extracting into separate procs as much as possible.
waku/v2/protocol/waku_rln_relay/group_manager/on_chain/group_manager.nim
Show resolved
Hide resolved
| @@ -246,17 +246,20 @@ suite "Onchain group manager": | |||
|
|
|||
| asyncTest "startGroupSync: should fetch history correctly": | |||
| let manager = await setup() | |||
| let credentials = generateCredentials(manager.rlnInstance, 5) | |||
| const credentialCount = 6 | |||
There was a problem hiding this comment.
Nit: The const keyword should be only used at "package level". Here, a let is sufficient.
There was a problem hiding this comment.
Used const since it can be evaluated at compile time, is that okay?
| var futures = [newFuture[void](), newFuture[void](), newFuture[void](), newFuture[void](), newFuture[void]()] | ||
|
|
||
| proc generateCallback(futs: array[0..4, Future[system.void]], credentials: seq[IdentityCredential]): OnRegisterCallback = | ||
| type VoidFuturesArray = array[0..credentialCount - 1, Future[void]] |
There was a problem hiding this comment.
I would rather avoid defining types in the middle of a test. I think the right place for type aliases and new types is the "package level".
There was a problem hiding this comment.
This type would be dependant on the credentialCount, which is set at the top of the test. Would you just prefer I hardcode it, or not use type aliases altogether?
There was a problem hiding this comment.
Sorry I come late to the party.
This PR handles events that have the "removed" field set to true, which indicates that the event was removed from the canonical chain (it was included in an uncle block)
Can we just rely on finalized blocks to avoid this kind of problems, which should also reduce the complexity? Finalized blocks are not reorganized nor modified (unless 66% of the chain changes their mind, which is almost impossible, and would require tons of slashings)
Its one of the cool thing of PoS vs PoW, PoS has some finality guarantees, wether in PoW anything could be reorged in theory.
|
https://discord.com/channels/613988663034118151/616299964242460682/1038023309960433674 waiting 12 minutes while you can be spammed is not good ux, this should serve better than waiting :) edit: while you can remove the offender from your merkle tree, that would lead to all other messages coming in being invalid since you have an indeterminate state of the tree (for 12 mins) |
|
@rymnc I agree that during these (around 12 minutes) you wont be able to slash onchain, but you will be able to detect spamming which allows us to i) Reject message (protecting the network) and ii) lower scoring, see P4 (protecting each node). Then after these 12 minutes when everything is settled, then that node can't even send messages since its removed from the tree. Perhaps im missing something?
|
|
Yeah, we have to validate proofs based on the state of the tree, i.e when a peer sends a message, they attach a RateLimitProof, which was generated with a set of elements which leads to their leaf in the tree. This constraint ensures that everyone maintains the same merkle tree locally, i.e if I had a different looking tree, the proofs generated would be invalid since they don't refer to a state the other peer considers "valid". To overcome network delay, etc, we include a window of acceptable roots, which changes per block (if a member registration/slashing is detected within the block). In your suggestion, if we slash them locally, this would lead to an invalid root being a part of the window (albeit, allowing other peers to still generate valid proofs from the last finalized block). This is a non-issue, since validating proofs will still work (sorry about prev comment) Also, someone wouldn't want to wait 12 minutes before they can start using waku with rln. This would be the main driver for this feature. |
Fixes #1338
This PR handles events that have the "removed" field set to
true, which indicates that the event was removed from the canonical chain (it was included in an uncle block)To handle this appropriately, we have to maintain a buffer of roots apart from the existing window of roots, to backfill valid roots when an uncle block has been detected. If this isn't done, then messages constructed in a valid state of the tree will be considered invalid, and dropped.
The length of the root buffer mentioned is dependent on the average reorg depth per chain. Should be updated accordingly.