-
Notifications
You must be signed in to change notification settings - Fork 13
Reject non mining protocol SetupConnection message in Pool #173
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
base: main
Are you sure you want to change the base?
Reject non mining protocol SetupConnection message in Pool #173
Conversation
304c516 to
4634ca6
Compare
| pool_translator_sniffer | ||
| .wait_for_message_type( | ||
| MessageDirection::ToDownstream, | ||
| MESSAGE_TYPE_SETUP_CONNECTION_ERROR, | ||
| ) | ||
| .await; |
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.
we should also assert for error_core
not sure if this will compile, but hopefully you get the point
| pool_translator_sniffer | |
| .wait_for_message_type( | |
| MessageDirection::ToDownstream, | |
| MESSAGE_TYPE_SETUP_CONNECTION_ERROR, | |
| ) | |
| .await; | |
| pool_translator_sniffer | |
| .wait_for_message_type( | |
| MessageDirection::ToDownstream, | |
| MESSAGE_TYPE_SETUP_CONNECTION_ERROR, | |
| ) | |
| .await; | |
| let setup_connection_error = sniffer.next_message_from_upstream(); | |
| let setup_connection_error = match setup_connection_error { | |
| Some((_, AnyMessage::Common(CommonMessages::SetupConnectionError(msg)))) => msg, | |
| msg => panic!("Expected SetupConnectionError message, found: {:?}", msg), | |
| }; | |
| assert_eq!( | |
| setup_connection_error.error_code, "unsupported-protocol", | |
| "SetupConnectionError message error code should be unsupported-protocol" | |
| ); |
|
running this new test on my machine I always get a timeout, like the one the CI is showing now: https://github.com/stratum-mining/sv2-apps/actions/runs/20849689219/job/59901524348?pr=173 tested at 4634ca6 seems that the SetupConnection scenario is always triggered correctly but the test never finishes: 2026-01-09T15:12:48.966563Z INFO integration_tests_sv2::utils: 🔍 Sv2 Sniffer 0 | Replaced: SetupConnection with SetupConnection | Direction: ⬆
2026-01-09T15:12:48.966715Z INFO pool_sv2::downstream::common_message_handler: Received `SetupConnection`: version=2, flags=0
2026-01-09T15:12:48.966722Z INFO pool_sv2::downstream::common_message_handler: Rejecting connection: SetupConnection asking for other protocols than mining protocol.
2026-01-09T15:12:48.966734Z ERROR pool_sv2::downstream: Failed to set up downstream connection e=Shutdown
2026-01-09T15:12:48.966742Z ERROR pool_sv2::status: Error in Downstream { downstream_id: 1, tx: Sender { .. } }: Shutdown
2026-01-09T15:12:48.966749Z WARN pool_sv2::status: Downstream [1] shutting down due to error: Shutdown
2026-01-09T15:12:48.966754Z DEBUG pool_sv2::status: Sending status from Downstream [1]: DownstreamShutdown { downstream_id: 1, reason: Shutdown }
2026-01-09T15:12:48.966812Z WARN pool_sv2: Downstream 1 disconnected — Channel manager.
2026-01-09T15:12:48.966840Z INFO pool_sv2::channel_manager: Channel Manager: removing downstream after shutdown downstream_id=1
2026-01-09T15:12:48.966858Z WARN pool_sv2::io_task: Reader task exited.
2026-01-09T15:12:48.966882Z WARN pool_sv2::io_task: Writer task exited.
2026-01-09T15:12:48.966972Z ERROR stratum_apps::network_helpers::noise_connection: Reader: error while reading frame: SocketClosed |
4af6ae2 to
8129fda
Compare
|
after running with the latests patches the test tested @ 8129fda |
… still send SetupConnection.Error)
8129fda to
d29c4ee
Compare
This PR enforces rejection of all downstream connections that do not use the mining protocol. It removes hardcoded address information from the SetupConnection message throughout the codebase and adds integration tests to cover the new behavior.
Closes: #92