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.

Sign up for GitHub

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

Jump to bottom

[P2P] Redefine P2P concrete types in terms of interfaces #552

Closed
9 tasks
jessicadaugherty opened this issue Feb 27, 2023 · 3 comments · Fixed by #558
Closed
9 tasks

[P2P] Redefine P2P concrete types in terms of interfaces #552

jessicadaugherty opened this issue Feb 27, 2023 · 3 comments · Fixed by #558
Assignees
@bryanchriswhite
Labels
p2p P2P specific changes
Milestone
M1: Pocket PoS (P...

Comments

@jessicadaugherty
Copy link
Contributor

jessicadaugherty commented Feb 27, 2023 •
edited
Loading

Objective

Redefine P2P concrete types in terms of interfaces between the P2P module and the underlying implementation.

Origin Document

Next steps identified in #500

Part of #347

Goals

  • Improve modularity
  • Ensure that rainTreeNetwork does not depend on concrete p2p types

Deliverable

  • Update interface definitions for PeersManager, Peer, AddrBook, and AddrBookMap types respective methods and properties and remove any Raintree dependencies
  • Refactor code that uses concrete p2p types and update tests

Non-goals / Non-deliverables

  • Any additional raintree improvements or implementation

General issue deliverables

  • Update the appropriate CHANGELOG
  • Update any relevant READMEs (local and/or global)
  • Update any relevant global documentation & references
  • If applicable, update the source code tree explanation
  • If applicable, add or update a state, sequence or flowchart diagram using mermaid

Testing Methodology

  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @jessicadaugherty
@jessicadaugherty jessicadaugherty converted this from a draft issue Feb 27, 2023
@jessicadaugherty jessicadaugherty added the p2p P2P specific changes label Feb 27, 2023
@jessicadaugherty
Copy link
Contributor Author

Hey @bryanchriswhite @Olshansk can you review this ticket and confirm it should be pulled into the current iteration when you have a chance? Thank you

@Olshansk Olshansk moved this to In Progress in V1 Dashboard Feb 28, 2023
@bryanchriswhite
Copy link
Contributor

@jessicadaugherty this looks great too me, thanks for putting it together! 🙌

I believe it's correct to move this to the current iteration because I'm currently working on it and it is the very next step after #347.

@bryanchriswhite bryanchriswhite mentioned this issue Mar 2, 2023
Merged
16 tasks
@jessicadaugherty
Copy link
Contributor Author

jessicadaugherty commented Mar 3, 2023 •
edited
Loading

Sorry for the delayed reply @bryanchriswhite - that sounds like a plan to me. Tysm! PS appreciate the estimate 🥇

@bryanchriswhite bryanchriswhite moved this from In Progress to In Review in V1 Dashboard Mar 6, 2023
@bryanchriswhite bryanchriswhite changed the title [WIP][P2P] Redefine P2P concrete types in terms of interfaces [P2P] Redefine P2P concrete types in terms of interfaces Mar 10, 2023
bryanchriswhite added a commit that referenced this issue Mar 21, 2023
@bryanchriswhite
[P2P] Refactor P2P interfaces (#558)
ce4ec9e 
## @Reviewer

This PR may be more digestible / reviewable on a commit-by-commit basis.
Commits are organized logically and any given line is only modified in a
single commit, with few exceptions.

---

## Description

Redifines P2P module packages in terms of new interfaces:
- `Peer` / `PeerList`
- `Peerstore`
- `PeerManager` / `PeersView`

This has the effect of decoupling the `Network` implementation (e.g.
`rainTreeNetwork`) from the peer representation and storage/lookup. This
will be necessary for use with the libp2p based P2P module as it uses
different implementations for these concerns.

### Organization

This PR adds a *temporary* `shared/p2p` package to facilitate the
migration to the new libp2p based P2P module implementation. This
package will be re-consolidated into the remaining P2P module as part of
#554.

```
├── p2p
│   └── * addrbook_delta_test.go -> peer_test.go
└── shared
    └── p2p
        ├── + peer.go
        ├── + peer_manager.go
        └── + peerstore.go
```

#### peer.go

- `Peer` interface
- abstracted from
[`typesP2P.NetworkPeer`](https://github.com/pokt-network/pocket/blob/main/p2p/types/network_peer.go#L7)
and its usage
- `PeerList` concrete type (`[]Peer`)
  - implements a `#Delta()` method
- ported from
[`GetAddrBookDelta`](https://github.com/pokt-network/pocket/blob/main/p2p/addrbook_delta.go#L6)


#### peerstore.go

- `Peerstore` interface plus a simple map-based implementation,
`PeerAddrMap`

#### peer_manager.go

- `PeerManager` and `PeerView` interfaces
- abstracted from
[`raintree.peersManager`](https://github.com/pokt-network/pocket/blob/main/p2p/raintree/peers_manager.go#L15)
and its usage
- `SortedPeerManager` implementation factored out of the same,
`raintree.peersManager`
- decouples sorting and "view construction"
([`networkView`](https://github.com/pokt-network/pocket/blob/main/p2p/raintree/peers_manager.go#L181))
behavior from any conception of raintree specific network features (e.g.
"levels")

### Nomenclature changes

`AddrBook` --> `Peerstore`: The word "address" is becoming ambiguous as
libp2p nomenclature uses it to refer to the network address (e.g. IP /
hostname). As it's currently used, this object holds more than just the
both identity and network address so regardless of interpretation, I
would argue that `AddrBook` doesn't accurately convey its
responsibility. `Peerstore` also better aligns with other libp2p
nomenclature and should result it in more consistent and coherent
integration and refactoring going forward.

### Potential next steps 

1. Decouple peer connection management from identity/address storage and
lookup.
    - Add a `ConnManager` interface to be responsible for this
      - Move `Peer#GetStream()` to it
      - Move `PeerManager#GetSelfAddr()` to it
      - Refactor accordingly


## Issue

Fixes #552 

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Add `Peer` and `Peerstore` interfaces
- Refactor `AddrBook` to `PeerList` type
- Refactor `getAddrBookDelta` to be a member of `PeerList`
- Move `typesP2P.AddrBookMap` to `sharedP2P.PeerAddrMap` and refactor to
implement the new `Peerstore` interface
- Factor `SortedPeerManager` out of `raintree.peersManager` and add
`peerManager` interface
- Refactor `raintree.peersManager` to use `SortedPeerManager` and
implement `PeerManager` interface
- Refactor `stdnetwork.Network` implementation to use P2P interfaces
- Refactor `AddrBookProvider` to use new P2P interfaces
- Refactor `typesP2P.Network` to use new P2P interfaces
- Refactor `typesP2P.Transport` to embed `io.ReadWriteCloser`
- Rename `NetworkPeer#Dialer` to `NetworkPeer#Transport`for readability
and consistency
- Refactor `typesP2P.NetworkPeer` to implement the new `Peer` interface
- Refactor debug CLI to use new P2P interfaces


## Testing

- [x] `make develop_test`
- [x]
[LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md)
w/ all of the steps outlined in the `README`

<!-- REMOVE this comment block after following the instructions
 If you added additional tests or infrastructure, describe it here.
 Bonus points for images and videos or gifs.
-->

## Required Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
@github-project-automation github-project-automation bot moved this from In Review to Done in V1 Dashboard Mar 21, 2023
dylanlott pushed a commit that referenced this issue Mar 24, 2023
@bryanchriswhite @dylanlott
[P2P] Refactor P2P interfaces (#558)
6fe2d78 
## @Reviewer

This PR may be more digestible / reviewable on a commit-by-commit basis.
Commits are organized logically and any given line is only modified in a
single commit, with few exceptions.

---

## Description

Redifines P2P module packages in terms of new interfaces:
- `Peer` / `PeerList`
- `Peerstore`
- `PeerManager` / `PeersView`

This has the effect of decoupling the `Network` implementation (e.g.
`rainTreeNetwork`) from the peer representation and storage/lookup. This
will be necessary for use with the libp2p based P2P module as it uses
different implementations for these concerns.

### Organization

This PR adds a *temporary* `shared/p2p` package to facilitate the
migration to the new libp2p based P2P module implementation. This
package will be re-consolidated into the remaining P2P module as part of
#554.

```
├── p2p
│   └── * addrbook_delta_test.go -> peer_test.go
└── shared
    └── p2p
        ├── + peer.go
        ├── + peer_manager.go
        └── + peerstore.go
```

#### peer.go

- `Peer` interface
- abstracted from
[`typesP2P.NetworkPeer`](https://github.com/pokt-network/pocket/blob/main/p2p/types/network_peer.go#L7)
and its usage
- `PeerList` concrete type (`[]Peer`)
  - implements a `#Delta()` method
- ported from
[`GetAddrBookDelta`](https://github.com/pokt-network/pocket/blob/main/p2p/addrbook_delta.go#L6)


#### peerstore.go

- `Peerstore` interface plus a simple map-based implementation,
`PeerAddrMap`

#### peer_manager.go

- `PeerManager` and `PeerView` interfaces
- abstracted from
[`raintree.peersManager`](https://github.com/pokt-network/pocket/blob/main/p2p/raintree/peers_manager.go#L15)
and its usage
- `SortedPeerManager` implementation factored out of the same,
`raintree.peersManager`
- decouples sorting and "view construction"
([`networkView`](https://github.com/pokt-network/pocket/blob/main/p2p/raintree/peers_manager.go#L181))
behavior from any conception of raintree specific network features (e.g.
"levels")

### Nomenclature changes

`AddrBook` --> `Peerstore`: The word "address" is becoming ambiguous as
libp2p nomenclature uses it to refer to the network address (e.g. IP /
hostname). As it's currently used, this object holds more than just the
both identity and network address so regardless of interpretation, I
would argue that `AddrBook` doesn't accurately convey its
responsibility. `Peerstore` also better aligns with other libp2p
nomenclature and should result it in more consistent and coherent
integration and refactoring going forward.

### Potential next steps 

1. Decouple peer connection management from identity/address storage and
lookup.
    - Add a `ConnManager` interface to be responsible for this
      - Move `Peer#GetStream()` to it
      - Move `PeerManager#GetSelfAddr()` to it
      - Refactor accordingly


## Issue

Fixes #552 

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Add `Peer` and `Peerstore` interfaces
- Refactor `AddrBook` to `PeerList` type
- Refactor `getAddrBookDelta` to be a member of `PeerList`
- Move `typesP2P.AddrBookMap` to `sharedP2P.PeerAddrMap` and refactor to
implement the new `Peerstore` interface
- Factor `SortedPeerManager` out of `raintree.peersManager` and add
`peerManager` interface
- Refactor `raintree.peersManager` to use `SortedPeerManager` and
implement `PeerManager` interface
- Refactor `stdnetwork.Network` implementation to use P2P interfaces
- Refactor `AddrBookProvider` to use new P2P interfaces
- Refactor `typesP2P.Network` to use new P2P interfaces
- Refactor `typesP2P.Transport` to embed `io.ReadWriteCloser`
- Rename `NetworkPeer#Dialer` to `NetworkPeer#Transport`for readability
and consistency
- Refactor `typesP2P.NetworkPeer` to implement the new `Peer` interface
- Refactor debug CLI to use new P2P interfaces


## Testing

- [x] `make develop_test`
- [x]
[LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md)
w/ all of the steps outlined in the `README`

<!-- REMOVE this comment block after following the instructions
 If you added additional tests or infrastructure, describe it here.
 Bonus points for images and videos or gifs.
-->

## Required Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
dylanlott pushed a commit to dylanlott/pocket that referenced this issue Mar 24, 2023
@bryanchriswhite @dylanlott
[P2P] Refactor P2P interfaces (pokt-network#558)
b878193 
This PR may be more digestible / reviewable on a commit-by-commit basis.
Commits are organized logically and any given line is only modified in a
single commit, with few exceptions.

---

Redifines P2P module packages in terms of new interfaces:
- `Peer` / `PeerList`
- `Peerstore`
- `PeerManager` / `PeersView`

This has the effect of decoupling the `Network` implementation (e.g.
`rainTreeNetwork`) from the peer representation and storage/lookup. This
will be necessary for use with the libp2p based P2P module as it uses
different implementations for these concerns.

This PR adds a *temporary* `shared/p2p` package to facilitate the
migration to the new libp2p based P2P module implementation. This
package will be re-consolidated into the remaining P2P module as part of

```
├── p2p
│   └── * addrbook_delta_test.go -> peer_test.go
└── shared
    └── p2p
        ├── + peer.go
        ├── + peer_manager.go
        └── + peerstore.go
```

- `Peer` interface
- abstracted from
[`typesP2P.NetworkPeer`](https://github.com/pokt-network/pocket/blob/main/p2p/types/network_peer.go#L7)
and its usage
- `PeerList` concrete type (`[]Peer`)
  - implements a `#Delta()` method
- ported from
[`GetAddrBookDelta`](https://github.com/pokt-network/pocket/blob/main/p2p/addrbook_delta.go#L6)

- `Peerstore` interface plus a simple map-based implementation,
`PeerAddrMap`

- `PeerManager` and `PeerView` interfaces
- abstracted from
[`raintree.peersManager`](https://github.com/pokt-network/pocket/blob/main/p2p/raintree/peers_manager.go#L15)
and its usage
- `SortedPeerManager` implementation factored out of the same,
`raintree.peersManager`
- decouples sorting and "view construction"
([`networkView`](https://github.com/pokt-network/pocket/blob/main/p2p/raintree/peers_manager.go#L181))
behavior from any conception of raintree specific network features (e.g.
"levels")

`AddrBook` --> `Peerstore`: The word "address" is becoming ambiguous as
libp2p nomenclature uses it to refer to the network address (e.g. IP /
hostname). As it's currently used, this object holds more than just the
both identity and network address so regardless of interpretation, I
would argue that `AddrBook` doesn't accurately convey its
responsibility. `Peerstore` also better aligns with other libp2p
nomenclature and should result it in more consistent and coherent
integration and refactoring going forward.

1. Decouple peer connection management from identity/address storage and
lookup.
    - Add a `ConnManager` interface to be responsible for this
      - Move `Peer#GetStream()` to it
      - Move `PeerManager#GetSelfAddr()` to it
      - Refactor accordingly

Fixes pokt-network#552

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

- Add `Peer` and `Peerstore` interfaces
- Refactor `AddrBook` to `PeerList` type
- Refactor `getAddrBookDelta` to be a member of `PeerList`
- Move `typesP2P.AddrBookMap` to `sharedP2P.PeerAddrMap` and refactor to
implement the new `Peerstore` interface
- Factor `SortedPeerManager` out of `raintree.peersManager` and add
`peerManager` interface
- Refactor `raintree.peersManager` to use `SortedPeerManager` and
implement `PeerManager` interface
- Refactor `stdnetwork.Network` implementation to use P2P interfaces
- Refactor `AddrBookProvider` to use new P2P interfaces
- Refactor `typesP2P.Network` to use new P2P interfaces
- Refactor `typesP2P.Transport` to embed `io.ReadWriteCloser`
- Rename `NetworkPeer#Dialer` to `NetworkPeer#Transport`for readability
and consistency
- Refactor `typesP2P.NetworkPeer` to implement the new `Peer` interface
- Refactor debug CLI to use new P2P interfaces

- [x] `make develop_test`
- [x]
[LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md)
w/ all of the steps outlined in the `README`

<!-- REMOVE this comment block after following the instructions
 If you added additional tests or infrastructure, describe it here.
 Bonus points for images and videos or gifs.
-->

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] I have updated the corresponding CHANGELOG

- [ ] I have updated the corresponding README(s); local and/or global
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bryanchriswhite bryanchriswhite

Labels
p2p P2P specific changes
Projects
V1 Dashboard
Status: Done
Milestone
M1: Pocket PoS (Proof of Stake)
Development

Successfully merging a pull request may close this issue.

[P2P] Refactor P2P interfaces pokt-network/pocket
2 participants
@bryanchriswhite @jessicadaugherty