feat(rln-relay): process blocks atomically #1349
Conversation
rymnc
added
the
track:rln
Jenkins BuildsClick to see older builds (51)
|
Co-authored-by: Lorenzo Delgado <lorenzo@status.im>
| return err("failed to parse the data field of the MemberRegistered event") | ||
|
|
||
| type BlockTable = OrderedTable[BlockNumber, seq[MembershipTuple]] | ||
| proc getHistoricalEvents*(ethClientUri: string, |
There was a problem hiding this comment.
I'm aware some of this looks unsafe. The goal is to get this proc to look like https://github.com/status-im/nimbus-eth2/blob/44f652f704f14c67e648f2b14dee652d44b858c5/ncli/deposit_downloader.nim#L42, which has a cleaner way to handle rate limiting, etc. Will track it in a separate issue, for common web3 utils.
|
Can you please explain the un-safety of the
|
| %*{"fromBlock": fromBlock, "address": contractAddress}, | ||
| onMemberRegistered, | ||
| onError) | ||
| proc parse*(event: type MemberRegistered, |
There was a problem hiding this comment.
Please also add a comment to each line of this code explaining what is happeing.
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
| if indexGap > 1.uint: | ||
| let indexGap = lastIndex - rlnPeer.lastSeenMembershipIndex | ||
| if not (@[startingIndex..lastIndex, uint] == members.map(index)): | ||
| return err("the indices of the new members are not in order") |
There was a problem hiding this comment.
return err("indexes of new members are not in order")
| let indexGap = lastIndex - rlnPeer.lastSeenMembershipIndex | ||
| if not (@[startingIndex..lastIndex, uint] == members.map(index)): | ||
| return err("the indices of the new members are not in order") | ||
| if indexGap > 0.uint: |
There was a problem hiding this comment.
a safer way from creative bugs would be to have if indexGap != 0.uint:
There was a problem hiding this comment.
Anyway I still don't understand the logic of this check: if blocks are processed atomically I expect lastSeenMembershipIndex to be the latest index seen in previous processed block, if any. Not the last seen in current processed block: only if the block is successfully processed you then update lastSeenMembershipIndex. For this reason before I was asking if it was more correct to check that startIndex = lastSeenMembershipIndex + 1 which ensure no event was missed in the meanwhile.
There was a problem hiding this comment.
a safer way from creative bugs would be to have
if indexGap != 0.uint:
Addressed in 5919e05
There was a problem hiding this comment.
Sorry for previous comment, it seems I was looking at a wrong commit. Here seems to be fixed https://github.com/status-im/nwaku/blob/0ddcd115a0faa755f78fe7a66fd7085dbd56cd37/waku/v2/protocol/waku_rln_relay/waku_rln_relay_utils.nim#L1015-L1022 I would just change the check on the gap to be != 1
There was a problem hiding this comment.
and fixed additionally in 103c80b
There was a problem hiding this comment.
What a great PR! Thank you for addressing the (many!) comments received and for solving the (many!) merge conflicts!
I'd say just track for future reference what discussed in https://github.com/status-im/nwaku/pull/1349/files#r1017619179 so that we can enforce blocks to have chronological order.
|
The test failure at the macOS test2 CI job seems unrelated to the PR changes. @rymnc Feel free to merge these changes ✅ |
Fixes #1266, includes the following changes -
2. Removes all references ofinsertMembersince it is unsafe and can brick merkle trees without a good reconciliation mechanisminsertMemberwithinsertMembers, which batches updates to the MTset_leaves_from: feat(rln): ability to set leaves from a given index vacp2p/zerokit#63)Misc: