Fix NonP2P to P2P interop problem #4467
Merged
Conversation
This file contains 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
Differentiate between incomming and outgoing connections. P2P uses the server port as source port which meant that it was possible for a non-p2p node to misstake in incomming connection from a p2p node for an outgoing connection to the same node.
In p2p mode the same port number is used which means that p2p nodes gets tangled up in the suspend peer logic. P2P nodes will reconnect after 10s, by lowering the producer suspensiontime from 20s to 1s p2p downstream peers aren't worse of than non-p2p nodes.
Let the server side know that it is permitted to accept new connections from peer. Previously DisallowConnection was returned causing the connection to be closed.
11 tasks
|
One thing, could you add an entry to |
|
bors r+ |
|
🔒 Permission denied Existing reviewers: click here to make karknu a reviewer |
|
bors merge |
iohk-bors bot
added a commit
that referenced
this pull request
Mar 23, 2023
4468: Backport fixes for 4465 to release/cardano-node-1.35.x branch r=coot a=coot # Description Cherry pick commits from #4467. # Checklist - Branch - [ ] Commit sequence broadly makes sense - [ ] Commits have useful messages - [ ] The documentation has been properly updated - [ ] New tests are added if needed and existing tests are updated - [ ] Any changes affecting Consensus packages must have an entry in the appropriate `changelog.d` directory created using [`scriv`](https://github.com/input-output-hk/scriv). If in doubt, see the [Consensus release process](../ouroboros-consensus/docs/ReleaseProcess.md). - [ ] Any changes in the Consensus API (every exposed function, type or module) that has changed its name, has been deleted, has been moved, or altered in some other significant way must leave behind a `DEPRECATED` warning that notifies downstream consumers. If deprecating a whole module, remember to add it to `./scripts/ci/check-stylish.sh` as otherwise `stylish-haskell` would un-deprecate it. - [ ] If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in [`interface-CHANGELOG.md`](../docs/interface-CHANGELOG.md) - [ ] If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional. - Pull Request - [ ] Self-reviewed the diff - [ ] Useful pull request description at least containing the following information: - What does this PR change? - Why these changes were needed? - How does this affect downstream repositories and/or end-users? - Which ticket does this PR close (if any)? If it does, is it [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)? - [ ] Reviewer requested Co-authored-by: Karl Knutsson <karl.fb.knutsson@gmail.com> Co-authored-by: Marcin Szamotulski <coot@coot.me>
iohk-bors bot
added a commit
that referenced
this pull request
Mar 24, 2023
4468: Backport fixes for 4465 to release/cardano-node-1.35.x branch r=disassembler a=coot # Description Cherry pick commits from #4467. # Checklist - Branch - [ ] Commit sequence broadly makes sense - [ ] Commits have useful messages - [ ] The documentation has been properly updated - [ ] New tests are added if needed and existing tests are updated - [ ] Any changes affecting Consensus packages must have an entry in the appropriate `changelog.d` directory created using [`scriv`](https://github.com/input-output-hk/scriv). If in doubt, see the [Consensus release process](../ouroboros-consensus/docs/ReleaseProcess.md). - [ ] Any changes in the Consensus API (every exposed function, type or module) that has changed its name, has been deleted, has been moved, or altered in some other significant way must leave behind a `DEPRECATED` warning that notifies downstream consumers. If deprecating a whole module, remember to add it to `./scripts/ci/check-stylish.sh` as otherwise `stylish-haskell` would un-deprecate it. - [ ] If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in [`interface-CHANGELOG.md`](../docs/interface-CHANGELOG.md) - [ ] If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional. - Pull Request - [ ] Self-reviewed the diff - [ ] Useful pull request description at least containing the following information: - What does this PR change? - Why these changes were needed? - How does this affect downstream repositories and/or end-users? - Which ticket does this PR close (if any)? If it does, is it [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)? - [ ] Reviewer requested Co-authored-by: Karl Knutsson <karl.fb.knutsson@gmail.com> Co-authored-by: Marcin Szamotulski <coot@coot.me>
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.
Description
P2P nodes uses the server port number for outbound connections. This can confuse NonP2P nodes into thinking that they already have a connection to them.
This PR changes the NonP2P connection table to take connection direction into account, similar to @dcoutts patches in https://github.com/input-output-hk/ouroboros-network/compare/dcoutts/old-connection-table .
The P2P nodes reuse of the server port also means that incoming connections from a P2P node can be refused by a NonP2P node if the peer is "suspended". This suspension never affected NonP2P nodes since they reconnect from a new ephemeral port each time. This PR lets NonP2P nodes differentiate between if it should refuse to accept new connections from a peer or if it should avoid creating new connections to a peer.
Fixes #4465 .
Checklist
changelog.ddirectory created usingscriv. If in doubt, see the Consensus release process.DEPRECATEDwarning that notifies downstream consumers. If deprecating a whole module, remember to add it to./scripts/ci/check-stylish.shas otherwisestylish-haskellwould un-deprecate it.interface-CHANGELOG.md