sniffer block from message #1510

Shourya742 · 2025-02-26T10:26:29Z

This PR adds Block from message Action to sniffer. Closes #1506

codecov · 2025-02-26T10:28:58Z

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 17.88%. Comparing base (01d6ec6) to head (0afdf9b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
roles/tests-integration/lib/sniffer.rs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1510      +/-   ##
==========================================
- Coverage   17.88%   17.88%   -0.01%     
==========================================
  Files         132      132              
  Lines       10068    10072       +4     
==========================================
  Hits         1801     1801              
- Misses       8267     8271       +4

Flag	Coverage Δ
binary_codec_sv2-coverage	`0.00% <ø> (ø)`
binary_sv2-coverage	`6.96% <ø> (ø)`
bip32_derivation-coverage	`0.00% <ø> (ø)`
buffer_sv2-coverage	`37.68% <ø> (ø)`
codec_sv2-coverage	`0.02% <ø> (ø)`
common_messages_sv2-coverage	`0.17% <ø> (ø)`
const_sv2-coverage	`0.00% <ø> (ø)`
error_handling-coverage	`0.00% <ø> (ø)`
framing_sv2-coverage	`0.37% <ø> (ø)`
jd_client-coverage	`0.42% <ø> (ø)`
jd_server-coverage	`13.07% <ø> (ø)`
job_declaration_sv2-coverage	`0.00% <ø> (ø)`
key-utils-coverage	`3.61% <ø> (ø)`
mining-coverage	`3.17% <ø> (ø)`
mining_device-coverage	`0.00% <ø> (ø)`
mining_proxy_sv2-coverage	`0.82% <ø> (ø)`
noise_sv2-coverage	`5.79% <ø> (ø)`
pool_sv2-coverage	`2.46% <ø> (ø)`
protocols	`23.91% <ø> (ø)`
roles	`6.96% <0.00%> (-0.01%)`	⬇️
roles_logic_sv2-coverage	`11.59% <ø> (ø)`
sv2_ffi-coverage	`0.00% <ø> (ø)`
template_distribution_sv2-coverage	`0.00% <ø> (ø)`
translator_sv2-coverage	`9.58% <ø> (ø)`
utils	`36.39% <ø> (ø)`
v1-coverage	`3.11% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

jbesraa

Looks good. Can you please add a test or expand another test under sniffer_integration to cover BlockFromMessage behavior?

roles/tests-integration/lib/sniffer.rs

Shourya742 · 2025-02-28T13:05:30Z

@plebhash @jbesraa All suggestions have been incorporated. Kindly review the changes.

roles/tests-integration/tests/sniffer_integration.rs

plebhash · 2025-02-28T16:02:55Z

roles/tests-integration/lib/sniffer.rs

+    BlockFromMessage(BlockFromMessage),
+    /// Intercepts and modifies a message before forwarding it.
+    InterceptMessage(Box<InterceptMessage>),


Clippy suggests Box<InterceptMessage> to optimize enum size by storing large variants on the heap, preventing excessive stack usage.

it is probably because of replacement_message: PoolMessages<'static>. It feels like something to actually fix in a different PR. i.e., try to optimize PoolMessages size.

I think the naming is a bit confusing around here: Maybe Action should be InterceptAction and InterceptMessage should be ReplaceMessage

I think the naming is a bit confusing around here: Maybe Action should be InterceptAction and InterceptMessage should be ReplaceMessage

I agree

roles/tests-integration/lib/sniffer.rs

roles/tests-integration/tests/sniffer_integration.rs

jbesraa · 2025-03-01T09:08:08Z

roles/tests-integration/lib/sniffer.rs

+    BlockFromMessage(BlockFromMessage),
+    /// Intercepts and modifies a message before forwarding it.
+    InterceptMessage(Box<InterceptMessage>),


it is probably because of replacement_message: PoolMessages<'static>. It feels like something to actually fix in a different PR. i.e., try to optimize PoolMessages size.

Shourya742 · 2025-03-02T04:59:01Z

@jbesraa @plebhash I added the suggested abstraction and made the test more stable by intercepting SetupConnectionSuccess. Everything is working as expected. I also think Action should have its own module, but this can be deferred to a later PR.

roles/tests-integration/tests/sniffer_integration.rs

jbesraa · 2025-03-04T07:42:27Z

roles/tests-integration/lib/sniffer.rs

+    BlockFromMessage(BlockFromMessage),
+    /// Intercepts and modifies a message before forwarding it.
+    InterceptMessage(Box<InterceptMessage>),


I think the naming is a bit confusing around here: Maybe Action should be InterceptAction and InterceptMessage should be ReplaceMessage

roles/tests-integration/lib/sniffer.rs

This commit introduces an action construct that encapsulates all sniffer actions. Additionally, it adds the `BlockFromMessage` action, allowing message streams to be blocked after a specific message is encountered. something

roles/tests-integration/lib/sniffer.rs

…ept action

jbesraa

Looks good. Added a few nits but still approving this as it blocking a couple of tests. I can handle the nits in a followup

jbesraa · 2025-03-05T10:53:40Z

roles/tests-integration/lib/sniffer.rs

+impl InterceptAction {
+    /// Returns the action if it is `IgnoreFromMessage` or `ReplaceMessage`  
+    /// with the specified message type.
+    pub fn find_matching_action(


Maybe something like the following to reduce duplicated code(and also receive &self instead of Option<action>, otherwise why would you call this function with a None value?):

Suggested change

pub fn find_matching_action(

pub fn find_matching_action(

&self,

msg_type: MsgType,

direction: MessageDirection,

) -> Option<InterceptAction> {

match self {

InterceptAction::IgnoreFromMessage(bm)

| InterceptAction::ReplaceMessage(bm)

if bm.direction == direction && bm.expected_message_type == msg_type =>

{

Some(action)

}

_ => None,

}

}

jbesraa · 2025-03-05T10:57:45Z

roles/tests-integration/tests/sniffer_integration.rs

+        MESSAGE_TYPE_SETUP_CONNECTION_SUCCESS,
+    );
+
+    // `sniffer_a` intercepts and blocks `SetupConnectionSuccess` messages.


sniffer_a is just forwarding the messages, I think this is for sniffer_b

jbesraa · 2025-03-05T10:58:08Z

roles/tests-integration/tests/sniffer_integration.rs

+    // `sniffer_a` intercepts and blocks `SetupConnectionSuccess` messages.
+    let (sniffer_a, sniffer_a_addr) = start_sniffer("A".to_string(), tp_addr, false, None).await;
+
+    // `sniffer_b` is placed downstream of `sniffer_a` and should receive nothing.


This should receive and block

Shourya742 mentioned this pull request Feb 26, 2025

add-jds-do-not-stackoverflow-when-no-token-test #1505

Draft

jbesraa suggested changes Feb 28, 2025

View reviewed changes

roles/tests-integration/lib/sniffer.rs Outdated Show resolved Hide resolved

roles/tests-integration/lib/sniffer.rs Outdated Show resolved Hide resolved

roles/tests-integration/lib/sniffer.rs Outdated Show resolved Hide resolved

plebhash reviewed Feb 28, 2025

View reviewed changes

roles/tests-integration/lib/sniffer.rs Outdated Show resolved Hide resolved

Shourya742 force-pushed the 2025-02-26-sniffer-block-message branch 2 times, most recently from 0219b08 to 4e70894 Compare February 28, 2025 12:56

Shourya742 requested review from jbesraa and plebhash February 28, 2025 13:04

plebhash reviewed Feb 28, 2025

View reviewed changes

roles/tests-integration/tests/sniffer_integration.rs Outdated Show resolved Hide resolved

plebhash reviewed Feb 28, 2025

View reviewed changes

jbesraa suggested changes Mar 1, 2025

View reviewed changes

Shourya742 force-pushed the 2025-02-26-sniffer-block-message branch 3 times, most recently from db527fe to 70ee5c9 Compare March 2, 2025 04:55

jbesraa reviewed Mar 4, 2025

View reviewed changes

Shourya742 added 2 commits March 4, 2025 17:24

Add action construct and BlockFromMessage action

46a6c94

This commit introduces an action construct that encapsulates all sniffer actions. Additionally, it adds the `BlockFromMessage` action, allowing message streams to be blocked after a specific message is encountered. something

add test to test blockfrommessage functionality

3454315

Shourya742 force-pushed the 2025-02-26-sniffer-block-message branch from 95b7cce to 3a40244 Compare March 4, 2025 11:54

plebhash reviewed Mar 4, 2025

View reviewed changes

roles/tests-integration/lib/sniffer.rs Outdated Show resolved Hide resolved

Shourya742 added 3 commits March 4, 2025 19:27

add abstraction for repeatitive code

a9cbf0a

Add doc on blocked flag used in sniffer and remove tracning duplicate

fcdb68e

rename InterceptMessage to replaceMessage and change action to interc…

0afdf9b

…ept action

Shourya742 force-pushed the 2025-02-26-sniffer-block-message branch from 3a40244 to 0afdf9b Compare March 4, 2025 14:11

jbesraa approved these changes Mar 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sniffer block from message #1510

sniffer block from message #1510

Shourya742 commented Feb 26, 2025

codecov bot commented Feb 26, 2025 •

edited

Loading

jbesraa left a comment

Shourya742 commented Feb 28, 2025

plebhash Feb 28, 2025

Shourya742 Feb 28, 2025

jbesraa Mar 1, 2025

jbesraa Mar 4, 2025

plebhash Mar 4, 2025

jbesraa Mar 1, 2025

Shourya742 commented Mar 2, 2025

jbesraa Mar 4, 2025

jbesraa left a comment •

edited

Loading

jbesraa Mar 5, 2025

jbesraa Mar 5, 2025

jbesraa Mar 5, 2025

-    pub fn find_matching_action(
+    pub fn find_matching_action(
+    &self,
+    msg_type: MsgType,
+    direction: MessageDirection,
+) -> Option<InterceptAction> {
+    match self {
+        InterceptAction::IgnoreFromMessage(bm)
+        | InterceptAction::ReplaceMessage(bm)
+            if bm.direction == direction && bm.expected_message_type == msg_type =>
+        {
+            Some(action)
+        }
+        _ => None,
+    }
+}

sniffer block from message #1510

Are you sure you want to change the base?

sniffer block from message #1510

Conversation

Shourya742 commented Feb 26, 2025

codecov bot commented Feb 26, 2025 • edited Loading

Codecov Report

jbesraa left a comment

Choose a reason for hiding this comment

Shourya742 commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shourya742 commented Mar 2, 2025

Choose a reason for hiding this comment

jbesraa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2025 •

edited

Loading

jbesraa left a comment •

edited

Loading