-
Notifications
You must be signed in to change notification settings - Fork 33
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] refactor: message handling #763
Conversation
461d794
to
d37d14a
Compare
d37d14a
to
90d9311
Compare
65ca42d
to
a87e5cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdyt about adding the mermaid diagram in the github description to the README for p2p?
@@ -1,3 +1,5 @@ | |||
//go:build test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a linter for this? Dima recently added a custom one in build/linters/tests.go
and it looks like we have access to the File.Name
via the dsl matcher, but I don't know if there's any easy way to check for the first line.
Can be done in a separate commit, not a blocker.
type File struct {
// Name is a file base name.
Name String
// PkgPath is a file package path.
// Examples: "io/ioutil", "strings", "github.com/quasilyte/go-ruleguard/dsl".
PkgPath String
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only necessary to add the test build tag to:
- non-test files in a non-internal package which exports a private member for testing purposes.
- test files which depend on something exported by a file which uses the build tag (assumes at least one instance of case 1).
Are you suggesting a linter which enforces the test
build tag on all test files or something?
I don't suppose there would really be a prohibitive downside to adding the test tag in places where it's not strictly necessary. I can imagine an argument for either but am leaning towards "ubiquitous build tags" at the moment (unexpectedly):
- ubiquitous build tags
- pros
- consistent
- simpler
- better justifies updates to the makefile (e.g.
-tags test
; necessary in either case)
- cons
- slightly unconventional
- requires running tests via make targets OR adding tags manually (e.g. IDE run/test configs)
- UX
go test
runs no tests
- pros
- only necessary build tags
- pros
- minimal impact on the codebase
- cons
- more mental overhead
- less obvious when test(s) didn't run because of a missing tag
- UX
go test
runs some but not all tests
- pros
|
||
remotePeer, err := utils.PeerFromLibp2pStream(stream) | ||
if err != nil { | ||
rtr.logger.Debug().Err(err).Msg("getting remote remotePeer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you go with Debug
instead of Error
here?
Ditto below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(see below)
In this case, my thinking was that this error only happens in the context of this logging helper function which is not a critical function. I didn't imagine that this logging helper producing an error would be useful to the end user (i.e. not actionable nor a useful signal). Perhaps I assume too much.
|
||
import libp2pNetwork "github.com/libp2p/go-libp2p/core/network" | ||
|
||
// RainTreeRouter exports `rainTreeRouter` for testing purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this design 💯
p2p/raintree/router.go
Outdated
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. | ||
rtr.logger.Debug().Err(err).Msg("setting stream read deadline") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Noticed you're using Debug instead of Err for a lot of these. Why is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for scrutinizing this, please continue to do so! 🙌
I was trying to be conservative with what we log to non-debug loggers. In this particular case, my thinking was that being unable to set the read deadline isn't actionable and isn't really useful as a signal for end-users.
@Olshansk I was planning on incorporating the diagrams from #505, which represents the composition of this and #769, into #732. My thinking was that the diagrams in this and #769 are trimmed down versions which only show what's relevant for understanding the change, whereas what I expect we want to keep in the repo is the representation of the desired state with sufficient context. Do you imagine a step in between? |
p2p/CHANGELOG.md
Outdated
- 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.50] - 2023-05-08 | ||
|
||
- Removed unused `Transport` interface |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
66a0baa
to
77d91f1
Compare
p2p/CHANGELOG.md
Outdated
- 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.50] - 2023-05-08 | ||
|
||
- Removed unused `Transport` interface |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
* pokt/main: Update client to p1 in makefile change gitsha of private-keys.yaml (#787) Fix places where we request a password and don't check the nonInterac… (#788) [Testing] [Tooling] chore: replace `gocuke` & `go-mockdns` in go.mod (#782) [k8s] Rename `client` to `p1` in k8s localnet (#764) [Persistence] Refactors BlockStore Interface (#774) [BUG] Int casting issue when sending tx (#783) add public keys to private-keys.yaml (#779)
- 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a brief review of the code patch:
- A new version (0.0.0.52) is being added with today's date.
- The
RainTreeConfig
andBackgroundConfig
structs are updated to include a new field,Handler
. - Logging methods in the
rainTreeRouter
have been refactored. - Methods
rainTreeRouter#HandleNetworkData()
andp2pModule#handleNetworkData()
are renamed to#handleRainTreeMsg()
and#handlePocketEnvelope()
, respectively, for better consistency. - The build flags now include
-tags=test
to all test make targets. - An issue with
mockdns
was fixed in theTestP2PModule_Insecure_Error
test. - Message handling is moved from the p2p module to the router.
Overall, the changes seem reasonably-safe and no significant risks or bugs stand out. One possible improvement suggestion is to ensure that unit tests are updated accordingly to reflect method name changes such as #handleRainTreeMsg()
and #handlePocketEnvelope()
. Additionally, it may be helpful to review the impact of refactored logging methods on overall system performance or log readability.
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #763 +/- ##
===========================
===========================
☔ View full report in Codecov by Sentry. |
@@ -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() | |||
}) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code patch provided seems reasonable, but I have a few minor suggestions for improvement. Please consider the following:
-
In line 19, add a comment to explain the purpose of moving the import statement and why it has been separated from the other import statements.
-
Although not incorrect, you might want to change lines 272-276 to avoid using the pointer to 'mod'. Instead, directly use 'p2pMod'. This increases readability and reduces complexity:
mod := p2pMod ... mod.router.(*raintree.RainTreeRouter).HandleStream(stream)
-
Make sure that there are unit tests (or create new unit tests) that cover the changes introduced in this patch. The current tests should pass, and any new edge cases should also be considered when testing.
-
Ensure that you follow the rest of the project's style and coding conventions when contributing your patch.
Overall, the code patch is generally fine, but addressing these points would lead to better quality code review.
## 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 { <<protobuf>> +Level uint32 +Data []byte +Nonce uint64 } class PocketEnvelope { <<protobuf>> +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 { <<protobuf>> +Level uint32 +Data []byte +Nonce uint64 } class PocketEnvelope { <<protobuf>> +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 <!-- add details here if it a different type of change --> ## 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)
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 resultingPocketEnvelope
is returned to the P2P module to be emitted over the application event bus.After
The router encapsulates handling incoming
RainTreeMessage
s, unpacking them and passing them along to the P2P module as serializedPocketEnvelope
s. The P2P module then deserializes and emits them over the application event bus.Issue
Second deliverable in #762
Type of change
Please mark the relevant option(s):
List of changes
Handler
field toRainTreeConfig
&BackgroundConfig
rainTreeRouter
logging methodsrainTreeRouter#HandleNetworkData()
to#handleRainTreeMsg()
p2pModule#handleNetworkData()
to#handleAppData()
TestP2PModule_Insecure_Error
testTesting
make develop_test
; if any code changes were mademake test_e2e
on k8s LocalNet; if any code changes were madee2e-devnet-test
passes tests on DevNet; if any code was changedRequired Checklist
godoc
format comments on touched members (see: tip.golang.org/doc/comment)If Applicable Checklist
shared/docs/*
if I updatedshared/*
README(s)