-
Notifications
You must be signed in to change notification settings - Fork 51
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): add isReady check #1989
Changes from 2 commits
b7c71b9
cbb266a
3c9d4be
15bfde4
b058c97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
const DefaultKeyStorePath* = "rlnKeystore.json" | ||
const DefaultKeyStorePassword* = "password" | ||
|
@@ -313,13 +315,18 @@ proc getAndHandleEvents(g: OnchainGroupManager, | |
fromBlock: BlockNumber, | ||
toBlock: Option[BlockNumber] = none(BlockNumber)): Future[void] {.async.} = | ||
initializedGuard(g) | ||
proc getLatestBlockNumber(): BlockNumber = | ||
if toBlock.isSome(): | ||
return toBlock.get() | ||
return fromBlock | ||
|
||
let blockTable = await g.getBlockTable(fromBlock, toBlock) | ||
if blockTable.len() > 0: | ||
g.lastSeenBlockHead = getLatestBlockNumber() | ||
await g.handleEvents(blockTable) | ||
await g.handleRemovedEvents(blockTable) | ||
|
||
g.latestProcessedBlock = if toBlock.isSome(): toBlock.get() | ||
else: fromBlock | ||
g.latestProcessedBlock = getLatestBlockNumber() | ||
let metadataSetRes = g.setMetadata() | ||
if metadataSetRes.isErr(): | ||
# this is not a fatal error, hence we don't raise an exception | ||
|
@@ -528,3 +535,27 @@ method stop*(g: OnchainGroupManager): Future[void] {.async.} = | |
error "failed to flush to the tree db" | ||
|
||
g.initialized = false | ||
|
||
proc isSyncing*(g: OnchainGroupManager): Future[bool] {.async,gcsafe.} = | ||
let ethRpc = g.ethRpc.get() | ||
|
||
try: | ||
let syncing = await ethRpc.provider.eth_syncing() | ||
return syncing.getBool() | ||
except CatchableError: | ||
error "failed to get the syncing status", error = getCurrentExceptionMsg() | ||
return false | ||
|
||
method isReady*(g: OnchainGroupManager): Future[bool] {.async,gcsafe.} = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps its cleaner to run the checks for
maybe just a personal prefernce, just a sugerence. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I did think of this, imo the way it is right now is slightly more readable |
||
initializedGuard(g) | ||
|
||
if g.ethRpc.isNone(): | ||
return false | ||
|
||
if g.lastSeenBlockHead == 0: | ||
return false | ||
|
||
if g.latestProcessedBlock < g.lastSeenBlockHead: | ||
return false | ||
|
||
return not (await g.isSyncing()) |
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.
not sure i understand the need of this variable, and perhaps there is an edge case?
afaiu we sync in batches:
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.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.
right, I'll just move it to the newHeadCallback, thanks
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.
Addressed in 3c9d4be
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 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.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.
Addressed in 15bfde4