Handle sync lookup request streams in network context #5583
Conversation
dapplion
force-pushed
the
handle-sync-lookup-requests
branch
from
38d21f0
to
2ef22b3
Compare
…ndle-sync-lookup-requests
realbigsean
added
under-review
|
|
||
| let blocks = self.add_child_block_to_chain(chain_hash, blocks, cx).into(); | ||
|
|
||
| let process_id = ChainSegmentProcessId::ParentLookup(chain_hash); |
There was a problem hiding this comment.
Is the ordering change here meant to drop the child lookup when the beacon processor is not available? makes sense
There was a problem hiding this comment.
The change is motivated by the borrow checker, but as you say the change also makes sense to drop the lookup completely on that case
realbigsean
added
waiting-on-author
| } | ||
| } | ||
| } | ||
| RpcEvent::StreamTermination => match request.remove().terminate() { |
There was a problem hiding this comment.
Do we need to handle the scenario where a peer doesn't send stream termination? (ActiveBlocksByRootRequest stored is quite small in size though)
There was a problem hiding this comment.
Current sync codebase expects the stream termination to always appear when syncing or doing blob lookups. The API contract with network's ReqResp behaviour is that you always get either an error or stream termination. There are timeouts in place in case the peer leaves the stream open and doesn't send the termination.
* add bad state warn log * add rust docs to new fields in `SyncNetworkContext` * remove timestamp todo * add back lookup verify error * remove TODOs
realbigsean
added
ready-for-merge
|
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at f7aca97 |
Issue Addressed
Part of
Currently the sync lookup request states deal with tracking streams, retries and their own sync logic (parent chains, etc). The complexities of streams can be abstracted up into the network context to simplify downstream code.
This abstraction would have avoided the previous month bug regarding
ExtraBlocksProposed Changes
Adopt the same accumulator pattern used today for range blocks + blobs for lookup requests. Allow to merge error handling into the same codepath and remove the concept of response validation from the RequestState trait.
In draft to wait for #5563