Make sure background sync is correctly started for chains being listened to #5188
Conversation
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
deuszx
left a comment
deuszx
left a comment
There was a problem hiding this comment.
I can see how this will start a chain listener if we add new chains but what if we change chains we already listen to? from FullChain to FollowOnly and vice versa?
|
Hmm, fair point. I mean - if we change to BTW, it's currently not possible to switch listening to a "lower" mode - the assumption being that if there are two separate reasons for listening, and one implies a "higher" mode than the other, we should just be listening in the "higher" mode of the two. More sophisticated management of the listening tasks would require a lot more work, I'm afraid 😬 |
| mut self, | ||
| enable_background_sync: bool, | ||
| ) -> Result<impl Future<Output = Result<(), Error>>, Error> { | ||
| pub async fn run(mut self) -> Result<impl Future<Output = Result<(), Error>>, Error> { |
There was a problem hiding this comment.
Yes, because background sync for a chain can now be started at a point different than when the whole Chain Listener is started - so we need to remember the setting in the struct itself, to know whether we want to start background sync for chains or not.
There was a problem hiding this comment.
This sounds wrong – i.e. we decide to spawn the background sync at a different point in time than when we decide whether we (will) want to run it. Shouldn't those two be done at the same time?
There was a problem hiding this comment.
But background sync is per chain, and we may want to start listening to a chain at any point while the listener (as in, the component) is running. So we initialize the listener at startup, but then we might want to start to listen to some chain much later, and if background sync is enabled, we will start the background sync for that chain then.
| if !self.enable_background_sync { | ||
| return None; | ||
| } | ||
| if mode != ListeningMode::FullChain { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
We could add some logging for the less obvious cases:
enable_background_sync == truebutmode != ListeningMode::FullChainenable_background_sync == falsebutmode == ListeningMode::FullChain
🤔
Actually, how could we end up with FullChain listening mode but disabled background sync?
There was a problem hiding this comment.
I think we enable background sync in all production scenarios, anyway. This setting only exists to make it possible to disable starting background sync tasks in unit tests, AFAIK.
There was a problem hiding this comment.
Can we add some logging regardless?
There was a problem hiding this comment.
I'm not sure if it's that useful - because both cases will be pretty common. The first one will pop up in production when listening to sparse event chains, or with followed chains. The second one will be everywhere in unit tests, where we disable background sync for convenience. Neither case is unusual in any way (but I can of course add some debug or maybe even trace logs).
3 participants
Motivation
Node service plugins introduced an interface in the
ChainListenerthat allows contracts to request listening to chains. However, the background sync task wasn't started for chains being listened to due to such requests.Proposal
Make sure background sync is started if there is a request to listen in the
FullChainmode.Test Plan
CI will catch regressions
Release Plan
testnetbranch, thenLinks