-
Notifications
You must be signed in to change notification settings - Fork 766
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
HandleInternal
interface method in place ofHandleInbound
for internal messages. - Updates
ChainRouter
to implementHandleInternal
and skip the allowlist check when routing internal messages. - Adds tracing for internal message handling in
tracedRouter
and 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
HandleInternal
mentions 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
go
invocation to a synchronous call may block the sender ifHandleInternal
does 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?