Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation of p2p layer in Tendermint v0.34 #9348

Merged
merged 57 commits into from Nov 3, 2022
Merged

Documentation of p2p layer in Tendermint v0.34 #9348

merged 57 commits into from Nov 3, 2022

Conversation

cason
Copy link
Contributor

@cason cason commented Aug 31, 2022

Documentation of the implementation of the p2p layer (p2p package in v0.34.x branch).

The documentation is based in the analysis of the source code, and follows its organization and the abstractions there defined. The target public of this document are developers, as I hope this documentation will help them to understand the code.

Having said that, there is an initial effort to map implementation abstractions to more high-level concepts. For instance, the documentation of the PEX reactor was split into three parts, representing the roles it implements. The intent here is to support a refactoring of the code that would allow a more clear separation of these roles.

What is not covered by this documentation:

  • The general concept of reactors, and their interaction with the p2p layer, specially the the Switch
  • The actual implementation of the communication between nodes, which is part of the p2p/conn package
  • The routines that allows exchanging messages with peers, part of Peer abstraction and p2p/conn package

Addresses #9089.

jmalicevic and others added 30 commits August 10, 2022 15:45
* Iniital comments on v0.34 p2p

* Added conf, updated text

* Moved everything to spec

* Update README.md
* 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>
* restructure README.md initial

* Fix typos

* Reorganization
* Iniital comments on v0.34 p2p

* Added conf, updated text

* Moved everything to spec

* Update README.md
* 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>
* restructure README.md initial

* Fix typos

* Reorganization
spec/p2p/v0.34/README.md Outdated Show resolved Hide resolved
Comment on lines 38 to 39
because outbound peers, that the node has dialed, are considered more trustworthy
than inbound peers, that the node has accepted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the rationale to consider them more trustworthy?
Is this distinction (inbound vs outbound) also used for point 2)? If not, then there is a contradiction, IMO, on what is considered trustworthy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, at the moment the ensurePeersRoutine does not take into consideration whether the peer in the address book is outbound or inbound but does check the rating of the peers (chooses which bucket to take peer from). The PEX request in case 1) is sent immediately to this outbound peer assuming that the node already trusted this peer as the outbound peers are usually preconfigured by the node operators.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, I'd add some footnote or something, as the "trustworthy" concept looks more of a heuristic than something well defined.

Co-authored-by: Sergio Mena <sergio@informal.systems>
- if the added address or the associated source address are nil
- if the added address is invalid
- if the added address is the local node's address
- if the added address ID is of a banned peer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"banned" is not defined at this point. Please add a forward reference to the "bad peers" section

Comment on lines +267 to +268
If the destination bucket of old addresses is full, the oldest address in the
bucket is moved (downgraded) to a bucket of new addresses.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this cause a periodic churn between buckets if all "good" peers don't fit their corresponding "old" buckets? This churn would be anything but useful, BTW.

are added back to the address book as new addresses, while the corresponding
node IDs are removed from the `badPeers` map.

## Removing addresses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a mechanism for a really bad address (simple example: an address that didn't ever work for, say, months) to be deleted from the address book altogether?
I'm thinking of a malicious actor (e.g. a malicious seed node) providing plenty of fake addresses that will then be kept around forever

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see there is such mechanism in the Buckets section. I guess I agree with @adizere in that (some form of) the Buckets section should appear early on in this file.

spec/p2p/v0.34/pex.md Outdated Show resolved Hide resolved
spec/p2p/v0.34/pex.md Outdated Show resolved Hide resolved
cason and others added 3 commits November 2, 2022 16:28
Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
Co-authored-by: Sergio Mena <sergio@informal.systems>
@cason cason merged commit 6bde634 into main Nov 3, 2022
@cason cason deleted the spec/p2p-v0.34 branch November 3, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:wip Work in progress (prevents stalebot from automatically closing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants