fastsync/event: emit fastsync status event when switching consensus/fastsync

[JayT106]

closes #2498
solves part of #3365

Note: difficult to test the event emit in SwitchToFastSync part, might need to change stateSyncReactor to an interface in the nodeImpl struct

[codecov]

Codecov Report

Merging #6619 (ba0e055) into master (2030875) will increase coverage by 0.11%.
The diff coverage is 27.27%.

@@            Coverage Diff             @@
##           master    #6619      +/-   ##
==========================================
+ Coverage   60.87%   60.98%   +0.11%     
==========================================
  Files         297      297              
  Lines       28281    28290       +9     
==========================================
+ Hits        17217    17254      +37     
+ Misses       9328     9301      -27     
+ Partials     1736     1735       -1     

Impacted Files	Coverage Δ
node/node.go	51.18% <0.00%> (-0.25%)	⬇️
internal/consensus/reactor.go	67.53% <33.33%> (+2.59%)	⬆️
types/event_bus.go	66.30% <50.00%> (-0.37%)	⬇️
types/events.go	100.00% <100.00%> (ø)
privval/signer_endpoint.go	75.75% <0.00%> (-6.07%)	⬇️
privval/signer_listener_endpoint.go	87.05% <0.00%> (-2.36%)	⬇️
libs/pubsub/pubsub.go	77.34% <0.00%> (-1.11%)	⬇️
internal/p2p/pex/pex_reactor.go	76.64% <0.00%> (-0.55%)	⬇️
internal/consensus/state.go	67.32% <0.00%> (-0.19%)	⬇️
light/client.go	74.88% <0.00%> (ø)
... and 9 more

[tac0turtle]

@Thunnini does this fit your use case?

[alexanderbez]

Looks great! This event will only be twice AFAICT, so I'm not sure how useful it'll be? At least the initial event won't be that useful I think because it should be immediately triggered. However, when it's finished the event could prove to be useful.

I'd recommend changing the On field name and updating the existing docs to make mention of this new event 👍

[alexanderbez]

@cmwaters do you mind taking a look at this?

[cmwaters]

What is the relation of this to statesync? Do we want to add an event when state sync is completed? Maybe not. I think it's better to not expose the notions of state sync and fast sync to the event consumer but rather to have a single event when Sync is complete. Since we don't currently go back to fast sync after finishing syncing, I think it's fine to emit a single event when switching to consensus rather than two. I also think that the Complete variable is unnecessary.

Honestly, I find this current start up stuff quite messy. Can't wait to reach a better solution to it.

[JayT106]

What is the relation of this to statesync? Do we want to add an event when state sync is completed? Maybe not. I think it's better to not expose the notions of state sync and fast sync to the event consumer but rather to have a single event when Sync is complete. Since we don't currently go back to fast sync after finishing syncing, I think it's fine to emit a single event when switching to consensus rather than two. I also think that the Complete variable is unnecessary.

Honestly, I find this current start up stuff quite messy. Can't wait to reach a better solution to it.

#2498 (comment)
In the original scenario, he needs to know when the fast-sync started. I don't know do we still need to fit the requirement.
If we don't need it anymore, then yap, I can remove the first event and the Complete variable.
@cmwaters @alexanderbez

[alexanderbez]

@JayT106 I don't see anything wrong with the current design. We have an event for when fast sync starts and when it completes, which seems good. We could also have one for state sync. I don't see why we'd want to merge these events, as they're different things. A client can subscribe to both.

[cmwaters]

I don't see why we'd want to merge these events, as they're different things.

My thinking was because a client doesn't care if it's state sync or fast sync they just want to know if a node is "synced" and at the head of the blockchain

[alexanderbez]

I don't think you can safely assume that -- clients may want both, neither, or just one of them.

[cmwaters]

I don't think you can safely assume that -- clients may want both, neither, or just one of them.

I can't see such a distinction providing too much value but what do I know. Do we want to add an event for starting / finishing state sync?

[alexanderbez]

@JayT106 can we also add an event for state sync please?

[JayT106]

@JayT106 can we also add an event for state sync please?
@alexanderbez
To emit an event when the node starts/ends the state sync?

[alexanderbez]

@JayT106 yes -- it can be in a separate PR :)

[JayT106]

@JayT106 yes -- it can be in a separate PR :)
@alexanderbez okay, I will do it separately.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[cmwaters]

Awesome! Let's try merge this