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] feat: add background router #707

Merged
merged 37 commits into from
May 8, 2023
Merged

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Apr 28, 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)


Description

Implements a "background router" to work alongside raintree (to be integrated in a future PR) which uses a kademlia DHT for peer discovery and a gossipsub pubsub router for broadcasting and listening.

Issue

Part 1 of #505
Part 1 of #712

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

  • Removed unused Transport interface
  • Moved and renamed raintree.RainTreeConfig to util.RouterConfig
  • Renamed protocol.DefaultTopicStr to protocol.BackgroundTopicStr
  • Added protocol.PeerDiscoveryNamespace
  • Added kademlia peer discovery baseline test
  • Added background router (kad + gossipsub)
  • Renamed P2PConfig#MaxMempoolCount to P2PConfig#MaxNonces
  • Renamed DefaultP2PMaxMempoolCount to DefaultP2PMaxNonces
  • Updated Dockerfiles using outdated go version to 1.19
  • Updated P2P README

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 p2p P2P specific changes medium gpt review Triggers a code review from https://github.com/anc95/ChatGPT-CodeReview cl validate Run the changelog validation workflow labels Apr 28, 2023
@bryanchriswhite bryanchriswhite self-assigned this Apr 28, 2023
@reviewpad reviewpad bot added large and removed medium labels Apr 28, 2023
github-actions[bot]

This comment was marked as outdated.

@github-actions github-actions bot dismissed their stale review April 28, 2023 09:05

The check succeeded, dismissing the review comment.

@bryanchriswhite bryanchriswhite force-pushed the feat/full-node-gossip branch 2 times, most recently from edef491 to a5393e8 Compare May 1, 2023 08:48
@bryanchriswhite bryanchriswhite force-pushed the feat/full-node-gossip branch from a5393e8 to 4f23c2d Compare May 1, 2023 09:46
@bryanchriswhite bryanchriswhite force-pushed the feat/full-node-gossip branch from 4f23c2d to 4407707 Compare May 1, 2023 10:17
@bryanchriswhite bryanchriswhite marked this pull request as ready for review May 1, 2023 10:31
github-actions[bot]

This comment was marked as outdated.

@bryanchriswhite bryanchriswhite force-pushed the feat/full-node-gossip branch from b70d4aa to 1bdf7ee Compare May 1, 2023 11:06
github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

@bryanchriswhite bryanchriswhite force-pushed the feat/full-node-gossip branch from 5d421c5 to e57bd7d Compare May 4, 2023 12:12
@github-actions github-actions bot dismissed stale reviews from themself May 4, 2023 12:12

The check succeeded, dismissing the review comment.

@bryanchriswhite bryanchriswhite removed push-image Build and push a container image on non-main builds e2e-devnet-test Runs E2E tests on devnet labels May 4, 2023
@bryanchriswhite bryanchriswhite requested a review from Olshansk May 4, 2023 12:46
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.

Just a couple questions but otherwise almost g2g

p2p/background/router_test.go Show resolved Hide resolved
runtime/defaults/defaults.go Show resolved Hide resolved
// height and peerstore providers. Intended for internal use by other `RouterConfig`
// implementations with common config parameters.
//
// NB: intentionally *not* embedding `baseConfig` to improve readability of usages
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand this. So when would you embed a struct inside of another one?

To me this seems like a worthwhile usecase

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite May 5, 2023

Choose a reason for hiding this comment

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

I'm suggesting never. It doesn't look/feel good from the usage side:

	rtCfg := RainTreeConfig{
		BaseConfig: BaseConfig{
			Host:                  nil,
			Addr:                  nil,
			CurrentHeightProvider: nil,
			PeerstoreProvider:     nil,
		},
		MaxNonces: 100,
	}

Seeing this 👆 makes me want to write a NewRainTreeConfig() function which would be a bit ridiculous IMHO given that that the purpose of the config originally was to simplify the NewRainTreeRouter() function signature.

image

Additionally, I expect significant simplification may be around the corner for the router interface. I'm anticipating being able to eliminate the need for the background router to hold a reference to either provider (maybe even the pokt addr as well). This would cause further divergence between BackgroundConfig and RainTreeConfig, likely making baseConfig another temporary step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel strongly about not taking embedding seriously as an option because of the impact on readability, given the context. An alternative to defining baseConfig would be to duplicate the IsValid() implementation across both RainTreeConfig and BackgroundConfig; however, I see the way it's currently written as a simplification of that.

Copy link
Member

Choose a reason for hiding this comment

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

giphy

@bryanchriswhite bryanchriswhite requested a review from Olshansk May 5, 2023 17:54
* pokt/main:
  [RPC][CLI] RPC Specification Parity (Issue #628) (#684)
  [Persistence] Index Transactions by Transaction Hash (#721)
  [Docs] Add missing README.md links (#722)
@bryanchriswhite bryanchriswhite merged commit d7cc85d into main May 8, 2023
@bryanchriswhite bryanchriswhite deleted the feat/full-node-gossip branch May 11, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cl validate Run the changelog validation workflow p2p P2P specific changes waiting-for-review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants