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

[P2P] Integrate background router #732

Merged
merged 70 commits into from
Jul 11, 2023
Merged

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented May 5, 2023

@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*.

*(In the interest of preserving the git-time-continuum 👮🚨, this applies in batches of commits between comments or reviews by humans, only once "in review")


Description

Before

While backgroundRouter exists and implements the Router interface, it had yet to be connected to anything. Additionally, it was not able to handle incoming unicast messages.

classDiagram
    class p2pModule {
        -router Router
        -handlePocketEnvelope([]byte) error
    }

    class P2PModule {
        <<interface>>
        GetAddress() (Address, error)
        HandleEvent(*anypb.Any) error
        Send([]byte, Address) error
        Broadcast([]byte) error
    }
    p2pModule --|> P2PModule

    class RainTreeRouter {
        UnicastRouter
        -handler MessageHandler
        +Broadcast([]byte) error
        -handleRainTreeMsg([]byte) error
    }

    class BackgroundRouter {
        UnicastRouter
        -handler MessageHandler
        +Broadcast([]byte) error
        -handleBackgroundMsg([]byte) error
        -readSubscription(subscription *pubsub.Subscription)
    }

    class UnicastRouter {
        -messageHandler MessageHandler
        -peerHandler PeerHandler
        +Send([]byte, Address) error
        -handleStream(libp2pNetwork.Stream)
        -readStream(libp2pNetwork.Stream)
    }
    RainTreeRouter --* UnicastRouter : (embedded)

    p2pModule --o "1" Router
    p2pModule ..* RainTreeRouter : (`router`)
    
    class Router {
        <<interface>>
        +Send([]byte, Address) error
        +Broadcast([]byte) error
    }
    BackgroundRouter --|> Router
    RainTreeRouter --|> Router
Loading

After

backgroundRouter embeds UnicastRouter and is integrated into p2pModule such that it is considered when calling P2PModule#Send() and P2PModule#Broadcast().

classDiagram
    class p2pModule {
        -stakedActorRouter Router
        -unstakedActorRouter Router
        -handlePocketEnvelope([]byte) error
    }

    class P2PModule {
        <<interface>>
        GetAddress() (Address, error)
        HandleEvent(*anypb.Any) error
        Send([]byte, Address) error
        Broadcast([]byte) error
    }
    p2pModule --|> P2PModule

    class RainTreeRouter {
        UnicastRouter
        -handler MessageHandler
        +Broadcast([]byte) error
        -handleRainTreeMsg([]byte) error
    }

    class BackgroundRouter {
        UnicastRouter
        -handler MessageHandler
        +Broadcast([]byte) error
        -handleBackgroundMsg([]byte) error
        -readSubscription(subscription *pubsub.Subscription)
    }

    class UnicastRouter {
        -messageHandler MessageHandler
        -peerHandler PeerHandler
        +Send([]byte, Address) error
        -handleStream(libp2pNetwork.Stream)
        -readStream(libp2pNetwork.Stream)
    }
    RainTreeRouter --* UnicastRouter : (embedded)
    BackgroundRouter --* UnicastRouter : (embedded)

    p2pModule --o "2" Router
    p2pModule ..* RainTreeRouter : (`stakedActorRouter`)
    p2pModule ..* BackgroundRouter : (`unstakedActorRouter`)
    
    class Router {
        <<interface>>
        +Send([]byte, Address) error
        +Broadcast([]byte) error
    }
    BackgroundRouter --|> Router
    RainTreeRouter --|> Router
Loading

Issue

Dependencies

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • added background message protobuf type
  • added Router#Close() interface method
  • separated raintree & bg protocol IDs
  • move PocketEnvelope nonce generation to PackMessage()
  • added Handler to router configs
  • refactored raintree router to support P2P module & Router interface changes
  • refactored raintree router bootstrapping
  • refactored background router to support P2P module integration & Router interface changes
  • integrated background router into P2P module
  • refactored staked actor router (raintree) peer discovery
  • updated tests, post-refactoring

Testing

  • make develop_test; if any code changes were made
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@bryanchriswhite bryanchriswhite added the p2p P2P specific changes label May 5, 2023
@bryanchriswhite bryanchriswhite self-assigned this May 5, 2023
@reviewpad reviewpad bot added the medium label May 5, 2023
@bryanchriswhite bryanchriswhite force-pushed the feat/integrate-bg-router branch from 7f58195 to ab81389 Compare May 11, 2023 12:44
@bryanchriswhite bryanchriswhite changed the base branch from feat/full-node-gossip to main May 11, 2023 12:44
@Olshansk Olshansk removed the medium label May 17, 2023
@reviewpad reviewpad bot added the medium Pull request is medium label May 17, 2023
@bryanchriswhite bryanchriswhite force-pushed the feat/integrate-bg-router branch from ab81389 to b01996d Compare May 29, 2023 12:58
@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels May 29, 2023
@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

Patch coverage: 66.66% and project coverage change: +0.45 🎉

Comparison is base (fe24824) 31.78% compared to head (d564ade) 32.23%.

Additional details and impacted files
@@                     Coverage Diff                     @@
##           refactor/unicast-router     #732      +/-   ##
===========================================================
+ Coverage                    31.78%   32.23%   +0.45%     
===========================================================
  Files                          106       87      -19     
  Lines                         8996     6099    -2897     
===========================================================
- Hits                          2859     1966     -893     
+ Misses                        5795     3861    -1934     
+ Partials                       342      272      -70     
Impacted Files Coverage Δ
p2p/types/errors.go 0.00% <ø> (ø)
p2p/utils/host.go 0.00% <0.00%> (ø)
shared/messaging/envelope.go 62.50% <100.00%> (+8.65%) ⬆️

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bryanchriswhite bryanchriswhite force-pushed the feat/integrate-bg-router branch from b01996d to a8206ec Compare June 1, 2023 11:32
@bryanchriswhite bryanchriswhite changed the base branch from main to test/refactor-testutils June 1, 2023 11:33
@bryanchriswhite bryanchriswhite force-pushed the feat/integrate-bg-router branch from a8206ec to 744cb2b Compare June 19, 2023 13:15
@bryanchriswhite bryanchriswhite changed the base branch from test/refactor-testutils to refactor/unicast-router June 19, 2023 13:15
@bryanchriswhite bryanchriswhite force-pushed the refactor/unicast-router branch from 30e94a6 to fe24824 Compare June 19, 2023 13:20
@bryanchriswhite bryanchriswhite force-pushed the feat/integrate-bg-router branch from 744cb2b to d564ade Compare June 19, 2023 13:20
@bryanchriswhite bryanchriswhite added the do not merge Prevent merging even with sufficient approvals label Jun 20, 2023
@bryanchriswhite bryanchriswhite mentioned this pull request Jun 21, 2023
20 tasks
@bryanchriswhite bryanchriswhite force-pushed the feat/integrate-bg-router branch 3 times, most recently from 86d739c to 53e81e5 Compare June 22, 2023 11:39
@bryanchriswhite bryanchriswhite added the e2e-devnet-test Runs E2E tests on devnet label Jun 22, 2023
@bryanchriswhite bryanchriswhite force-pushed the feat/integrate-bg-router branch from ca0b962 to f80e6e9 Compare June 22, 2023 12:56
@gitguardian

This comment was marked as off-topic.

p2p/README.md Show resolved Hide resolved
p2p/README.md Show resolved Hide resolved
p2p/README.md Show resolved Hide resolved
p2p/README.md Show resolved Hide resolved
p2p/README.md Show resolved Hide resolved
p2p/README.md Show resolved Hide resolved
p2p/README.md Outdated

| Sender | Receiver | Router | Example Usage |
|----------------|----------------|-----------------|------------------------------------------------------|
| Staked Actor | Staked Actor | Raintree only | Consensus (state sync) messages (to validators only) |
Copy link
Member

Choose a reason for hiding this comment

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

As noted in the P2P knowledge transfer (#438) ticket, for hotstuff consensus, we don't use RainTree for messages propagation between staked actors (e.g. state sync) but for consensus, it's a star pattern (i.e. direct)

@bryanchriswhite bryanchriswhite requested a review from Olshansk July 6, 2023 07:58
p2p/event_handler.go Outdated Show resolved Hide resolved
p2p/background/router_test.go Outdated Show resolved Hide resolved
p2p/background/router_test.go Outdated Show resolved Hide resolved
p2p/background/router_test.go Outdated Show resolved Hide resolved
p2p/background/router_test.go Outdated Show resolved Hide resolved
p2p/background/router_test.go Outdated Show resolved Hide resolved
p2p/README.md Outdated

| Sender | Receiver | Router | Example Usage |
|----------------|----------------|-----------------|------------------------------------------------------|
| Staked Actor | Staked Actor | Raintree only | Consensus (state sync) messages (to validators only) |
Copy link
Member

Choose a reason for hiding this comment

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

How about this:

 Sender         | Receiver       | Router          | Example Usage                                        |
|----------------|----------------|-----------------|------------------------------------------------------|
| Staked Actor   | Staked Actor   | No router  | Consensus (hotstuff) messages (to validators only) |
| Staked Actor   | Staked Actor   | Raintree only   | Consensus (state sync) messages (to validators only) |
| Unstaked Actor | Staked Actor   | Background only | Consensus (state sync) messages (to validators only) |
| Unstaked Actor | Unstaked Actor | Background only | Consensus (state sync) & Debug (CLI) messages        |

p2p/README.md Outdated Show resolved Hide resolved
@bryanchriswhite bryanchriswhite force-pushed the feat/integrate-bg-router branch from d2dc98a to b76efdf Compare July 7, 2023 08:54
* pokt/main:
  [Utility] Update E2E feature path template doc (#870)
  [IBC] Add nil check on proof for membership and non-membership proof creation (#877)
  Added git diff state to devlog10
  Devlog 10 (#872)
  [Documentation] Add IBC Module introduction as an example (#853)
  [Persistence][Bug] Fix Actor Schema Assignment for ValidatorActor in GetActor (#857)
  QOL: add bash completion for p1 to localnet client (#865)
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Let's 🚢 this!!

{
name: "empty PocketEnvelope",
msgBz: mustMarshal(t, &typesP2P.BackgroundMessage{
// NB: `Data` is normally a serialized `PocketEnvelope`.
Copy link
Member

Choose a reason for hiding this comment

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

Appreciate the comments 🙌

t, receiverPeer,
receiverHost,
func(data []byte) error {
receivedChan <- data
Copy link
Member

Choose a reason for hiding this comment

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

Nice change

p2p/README.md Outdated Show resolved Hide resolved
@bryanchriswhite bryanchriswhite merged commit 6b875ee into main Jul 11, 2023
bryanchriswhite added a commit that referenced this pull request Jul 11, 2023
* pokt/main:
  [P2P] Integrate background router (#732)
  Update main README.md
  [Bug] Fix CI linter errors (#885)
  [Tooling] Block `IN_THIS_*` comments from passing CI (#889)
@bryanchriswhite bryanchriswhite deleted the feat/integrate-bg-router branch July 11, 2023 09:19
bryanchriswhite added a commit that referenced this pull request Jul 11, 2023
* pokt/main:
  [P2P] Integrate background router (#732)
  Update main README.md
  [Bug] Fix CI linter errors (#885)
  [Tooling] Block `IN_THIS_*` comments from passing CI (#889)
  [Utility] Update E2E feature path template doc (#870)
  [IBC] Add nil check on proof for membership and non-membership proof creation (#877)
  Added git diff state to devlog10
  Devlog 10 (#872)
bryanchriswhite added a commit that referenced this pull request Jul 12, 2023
## Description

Because `0` is the zero (default) value for `uint64`, if it is valid to
be used as a nonce, it becomes difficult to distinguish the scenario
where sender did not set a nonce from one where they explicitly set it
to `0`.

I'm not confident whether the ability to make this distinction matters
now or has the potential to later but was following a feeling.

<!-- reviewpad:summarize:start -->
### Summary generated by Reviewpad on 11 Jul 23 11:56 UTC
This pull request includes the following changes:

- In the `p2p/module.go` file, the `handlePocketEnvelope` function now
checks if `poktEnvelope.Nonce` is zero and returns an error if it is.
The error message includes the hex-encoded nonce value.
- In the `p2p/module_test.go` file, new test cases have been added to
test the handling of an invalid nonce.
- In the `p2p/types/errors.go` file, a new error variable
`ErrInvalidNonce` has been added to represent an invalid nonce value.
- In the `shared/crypto/rand.go` file, a check has been added to ensure
that the generated nonce value is not zero. If it is zero, the function
recursively calls itself to generate a new nonce.
<!-- reviewpad:summarize:end -->

## Issue

N/A; observation made while working on #732.

## Type of change

Please mark the relevant option(s):

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

## List of changes

- Adds `ErrInvalidNonce` P2P error type
- Ensures the P2P message handler rejects the zero value `Nonce`
(`uint64(0)`) is invalid
- Ensures the `GetNonce` function never returns the zero value

## Testing

- [ ] `make develop_test`; if any code changes were made
- [ ] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [ ] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [ ] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [ ] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made


## Required Checklist

- [ ] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-devnet-test Runs E2E tests on devnet large Pull request is large p2p P2P specific changes push-image Build and push a container image on non-main builds waiting-for-review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[P2P] Background gossip using libp2p-pubsub
3 participants