feat: limit per peer per block concurrent requests#1773
Merged
rach-id merged 49 commits intofeature/recoveryfrom Apr 29, 2025
Merged
feat: limit per peer per block concurrent requests#1773rach-id merged 49 commits intofeature/recoveryfrom
rach-id merged 49 commits intofeature/recoveryfrom
Conversation
rach-id
requested review from
cmwaters and
rootulp
and removed request for
a team
rach-id
requested review from
Copilot and
evan-forbes
and removed request for
cmwaters and
rootulp
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new mechanism to limit per-peer per-block concurrent requests, refactoring the peer state initialization and request batching logic. Key changes include:
- Simplifying the global variable declaration for RetryTime.
- Adding request-related channels and counters to the peer state.
- Updating the flow for handling haves and want requests with a new concurrent request limit and batching logic.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| consensus/propagation/reactor.go | Refactored AddPeer to initialize peer state, start a new sending routine, and adjust the order of processing the compact block. |
| consensus/propagation/peer_state.go | Introduced requestChan, receivedPart channel, and atomic request counter helper functions. |
| consensus/propagation/have_wants.go | Modified logging levels for unknown proposals, implemented per-peer request limiting and batch size calculation, and updated request sending logic. |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a mechanism to limit per-peer per-block concurrent requests in order to prevent one peer from overwhelming the network with requests. Key changes include:
- Refactoring peer initialization and launching a dedicated requests sending routine.
- Adding atomic counters and channels to manage request state in peer_state.go.
- Updating request batching logic in have_wants.go to support controlled concurrent requests.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| consensus/propagation/reactor.go | Reordered peer state setup and added a dedicated sending routine for requests. |
| consensus/propagation/peer_state.go | Introduced atomic counter management and new functions for request count adjustments. |
| consensus/propagation/have_wants.go | Modified request logic to batch part requests and switch error logging from error to debug. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
evan-forbes
reviewed
Apr 25, 2025
rach-id
force-pushed
the
rachid/recovery/limit-concurrent-peer-limit
branch
from
d631fae to
2d14c90
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements a per-peer per-block request concurrency limit by tracking and limiting the number of concurrent part requests, and it adjusts the behavior of part request batching and propagation. Key changes include updating comments to reflect parity parts, refactoring global variables and peer state routines to start new sending routines for peers, and adding new functions and tests to manage and validate request count operations.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| consensus/propagation/types/types.go | Updated comment to indicate inclusion of parity part set hashes |
| consensus/propagation/reactor.go | Refactored global variable initialization; updated peer handling routines and channel closures |
| consensus/propagation/peer_state.go | Added atomic request count functions for increasing, setting, and decreasing request counts |
| consensus/propagation/peer_state_test.go | New tests for the request count operations |
| consensus/propagation/have_wants.go | Introduced per-peer request limit and updated want sending logic |
| consensus/propagation/have_want_test.go | Added tests covering want and have propagation under concurrent limits |
| consensus/propagation/catchup_test.go | Updated test configuration and helper function for creating compact blocks |
Closes #1777 #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments
…gn that requires zero timeouts (#1778) submitting a quick PR to reduce review rounds 🤞
evan-forbes
approved these changes
Apr 29, 2025
Closes #1776 #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments
rach-id
added a commit
that referenced
this pull request
May 21, 2025
#### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments