Extract block announce validation from ChainSync
#14675
Conversation
dmitry-markin
added
A0-please_review
dmitry-markin
changed the title
ChainSync to SyncingEngineChainSync
| /// Wake-up event when new validations are pushed. | ||
| event: Event, | ||
| /// Listener for wake-up events in [`Stream::poll_next`] implementation. | ||
| event_listener: Option<EventListener>, |
There was a problem hiding this comment.
Could you explain why this is needed? I read your comment and the stream is not getting woken up but why is it necessary to add this machinery to make it wake up?
By the looks of it, a block announcement is pushed if we receive a notification and at the end of SyncingEngine::poll(), the block announcement stream is polled so by my reasoning the newly added announcement should be polled as well (albeit not explicitly but along with any other block announcement) but this is not the case, why?
There was a problem hiding this comment.
You are correct, in our implementation of SyncingEngine::poll() futures unordered are always polled after pushing a block announcement. But I decided to make BlockAnnounceValidator stream future-proof by ensuring the task is woken up even if the block announcements are pushed from somewhere else, and not before polling the stream. We can eliminate this and simplify the implementation, but then we would need to write warnings in the comments stating that the BlockAnnounceValidator stream must be explicitly polled after pushing a block announcement.
There was a problem hiding this comment.
By the way, looking at it again I think there is an issue with the implementation. In case some futures are already added to self.validations, and we add one more, the task won't be woken up and this new future won't be polled until some other future wakes the task up.
There was a problem hiding this comment.
Would any of this be an issue in the first place of SyncingEngine would use tokio::select! for polling all of its futures simultaneously? That's what we ultimately want and AFAIU it would also give the possibility of pushing a block announcement anywhere in the code and still getting polled.
There was a problem hiding this comment.
Yes, in case we use proper .await and select! this can introduce an unneeded latency in block announce validation: the newly added block announcement might not be polled until an already added one resolves. I will look what can be done with this.
There was a problem hiding this comment.
the newly added block announcement might not be polled until an already added one resolves
I don't understand this. If the block announcements are stored in FuturesUnordered and they're getting polled as a group, I don't understand why some block announcement wouldn't get polled unless another one resolved. Am I overlooking some detail in the implementation?
There was a problem hiding this comment.
Yes, this is the property of FuturesUnordered:
When new futures are added, poll_next must be called in order to begin receiving wake-ups for new futures.
So, if there are some futures already and the task is waiting, FuturesUnordered::poll_next might not be called until some of them wakes the task up. Only after that the newly added future will be registered in the task.
There was a problem hiding this comment.
@altonen Could you take a look again, please? Now the code must be ready for pushing block announcements from a different place and just select!-ing on block_announce_validator.next() in SyncingEngine.
There was a problem hiding this comment.
I'm not super happy with this approach either because I feel as though it complicates a relatively simple problem (push a future to FuturesUnordered and poll that container to get next ready future) but I understand that with the current architecture in SyncingEngine it may not be possible. The code itself looks fine but I still think we can improve this polling in the future so could you create an issue for it?
This PR probably has to be migrated to polkadot-sdk since Anton's review has gone stale
Co-authored-by: Aaro Altonen <[email protected]> Co-authored-by: Anton <[email protected]>
…m somewhere else
This PR is part of Sync 2.0 preparation work. Resolves paritytech/polkadot-sdk#503.
polkadot companion: paritytech/polkadot#7569
cumulus companion: paritytech/cumulus#2959