feat(network)!: regulate GetBlocks with paired streams - #892
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
b24ab05 to
85401ce
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
one thing we have to think about when merging to main is that if we cut a releases imbetween work here we'll have to update the blocksync / headersync / discovery versions a few times which is cumbersome |
| /// charges until the underlying state work finishes. | ||
| #[serde(with = "humantime_serde")] | ||
| pub query_timeout: Duration, | ||
| } |
There was a problem hiding this comment.
one of the ideas in 747 was to reuse a lot of these logic instead of making one per message
not sure if we can do this for everything but many of these seem like they could be generalized
I'm biased, and not sure how feasible it is irl, but the idea of defining the message then adding and configuring the functions then and there seems nice
There was a problem hiding this comment.
This is now reduced to the following config:
pub struct GetBlocksRegulationConfig {
pub node_active_requests: usize,
pub query_timeout: Duration,
}
Because we might want these to differ per message.
There was a problem hiding this comment.
I left this unresolved because I could easily be convinced that maybe we just keep these global as well.
czarcas7ic
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11beaec168
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| timeout(write_timeout, send.write_all(&frame)) | ||
| .await | ||
| .map_err(|_| -> BoxError { "Zakura outbound frame write timed out".into() })??; |
There was a problem hiding this comment.
When the block-sync data stream remains flow-controlled for 32 seconds—for example, because its receiver is paused while header-sync or gossip siblings are still healthy—this timeout becomes a generic write error. persistent_stream_worker_with_policy handles that error by calling context.connection_token.cancel() at lines 4301-4310, so the entire QUIC connection and every unrelated service stream are dropped instead of only retiring and reopening the paired block-sync session. Classify paired data timeouts separately and reset/cancel only the pair.
Useful? React with 👍 / 👎.
| }); | ||
| } | ||
|
|
||
| pub(super) fn trace_queue_send_failed(&self, msg: &BlockSyncMessage, error: &OrderedSendError) { |
There was a problem hiding this comment.
Before we would reserve download resources, then try to queue the request, then if failed we would emit this and release the resources. Now, we reserve a queue slot first here
and if the queue is full, we stop before taking work or reserving download resources.
2 participants
Motivation
This draft is retained as the reference for the replacement stack: #942 (owned storage), #943 (generic paired transport), #944 (outgoing request ownership), and #945 (serving activation). The initial stack preserved this PR at
953b85b18; #941 was later closed and its backport removed from the replacement drafts in favor of the separate Iroh upgrade in #935. This reference still records the original backport-based implementation. See the audit map.Solution
Use paired native block-sync streams and sequential serving, with one response slot per authenticated identity and 64 per node. Retain capacity through storage, results, encodes, and writes across cancellation and reconnects. Settle outgoing requests across publication, expiry, reset, and writer startup. Preserve cooldowns for unanswered remote resets and defer session-capacity races without closing healthy connections. The previous native layout is replaced while other services and legacy fallback remain available. Property coverage remains separate in #896.
Testing
The latest two fixes pass 29 targeted regressions and network Clippy. Before withdrawing the backport, the identical completed stack passed 405 focused tests, 22 cluster integration tests without retries, and workspace all-target Clippy, formatting, Markdown lint, and changelog checks. Those results do not validate the replacement stack with the separate Iroh upgrade; activation needs that integration check.
Earlier long transport results and their limitations remain in the validation evidence. Those long gates were not rerun. The earlier full workspace run had three unavailable loopback source addresses and two legacy mainnet-sync timeouts; it was not repeated for this split.
Changelog
The combined fragment remains here; each replacement draft has its own fragment. Used Codex for implementation, tests, and documentation.