feat(p2p/nat): limit UPNP request concurrency #21390#2426
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bbc869b to
542fe53
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the UPnP NAT implementation to better handle routers that misbehave under concurrent SOAP requests by serializing and rate-limiting requests, and adjusts port-mapping refresh timing to avoid overlapping/duplicate mappings.
Changes:
- Add per-UPnP-instance request serialization and a 200ms minimum delay between SOAP requests (
withRateLimit), and route UPnP client calls through it. - Change UPnP discovery to check NAT enablement via a rate-limited helper (
natEnabled) and refactor the matcher callback signature. - Adjust
nat.Maprefresh scheduling to renew mappings exactly at the mapping timeout (but also changes the timeout value).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| p2p/nat/natupnp.go | Serializes and rate-limits UPnP SOAP requests; refactors discovery and NAT status check. |
| p2p/nat/nat.go | Changes mapping refresh logic to align renewal with the timeout; modifies Map channel type. |
542fe53 to
f500f88
Compare
This adds a lock around requests because some routers can't handle concurrent requests. Requests are also rate-limited. The Map function request a new mapping exactly when the map timeout occurs instead of 5 minutes earlier. This should prevent duplicate mappings.
f500f88 to
2a3d79c
Compare
Proposed changes
This adds a lock around requests because some routers can't handle concurrent requests. Requests are also rate-limited.
The Map function request a new mapping exactly when the map timeout occurs instead of 5 minutes earlier. This should prevent duplicate mappings.
Ref: ethereum#21390
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that