chore(rln-relay): add isReady check #1989

rymnc · 2023-09-04T13:18:25Z

Description

Adds an isReady proc to waku_node, waku_rln_relay

Changes

WakuNode.isReady => if all protocols are ready, returns true
WakuRlnRelay.isReady => if group manager is ready, returns true

Issue

Addresses 25. #1906

github-actions · 2023-09-04T13:26:51Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1989

github-actions · 2023-09-04T15:10:02Z

You can find the experimental image built from this PR at

quay.io/wakuorg/nwaku-pr:1989-experimental

rymnc · 2023-09-04T17:23:10Z

cc: @alrevuelta i think this pr should be separate from the one that creates an api endpoint to reduce cognitive load - wdyt?

alrevuelta

@rymnc agree, we can have 2 PRs.

Not sure having a isReady variable is a good idea. I would have just a function isReady() that check the difference between the last processed block in the rln tree and the latest head block in the blockchain (perhaps allowing 1 or 2 blocks of difference).

As it is, you may set g.isReady = true but at any point in time you can lose sync and unless im missing something you wont set isReady back to false. Imagine for example that the eth node falls behind sync.

tldr: isReady() function but no stored isReady state. This state is calculated upon request. wdyt?

rymnc · 2023-09-05T07:13:27Z

@rymnc agree, we can have 2 PRs.

Not sure having a isReady variable is a good idea. I would have just a function isReady() that check the difference between the last processed block in the rln tree and the latest head block in the blockchain (perhaps allowing 1 or 2 blocks of difference).

As it is, you may set g.isReady = true but at any point in time you can lose sync and unless im missing something you wont set isReady back to false. Imagine for example that the eth node falls behind sync.

tldr: isReady() function but no stored isReady state. This state is calculated upon request. wdyt?

I think that's overkill - when we are out of sync, we do set isReady to false (check the ethRpc.onDisconnect)

alrevuelta · 2023-09-05T07:21:22Z

I think that's overkill - when we are out of sync, we do set isReady to false (check the ethRpc.onDisconnect)

yep but there are other ways you can get out of sync without ethRpc.onDisconnect. For example, if the eth node gets out of sync, then you will fall behind while staying connected. You assume that once you are in sync for the first time, you stay in sync forever (unless disconnected). What happens also if your node can't process all the membership insertions realtime and falls behind?

rymnc · 2023-09-05T07:27:08Z

I think that's overkill - when we are out of sync, we do set isReady to false (check the ethRpc.onDisconnect)

yep but there are other ways you can get out of sync without ethRpc.onDisconnect. For example, if the eth node gets out of sync, then you will fall behind while staying connected. You assume that once you are in sync for the first time, you stay in sync forever (unless disconnected). What happens also if your node can't process all the membership insertions realtime and falls behind?

hmm, how would it be possible to detect if the eth node itself is out of sync without using multiple providers?

What happens also if your node can't process all the membership insertions realtime and falls behind?
should we set it to false then during inserts? and set it back to true right after?

alrevuelta · 2023-09-05T07:33:33Z

hmm, how would it be possible to detect if the eth node itself is out of sync without using multiple providers?

afaik even if the eth node is out of sync, it knows that its out sync. meaning that it knows the latest head block known by the blockchain and the latest known block by the node. I think this is the rpc link

I guess you will also notice indirectly. If the eth node falls behind sync, it wont give you the latest blocks, so you will fall behind sync, seeing that the latestHead is way beyond your latest localRlnLatestProcessedBlock.

rymnc · 2023-09-05T07:38:27Z

hmm, how would it be possible to detect if the eth node itself is out of sync without using multiple providers?

afaik even if the eth node is out of sync, it knows that its out sync. meaning that it knows the latest head block known by the blockchain and the latest known block by the node. I think this is the rpc link

I guess you will also notice indirectly. If the eth node falls behind sync, it wont give you the latest blocks, so you will fall behind sync, seeing that the latestHead is way beyond your latest localRlnLatestProcessedBlock.

if the eth node falls behind sync, would it still emit the newHead events? unsure about this behaviour - i do agree about calculating every time isReady is called though

alrevuelta · 2023-09-05T07:43:47Z

if the eth node falls behind sync, would it still emit the newHead events? unsure about this behaviour - i do agree about calculating every time isReady is called though

have never tried empirically. I would bet that it stops emitting events, and you will fall behind sync without noticing.

rymnc · 2023-09-05T07:48:17Z

if the eth node falls behind sync, would it still emit the newHead events? unsure about this behaviour - i do agree about calculating every time isReady is called though

have never tried empirically. I would bet that it stops emitting events, and you will fall behind sync without noticing.

yes so then there are 2 ways we will be out of sync -

eth node out of sync
we do not process insertions in time

we cannot account for 1 unless we use multiple providers
for 2, we can do this by having the isReady proc compute the difference between latestBlockSeen and latestBlockProcessed, as you said. wdyt?

alrevuelta · 2023-09-05T08:56:20Z

we cannot account for 1 unless we use multiple providers

why? an eth node out of sync will know its out of sync. so with just one node you should know that its out of sync. or?

afaik we can address both 1 and 2 without multiple providers unless im missing something.

rymnc · 2023-09-05T09:51:37Z

we cannot account for 1 unless we use multiple providers

why? an eth node out of sync will know its out of sync. so with just one node you should know that its out of sync. or?

afaik we can address both 1 and 2 without multiple providers unless im missing something.

maybe are you suggesting that we make the rpc call every time the isReady proc is called?

Ivansete-status

LGTM! Super interesting idea!

In further PRs, I think we will need to add a /healthcheck endpoint as a valid REST path ( cc @NagyZoltanPeter )

alrevuelta · 2023-09-05T10:07:13Z

maybe are you suggesting that we make the rpc call every time the isReady proc is called?

Yes exactly, perhaps this: https://docs.infura.io/networks/ethereum/json-rpc-methods/eth_syncing#returns

I'm fine with leaving a TODO for this (your point 1). But would like to address 2. here.

In further PRs, I think we will need to add a /healthcheck endpoint as a valid REST path ( cc @NagyZoltanPeter )

tracked here:
#1988

NagyZoltanPeter · 2023-09-05T11:08:14Z

I like the idea of having /healthcheck!

SionoiS

LGTM

rymnc · 2023-09-05T13:26:10Z

@alrevuelta I hope cbb266a covers the requirement (1 & 2)

alrevuelta

thanks! left a comment of a possible edge case, let me know if im missing something.

alrevuelta · 2023-09-05T13:47:13Z

waku/waku_rln_relay/group_manager/on_chain/group_manager.nim

@@ -73,6 +73,8 @@ type
    # in event of a reorg. we store 5 in the buffer. Maybe need to revisit this,
    # because the average reorg depth is 1 to 2 blocks.
    validRootBuffer*: Deque[MerkleNode]
+    # this variable tracks the last seen head
+    lastSeenBlockHead*: BlockNumber


not sure i understand the need of this variable, and perhaps there is an edge case?

afaiu we sync in batches:

batch1: fromblock1 toblock1

batch2: fromblock2 toblock2

etc

And lastSeenBlockHead takes the values of toblock1, toblock2, etc?

So what happens when we just processed batch1. In this exact moment we latestProcessedBlock = lastSeenBlockHead so we may think we are in sync. But in reality we are in batch1 and batch2 is still left.

So the lastSeenBlockHead should be the last head block known by the blockchain.

right, I'll just move it to the newHeadCallback, thanks

Addressed in 3c9d4be

So the lastSeenBlockHead should be the last head block known by the blockchain.

I think its safer to get this from eth_blockNumber endpoint rather than relying on the newHeadcallback. If the eth node fails to keep you updated with the latest head (but without disconnecting) then your lastSeenBlockHead will be outdated.

But if you use eth_blockNumber, since its req/rep, you ensure that your latest head is correct. If then node fails to reply, then its not ready.

Addressed in 15bfde4

alrevuelta · 2023-09-05T13:49:27Z

waku/waku_rln_relay/group_manager/on_chain/group_manager.nim

+    error "failed to get the syncing status", error = getCurrentExceptionMsg()
+    return false
+
+method isReady*(g: OnchainGroupManager): Future[bool] {.async,gcsafe.} =


perhaps its cleaner to run the checks for true all together?

if g.ethRpc.isSome() && g.lastSeenBlockHead !=0 && g.latestProcessedBlock >= g.lastSeenBlockHead && !await g.isSyncing(): return true return false

maybe just a personal prefernce, just a sugerence.

Yeah, I did think of this, imo the way it is right now is slightly more readable

alrevuelta · 2023-09-06T07:54:31Z

waku/waku_rln_relay/group_manager/on_chain/group_manager.nim

@@ -551,10 +553,12 @@ method isReady*(g: OnchainGroupManager): Future[bool] {.async,gcsafe.} =
  if g.ethRpc.isNone():
    return false

-  if g.lastSeenBlockHead == 0:
-    return false
+  let currentBlock = cast[BlockNumber](await g.ethRpc


guess if this fails an exceptions is thrown? should we catch it and return false if so. guess if we dont catch it the node will crash?

Addressed in b058c97

alrevuelta

lgtm!

rymnc self-assigned this Sep 4, 2023

rymnc force-pushed the rln-in-sync branch from bcd473d to fd05ede Compare September 4, 2023 13:21

rymnc added the E:2023-rln label Sep 4, 2023

rymnc force-pushed the rln-in-sync branch 2 times, most recently from e710467 to 99670c6 Compare September 4, 2023 13:31

rymnc mentioned this pull request Sep 4, 2023

chore(rln-relay): Requirements to consider RLN ready (non experimental) #1906

Closed

56 tasks

rymnc force-pushed the rln-in-sync branch from 99670c6 to a3a29fc Compare September 4, 2023 13:44

rymnc marked this pull request as ready for review September 4, 2023 17:01

rymnc requested review from Ivansete-status, alrevuelta and SionoiS September 4, 2023 17:02

chore(rln-relay): add isReady check

b7c71b9

rymnc force-pushed the rln-in-sync branch from a3a29fc to b7c71b9 Compare September 5, 2023 05:07

alrevuelta reviewed Sep 5, 2023

View reviewed changes

Ivansete-status approved these changes Sep 5, 2023

View reviewed changes

SionoiS approved these changes Sep 5, 2023

View reviewed changes

fix(rln-relay): multiple parameters for checking if node is in sync

cbb266a

alrevuelta reviewed Sep 5, 2023

View reviewed changes

fix: set latesthead in newHeadCallback

3c9d4be

rymnc requested a review from alrevuelta September 6, 2023 06:53

fix: explicit rpc call

15bfde4

alrevuelta reviewed Sep 6, 2023

View reviewed changes

fix: unhandled exception

b058c97

alrevuelta self-requested a review September 6, 2023 08:34

alrevuelta approved these changes Sep 6, 2023

View reviewed changes

rymnc merged commit 5638bd0 into master Sep 6, 2023
14 checks passed

rymnc deleted the rln-in-sync branch September 6, 2023 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(rln-relay): add isReady check #1989

chore(rln-relay): add isReady check #1989

rymnc commented Sep 4, 2023

github-actions bot commented Sep 4, 2023

github-actions bot commented Sep 4, 2023

rymnc commented Sep 4, 2023

alrevuelta left a comment

rymnc commented Sep 5, 2023

alrevuelta commented Sep 5, 2023

rymnc commented Sep 5, 2023

alrevuelta commented Sep 5, 2023

rymnc commented Sep 5, 2023

alrevuelta commented Sep 5, 2023 •

edited

Loading

rymnc commented Sep 5, 2023

alrevuelta commented Sep 5, 2023

rymnc commented Sep 5, 2023

Ivansete-status left a comment

alrevuelta commented Sep 5, 2023

NagyZoltanPeter commented Sep 5, 2023

SionoiS left a comment

rymnc commented Sep 5, 2023

alrevuelta left a comment

alrevuelta Sep 5, 2023

rymnc Sep 5, 2023

rymnc Sep 5, 2023

alrevuelta Sep 6, 2023

rymnc Sep 6, 2023

alrevuelta Sep 5, 2023

rymnc Sep 5, 2023

alrevuelta Sep 6, 2023

rymnc Sep 6, 2023

alrevuelta left a comment

chore(rln-relay): add isReady check #1989

chore(rln-relay): add isReady check #1989

Conversation

rymnc commented Sep 4, 2023

Description

Changes

Issue

github-actions bot commented Sep 4, 2023

github-actions bot commented Sep 4, 2023

rymnc commented Sep 4, 2023

alrevuelta left a comment

Choose a reason for hiding this comment

rymnc commented Sep 5, 2023

alrevuelta commented Sep 5, 2023

rymnc commented Sep 5, 2023

alrevuelta commented Sep 5, 2023

rymnc commented Sep 5, 2023

alrevuelta commented Sep 5, 2023 • edited Loading

rymnc commented Sep 5, 2023

alrevuelta commented Sep 5, 2023

rymnc commented Sep 5, 2023

Ivansete-status left a comment

Choose a reason for hiding this comment

alrevuelta commented Sep 5, 2023

NagyZoltanPeter commented Sep 5, 2023

SionoiS left a comment

Choose a reason for hiding this comment

rymnc commented Sep 5, 2023

alrevuelta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrevuelta left a comment

Choose a reason for hiding this comment

alrevuelta commented Sep 5, 2023 •

edited

Loading