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: refactor gossipsub to validation and handling #5976

Merged

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Apr 8, 2024

Summary

  • Added MessageProcessor similar to MessageHandler but working in two stages: validate and handle
  • Split processIncomingTxn txhandler method into two validate + handle
  • This allowed to use gossipsub's validate and handing framework as intended.

Test Plan

  • Existing tests should work

@algorandskiy algorandskiy added Enhancement p2p Work related to the p2p project labels Apr 8, 2024
@algorandskiy algorandskiy requested a review from cce April 8, 2024 22:37
@algorandskiy algorandskiy self-assigned this Apr 8, 2024
Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Attention: Patch coverage is 65.98639% with 50 lines in your changes are missing coverage. Please review.

Project coverage is 55.24%. Comparing base (d38f35c) to head (6f9e914).

Files Patch % Lines
data/txHandler.go 52.63% 35 Missing and 1 partial ⚠️
network/hybridNetwork.go 0.00% 6 Missing ⚠️
network/multiplexer.go 87.75% 4 Missing and 2 partials ⚠️
network/wsNetwork.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           feature/p2p    #5976      +/-   ##
===============================================
- Coverage        55.97%   55.24%   -0.74%     
===============================================
  Files              495      495              
  Lines            68896    69009     +113     
===============================================
- Hits             38562    38121     -441     
- Misses           27708    28273     +565     
+ Partials          2626     2615      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@algorandskiy algorandskiy marked this pull request as ready for review April 9, 2024 23:41
@algorandskiy algorandskiy changed the title WIP: refactor gossipsub to validation and handling p2p: refactor gossipsub to validation and handling Apr 9, 2024
@@ -24,32 +24,55 @@ import (
// Multiplexer is a message handler that sorts incoming messages by Tag and passes
// them along to the relevant message handler for that type of message.
type Multiplexer struct {
msgHandlers atomic.Value // stores map[Tag]MessageHandler, an immutable map.
msgHandlers atomic.Value // stores map[Tag]MessageHandler, an immutable map.
msgProcessors atomic.Value // stores map[Tag]MessageProcessor, an immutable map.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth updating network/multiplexer_test.go as well

network/p2pNetwork.go Outdated Show resolved Hide resolved
@algorandskiy
Copy link
Contributor Author

tools cross-check fails but this is OK since the entire p2p branch needs updates from master. I plan to do that after merging these small PRs in to feature/p2p

data/txHandler.go Show resolved Hide resolved
data/txHandler.go Outdated Show resolved Hide resolved
data/txHandler.go Outdated Show resolved Hide resolved
data/txHandler.go Show resolved Hide resolved
data/txHandler.go Show resolved Hide resolved
data/txHandler.go Show resolved Hide resolved
Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

LGTM! I added a few suggestions for a little additional documentation but I like it, I can't imagine chopping up processor/validator would have any impact on performance for the wsNetwork?

var isDup bool
if handler.msgCache != nil {
// check for duplicate messages
// this helps against relaying duplicates
if msgKey, isDup = handler.msgCache.CheckAndPut(rawmsg.Data); isDup {
if msgKey, isDup = handler.msgCache.CheckAndPut(data); isDup {
Copy link
Contributor

Choose a reason for hiding this comment

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

gossipsub has its own duplicate checker too, depending on the message hash function implementation? I wonder how often you will hit this when gossipsub is in front of it

@algorandskiy algorandskiy merged commit 411837f into algorand:feature/p2p May 10, 2024
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement p2p Work related to the p2p project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants