Skip to content

Conversation

cason
Copy link

@cason cason commented Jun 20, 2023

This PR backports 3 PRs merged into the main branch: #714, #851, and #966.

Note to reviewers: there is no new content added here, this is just a manual backport of multiple PRs already merged into main.

Addresses #598

@cason cason marked this pull request as ready for review June 20, 2023 15:03
@cason cason requested review from a team as code owners June 20, 2023 15:03
@cason cason self-assigned this Jun 20, 2023
@cason cason added p2p spec Specification-related labels Jun 20, 2023
p2p/README.md Outdated
Comment on lines 7 to 10
- [Connection](https://github.com/cometbft/cometbft/blob/v0.37.x/spec/p2p/legacy-docs/connection.md) for details on how connections and multiplexing work
- [Peer](https://github.com/cometbft/cometbft/blob/v0.37.x/spec/p2p/legacy-docs/node.md) for details on peer ID, handshakes, and peer exchange
- [Node](https://github.com/cometbft/cometbft/blob/v0.37.x/spec/p2p/legacy-docs/node.md) for details about different types of nodes and how they should work
- [Config](https://github.com/cometbft/cometbft/blob/v0.37.x/spec/p2p/legacy-docs/config.md) for details on some config option
Copy link
Contributor

Choose a reason for hiding this comment

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

These must be relative links if you want this rendered correctly on the docs site.

Copy link
Author

Choose a reason for hiding this comment

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

Are you sure? This is on p2p/README.md, not part of the docs or spec dirs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, you're right. This wouldn't affect the docs then. But it's still a hard link to the repo as opposed to a relative link to a file that exists within the same repo - is that intentional?

Copy link
Author

Choose a reason for hiding this comment

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

I have seen some hard links on source code directories, there are several actually. I don't really like this idea, to be honest.

Anyway, I will fix all the links in the files involved in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Removed full links on 06d0804

@cason
Copy link
Author

cason commented Jun 23, 2023

For reviewers, the changes to have in mind for this backport PR are few, the following (remove --stat for the textual diff):

$ git checkout cason/p2p-backport-to-0.37
$ git diff --stat main -- spec/p2p/ spec/abci/ p2p/README.md
 p2p/README.md                               |   8 +-
 spec/abci/README.md                         |   2 +-
 spec/abci/abci++_app_requirements.md        | 327 ++++++++++++++++++++++++++-------------------------------------
 spec/abci/abci++_basic_concepts.md          | 121 ++++++++++++++---------
 spec/abci/abci++_client_server.md           |   4 +-
 spec/abci/abci++_comet_expected_behavior.md | 129 +++++++++++--------------
 spec/abci/abci++_methods.md                 | 209 +++++++++++++++++++++++++++++++---------
 spec/p2p/legacy-docs/config.md              |   8 --
 spec/p2p/legacy-docs/connection.md          |   2 +-
 spec/p2p/legacy-docs/messages/block-sync.md |  26 +++--
 spec/p2p/legacy-docs/messages/consensus.md  |   2 +-
 spec/p2p/legacy-docs/messages/mempool.md    |   2 +-
 12 files changed, 446 insertions(+), 394 deletions(-)

Notice that most of the diff under spec/abci is not introduced by this PR.

@cason
Copy link
Author

cason commented Jun 27, 2023

Any blocker for merging this PR?

Copy link
Contributor

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

I skimmed through the changes and they LGTM.

Just one note for next time:

The command
git diff --stat main -- spec/p2p/ spec/abci/ p2p/README.md
is actually showing noise: changes that are not really contained in this PR (I got confused by it). Best method is just to backport, 1 by 1, the PRs on main. This might seem more tedious, but makes the reviewer's life much easier (in that the same changes don't need to be reviewed twice, whilst having confidence we're not missing anything to be reviewed).

@cason
Copy link
Author

cason commented Jun 29, 2023

Thank you @sergio-mena.

I had the same thoughts after this attempt, what is more appropriate: multiple backports PRs or a single multiple-PR backport?

I planned to pose this question to the team, but in general I agree with yout that backporting PRs one by one is more bureaucratic but probably more appropriate than opting for a multiple-PR backport.

Daniel and others added 4 commits June 29, 2023 15:47
* spec/p2p: starting the specification of a Reactor

* spec/p2p: generic service modelled in Quint

* spec/p2p: minimal/initial reactor model in Quint

* spec/p2p: P2P -> reactor interaction grammar

* spec/p2p: routines modeled in reactor.qnt

* spec/p2p: reactor.Receive) modeled in Quint

* spec/p2p: reactor types moved to the beginning

* spec/p2p: reactor Quint state encapsulated

* spec/p2p: reactor with actions and defintions

* spec/p2p: reactor SetSwitch() method modeled

* spec/p2p: reactor Service interface modeled

* spec/p2p: reactor setup action added

* spec/p2p: reactor Quint spec cleanup

* spec/p2p: reactor InitPeer spec updated

* spec/p2p: reactor Quint simulation support

* spec/p2p: reactor Quint spec refactored

* spec/p2p: reactor grammar added to a rephrased README

* spec/p2p: README structure reorganized

* spec/p2p: reactor registration, overview

* spec/p2p: reactor implements a service (start/stop)

* spec/p2p: reactor peer management, overview

* spec/p2p: reactor receive message method documented

* spec/p2p: renamed to registration/register reactor

* spec/p2p: reactor grammar refactoring, part I

* spec/p2p: reactor grammar refactoring, part II

* spec/p2p: reactor grammar refactoring, part III

* spec/p2p: reactor grammar refactoring, part IV

* spec/p2p: removing Quint model used as an example

* spec/p2p: reactor grammar refactoring, part V

* spec/p2p: reactor grammar refactoring, part VI

* spec/p2p: reactor grammar refactoring, part VII

* Apply suggestions from code review

Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>

* spec/p2p: reactor Quint model, some comments added

* Update spec/p2p/reactor/reactor.qnt

Co-authored-by: Lasaro <lasaro@informal.systems>

* spec/p2p: syntax HL for Quint files

Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>

* spec/p2p: suggestions from code review

Co-authored-by: Sergio Mena <sergio@informal.systems>

* spec/p2p: reactor Quint model, channel ID as str

* spec/p2p: applying suggestions from code review

* spec/p2p: API methods first, then actions on Quint

* spec/p2p: reactor Quint model, minor code cleanup

* spec/p2p: reactor README intro slightly rephrased

* spec/p2p: reactor grammar intro slightly rephrased

* spec/p2p: some reactor "should" replaced by "must"

* spec/p2p: Quint model reference on README updated

* spec/p2p: minor fixes on reactor's README

---------

Co-authored-by: Daniel Cason <cason@lausanne.local>
Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>
Co-authored-by: Lasaro <lasaro@informal.systems>
Co-authored-by: Sergio Mena <sergio@informal.systems>
* spec/p2p: registered flag in Quint reactor

* spec/p2p: NewReactor method in Quint reactor

* spec/p2p: reactor is registered with a name

* spec/p2p: reactor actions receive ReactorState

* spec/p2p: Quint model supports multiple reactors

* spec/p2p: Quint reactor/service state simplified

* spec/p2p: Quint reactor has channels' list as field

* spec/p2p: Quint updateReactorTo action helper

* spec/p2p: reactors are assigned to channel IDs

* spec/p2p: reactor API specification, small fixes

* spec/p2p: API p2p layer provides to reactors

* spec/p2p: reactor API content to reactor.md file

* spec/p2p: new README for reactors with a diagram

* spec/p2p: API for reactors, introduction text

* spec/p2p: links between the two API documentations

* spec/p2p: suggestions from code review

Co-authored-by: Sergio Mena <sergio@informal.systems>

* spec/p2p: Apply suggestions from code review

Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>
Co-authored-by: Lasaro <lasaro@informal.systems>

* spec/p2p: reactor documentation intro, small fix

* spec/p2p: a paragraph about concurrency and reactors

* spec/p2p: intro to the reactor grammar, minor add

* spec/p2p: notes about switch broadcast method

* spec/p2p: note on NumPeers() switch method

* spec/p2p: reactor veting peers, some rephrasing

* spec/p2p: link to discussion on peer quality

* spec/p2p: link to stop peer discussion on other repo

* spec/p2p: link to state sharing between reactors added

* spec/p2p: link to reactor API split added

---------

Co-authored-by: Sergio Mena <sergio@informal.systems>
Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>
Co-authored-by: Lasaro <lasaro@informal.systems>
* spec/p2p: moving legacy documents to legacy/ dir

* spec/p2p: using capitalized names for README.md

* spec/p2p: intro to p2p spec, moved from other README

* spec/p2p: v0.34.x documentation, README shortened

* spec/p2p: minor changes on READMEs

* spec/p2p: moving messages/ content to legacy/ dir

* spec/p2p: dir for docs of the p2p implementation

* spec/p2p: renamed reactor dirs to reactor-api

* spec/p2p: legacy content to legacy-docs/ dir

* spec/p2p: fixes on implementation/README.md file

* spec/p2p: moving images to a single subdir

* spec/p2p: fixing Markdown links

* spec/p2p: fixing Markdown links in all repo

* spec/p2p: table of contents for the p2p docs

* spec/p2p: applying suggestions from Josef

Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>

---------

Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>
@cason cason force-pushed the cason/p2p-backport-to-0.37 branch from 06d0804 to b03a6d1 Compare June 29, 2023 13:47
@cason cason merged commit 75f090d into v0.37.x Jun 29, 2023
@cason cason deleted the cason/p2p-backport-to-0.37 branch June 29, 2023 14:25
@sergio-mena
Copy link
Contributor

multiple backports PRs or a single multiple-PR backport?

My impression is that 1:1 backports, as they are more mechanical, they are easier to review (e.g. if you use diff-of-diffs for reviewing backports it's a 5 min task... and if you let mergify do it and there are no conflicts, it's a 0 min task). But, since you're asking formally, I'm 100% convinced I'm right.

What I propose: next time it happens, ping me and we look at it together.

nivasan1 pushed a commit to skip-mev/cometbft that referenced this pull request Jan 16, 2024
This is backport of three PRs originally merged against main:

* spec/p2p: Specify the operation of a Reactor (cometbft#714)
* spec/p2p: document the p2p API used by Reactors (cometbft#851)
* spec/p2p: new structure for the p2p specification (cometbft#966)

Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>
Co-authored-by: Lasaro <lasaro@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
Labels
p2p spec Specification-related
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants