-
Notifications
You must be signed in to change notification settings - Fork 57
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.
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): health check should account for window of roots #2664
Conversation
You can find the image built from this PR at
Built from 6db6820 |
You can find the image built from this PR at
Built from 6db6820 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank! 💯
@@ -855,7 +855,8 @@ method isReady*(g: OnchainGroupManager): Future[bool] {.async.} = | |||
g.retryWrapper(currentBlock, "Failed to get the current block number"): | |||
cast[BlockNumber](await g.ethRpc.get().provider.eth_blockNumber()) | |||
|
|||
if g.latestProcessedBlock < currentBlock: | |||
# the node is still able to process messages if it is behind the latest block by a factor of the valid roots | |||
if uint(g.latestProcessedBlock) < uint(currentBlock) - uint(g.validRoots.len): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if is safer to add parenthesis:
if uint(g.latestProcessedBlock) < uint(currentBlock) - uint(g.validRoots.len): | |
if uint(g.latestProcessedBlock) < ( uint(currentBlock) - uint(g.validRoots.len) ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't harm, i would add it as well :)
@@ -855,7 +855,8 @@ method isReady*(g: OnchainGroupManager): Future[bool] {.async.} = | |||
g.retryWrapper(currentBlock, "Failed to get the current block number"): | |||
cast[BlockNumber](await g.ethRpc.get().provider.eth_blockNumber()) | |||
|
|||
if g.latestProcessedBlock < currentBlock: | |||
# the node is still able to process messages if it is behind the latest block by a factor of the valid roots | |||
if uint(g.latestProcessedBlock) < uint(currentBlock) - uint(g.validRoots.len): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it better widening rather than narrowing? i mean instead of casting everything to unit and operating with that, using the latestProcessedBlock
type which has more bits and then "widening" validRoots to do the -
?
wondering also if in the first run we can hit currentBlock-g.validRoots.len
where currentBlock=0
and since we are using unit that will lead to a huge value, meaning that we are ready when we are not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentBlock will never be 0, since that is fetched from the chain (and we can operate under the assumption that it will always be greater than 0). good point about widening vs narrowing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casted to u256
- 442ca43
@@ -855,7 +855,8 @@ method isReady*(g: OnchainGroupManager): Future[bool] {.async.} = | |||
g.retryWrapper(currentBlock, "Failed to get the current block number"): | |||
cast[BlockNumber](await g.ethRpc.get().provider.eth_blockNumber()) | |||
|
|||
if g.latestProcessedBlock < currentBlock: | |||
# the node is still able to process messages if it is behind the latest block by a factor of the valid roots | |||
if uint(g.latestProcessedBlock) < uint(currentBlock) - uint(g.validRoots.len): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't harm, i would add it as well :)
Description
This PR modifies the
ready
behaviour of rln-relay's onchain mode. If the node is behind the tip of the blockchain bywindow
blocks, it should still be marked as ready IMO.Changes
ready
if the node is behind the tip of the blockchain bywindow
blocks