spec: overview of the p2p implementation in v0.34 #9126
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
cason
requested review from
ebuchman,
cmwaters,
tychoish,
williambanfield and
creachadair
as code owners
jmalicevic
reviewed
Jul 29, 2022
jmalicevic
reviewed
Jul 29, 2022
spec/p2p/v0.34/switch.md
Outdated
| This method receives a list of peer addresses (strings) and dials all of | ||
| them in parallel. | ||
|
|
||
| > TODO: who invokes this? Apparently, it is invoked at startup to dial all |
There was a problem hiding this comment.
Yes these are the only places where this is called. The PEX reactor calls DialPeerWithAddress.
There was a problem hiding this comment.
So, we should document it as startup procedure.
jmalicevic
reviewed
Jul 29, 2022
* PEX protocol documentation is a separated file
* PEX reactor documentation with a general documentation, including
the address book and its role as (outbound) peer manager.
cason
force-pushed
the
cason/p2p-doc-v0.34
branch
from
9aac783
to
7c4ff39
Compare
cason
changed the title
jmalicevic
reviewed
Aug 19, 2022
jmalicevic
reviewed
Aug 19, 2022
jmalicevic
reviewed
Aug 19, 2022
| After sending a PEX request is sent to a peer, the node expect to receive, | ||
| as a response, a `PexAddrs` message from the peer. | ||
| This message encodes a list of peer addresses that are | ||
| [added to address book](./addressbook.md#new-addresses), |
There was a problem hiding this comment.
Suggested change
| [added to address book](./addressbook.md#new-addresses), | |
| [added to the address book](./addressbook.md#new-addresses), |
jmalicevic
reviewed
Aug 19, 2022
jmalicevic
reviewed
Aug 19, 2022
jmalicevic
reviewed
Aug 19, 2022
spec/p2p/v0.34/pex-protocol.md
Outdated
| Address Book. | ||
| This selection is produced in the same way as in the random selection of peer | ||
| addresses that are [provided](#providing-addresses) to a requesting peer. | ||
| Are removed from this selection peers that the seed node has crawled recently, |
There was a problem hiding this comment.
Suggested change
| Are removed from this selection peers that the seed node has crawled recently, | |
| From this selection peers that the seed node has crawled recently are removed, |
There was a problem hiding this comment.
I adopted the suggestion, but with a different format.
jmalicevic
reviewed
Aug 19, 2022
jmalicevic
reviewed
Aug 19, 2022
jmalicevic
reviewed
Aug 19, 2022
spec/p2p/v0.34/pex-protocol.md
Outdated
| > received from a peer, and the entry relative to a peer is removed from this | ||
| > map when the peer is disconnected. | ||
| > | ||
| > It is debatable whether this approach indeed prevent abuse against seed nodes. |
There was a problem hiding this comment.
While we should be concerned with abusing a seed node, we should also consider the scenario where in a network with few seed nodes a peer might be right to request addresses more then once. Maybe we can have an algorithm where we provide to the peer more then one address? Or each peer sends a desired number of addresses (this number can be derived from the currently connected peers and the desired number of peers)?
jmalicevic
reviewed
Aug 19, 2022
jmalicevic
reviewed
Aug 19, 2022
| The `RemoveAddress` method removes an address from the address book. | ||
|
|
||
| It is invoked by the switch when it dials a peer or accepts a connection from a | ||
| peer that end ups being the node itself (`IsSelf` error). |
There was a problem hiding this comment.
In which cases does a nodes own address end up being in the address book? In a lot of places I see a guard before adding a peer to the address book checking whether the address is ours. Unless this exists for the case we get this as persistent peers on startup. But even then I think there is a check before adding them to the address book.
There was a problem hiding this comment.
This is a good point. Although I believe this code is there for a practical reason :D
jmalicevic
reviewed
Aug 19, 2022
jmalicevic
reviewed
Aug 19, 2022
|
|
||
| ## RemovePeer | ||
|
|
||
| The `RemovePeer` method, from the `Reactor` interface, |
There was a problem hiding this comment.
This function does not actually remove the peer from the address book, right?
There was a problem hiding this comment.
Nope. This is a method of the Reactor interface, which essentially means the peer was disconnected.
Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
JayT106
reviewed
Aug 26, 2022
|
|
||
| 1. When an *outbound* peer is added, causing the node to request addresses from | ||
| the new peer | ||
| 1. Periodically, by the `ensurePeersRoutine`, causing the node to request peer |
There was a problem hiding this comment.
Thank you for taking the time to review the spec. We have merged this PR because we are preparing a next one with new edits and will then open a PR against main. I will incorporated your suggestions there though.
JayT106
reviewed
Aug 26, 2022
|
|
||
| > FIXME: The current logic was introduced in [#3762](https://github.com/tendermint/tendermint/pull/3762). | ||
| > Although it fix the issue, the delay between receiving an address and dialing | ||
| > the peer, it does not impose ant limit on how many addresses are dialed in this |
jmalicevic
approved these changes
Aug 28, 2022
cason
added a commit
that referenced
this pull request
Aug 29, 2022
* Spec: p2p v0.34 doc, switch initial documentation
* Spec: p2p v0.34 doc, list of source files
* Spec: p2p v0.34 doc, transport documentation
* Spec: p2p v0.34 doc, transport error handling
* Spec: p2p v0.34 doc, PEX initial documentation
* PEX protocol documentation is a separated file
* PEX reactor documentation with a general documentation, including
the address book and its role as (outbound) peer manager.
* Spec: p2p v0.34 doc, PEX protocol documentation
* Spec: p2p v0.34 doc, PEX protocol on seed nodes
* Spec: p2p v0.34 doc, address book
* Spec: p2p v0.34 doc, address book, more details
* Spec: p2p v0.34 doc, address book persistence
* Spec: p2p v0.34 doc, address book random samples
* Spec: p2p v0.34 doc, status of this documentation
* Spec: p2p v0.34 doc, pex reactor documentation
* Spec: p2p v0.34 doc, addressing PR #9126 comments
Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
* Spec: p2p v0.34 doc, peer manager, outbound peers
Co-authored-by: Daniel Cason <cason@gandria>
Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
cason
added a commit
that referenced
this pull request
Nov 3, 2022
* spec: overview of p2p in v0.34 (#9120) * Iniital comments on v0.34 p2p * Added conf, updated text * Moved everything to spec * Update README.md * spec: overview of the p2p implementation in v0.34 (#9126) * Spec: p2p v0.34 doc, switch initial documentation * Spec: p2p v0.34 doc, list of source files * Spec: p2p v0.34 doc, transport documentation * Spec: p2p v0.34 doc, transport error handling * Spec: p2p v0.34 doc, PEX initial documentation * PEX protocol documentation is a separated file * PEX reactor documentation with a general documentation, including the address book and its role as (outbound) peer manager. * Spec: p2p v0.34 doc, PEX protocol documentation * Spec: p2p v0.34 doc, PEX protocol on seed nodes * Spec: p2p v0.34 doc, address book * Spec: p2p v0.34 doc, address book, more details * Spec: p2p v0.34 doc, address book persistence * Spec: p2p v0.34 doc, address book random samples * Spec: p2p v0.34 doc, status of this documentation * Spec: p2p v0.34 doc, pex reactor documentation * Spec: p2p v0.34 doc, addressing PR #9126 comments Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> * Spec: p2p v0.34 doc, peer manager, outbound peers Co-authored-by: Daniel Cason <cason@gandria> Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> * spec:p2p v0.34 introduction (#9319) * restructure README.md initial * Fix typos * Reorganization * spec: overview of p2p in v0.34 (#9120) * Iniital comments on v0.34 p2p * Added conf, updated text * Moved everything to spec * Update README.md * spec: overview of the p2p implementation in v0.34 (#9126) * Spec: p2p v0.34 doc, switch initial documentation * Spec: p2p v0.34 doc, list of source files * Spec: p2p v0.34 doc, transport documentation * Spec: p2p v0.34 doc, transport error handling * Spec: p2p v0.34 doc, PEX initial documentation * PEX protocol documentation is a separated file * PEX reactor documentation with a general documentation, including the address book and its role as (outbound) peer manager. * Spec: p2p v0.34 doc, PEX protocol documentation * Spec: p2p v0.34 doc, PEX protocol on seed nodes * Spec: p2p v0.34 doc, address book * Spec: p2p v0.34 doc, address book, more details * Spec: p2p v0.34 doc, address book persistence * Spec: p2p v0.34 doc, address book random samples * Spec: p2p v0.34 doc, status of this documentation * Spec: p2p v0.34 doc, pex reactor documentation * Spec: p2p v0.34 doc, addressing PR #9126 comments Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> * Spec: p2p v0.34 doc, peer manager, outbound peers Co-authored-by: Daniel Cason <cason@gandria> Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> * spec:p2p v0.34 introduction (#9319) * restructure README.md initial * Fix typos * Reorganization * spec: p2p v0.34, addressbook review * spec: p2p v0.34, peer manager review * spec: p2p v0.34, peer manager review * spec: p2p v0.34, peer manager review * spec: p2p v0.34, peer manager review * spec: p2p v0.34, peer manager review * spec: p2p v0.34, peer manager review * spec: p2p v0.34, peer manager review * Filled config description * spec: p2p v0.34, transport review * spec: p2p v0.34, switch review * spec: p2p v0.34, overview, first version * spec: p2p v0.34, peer manager review * spec: p2p v0.34, shorter readme * Configuration update * Configuration update * Shortened README * spec: p2p v0.34, readme intro * spec: p2p v0.34, readme contents * spec: p2p v0.34, readme references * spec: p2p readme points to v0.34 * spec: p2p, v0.34, fixing brokend markdown links * Makrdown fix * Apply suggestions from code review Co-authored-by: Adi Seredinschi <adizere@gmail.com> Co-authored-by: Zarko Milosevic <zarko@informal.systems> * spec: p2p v0.34, address book new intro * spec: p2p v0.34, address book buckets summary * spec: p2p v0.34, peer manager, issue link * spec: p2p v0.34, fixing links * spec: p2p v0.34, addressing comments from reviews * spec: p2p v0.34, addressing comments from reviews * Apply suggestions from Jasmina's code review Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> * spec: p2p v0.34, addressing comments from reviews * Apply suggestions from code review Co-authored-by: Sergio Mena <sergio@informal.systems> * Apply suggestions from code review Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> Co-authored-by: Sergio Mena <sergio@informal.systems> * spec: p2p, v0.34, address book section reorganized * spec: p2p, v0.34, addressing review comments * Typos * Typo * spec: p2p, v0.34, address book markbad Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> Co-authored-by: Daniel Cason <cason@gandria> Co-authored-by: Adi Seredinschi <adizere@gmail.com> Co-authored-by: Zarko Milosevic <zarko@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems>
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.
Addresses #9089.
Description of the implementation of the p2p layer in v0.34. This is a work in progress.