-
Notifications
You must be signed in to change notification settings - Fork 842
Allow internal messages from disallowed nodeIDs #4024
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
Conversation
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.
Pull Request Overview
This PR updates the networking layer to introduce and handle internal messages separately, skipping the usual allowlist check for internal nodeIDs, and adds unit tests and tracing support to cover the new behavior.
- Switches the sender to use a new
HandleInternalinterface method in place ofHandleInboundfor internal messages. - Updates
ChainRouterto implementHandleInternaland skip the allowlist check when routing internal messages. - Adds tracing for internal message handling in
tracedRouterand updates mocks and tests to useHandleInternal.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| snow/networking/sender/sender_test.go | Added subtests for validatorOnly flag and replaced HandleInbound expectations with HandleInternal. |
| snow/networking/sender/sender.go | Changed router interface to InternalHandler and replaced all go HandleInbound calls with HandleInternal. |
| snow/networking/router/traced_router.go | Implemented tracing in new HandleInternal method. |
| snow/networking/router/routermock/router.go | Added mock methods and recorders for HandleInternal. |
| snow/networking/router/router.go | Extended InternalHandler interface with HandleInternal. |
| snow/networking/router/chain_router.go | Implemented HandleInternal in ChainRouter and updated routing to skip allowlist for internal messages. |
Comments suppressed due to low confidence (2)
snow/networking/router/chain_router.go:210
- [nitpick] The comment above
HandleInternalmentions lock ordering but could be updated to better explain how internal messages are enqueued and how they differ from inbound messages.
func (cr *ChainRouter) HandleInternal(ctx context.Context, msg message.InboundMessage) {
snow/networking/sender/sender.go:120
- [nitpick] Switching from asynchronous
goinvocation to a synchronous call may block the sender ifHandleInternaldoes not spawn its own goroutine. Ensure this synchronous path cannot cause deadlocks or long blocking.
s.router.HandleInternal(ctx, inMsg)
Why this should be merged
Fixes handling of internal messages from disallowed nodeIDs.
How this works
Skips allowlist check for internal messages.
How this was tested
Added unit test.
Need to be documented in RELEASES.md?