fix: correctly poll BasicBlockDownloader

[klkvr]

Right now if we schedule block download, on next poll of BasicBlockDownloader we'll return Poll:Ready with NewDownloadStarted event and never register waker with actual downloading future.

Found this to be an issue when writing e2e tests, not sure if can occur during normal node operation

[mattsse]

we'll return Poll:Ready with NewDownloadStarted event and never register waker with actual downloading future.

hmm, this is expected because a ready indicates that there can be more work and the caller is supposed to call it again

which I think we do:

reth/crates/engine/tree/src/engine.rs

Lines 115 to 120 in 5532d34

	// advance the downloader
	if let Poll::Ready(DownloadOutcome::Blocks(blocks)) = self.downloader.poll(cx) {
	// delegate the downloaded blocks to the handler
	self.handler.on_event(FromEngine::DownloadedBlocks(blocks));
	continue
	}

Found this to be an issue when writing e2e tests, not sure if can occur during normal node operation
which issue did you encounter?

[klkvr]

ah, then I think the issue is in this line:

reth/crates/engine/tree/src/engine.rs

Line 116 in 5532d34

if let Poll::Ready(DownloadOutcome::Blocks(blocks)) = self.downloader.poll(cx) {

if we get Poll::Ready with any other DownloadOutcome variant we won't poll it again

[klkvr]

the issue I've seen is that block fetching future was not being polled

[klkvr]

Updated logic to instead make sure that we drain downloader events in EngineHandler

[mattsse]

ah, yes of course lol

initial I wanted to bubble up the newdownload started event, but came to the conclusion that this isn't very useful