-
Notifications
You must be signed in to change notification settings - Fork 186
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
Adds Application Specific RPC Inspector #509
Conversation
* Enables non-atomic validation for peer scoring parameters (libp2p#499) * decouples topic scoring parameters * adds skiping atomic validation for topic parameters * cleans up * adds skip atomic validation to peer score threshold * adds skip atomic validation for peer parameters * adds test for non-atomic validation * adds tests for peer score * adds tests for peer score thresholds * refactors tests * chore: Update .github/workflows/stale.yml [skip ci] * adds with gossipsub tracker Co-authored-by: libp2p-mgmt-read-write[bot] <104492852+libp2p-mgmt-read-write[bot]@users.noreply.github.com>
This reverts commit 1c91995.
This reverts commit 352d747.
…ahya/adds-rpc-inspector
…' into yahya/adds-rpc-inspector" This reverts commit 586c5cb.
This reverts commit 2e13ee8.
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.
Much better!
Lets move this to the base pubsub object, it is not limited to gossipsub!
@vyzo applied |
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.
lets fix the func signature and also type alias it, other than that LGTM.
pubsub.go
Outdated
// appSpecificRpcInspector is an auxiliary that may be set by the application to inspect incoming RPCs prior to | ||
// processing them. The inspector is invoked on an accepted RPC right prior to handling it. | ||
// The return value of the inspector function is a boolean indicating whether the RPC should be processed or not. | ||
appSpecificRpcInspector func(peer.ID, *RPC) bool |
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.
lets return a loggable error to provide some context.
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.
pubsub.go
Outdated
@@ -527,6 +532,13 @@ func WithSeenMessagesTTL(ttl time.Duration) Option { | |||
} | |||
} | |||
|
|||
func WithAppSpecificRpcInspector(inspector func(peer.ID, *RPC) bool) Option { |
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.
Also, lets make a type alias for the user inspector fun, returning error.
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.
pubsub.go
Outdated
// pass the rpc through app specific validation (if any available). | ||
if p.appSpecificRpcInspector != nil { | ||
// check if the RPC is allowed by the external inspector | ||
if accept := p.appSpecificRpcInspector(rpc.from, rpc); !accept { |
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.
If err := ....; err != nil {
Log.Debugf(...)
return
}
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.
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.
This is great, thank you.
@@ -527,6 +533,13 @@ func WithSeenMessagesTTL(ttl time.Duration) Option { | |||
} | |||
} | |||
|
|||
func WithAppSpecificRpcInspector(inspector func(peer.ID, *RPC) error) Option { |
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 know this is already merged, but we probably want a docstring on this. Just wanted to flag this 🙂
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.
Good point.
I am ready to cut a release,but i'll wait for a docstring here.
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.
👍 PR incoming.
3564: [BFT Testing] Gossipsub spam test Framework - IHAVE r=gomisha a=gomisha This is Framework implementation of a spam test using libp2p gossipsub IHAVE messages. It sets up a 2 node test between a victim node and a spammer. The spammer sends a few IHAVE control messages to the victim node without being subscribed to any of the same topics. The test then checks that the victim node received all the messages from the spammer. - initial implementation of a general purpose Spammer that will be used for future libp2p spam testing - uses recent pubsub improvements like the RPC ingress message inspector (libp2p/go-libp2p-pubsub#509) to detect received messages on the victim node ref: https://github.com/dapperlabs/flow-go/issues/6423 Co-authored-by: Yahya Hassanzadeh <yhassanzadeh13@ku.edu.tr> Co-authored-by: Misha <misha.rybalov@dapperlabs.com>
3564: [BFT Testing] Gossipsub spam test Framework - IHAVE r=gomisha a=gomisha This is Framework implementation of a spam test using libp2p gossipsub IHAVE messages. It sets up a 2 node test between a victim node and a spammer. The spammer sends a few IHAVE control messages to the victim node without being subscribed to any of the same topics. The test then checks that the victim node received all the messages from the spammer. - initial implementation of a general purpose Spammer that will be used for future libp2p spam testing - uses recent pubsub improvements like the RPC ingress message inspector (libp2p/go-libp2p-pubsub#509) to detect received messages on the victim node ref: https://github.com/dapperlabs/flow-go/issues/6455 Co-authored-by: Yahya Hassanzadeh <yhassanzadeh13@ku.edu.tr> Co-authored-by: Misha <misha.rybalov@dapperlabs.com>
3564: [BFT Testing] Gossipsub spam test Framework - IHAVE r=gomisha a=gomisha This is Framework implementation of a spam test using libp2p gossipsub IHAVE messages. It sets up a 2 node test between a victim node and a spammer. The spammer sends a few IHAVE control messages to the victim node without being subscribed to any of the same topics. The test then checks that the victim node received all the messages from the spammer. - initial implementation of a general purpose Spammer that will be used for future libp2p spam testing - uses recent pubsub improvements like the RPC ingress message inspector (libp2p/go-libp2p-pubsub#509) to detect received messages on the victim node ref: https://github.com/dapperlabs/flow-go/issues/6455 Co-authored-by: Yahya Hassanzadeh <yhassanzadeh13@ku.edu.tr> Co-authored-by: Misha <misha.rybalov@dapperlabs.com>
This PR implements an application-specific RPC inspector for
GossipSubRouter
. This is an optional and auxilary call-back method that can be set by the application so that all accepted RPCs are inspected and only approved ones resume processing.When there is no inspected set, handling an accepted RPC goes as normal. This app-specific RPC inspector allows the application developer to: