fix(p2p/pex): respect MaxNumOutboundPeers limit while dialing peers provided by a seed node by cason · Pull Request #3360 · cometbft/cometbft

cason · 2024-06-28T10:10:52Z

Closes #486.

Commits:

Make TestConnectionSpeedForPeerReceivedFromSeed to fail, as it will make the node to connect to more than the configured MaxNumOutboundPeers
Solve the issue by introducing a channel to wake up ensurePeersRoutine(). In this way, the node will not to wait until the next defaultEnsurePeersPeriod (30s) before attempting again to connect to new peers (ensurePeers() method) whose addresses were provide by the seed node.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

melekes

👍

p2p/pex/pex_reactor.go

melekes · 2024-06-28T12:22:54Z

Not sure whether we should backport this PR. It is not API breaking, but it changes the way that new nodes connect to peers. For instance, the number of peers of nodes in our QA experiments will change with this PR, as all nodes, except seed nodes, will have at most 10 outbound connections and at most 50 peers in total.

But the previous behavior is unintended, isn't it? If so, I think we should backport the fix.

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

cason · 2024-06-28T15:42:30Z

Yes, it was unintended.

This also needs a changelog, which folder should I use?

cason · 2024-06-28T15:45:51Z

Another point is that the removed code was also used when the node is in seed mode. In this case, it runs the crawlPeersRoutine() instead of the ensurePeersRoutine(). I can add the same logic in the crawlPeersRoutine() routine, although no test is failing at the moment.

Actually, when running in seed mode, apparently there is no maximum number of connections or stuff like that.

czarcas7ic

Thanks for addressing this so quickly, implementation looks great :)

sergio-mena

LGTM 👍

…rovided by a seed node (#3360) Closes #486. Commits: 1. Make `TestConnectionSpeedForPeerReceivedFromSeed` to fail, as it will make the node to connect to more than the configured `MaxNumOutboundPeers` 2. Solve the issue by introducing a channel to wake up `ensurePeersRoutine()`. In this way, the node will not to wait until the next `defaultEnsurePeersPeriod` (30s) before attempting again to connect to new peers (`ensurePeers()` method) whose addresses were provide by the seed node. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit 4241776) # Conflicts: # p2p/pex/pex_reactor.go

…rovided by a seed node (#3360) Closes #486. Commits: 1. Make `TestConnectionSpeedForPeerReceivedFromSeed` to fail, as it will make the node to connect to more than the configured `MaxNumOutboundPeers` 2. Solve the issue by introducing a channel to wake up `ensurePeersRoutine()`. In this way, the node will not to wait until the next `defaultEnsurePeersPeriod` (30s) before attempting again to connect to new peers (`ensurePeers()` method) whose addresses were provide by the seed node. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit 4241776) # Conflicts: # .changelog/v0.38.3/bug-fixes/486-p2p-max-outbound.md # p2p/pex/pex_reactor.go # p2p/pex/pex_reactor_test.go

…rovided by a seed node (#3360) Closes #486. Commits: 1. Make `TestConnectionSpeedForPeerReceivedFromSeed` to fail, as it will make the node to connect to more than the configured `MaxNumOutboundPeers` 2. Solve the issue by introducing a channel to wake up `ensurePeersRoutine()`. In this way, the node will not to wait until the next `defaultEnsurePeersPeriod` (30s) before attempting again to connect to new peers (`ensurePeers()` method) whose addresses were provide by the seed node. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit 4241776) # Conflicts: # p2p/pex/pex_reactor.go # p2p/pex/pex_reactor_test.go

…rovided by a seed node (backport #3360) (#3397) Closes #486. Commits: 1. Make `TestConnectionSpeedForPeerReceivedFromSeed` to fail, as it will make the node to connect to more than the configured `MaxNumOutboundPeers` 2. Solve the issue by introducing a channel to wake up `ensurePeersRoutine()`. In this way, the node will not to wait until the next `defaultEnsurePeersPeriod` (30s) before attempting again to connect to new peers (`ensurePeers()` method) whose addresses were provide by the seed node. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3360 done by [Mergify](https://mergify.com). --------- Co-authored-by: Daniel <daniel.cason@informal.systems>

…rovided by a seed node (backport #3360) (#3396) Closes #486. Commits: 1. Make `TestConnectionSpeedForPeerReceivedFromSeed` to fail, as it will make the node to connect to more than the configured `MaxNumOutboundPeers` 2. Solve the issue by introducing a channel to wake up `ensurePeersRoutine()`. In this way, the node will not to wait until the next `defaultEnsurePeersPeriod` (30s) before attempting again to connect to new peers (`ensurePeers()` method) whose addresses were provide by the seed node. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3360 done by [Mergify](https://mergify.com). --------- Co-authored-by: Daniel <daniel.cason@informal.systems>

…rovided by a seed node (backport #3360) (#3395) Closes #486. Commits: 1. Make `TestConnectionSpeedForPeerReceivedFromSeed` to fail, as it will make the node to connect to more than the configured `MaxNumOutboundPeers` 2. Solve the issue by introducing a channel to wake up `ensurePeersRoutine()`. In this way, the node will not to wait until the next `defaultEnsurePeersPeriod` (30s) before attempting again to connect to new peers (`ensurePeers()` method) whose addresses were provide by the seed node. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3360 done by [Mergify](https://mergify.com). --------- Co-authored-by: Daniel <daniel.cason@informal.systems>

Solves #4620. Fixes an issue introduced by #3360. In short, when receiving addresses from a configured seed node, the peer immediately dials the received addresses, without waiting for the `defaultEnsurePeersPeriod` (30s). This is a desired behavior, the "fast dial mode" in the title. However, for preventing abuse, a node only accepts PEX requests from a peer every `minReceiveRequestInterval()` time, set to `defaultEnsurePeersPeriod/3` (10s). When running this "fast dial mode", however, a PEX request can be send, in some unlucky setup, to the same peer without waiting for the full defaultEnsurePeersPeriod` (30s). The problem is that at the receive side, a node keeps track of the latest PEX request received from each peer. If two requests are received with an interval lower than `minReceiveRequestInterval()`, the peer is considered abusive: it is disconnected with an `ErrReceivedPEXRequestTooSoon` error and banned from the address book. This PR proposes a workaround to prevent the above mentioned scenario. --- #### 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: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Solves #4620. Fixes an issue introduced by #3360. In short, when receiving addresses from a configured seed node, the peer immediately dials the received addresses, without waiting for the `defaultEnsurePeersPeriod` (30s). This is a desired behavior, the "fast dial mode" in the title. However, for preventing abuse, a node only accepts PEX requests from a peer every `minReceiveRequestInterval()` time, set to `defaultEnsurePeersPeriod/3` (10s). When running this "fast dial mode", however, a PEX request can be send, in some unlucky setup, to the same peer without waiting for the full defaultEnsurePeersPeriod` (30s). The problem is that at the receive side, a node keeps track of the latest PEX request received from each peer. If two requests are received with an interval lower than `minReceiveRequestInterval()`, the peer is considered abusive: it is disconnected with an `ErrReceivedPEXRequestTooSoon` error and banned from the address book. This PR proposes a workaround to prevent the above mentioned scenario. --- #### 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: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 491379f) # Conflicts: # p2p/pex/pex_reactor.go

Solves #4620. Fixes an issue introduced by #3360. In short, when receiving addresses from a configured seed node, the peer immediately dials the received addresses, without waiting for the `defaultEnsurePeersPeriod` (30s). This is a desired behavior, the "fast dial mode" in the title. However, for preventing abuse, a node only accepts PEX requests from a peer every `minReceiveRequestInterval()` time, set to `defaultEnsurePeersPeriod/3` (10s). When running this "fast dial mode", however, a PEX request can be send, in some unlucky setup, to the same peer without waiting for the full defaultEnsurePeersPeriod` (30s). The problem is that at the receive side, a node keeps track of the latest PEX request received from each peer. If two requests are received with an interval lower than `minReceiveRequestInterval()`, the peer is considered abusive: it is disconnected with an `ErrReceivedPEXRequestTooSoon` error and banned from the address book. This PR proposes a workaround to prevent the above mentioned scenario. --- #### 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: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 491379f)

…) (#4649) Solves #4620. Fixes an issue introduced by #3360. --------- Co-authored-by: Daniel <daniel.cason@informal.systems> Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

…) (#4648) Solves #4620. Fixes an issue introduced by #3360. Co-authored-by: Daniel <daniel.cason@informal.systems>

p2p/pex: make test to fail, in line with issue #486

7366692

cason changed the title ~~p2p/pex: respect MaxNumOutboundPeers while dialing peers provided by a seed node~~ fix(p2p/pex): respect MaxNumOutboundPeers while dialing peers provided by a seed node Jun 28, 2024

Daniel Cason added 2 commits June 28, 2024 12:36

p2p/pex: make test to fail in CI too (issue #486)

a8b708c

p2p/pex: ensurePeersCh wakes ensurePeersRoutine() up

b72bc6c

cason force-pushed the cason/486-seed-addresses branch 2 times, most recently from b72bc6c to a8b708c Compare June 28, 2024 10:37

Daniel Cason added 2 commits June 28, 2024 12:39

Merge branch 'main' into cason/486-seed-addresses

f6f57e6

p2p/pex: refactor code to dial to new peers asap

3d114e2

cason self-assigned this Jun 28, 2024

cason added the p2p label Jun 28, 2024

cason marked this pull request as ready for review June 28, 2024 10:50

cason requested a review from a team as a code owner June 28, 2024 10:50

cason requested a review from a team June 28, 2024 10:50

spec/p2p: removed FIXME as the problem should be solved now

d2aafcf

cason requested a review from a team as a code owner June 28, 2024 10:56

cason linked an issue Jun 28, 2024 that may be closed by this pull request

p2p: more peers connect than the maxInbound / maxOutbound set #3315

Closed

melekes approved these changes Jun 28, 2024

View reviewed changes

p2p/pex/pex_reactor.go Outdated Show resolved Hide resolved

p2p/pex/pex_reactor.go Outdated Show resolved Hide resolved

Apply suggestions from code review

8e1e045

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

p2p/pex: adding changelog entry

6a88a3d

czarcas7ic approved these changes Jun 28, 2024

View reviewed changes

sergio-mena approved these changes Jul 2, 2024

View reviewed changes

sergio-mena added the bug Something isn't working label Jul 2, 2024

melekes added backport-to-v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels Jul 2, 2024

Daniel Cason added 2 commits July 2, 2024 11:32

Merge branch 'main' into cason/486-seed-addresses

0da0e23

p2p/pex: minor change on changelog entry

ec7f2e4

cason changed the title ~~fix(p2p/pex): respect MaxNumOutboundPeers while dialing peers provided by a seed node~~ fix(p2p/pex): respect MaxNumOutboundPeers limit while dialing peers provided by a seed node Jul 2, 2024

cason enabled auto-merge July 2, 2024 09:44

cason added this pull request to the merge queue Jul 2, 2024

Merged via the queue into main with commit 4241776 Jul 2, 2024

cason deleted the cason/486-seed-addresses branch July 2, 2024 09:48

mergify bot mentioned this pull request Jul 2, 2024

fix(p2p/pex): respect MaxNumOutboundPeers limit while dialing peers provided by a seed node (backport #3360) #3395

Merged

4 tasks

This was referenced Jul 2, 2024

fix(p2p/pex): respect MaxNumOutboundPeers limit while dialing peers provided by a seed node (backport #3360) #3396

Merged

fix(p2p/pex): respect MaxNumOutboundPeers limit while dialing peers provided by a seed node (backport #3360) #3397

Merged

cason mentioned this pull request Aug 5, 2024

Fix TestConnectionSpeedForPeerReceivedFromSeed test #3620

Open

faddat mentioned this pull request Oct 15, 2024

chore: use latest cometbft-db in v0.38.x #4296

Closed

3 tasks

cason mentioned this pull request Dec 11, 2024

fix(p2p/pex): do not send PEX request in fast dial mode #4644

Merged

3 tasks

This was referenced Dec 12, 2024

fix(p2p/pex): do not send PEX request in fast dial mode (backport #4644) #4648

Merged

fix(p2p/pex): do not send PEX request in fast dial mode (backport #4644) #4649

Merged

jmalicevic pushed a commit that referenced this pull request Dec 12, 2024

fix(p2p/pex): do not send PEX request in fast dial mode (backport #4644…

a4d9e85

…) (#4648) Solves #4620. Fixes an issue introduced by #3360. Co-authored-by: Daniel <daniel.cason@informal.systems>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(p2p/pex): respect MaxNumOutboundPeers limit while dialing peers provided by a seed node#3360

fix(p2p/pex): respect MaxNumOutboundPeers limit while dialing peers provided by a seed node#3360
cason merged 10 commits intomainfrom
cason/486-seed-addresses

cason commented Jun 28, 2024 •

edited

Loading

Uh oh!

melekes left a comment

Uh oh!

Uh oh!

Uh oh!

melekes commented Jun 28, 2024

Uh oh!

cason commented Jun 28, 2024

Uh oh!

cason commented Jun 28, 2024

Uh oh!

czarcas7ic left a comment

Uh oh!

sergio-mena left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cason commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

melekes commented Jun 28, 2024

Uh oh!

cason commented Jun 28, 2024

Uh oh!

cason commented Jun 28, 2024

Uh oh!

czarcas7ic left a comment

Choose a reason for hiding this comment

Uh oh!

sergio-mena left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cason commented Jun 28, 2024 •

edited

Loading