Improve ReceiptsDownloader: replace timeout with request counter

[mkalinin]

The logic worth to be implemented can be found in BlockDownloader

[dhingra]

@mkalinin Can I pick this one?

[mkalinin]

@dhingra sure! feel free to ask me if anything unclear will be found

[dhingra]

@mkalinin Few questions:

1. When downloading headers, use a predefined counter to download, number of "max Blocks / Headers " in one request ?

Can you please elaborate little more on requirement?

thanks

[JonathanScialpi]

Hi @mkalinin

I submitted a pull request for this enhancement. I hope this is what was required but if not, feel free to reach out to me as I don't mind working on it more and I am eager to learn.

[mkalinin]

Current implementation

1. If download queue is empty ReceiptsDownloader tries to fill it.
2. If nothing new added (which in most of the cases means that nothing has been downloaded yet) then wait for REQUEST_TIMEOUT and re-initialize downloader queue.
3. Otherwise (if there is something new), re-initialize downloader queue immediately.

Above steps describe this piece of code

ethereumj/ethereumj-core/src/main/java/org/ethereum/sync/ReceiptsDownloader.java

Lines 170 to 175 in 7e3b9dd

	if (toDownload.isEmpty()) {
	if (fillBlockQueue() > 0 || System.currentTimeMillis() - t > REQUEST_TIMEOUT) {
	toDownload = getToDownload();
	t = System.currentTimeMillis();
	}
	}

Proposed improvement

An improvement should update step 2 in a following way:
2. If nothing new added then wait for REQUEST_TIMEOUT OR getting the responses to all sent requests, which event would happen first, and re-initialize downloader queue.

[JonathanScialpi]

@mkalinin thank you for the clarification. I will work on this and get back to you with an updated PR.

[JonathanScialpi]

For this enhancement I created counters requestedBlocksCounter and receivedBlocksCounter

Commit Link

My thought process being that each time we create a callback/request we will increment requestedBlocksCounter.

Upon a successful response back we will increment receivedBlocksCounter.

Whenever we had previously requested in the past (requestedBlocksCounter > 0) AND the value also matches the receivedBlocksCounter, we can skip the REQUEST_TIMEOUT and reinitialize the download queue.

The reason why I didn't use one counter to keep track (by increment/decrement) is because we intialize the counter to 0 and we would also skip the REQUEST_TIMEOUT on the first loop. Comparing two counters help to prevent this.

@mkalinin Does this pull request / logic make more sense? If not, how could I improve it? Thank you for your patience.

[mkalinin]

I see your point. But some of requests may never be responded. Since one of such requests occurs there will be no way that this specific logic with counters works, each time it will fallback to the old way with timeout. So, counter should be reset from time to time. And there is no way to find an algo which would work in all cases, and we don't need it, actually. Just need a solution that works in most of the cases.

[JonathanScialpi]

@mkalinin Okay how about we use the size of toDownload() as the counter?

This way it will always start with the correct size and eventually get reset to 0 as we removed requests from the list.

The first time its empty we will not have to wait for the timeout.
Then when the size > 0 we will either reinitialize the Download (if we have something new) or just wait on the timeout.
After we looped enough that the size is 0, then we can skip the REQUEST_TIMEOUT.

Like this:

if (toDownload.isEmpty()) { if (fillBlockQueue() > 0 || System.currentTimeMillis() - t > REQUEST_TIMEOUT || toDownload.size() == 0) { toDownload = getToDownload(); t = System.currentTimeMillis(); } }