Accept empty header set in range headers validation #4189
Conversation
If a response to the get header P2P request only returns the header that is the start of the range we may need to trim it to an empty response, this is breaking the validation response. One client has started returning only the range header start in some circumstances (which is a valid response per spec). Because we sometimes request an overlapping header this results in an empty stream once we cut the duplicates. Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
| @@ -50,7 +41,7 @@ public List<BlockHeader> getHeadersToImport() { | |||
| } | |||
|
|
|||
| public BlockHeader getFirstHeaderToImport() { | |||
| return headersToImport.get(0); | |||
| return headersToImport.size() > 0 ? headersToImport.get(0) : null; | |||
There was a problem hiding this comment.
Making this return Optional might be a bit more helpful to the reader, and would lend itself to simpler conversion to an empty stream later on.
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
I see what you did there. |
| if (headersToImport.isEmpty()) { | ||
| LOG.debug(String.format("Headers list empty. Range: %s", checkpointRange.toString())); | ||
| } | ||
| checkArgument(!headersToImport.isEmpty(), "Must have at least one header to import"); |
There was a problem hiding this comment.
Only question here is if this changes the sync process in any meaningful way - could this cause sync to hang, for example, if we have a peer that is repeatedly returning nothing? It seems that previously we would throw here, and I'm guessing the sync process would be interrupted because of the exception.
There was a problem hiding this comment.
Throwing the exception did not disconnect the peer, so we still had the peer.
But per spec (a) an empty response was 100% fine and (b) Erigon's response was not empty, we just trimmed off the one value we already had. So there are two routes we may be passing in an empty resposne down the pipeline and both cases the peer is behaving validly, so disconnecting doesn't seem right.
There was a problem hiding this comment.
Not saying we should disconnect, just want to make sure we're not introducing anything that could cause the sync to stall / get stuck. But we handle useless responses elsewhere, so even if we got stuck asking for the same range of headers from a single unresponsive peer, I think we would eventually disconnect and move on ...
There was a problem hiding this comment.
I haven't seen any pauses. Rather than breaking the pipe an empty stream is the result, and that would get aggregated the same as a broken step, as nothing gets added to the next set of object for the next step in the pipeline. I haven't seen any slowdown overnight and I started a fresh sync and it's proceeding the same as before with no observable differences.
Side note: I'm really looking forward to being able to bit-torrent the blocks prior to the merge. EIP-4444 and friends will be welcomed with open arms.
|
should fix #3336 |
If a response to the get header P2P request only returns the header that is the start of the range we may need to trim it to an empty response, this is breaking the validation response. One client has started returning only the range header start in some circumstances (which is a valid response per spec). Because we sometimes request an overlapping header this results in an empty stream once we cut the duplicates. fixes hyperledger#3336 Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
PR description
If a response to the get header P2P request only returns the header that
is the start of the range we may need to trim it to an empty response,
this is breaking the validation response. One client has started
returning only the range header start in some circumstances (which is a
valid response per spec). Because we sometimes request an overlapping
header this results in an empty stream once we cut the duplicates.
Signed-off-by: Danno Ferrin danno.ferrin@gmail.com
Fixed Issue(s)
Documentation
doc-change-requiredlabel to this PR ifupdates are required.
Changelog