fix(rln-relay): window of acceptable roots synced to rln metadata #1953
Conversation
| proc setMetadata*(g: OnchainGroupManager): RlnRelayResult[void] = | ||
| if g.latestProcessedBlock.isNone(): | ||
| return err("latest processed block is not set") | ||
| try: | ||
| let metadataSetRes = g.rlnInstance.setMetadata(RlnMetadata( | ||
| lastProcessedBlock: g.latestProcessedBlock.get(), | ||
| chainId: uint64(g.chainId.get()), | ||
| contractAddress: g.ethContractAddress, | ||
| validRoots: g.validRoots.toSeq())) | ||
| if metadataSetRes.isErr(): | ||
| return err("failed to persist rln metadata: " & metadataSetRes.error()) | ||
| except CatchableError: | ||
| return err("failed to persist rln metadata: " & getCurrentExceptionMsg()) | ||
| return ok() | ||
|
|
rymnc
force-pushed
the
fix-window-of-roots-sync
branch
from
a7eb6e0
to
9627494
Compare
|
You can find the image built from this PR at |
waku/waku_rln_relay/rln/wrappers.nim
Outdated
| let len = uint64.fromBytes(merkleNodeByteSeq[0..7], Endianness.littleEndian) | ||
| trace "length of valid roots", len | ||
| var offset = 8'u64 | ||
| while i <= len: |
There was a problem hiding this comment.
Minor Q: any reason why not a for loop?
There was a problem hiding this comment.
Thanks, addressed in c69728f
waku/waku_rln_relay/rln/wrappers.nim
Outdated
| var i = 1'u64 | ||
| let len = uint64.fromBytes(merkleNodeByteSeq[0..7], Endianness.littleEndian) | ||
| trace "length of valid roots", len | ||
| var offset = 8'u64 |
There was a problem hiding this comment.
Minor Q: any particular reason why var and not let?
There was a problem hiding this comment.
Thanks, addressed in c69728f
|
Shouldn't |
not really, the buffer is mainly used for chain reorgs, so it should be short-lived, right? |
| if setMetadataRes.isErr(): | ||
| error "failed to persist rln metadata", error=setMetadataRes.error |
There was a problem hiding this comment.
I see that this method can either raise an exception or like in this case, just print the error.
For the future I think it worth reviewing this file and make all funcs/methods/procs to return Result[T] rather than raising exceptions.
There was a problem hiding this comment.
yes, you're right! for all the functions that return Future's, I abstain from returning results as 2 errors need to be handled then - will create a follow up pr for this
There was a problem hiding this comment.
but, setting the metadata is not a fatal error, we should be okay if we can't set it for any reason
rymnc
force-pushed
the
fix-window-of-roots-sync
branch
from
41ac2be
to
5606069
Compare
rymnc
force-pushed
the
fix-window-of-roots-sync
branch
from
5606069
to
8b884fa
Compare
Description
It is noticed that when a node is restarted, and has access to a persisted tree, the window of acceptable roots is not
persisted, and may result in an empty deque. To mitigate this bug, we are persisting the window of acceptable roots in
the rln metadata.
Changes
RlnMetadatanow also contains a fieldvalidRootswhich is a seq ofMerkleNode'sIssue
closes #1952
Thanks @alrevuelta for the report!