From 17495efadc5416bb78e9e95032626d7f03ae6542 Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Tue, 30 May 2023 13:08:31 -0700 Subject: [PATCH 1/3] [Utility] E2E Feature Path Template (#734) ## Description This PR introduces one new document related to end-to-end (E2E) features: `E2E_FEATURE_PATH_TEMPLATE.md`. `E2E_FEATURE_PATH_TEMPLATE` outlines a well-defined process for adopting an end-to-end feature path approach, intending to make each feature/task easier to scope, reason about, design, and implement. It includes two main stages: 1. Feature Specification 2. Feature Implementation (moving from Proof of Concept to Minimum Viable Product and finally to Production) It also provides tips and templates for creating GitHub issues and Origin Documents for scoping and tracking E2E features. The goal is to enable internal and external team members individuals to help with the implementation of the utility module. ## Issue Fixes #733 ## Type of change Please mark the relevant option(s): - [ ] New feature, functionality or library - [ ] Bug fix - [ ] Code health or cleanup - [ ] Major breaking change - [X] Documentation - [ ] Other ## List of changes - Added a list of all needed E2E features (to the best of our current knowledge) - Linked to #754 and #755 --- Co-authored-by: Bryan White --- Makefile | 12 ++ utility/doc/CHANGELOG.md | 4 + utility/doc/E2E_FEATURE_PATH_TEMPLATE.md | 198 +++++++++++++++++++++++ 3 files changed, 214 insertions(+) create mode 100644 utility/doc/E2E_FEATURE_PATH_TEMPLATE.md diff --git a/Makefile b/Makefile index 514f773cf..6c68ba9f9 100644 --- a/Makefile +++ b/Makefile @@ -560,3 +560,15 @@ send_local_tx: ## A hardcoded send tx to make LocalNet debugging easier .PHONY: query_chain_params query_chain_params: ## A hardcoded ChainParams query to make LocalNet debugging easier go run app/client/main.go Query AllChainParams + +.PHONY: search_structs +search_structs: ## Greps and outputs all of the structs in the project (excluding vendor or proto generated files) + grep -r "type .* struct" --exclude-dir="vendor" --exclude="*.gen.go" --exclude="*.pb.go" . + +.PHONY: search_interfaces +search_interfaces: ## Greps and outputs all of the structs in the project (excluding vendor or proto generated files) + grep -r "type .* interface" --exclude-dir="vendor" --exclude="*.gen.go" --exclude="*.pb.go" . + +.PHONY: search_protos +search_protos: ## Finds all of the proto files in the project (excluding vendor) + find . -name "*.proto" -not -path "./vendor/*" \ No newline at end of file diff --git a/utility/doc/CHANGELOG.md b/utility/doc/CHANGELOG.md index 1ce74f466..09cb2dfcc 100644 --- a/utility/doc/CHANGELOG.md +++ b/utility/doc/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.0.0.41] - 2023-05-30 + +- Added the E2E Feature Path Template to document, present and explain how to go about defining and scoping utility level features + ## [0.0.0.40] - 2023-05-04 - Expose `HydrateIdxTx` for use outside of utility diff --git a/utility/doc/E2E_FEATURE_PATH_TEMPLATE.md b/utility/doc/E2E_FEATURE_PATH_TEMPLATE.md new file mode 100644 index 000000000..946cc99b0 --- /dev/null +++ b/utility/doc/E2E_FEATURE_PATH_TEMPLATE.md @@ -0,0 +1,198 @@ +# E2E Feature Path + +_IMPROVE(olshansky): Once we've completed the entire process at least once, we'll add links to each step._ + +- [Introduction \& Goals](#introduction--goals) +- [Developer Journey](#developer-journey) +- [E2E Feature Specification](#e2e-feature-specification) + - [Spot Feature](#spot-feature) + - [Spike Feature](#spike-feature) + - [Scope Feature](#scope-feature) + - [1. GitHub Ticket](#1-github-ticket) + - [2. Origin Document](#2-origin-document) +- [E2E Feature Implementation](#e2e-feature-implementation) + - [POC: Proof of Concept](#poc-proof-of-concept) + - [MVC: Minimum Viable Change](#mvc-minimum-viable-change) + - [PROD: Production](#prod-production) + +## Introduction & Goals + +The [Pocket Network Specification](https://github.com/pokt-network/pocket-network-protocol/tree/main/utility) implementation is driven by various [milestones](https://github.com/pokt-network/pocket/milestones) and protocol/module/component specific tasks. Each feature crosses the boundaries of business logic, data types, and interfaces for different components. Due to the complex nature of implementation, we've designed a streamlined "developer journey". + +**The goal** of this document is to outline a well-defined process for incorporating an end-to-end feature path. This makes each feature/task easier to scope, reason about, design, and implement. + +## Developer Journey + +```mermaid +%%{init: { 'logLevel': 'debug', 'theme': 'base' } }%% + timeline + title E2E Feature Path Developer Journey + + section E2E Feature Specification + Spot Feature: + User research: + Product management: + Protocol intuition/experience: + Addition/removal/selection of feature from the list + Spike Feature: + Research details: + Identify pointers: + Note dependencies: + Find blockers + Scope Feature: + Define requirements: + Document E2E implementation + + section E2E Feature Implementation + POC: + POC Spike: + Explore: + Hack: + Have fun! + MVC: + Data Structures: + Interfaces: + Implementation: + Unit Tests: + E2E Tests: + Documentation + PROD: + Identify workarounds & hacks: + Document future work: + Ideate! +``` + +## E2E Feature Specification + +### Spot Feature + +Choose or add a feature from the Utility E2E feature list [here](./E2E_FEATURE_LIST.md). + +### Spike Feature + +Create a SPIKE GitHub issue, like [this](<[http](https://github.com/pokt-network/pocket/issue/TODO_LINK_TO_ISSUE_ONCE_WE_HAVE_EXAMPLE)>) to scope the feature. This ticket is responsible for creating the ticket that'll track the work. + +### Scope Feature + +Leverage the results from the SPIKE to create an implementation GitHub issue, like [this](<[http](https://github.com/pokt-network/pocket/issue/TODO_LINK_TO_ISSUE_ONCE_WE_HAVE_EXAMPLE)>) to track the actual implementation. + +#### 1. GitHub Ticket + +Open a [new issue](https://github.com/pokt-network/pocket/issues/new?assignees=&labels=&projects=&template=issue.md&title=%5BREPLACE+ME%5D+with+a+descriptive+title) and populate its description, respectively, with the following additional elements: + +**Objective**: `Implement MVC E2E Feature Path .: ` + +**Origin Document**: _Specify the details from the [Origin Document](#origin-document) below_ + +**Goals**: + +- Complete the MVC implementation of the E2E Feature Path outlined in the objective +- Identify future tasks and test requirements to transition the feature to production + +**Deliverables**: + +**POC**: + +- [ ] A POC SPIKE to be discarded, refactored, and/or restructured into multiple PRs + +**MVC**: + +- [ ] A PR that adds or modifies relevant structures and interfaces; such as [shared/core/types/proto](../../shared/core/types/proto), [shared/modules](../../shared/modules), etc +- [ ] A PR that materializes an MVC of the feature along with unit tests +- [ ] A PR that introduces a new E2E tests with **one or more happy** and **one or more sad** path scenarios as described in the origin document (refer to [e2e/README.md](../../e2e/README.md)); this may require additions to the [cli](https://github.com/pokt-network/pocket/tree/main/app/client) +- [ ] A PR that updates all pertinent documentation + +**PROD**: + +- [ ] One or more subsequent GitHub issues that track future work including, but not limited to: + - Enhancing test coverage + - Adding subsequent features + - Patching hacks or workarounds + - Enabling your [imagination](https://github.com/pokt-network/pocket/assets/1892194/6aff9004-8d3b-48e8-b6d5-9b67ac266e3d)! + +#### 2. Origin Document + +**Purpose:** [Replace this with a single sentence that captures the intended purpose, behaviour and goal of the E2E feature] +**Actors**: Check all of the protocol actors involved in the feature: + +- [ ] Validator +- [ ] Application +- [ ] Servicer +- [ ] Fisherman +- [ ] Portal + +**Data Structures**: + +- A list of the core types (protobufs, structs, etc) that will be used, added or modified in this feature +- Mention or link to specific files if applicable +- See [shared/core/types/proto](../../shared/core/types/proto) as a reference as they will most likely, but not necessarily, be part of that package +- _TIPS:_ + - _This will be non-exhaustive and will likely change during the POC or MVC stages_ + - You can find all other structs by running `make search_structs` + - You can find all other protobufs by running `make search_protos` + +**Interfaces**: + +- A list of the interface (go interfaces, placeholder functions, grpc, etc) that will be used, added or modified in this feature +- Mention or link to specific files if applicable +- See [shared/modules](../../shared/modules) as a reference as they will most likely, but not necessarily, be part of that package +- _TIPS:_ + - _This will be non-exhaustive and will likely change during the POC or MVC stages_ + - You can find all other structs by running `make search_interfaces` + - You can find all other protobufs by running `make search_protos` + +**Diagram**: + +- One or more mermaid diagrams that will visualize the E2E feature +- _TIPs:_ + - Use multiple diagrams if a single one ends up exceeding 7 or more core elements or steps + - See if there’s anything in [pokt-network/pocket-network-protocol/tree/main/utility](../../utility/) or [pokt-network/pocket/tree/main/utility/doc](../../utility/doc) that you can use as a starting point + +**User Stories as Tests**: + +- Use natural language (long-form or bullet points) to define: + - One (or more) HAPPY E2E path(s) from start to end with all the relevant details + - One (or more) SAD E2E path(s) from start to end with all the relevant details + - **[IMPROVE] Guiding template**: A [User | Actor | Source | etc] [performs an action] [where | when | at] [some specific context or state is guaranteed] and [the expected result is...]. + - **Example**: An Application requests the account balance of a specific address at a specific height when there is a Servicer staked for the Ethereum RelayChain in the same GeoZone, and receives a successful response. +- _NOTE: Keep in mind that these tests will be used to:_ + - _Interact with our [CLI](../../app/client/cli/) and [E2E testing framework](../../e2e) but do not design it for that_ + - _Train ChatGPT to expand on the happy and sad E2E test cases_ + +**Blockers**: + +- A list of other E2E feature paths (in the format `.: `) that: + - must be implemented prior to this + - will be mocked or added as placeholders to unblock this work +- Any other blockers, requirements or dependencies this will need and may need to be implemented as part of the feature implementation (e.g. infrastructure needs) + +## E2E Feature Implementation + +### POC: Proof of Concept + +Create a single PR where you _"do everything"_ with the knowledge that it'll be closed without being merged in. This is an opportunity to get your hands dirty, understand the problem more deeply and have some fun. This PR may be split into multiple smaller PRs or just refactored altogether. + +_TODO(example): Link to Alessandro's KISS or Bryan's P2P._ + +### MVC: Minimum Viable Change + +This is the 🥩 (or 🥙) of the whole feature. In several PRs, you will implement, get feedback and merge in the feature to the main branch. Use your best judgment on how to split it so it's easier for the reviewer to give feedback without blocking yourself. It'll include some, or all, of the following PRs: + +- Updates to data structures or protobufs +- Updates to interfaces +- The core implementation +- Updates to the [CLI](../../app/client/cli/) +- Updates to the [RPC](../../rpc/) service +- Additions / modifications to the [E2E testing framework](../../e2e) +- Additions / modifications to the documentation, code structure and diagrams + +### PROD: Production + +One or more follow-up GitHub issues that track follow-up work that my include, but not limited to: + +- Increase the test coverage +- Adding subsequent features +- Fixing hacks +- Your [imagination](https://github.com/pokt-network/pocket/assets/1892194/6aff9004-8d3b-48e8-b6d5-9b67ac266e3d)! + + From 5d1944c8b41bd4047bf9d60a7c672d91cfa18c69 Mon Sep 17 00:00:00 2001 From: harry <53987565+h5law@users.noreply.github.com> Date: Tue, 30 May 2023 22:51:11 +0100 Subject: [PATCH 2/3] Remove coverage prep stage as this corrupts the coverage report (#792) ## Description In the prepare code coverage report workflow job this actually corrupts the coverage report being uploaded to codecov resulting in a no usable upload error. This command (from my understanding) makes the coverage report readable for humans. ## Issue Fixes N/A ## 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 ## List of changes - Remove code coverage preparation step in workflow - Explicitly state the coverage file to be uploaded to codecov ## 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 - [x] 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](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 - [ ] 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/workflows/main.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index b3bc7911e..49fcbc848 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -82,12 +82,11 @@ jobs: uses: guyarb/golang-test-annotations@v0.5.1 with: test-results: test_results.json - - name: Prepare code coverage report - if: ${{ always() && env.TARGET_GOLANG_VERSION == matrix.go }} - run: go tool cover -func=coverage.out -o=coverage.out - name: Upload coverage to Codecov if: ${{ always() && env.TARGET_GOLANG_VERSION == matrix.go }} uses: codecov/codecov-action@v3 + with: + files: ./coverage.out - name: golangci-lint if: ${{ always() && env.TARGET_GOLANG_VERSION == matrix.go }} uses: golangci/golangci-lint-action@v3 From fba11c3f7cf66379028a9396e6eb4b66f0c43199 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Wed, 31 May 2023 11:26:32 +0200 Subject: [PATCH 3/3] [P2P] refactor: message handling (#763) ## Description ### Before The P2P module is responsible for handling incoming messages from the router's host. When it receives a message, it calls into the router to unpack the `RainTreeMessage` (and facilitate further broadcast propagation). The resulting `PocketEnvelope` is returned to the P2P module to be emitted over the application event bus. ```mermaid classDiagram class RainTreeMessage { <> +Level uint32 +Data []byte +Nonce uint64 } class PocketEnvelope { <> +Content *anypb.Any } RainTreeMessage --* PocketEnvelope : serialized as `Data` class P2PModule { -handleNetworkData([]byte) error -handleStream(stream libp2pNetwork.Stream) -readStream(stream libp2pNetwork.Stream) } class RainTreeRouter { +HandleNetworkData func([]byte) ([]byte, error) } RainTreeRouter --> P2PModule P2PModule --> RainTreeRouter RainTreeRouter --o RainTreeMessage P2PModule --o PocketEnvelope ``` ### After The router encapsulates handling incoming `RainTreeMessage`s, unpacking them and passing them along to the P2P module as serialized `PocketEnvelope`s. The P2P module then deserializes and emits them over the application event bus. ```mermaid classDiagram class RainTreeMessage { <> +Level uint32 +Data []byte +Nonce uint64 } class PocketEnvelope { <> +Content *anypb.Any } RainTreeMessage --* PocketEnvelope : serialized as `Data` class P2PModule { -handlePocketEnvelope([]byte) error } class RainTreeRouter { -handler RouterHandler -handleRainTreeMsg([]byte) ([]byte, error) -handleStream(stream libp2pNetwork.Stream) -readStream(stream libp2pNetwork.Stream) } RainTreeRouter --> P2PModule : `handler` == `handlePocketEnvelope` RainTreeRouter --o RainTreeMessage P2PModule --o PocketEnvelope ``` ## Issue Second deliverable in #762 ## 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 ## List of changes - Added `Handler` field to `RainTreeConfig` & `BackgroundConfig` - Refactored `rainTreeRouter` logging methods - Renamed `rainTreeRouter#HandleNetworkData()` to `#handleRainTreeMsg()` - Renamed `p2pModule#handleNetworkData()` to `#handleAppData()` - Added -tags=test to all test make targets - Fixed mockdns usage in `TestP2PModule_Insecure_Error` test - Moved message handling from p2p module to router ## 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 - [x] [Docker Compose LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md); if any major functionality was changed or introduced - [x] [k8s LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md); if any infrastructure or configuration changes were made ## 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 - [ ] 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 - [ ] 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) --- Makefile | 52 ++++++------- p2p/CHANGELOG.md | 10 +++ p2p/config/config.go | 2 + p2p/module.go | 119 ++--------------------------- p2p/module_raintree_test.go | 10 ++- p2p/raintree/logging.go | 31 ++++++++ p2p/raintree/peers_manager_test.go | 1 + p2p/raintree/router.go | 105 ++++++++++++++++++++++++- p2p/raintree/testutil.go | 13 ++++ p2p/transport_encryption_test.go | 11 ++- p2p/types/router.go | 7 +- 11 files changed, 210 insertions(+), 151 deletions(-) create mode 100644 p2p/raintree/logging.go create mode 100644 p2p/raintree/testutil.go diff --git a/Makefile b/Makefile index 6c68ba9f9..4b2472608 100644 --- a/Makefile +++ b/Makefile @@ -339,102 +339,102 @@ generate_node_state_machine_diagram: ## (Re)generates the Node State Machine dia .PHONY: test_all test_all: ## Run all go unit tests - go test -p=1 -count=1 ./... + go test -p=1 -count=1 -tags=test ./... .PHONY: test_e2e test_e2e: kubectl_check ## Run all E2E tests echo "IMPROVE(#759): Make sure you ran 'make localnet_up' in case this fails with infrastructure related errors." - go test ${VERBOSE_TEST} -count=1 ./e2e/tests/... -tags=e2e + go test ${VERBOSE_TEST} -count=1 -tags=test,e2e ./e2e/tests/... .PHONY: test_all_with_json_coverage test_all_with_json_coverage: generate_rpc_openapi ## Run all go unit tests, output results & coverage into json & coverage files - go test -p=1 -count=1 -json ./... -covermode=count -coverprofile=coverage.out | tee test_results.json | jq + go test -p=1 -count=1 -tags=test -json ./... -covermode=count -coverprofile=coverage.out | tee test_results.json | jq .PHONY: test_race test_race: ## Identify all unit tests that may result in race conditions - go test ${VERBOSE_TEST} -race -count=1 ./... + go test ${VERBOSE_TEST} -race -count=1 -tags=test ./... .PHONY: test_app test_app: ## Run all go app module unit tests - go test ${VERBOSE_TEST} -p=1 -count=1 ./app/... + go test ${VERBOSE_TEST} -p=1 -count=1 -tags=test ./app/... .PHONY: test_utility test_utility: ## Run all go utility module unit tests - go test ${VERBOSE_TEST} -p=1 -count=1 ./utility/... + go test ${VERBOSE_TEST} -p=1 -count=1 -tags=test ./utility/... .PHONY: test_shared test_shared: ## Run all go unit tests in the shared module - go test ${VERBOSE_TEST} -p=1 -count=1 ./shared/... + go test ${VERBOSE_TEST} -p=1 -count=1 -tags=test ./shared/... .PHONY: test_consensus test_consensus: ## Run all go unit tests in the consensus module - go test ${VERBOSE_TEST} -p=1 -count=1 ./consensus/... + go test ${VERBOSE_TEST} -p=1 -count=1 -tags=test ./consensus/... # These tests are isolated to a single package which enables logs to be streamed in realtime. More details here: https://stackoverflow.com/a/74903989/768439 .PHONY: test_consensus_e2e test_consensus_e2e: ## Run all go t2e unit tests in the consensus module w/ log streaming - go test ${VERBOSE_TEST} -count=1 ./consensus/e2e_tests/... + go test ${VERBOSE_TEST} -count=1 -tags=test ./consensus/e2e_tests/... .PHONY: test_consensus_concurrent_tests test_consensus_concurrent_tests: ## Run unit tests in the consensus module that could be prone to race conditions (#192) - for i in $$(seq 1 100); do go test -timeout 2s -count=1 -run ^TestPacemakerTimeoutIncreasesRound$ ./consensus/e2e_tests; done; - for i in $$(seq 1 100); do go test -timeout 2s -count=1 -run ^TestHotstuff4Nodes1BlockHappyPath$ ./consensus/e2e_tests; done; - for i in $$(seq 1 100); do go test -timeout 2s -count=1 -race -run ^TestPacemakerTimeoutIncreasesRound$ ./consensus/e2e_tests; done; - for i in $$(seq 1 100); do go test -timeout 2s -count=1 -race -run ^TestHotstuff4Nodes1BlockHappyPath$ ./consensus/e2e_tests; done; + for i in $$(seq 1 100); do go test -timeout 2s -count=1 -tags=test -run ^TestPacemakerTimeoutIncreasesRound$ ./consensus/e2e_tests; done; + for i in $$(seq 1 100); do go test -timeout 2s -count=1 -tags=test -run ^TestHotstuff4Nodes1BlockHappyPath$ ./consensus/e2e_tests; done; + for i in $$(seq 1 100); do go test -timeout 2s -count=1 -tags=test -race -run ^TestPacemakerTimeoutIncreasesRound$ ./consensus/e2e_tests; done; + for i in $$(seq 1 100); do go test -timeout 2s -count=1 -tags=test -race -run ^TestHotstuff4Nodes1BlockHappyPath$ ./consensus/e2e_tests; done; .PHONY: test_hotstuff test_hotstuff: ## Run all go unit tests related to hotstuff consensus - go test ${VERBOSE_TEST} -count=1 ./consensus/e2e_tests -run Hotstuff + go test ${VERBOSE_TEST} -count=1 -tags=test ./consensus/e2e_tests -run Hotstuff .PHONY: test_pacemaker test_pacemaker: ## Run all go unit tests related to hotstuff pacemaker - go test ${VERBOSE_TEST} -count=1 ./consensus/e2e_tests -run Pacemaker + go test ${VERBOSE_TEST} -count=1 -tags=test ./consensus/e2e_tests -run Pacemaker .PHONY: test_statesync test_statesync: ## Run all go unit tests related to hotstuff statesync - go test -v ${VERBOSE_TEST} -count=1 -run StateSync ./consensus/e2e_tests + go test -v ${VERBOSE_TEST} -count=1 -tags=test -run StateSync ./consensus/e2e_tests .PHONY: test_vrf test_vrf: ## Run all go unit tests in the VRF library - go test ${VERBOSE_TEST} -count=1 ./consensus/leader_election/vrf + go test ${VERBOSE_TEST} -count=1 -tags=test ./consensus/leader_election/vrf .PHONY: test_sortition test_sortition: ## Run all go unit tests in the Sortition library - go test ${VERBOSE_TEST} -count=1 ./consensus/leader_election/sortition + go test ${VERBOSE_TEST} -count=1 -tags=test ./consensus/leader_election/sortition .PHONY: test_persistence test_persistence: ## Run all go unit tests in the Persistence module - go test ${VERBOSE_TEST} -count=1 -p=1 ./persistence/... + go test ${VERBOSE_TEST} -count=1 -tags=test -p=1 ./persistence/... .PHONY: test_persistence_state_hash test_persistence_state_hash: ## Run all go unit tests in the Persistence module related to the state hash - go test ${VERBOSE_TEST} -count=1 -run TestStateHash ./persistence/... + go test ${VERBOSE_TEST} -count=1 -tags=test -run TestStateHash ./persistence/... .PHONY: test_p2p test_p2p: ## Run all p2p related tests - go test ${VERBOSE_TEST} -count=1 ./p2p/... + go test ${VERBOSE_TEST} -count=1 -tags=test ./p2p/... .PHONY: test_p2p_raintree test_p2p_raintree: ## Run all p2p raintree related tests - go test ${VERBOSE_TEST} -count=1 -run RainTreeNetwork -count=1 ./p2p/... + go test ${VERBOSE_TEST} -count=1 -tags=test -run RainTreeNetwork -count=1 ./p2p/... .PHONY: test_p2p_raintree_addrbook test_p2p_raintree_addrbook: ## Run all p2p raintree addr book related tests - go test ${VERBOSE_TEST} -count=1 -run RainTreeAddrBook -count=1 ./p2p/... + go test ${VERBOSE_TEST} -count=1 -tags=test -run RainTreeAddrBook -count=1 ./p2p/... # TIP: For benchmarks, consider appending `-run=^#` to avoid running unit tests in the same package .PHONY: benchmark_persistence_state_hash benchmark_persistence_state_hash: ## Benchmark the state hash computation - go test ${VERBOSE_TEST} -count=1 -cpu 1,2 -benchtime=1s -benchmem -bench=. -run BenchmarkStateHash -count=1 ./persistence/... + go test ${VERBOSE_TEST} -count=1 -tags=test -cpu 1,2 -benchtime=1s -benchmem -bench=. -run BenchmarkStateHash -count=1 ./persistence/... .PHONY: benchmark_sortition benchmark_sortition: ## Benchmark the Sortition library - go test ${VERBOSE_TEST} -count=1 -bench=. -run ^# ./consensus/leader_election/sortition + go test ${VERBOSE_TEST} -count=1 -tags=test -bench=. -run ^# ./consensus/leader_election/sortition .PHONY: benchmark_p2p_addrbook benchmark_p2p_peerstore: ## Run P2P peerstore benchmarks - go test ${VERBOSE_TEST} -count=1 -bench=. -run BenchmarkPeerstore ./p2p/... + go test ${VERBOSE_TEST} -count=1 -tags=test -bench=. -run BenchmarkPeerstore ./p2p/... ### Inspired by @goldinguy_ in this post: https://goldin.io/blog/stop-using-todo ### # TODO - General Purpose catch-all. diff --git a/p2p/CHANGELOG.md b/p2p/CHANGELOG.md index 36ecdfa97..32c4fa261 100644 --- a/p2p/CHANGELOG.md +++ b/p2p/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.0.0.52] - 2023-05-26 + +- Added `Handler` field to `RainTreeConfig` & `BackgroundConfig` +- Refactored `rainTreeRouter` logging methods +- Renamed `rainTreeRouter#HandleNetworkData()` to `#handleRainTreeMsg()` +- Renamed `p2pModule#handleNetworkData()` to `#handlePocketEnvelope()` +- Added -tags=test to all test make targets +- Fixed mockdns usage in `TestP2PModule_Insecure_Error` test +- Moved message handling from p2p module to router + ## [0.0.0.51] - 2023-05-23 - Use the shared codec module when marshaling the data sent over the wire diff --git a/p2p/config/config.go b/p2p/config/config.go index 4f1d87007..7e5b38f19 100644 --- a/p2p/config/config.go +++ b/p2p/config/config.go @@ -28,6 +28,7 @@ type BackgroundConfig struct { Addr crypto.Address CurrentHeightProvider providers.CurrentHeightProvider PeerstoreProvider providers.PeerstoreProvider + Handler func(data []byte) error } // RainTreeConfig implements `RouterConfig` for use with `RainTreeRouter`. @@ -36,6 +37,7 @@ type RainTreeConfig struct { Addr crypto.Address CurrentHeightProvider providers.CurrentHeightProvider PeerstoreProvider providers.PeerstoreProvider + Handler func(data []byte) error MaxNonces uint64 } diff --git a/p2p/module.go b/p2p/module.go index a784e019e..36cc25f12 100644 --- a/p2p/module.go +++ b/p2p/module.go @@ -3,16 +3,11 @@ package p2p import ( "errors" "fmt" - "io" - "time" - "github.com/libp2p/go-libp2p" libp2pHost "github.com/libp2p/go-libp2p/core/host" - libp2pNetwork "github.com/libp2p/go-libp2p/core/network" "github.com/multiformats/go-multiaddr" "github.com/pokt-network/pocket/logger" "github.com/pokt-network/pocket/p2p/config" - "github.com/pokt-network/pocket/p2p/protocol" "github.com/pokt-network/pocket/p2p/providers" "github.com/pokt-network/pocket/p2p/providers/current_height_provider" "github.com/pokt-network/pocket/p2p/providers/peerstore_provider" @@ -32,12 +27,6 @@ import ( "google.golang.org/protobuf/types/known/anypb" ) -// TECHDEBT(#629): configure timeouts. Consider security exposure vs. real-world conditions. -// TECHDEBT(#629): parameterize and expose via config. -// readStreamTimeout is the duration to wait for a read operation on a -// stream to complete, after which the stream is closed ("timed out"). -const readStreamTimeout = time.Second * 10 - var _ modules.P2PModule = &p2pModule{} type p2pModule struct { @@ -173,11 +162,6 @@ func (m *p2pModule) Start() (err error) { return fmt.Errorf("setting up router: %w", err) } - // Don't handle incoming streams in client debug mode. - if !m.isClientDebugMode() { - m.host.SetStreamHandler(protocol.PoktProtocolID, m.handleStream) - } - m.GetBus(). GetTelemetryModule(). GetTimeSeriesAgent(). @@ -290,6 +274,7 @@ func (m *p2pModule) setupRouter() (err error) { CurrentHeightProvider: m.currentHeightProvider, PeerstoreProvider: m.pstoreProvider, Host: m.host, + Handler: m.handlePocketEnvelope, MaxNonces: m.cfg.MaxNonces, }, ) @@ -340,100 +325,16 @@ func (m *p2pModule) isClientDebugMode() bool { return m.GetBus().GetRuntimeMgr().GetConfig().ClientDebugMode } -// handleStream is called each time a peer establishes a new stream with this -// module's libp2p `host.Host`. -func (m *p2pModule) handleStream(stream libp2pNetwork.Stream) { - m.logger.Debug().Msg("handling incoming stream") - peer, err := utils.PeerFromLibp2pStream(stream) - if err != nil { - m.logger.Error().Err(err). - Str("address", peer.GetAddress().String()). - Msg("parsing remote peer identity") - - if err = stream.Reset(); err != nil { - m.logger.Error().Err(err).Msg("resetting stream") - } - return - } - - if err := m.router.AddPeer(peer); err != nil { - m.logger.Error().Err(err). - Str("address", peer.GetAddress().String()). - Msg("adding remote peer to router") - } - - go m.readStream(stream) -} - -// readStream is intended to be called in a goroutine. It continuously reads from -// the given stream for handling at the network level. Used for handling "direct" -// messages (i.e. one specific target node). -func (m *p2pModule) readStream(stream libp2pNetwork.Stream) { - // Time out if no data is sent to free resources. - if err := stream.SetReadDeadline(newReadStreamDeadline()); err != nil { - // NB: tests using libp2p's `mocknet` rely on this not returning an error. - // `SetReadDeadline` not supported by `mocknet` streams. - m.logger.Debug().Err(err).Msg("setting stream read deadline") - } - - // debug logging: stream scope stats - // (see: https://pkg.go.dev/github.com/libp2p/go-libp2p@v0.27.0/core/network#StreamScope) - if err := utils.LogScopeStatFactory( - &logger.Global.Logger, - "stream scope (read-side)", - )(stream.Scope()); err != nil { - m.logger.Debug().Err(err).Msg("logging stream scope stats") - } - // --- - - data, err := io.ReadAll(stream) - if err != nil { - m.logger.Error().Err(err).Msg("reading from stream") - if err := stream.Reset(); err != nil { - m.logger.Debug().Err(err).Msg("resetting stream (read-side)") - } - return - } - - if err := stream.Reset(); err != nil { - m.logger.Debug().Err(err).Msg("resetting stream (read-side)") - } - - // debug logging - remotePeer, err := utils.PeerFromLibp2pStream(stream) - if err != nil { - m.logger.Debug().Err(err).Msg("getting remote remotePeer") - } else { - utils.LogIncomingMsg(m.logger, m.cfg.Hostname, remotePeer) - } - // --- - - if err := m.handleNetworkData(data); err != nil { - m.logger.Error().Err(err).Msg("handling network data") - } -} - -// handleNetworkData passes a network message to the configured -// `Router`implementation for routing. -func (m *p2pModule) handleNetworkData(data []byte) error { - appMsgData, err := m.router.HandleNetworkData(data) - if err != nil { - return err - } - - // There was no error, but we don't need to forward this to the app-specific bus. - // For example, the message has already been handled by the application. - if appMsgData == nil { - return nil - } - - networkMessage := messaging.PocketEnvelope{} - if err := proto.Unmarshal(appMsgData, &networkMessage); err != nil { +// handlePocketEnvelope deserializes the received `PocketEnvelope` data and publishes +// a copy of its `Content` to the application event bus. +func (m *p2pModule) handlePocketEnvelope(pocketEnvelopeBz []byte) error { + poktEnvelope := messaging.PocketEnvelope{} + if err := proto.Unmarshal(pocketEnvelopeBz, &poktEnvelope); err != nil { return fmt.Errorf("decoding network message: %w", err) } event := messaging.PocketEnvelope{ - Content: networkMessage.Content, + Content: poktEnvelope.Content, } m.GetBus().PublishEventToBus(&event) return nil @@ -448,9 +349,3 @@ func (m *p2pModule) getMultiaddr() (multiaddr.Multiaddr, error) { "%s:%d", m.cfg.Hostname, m.cfg.Port, )) } - -// newReadStreamDeadline returns a future deadline -// based on the read stream timeout duration. -func newReadStreamDeadline() time.Time { - return time.Now().Add(readStreamTimeout) -} diff --git a/p2p/module_raintree_test.go b/p2p/module_raintree_test.go index a42ea199e..9bd873913 100644 --- a/p2p/module_raintree_test.go +++ b/p2p/module_raintree_test.go @@ -1,3 +1,5 @@ +//go:build test + package p2p import ( @@ -13,10 +15,12 @@ import ( libp2pNetwork "github.com/libp2p/go-libp2p/core/network" mocknet "github.com/libp2p/go-libp2p/p2p/net/mock" - "github.com/pokt-network/pocket/internal/testutil" - "github.com/pokt-network/pocket/p2p/protocol" "github.com/stretchr/testify/require" "google.golang.org/protobuf/types/known/anypb" + + "github.com/pokt-network/pocket/internal/testutil" + "github.com/pokt-network/pocket/p2p/protocol" + "github.com/pokt-network/pocket/p2p/raintree" ) // TODO(#314): Add the tooling and instructions on how to generate unit tests in this file. @@ -272,7 +276,7 @@ func testRainTreeCalls(t *testing.T, origNode string, networkSimulationConfig Te mod := *p2pMod p2pMod.host.SetStreamHandler(protocol.PoktProtocolID, func(stream libp2pNetwork.Stream) { log.Printf("[valID: %s] Read\n", sURL) - (&mod).handleStream(stream) + (&mod).router.(*raintree.RainTreeRouter).HandleStream(stream) wg.Done() }) } diff --git a/p2p/raintree/logging.go b/p2p/raintree/logging.go new file mode 100644 index 000000000..bbe3e6c3b --- /dev/null +++ b/p2p/raintree/logging.go @@ -0,0 +1,31 @@ +package raintree + +import ( + libp2pNetwork "github.com/libp2p/go-libp2p/core/network" + + "github.com/pokt-network/pocket/logger" + "github.com/pokt-network/pocket/p2p/utils" +) + +// logStream logs the incoming stream and its scope stats +func (rtr *rainTreeRouter) logStream(stream libp2pNetwork.Stream) { + rtr.logStreamScopeStats(stream) + + remotePeer, err := utils.PeerFromLibp2pStream(stream) + if err != nil { + rtr.logger.Debug().Err(err).Msg("getting remote remotePeer") + } else { + utils.LogIncomingMsg(rtr.logger, rtr.getHostname(), remotePeer) + } +} + +// logStreamScopeStats logs the incoming stream's scope stats +// (see: https://pkg.go.dev/github.com/libp2p/go-libp2p@v0.27.0/core/network#StreamScope) +func (rtr *rainTreeRouter) logStreamScopeStats(stream libp2pNetwork.Stream) { + if err := utils.LogScopeStatFactory( + &logger.Global.Logger, + "stream scope (read-side)", + )(stream.Scope()); err != nil { + rtr.logger.Debug().Err(err).Msg("logging stream scope stats") + } +} diff --git a/p2p/raintree/peers_manager_test.go b/p2p/raintree/peers_manager_test.go index fd1f6a906..4a7523208 100644 --- a/p2p/raintree/peers_manager_test.go +++ b/p2p/raintree/peers_manager_test.go @@ -288,6 +288,7 @@ func testRainTreeMessageTargets(t *testing.T, expectedMsgProp *ExpectedRainTreeM hostMock := mocksP2P.NewMockHost(ctrl) hostMock.EXPECT().Peerstore().Return(libp2pPStore).AnyTimes() + hostMock.EXPECT().SetStreamHandler(gomock.Any(), gomock.Any()).Times(1) rtCfg := &config.RainTreeConfig{ Host: hostMock, diff --git a/p2p/raintree/router.go b/p2p/raintree/router.go index f34611197..bceb4b9dc 100644 --- a/p2p/raintree/router.go +++ b/p2p/raintree/router.go @@ -2,11 +2,17 @@ package raintree import ( "fmt" + "io" "log" + "time" libp2pHost "github.com/libp2p/go-libp2p/core/host" + libp2pNetwork "github.com/libp2p/go-libp2p/core/network" + "google.golang.org/protobuf/proto" + "github.com/pokt-network/pocket/logger" "github.com/pokt-network/pocket/p2p/config" + "github.com/pokt-network/pocket/p2p/protocol" "github.com/pokt-network/pocket/p2p/providers" "github.com/pokt-network/pocket/p2p/providers/peerstore_provider" typesP2P "github.com/pokt-network/pocket/p2p/types" @@ -19,9 +25,14 @@ import ( "github.com/pokt-network/pocket/shared/modules" "github.com/pokt-network/pocket/shared/modules/base_modules" telemetry "github.com/pokt-network/pocket/telemetry" - "google.golang.org/protobuf/proto" ) +// TECHDEBT(#629): configure timeouts. Consider security exposure vs. real-world conditions. +// TECHDEBT(#629): parameterize and expose via config. +// readStreamTimeout is the duration to wait for a read operation on a +// stream to complete, after which the stream is closed ("timed out"). +const readStreamTimeout = time.Second * 10 + var ( _ typesP2P.Router = &rainTreeRouter{} _ modules.IntegratableModule = &rainTreeRouter{} @@ -34,7 +45,9 @@ type rainTreeRouter struct { base_modules.IntegratableModule logger *modules.Logger - // host represents a libp2p network node, it encapsulates a libp2p peerstore + // handler is the function to call when a message is received. + handler typesP2P.RouterHandler + // host represents a libp2p libp2pNetwork node, it encapsulates a libp2p peerstore // & connection manager. `libp2p.New` configures and starts listening // according to options. // (see: https://pkg.go.dev/github.com/libp2p/go-libp2p#section-readme) @@ -66,6 +79,7 @@ func (*rainTreeRouter) Create(bus modules.Bus, cfg *config.RainTreeConfig) (type pstoreProvider: cfg.PeerstoreProvider, currentHeightProvider: cfg.CurrentHeightProvider, logger: routerLogger, + handler: cfg.Handler, } rtr.SetBus(bus) @@ -73,6 +87,7 @@ func (*rainTreeRouter) Create(bus modules.Bus, cfg *config.RainTreeConfig) (type return nil, err } + rtr.host.SetStreamHandler(protocol.PoktProtocolID, rtr.handleStream) return typesP2P.Router(rtr), nil } @@ -181,8 +196,9 @@ func (rtr *rainTreeRouter) sendInternal(data []byte, address cryptoPocket.Addres return nil } -// HandleNetworkData implements the respective member of `typesP2P.Router`. -func (rtr *rainTreeRouter) HandleNetworkData(data []byte) ([]byte, error) { +// handleRainTreeMsg handles a RainTree message, continuing broadcast propagation +// if applicable. Returns the serialized `PocketEnvelope` data contained within. +func (rtr *rainTreeRouter) handleRainTreeMsg(data []byte) ([]byte, error) { blockHeightInt := rtr.GetBus().GetConsensusModule().CurrentHeight() blockHeight := fmt.Sprintf("%d", blockHeightInt) @@ -282,6 +298,81 @@ func (rtr *rainTreeRouter) Size() int { return rtr.peersManager.GetPeerstore().Size() } +// handleStream ensures the peerstore contains the remote peer and then reads +// the incoming stream in a new go routine. +func (rtr *rainTreeRouter) handleStream(stream libp2pNetwork.Stream) { + rtr.logger.Debug().Msg("handling incoming stream") + peer, err := utils.PeerFromLibp2pStream(stream) + if err != nil { + rtr.logger.Error().Err(err). + Str("address", peer.GetAddress().String()). + Msg("parsing remote peer identity") + + if err = stream.Reset(); err != nil { + rtr.logger.Error().Err(err).Msg("resetting stream") + } + return + } + + if err := rtr.AddPeer(peer); err != nil { + rtr.logger.Error().Err(err). + Str("address", peer.GetAddress().String()). + Msg("adding remote peer to router") + } + + go rtr.readStream(stream) +} + +// readStream reads the incoming stream, extracts the serialized `PocketEnvelope` +// data from the incoming `RainTreeMessage`, and passes it to the application by +// calling the configured `rtr.handler`. Intended to be called in a go routine. +func (rtr *rainTreeRouter) readStream(stream libp2pNetwork.Stream) { + // Time out if no data is sent to free resources. + // NB: tests using libp2p's `mocknet` rely on this not returning an error. + if err := stream.SetReadDeadline(newReadStreamDeadline()); err != nil { + // `SetReadDeadline` not supported by `mocknet` streams. + rtr.logger.Error().Err(err).Msg("setting stream read deadline") + } + + // log incoming stream + rtr.logStream(stream) + + // read stream + rainTreeMsgBz, err := io.ReadAll(stream) + if err != nil { + rtr.logger.Error().Err(err).Msg("reading from stream") + if err := stream.Reset(); err != nil { + rtr.logger.Error().Err(err).Msg("resetting stream (read-side)") + } + return + } + + // done reading; reset to signal this to remote peer + // NB: failing to reset the stream can easily max out the number of available + // network connections on the receiver's side. + if err := stream.Reset(); err != nil { + rtr.logger.Error().Err(err).Msg("resetting stream (read-side)") + } + + // extract `PocketEnvelope` from `RainTreeMessage` (& continue propagation) + poktEnvelopeBz, err := rtr.handleRainTreeMsg(rainTreeMsgBz) + if err != nil { + rtr.logger.Error().Err(err).Msg("handling raintree message") + return + } + + // There was no error, but we don't need to forward this to the app-specific bus. + // For example, the message has already been handled by the application. + if poktEnvelopeBz == nil { + return + } + + // call configured handler to forward to app-specific bus + if err := rtr.handler(poktEnvelopeBz); err != nil { + rtr.logger.Error().Err(err).Msg("handling pocket envelope") + } +} + // shouldSendToTarget returns false if target is self. func shouldSendToTarget(target target) bool { return !target.isSelf @@ -311,3 +402,9 @@ func (rtr *rainTreeRouter) setupPeerManager(pstore typesP2P.Peerstore) (err erro func (rtr *rainTreeRouter) getHostname() string { return rtr.GetBus().GetRuntimeMgr().GetConfig().P2P.Hostname } + +// newReadStreamDeadline returns a future deadline +// based on the read stream timeout duration. +func newReadStreamDeadline() time.Time { + return time.Now().Add(readStreamTimeout) +} diff --git a/p2p/raintree/testutil.go b/p2p/raintree/testutil.go new file mode 100644 index 000000000..ebcb464b5 --- /dev/null +++ b/p2p/raintree/testutil.go @@ -0,0 +1,13 @@ +//go:build test + +package raintree + +import libp2pNetwork "github.com/libp2p/go-libp2p/core/network" + +// RainTreeRouter exports `rainTreeRouter` for testing purposes. +type RainTreeRouter = rainTreeRouter + +// HandleStream exports `rainTreeRouter#handleStream` for testing purposes. +func (rtr *rainTreeRouter) HandleStream(stream libp2pNetwork.Stream) { + rtr.handleStream(stream) +} diff --git a/p2p/transport_encryption_test.go b/p2p/transport_encryption_test.go index 76ab15fae..d95cb6496 100644 --- a/p2p/transport_encryption_test.go +++ b/p2p/transport_encryption_test.go @@ -9,6 +9,9 @@ import ( "github.com/golang/mock/gomock" "github.com/libp2p/go-libp2p" "github.com/multiformats/go-multiaddr" + "github.com/stretchr/testify/require" + + "github.com/pokt-network/pocket/internal/testutil" "github.com/pokt-network/pocket/p2p/protocol" typesP2P "github.com/pokt-network/pocket/p2p/types" "github.com/pokt-network/pocket/p2p/utils" @@ -18,7 +21,6 @@ import ( cryptoPocket "github.com/pokt-network/pocket/shared/crypto" "github.com/pokt-network/pocket/shared/modules" mockModules "github.com/pokt-network/pocket/shared/modules/mocks" - "github.com/stretchr/testify/require" ) func TestP2pModule_Insecure_Error(t *testing.T) { @@ -64,6 +66,13 @@ func TestP2pModule_Insecure_Error(t *testing.T) { telemetryMock.EXPECT().GetBus().Return(busMock).AnyTimes() telemetryMock.EXPECT().SetBus(busMock).AnyTimes() + serviceURLs := make([]string, len(genesisStateMock.Validators)) + for i, actor := range genesisStateMock.Validators { + serviceURLs[i] = actor.ServiceUrl + } + dnsDone := testutil.PrepareDNSMockFromServiceURLs(t, serviceURLs) + t.Cleanup(dnsDone) + p2pMod, err := Create(busMock) require.NoError(t, err) diff --git a/p2p/types/router.go b/p2p/types/router.go index 5a972d57f..f9c7f2d74 100644 --- a/p2p/types/router.go +++ b/p2p/types/router.go @@ -20,13 +20,10 @@ type Router interface { GetPeerstore() Peerstore AddPeer(peer Peer) error RemovePeer(peer Peer) error - - // This function was added to specifically support the RainTree implementation. - // Handles the raw data received from the network and returns the data to be processed - // by the application layer. - HandleNetworkData(data []byte) ([]byte, error) } +type RouterHandler func(data []byte) error + // RouterConfig is used to configure `Router` implementations and to test a // given configuration's validity. type RouterConfig interface {