fix(rpc): Fix status CatchingUp field updating
#971
Conversation
rpc/client/client.go
Outdated
| @@ -751,13 +748,13 @@ func (c *Client) Status(ctx context.Context) (*ctypes.ResultStatus, error) { | |||
| LatestAppHash: latestAppHash[:], | |||
| LatestBlockHeight: int64(latestHeight), | |||
| LatestBlockTime: time.Unix(0, int64(latestBlockTimeNano)), | |||
| CatchingUp: c.node.BlockManager.TargetHeight.Load() > latestHeight, | |||
There was a problem hiding this comment.
u need to set TargetHeight also in syncBlockManager , currnetly u made it event based only
There was a problem hiding this comment.
this field needs a docstring as well IMO
the other ones are obvious and fine
block/manager.go
Outdated
| @@ -64,6 +64,7 @@ type Manager struct { | |||
| // It is ALSO used by the producer, because the producer needs to check if it can prune blocks and it wont' | |||
| // prune anything that might be submitted in the future. Therefore, it must be atomic. | |||
| LastSubmittedHeight atomic.Uint64 | |||
| TargetHeight atomic.Uint64 | |||
There was a problem hiding this comment.
I think it's redundent. both LastSubmittedHeight and TargetHeight are actually LatestHeightOnSL
block/sync.go
Outdated
| @@ -42,6 +42,7 @@ func (m *Manager) SyncToTargetHeightLoop(ctx context.Context) (err error) { | |||
| continue | |||
| } | |||
| types.RollappHubHeightGauge.Set(float64(h)) | |||
| m.LastSubmittedHeight.Store(h) | |||
There was a problem hiding this comment.
I think that name is confusing.
as a node which is catching up you're most likely didn't submit any height. I guess you mean here last settlement height, cause this is only for heights recieved by the DA - However, you can have also heights recieved by p2p (most likely this will be the case for syncing) which you don't capture currently.
I think the correct way to go around it is:
- name it
LastSeenHeight - update it on all sync options (da, p2p)
rpc/client/client.go
Outdated
| @@ -751,13 +748,13 @@ func (c *Client) Status(ctx context.Context) (*ctypes.ResultStatus, error) { | |||
| LatestAppHash: latestAppHash[:], | |||
| LatestBlockHeight: int64(latestHeight), | |||
| LatestBlockTime: time.Unix(0, int64(latestBlockTimeNano)), | |||
| CatchingUp: c.node.BlockManager.TargetHeight.Load() > latestHeight, | |||
There was a problem hiding this comment.
this field needs a docstring as well IMO
the other ones are obvious and fine
(cherry picked from commit 402dd00)
PR Standards
Opening a pull request should be able to meet the following requirements
--
PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A
Close #935
Basically, the idea is to have the CatchingUp value be derived as such:
CatchingUp = targetHeight > latestHeightMy guess is that there might need to be some tolerance there, in case a new settlement batch is submitted, and at the same moment the
/statusis queried.For Author:
godoccommentsFor Reviewer:
After reviewer approval: