Skip to content
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

fix: dial_feeler(..) function dial identify protocol #4784

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

yangby-cryptape
Copy link
Collaborator

What problem does this PR solve?

The dial_feeler(..) function dial with the identify protocol acctually.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Release note

None: Exclude this PR from the release note.

@yangby-cryptape yangby-cryptape requested a review from a team as a code owner January 16, 2025 09:19
@yangby-cryptape yangby-cryptape requested review from zhangsoledad and removed request for a team January 16, 2025 09:19
Copy link
Collaborator

@driftluo driftluo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yangby-cryptape
Copy link
Collaborator Author

yangby-cryptape commented Jan 16, 2025

all sessions open with identify,then it will open others ...

As your said, is it just a bad function name with bad comments?

  • I just confused why a function named dial_feeler(..) but dial with the identify protocol acctually.

ckb/network/src/network.rs

Lines 488 to 501 in b8f655a

/// Dial just feeler protocol
pub fn dial_feeler(&self, p2p_control: &ServiceControl, addr: Multiaddr) {
if let Err(err) = self.dial_inner(
p2p_control,
addr.clone(),
TargetProtocol::Single(SupportProtocols::Identify.protocol_id()),
) {
debug!("dial_feeler error {err}");
} else {
self.with_peer_registry_mut(|reg| {
reg.add_feeler(&addr);
});
}
}

Note

"... Dial just feeler protocol ..."

  • If dial with the feeler is inside the identify protocol, why don't just call dial_identify(..), with error handling.

ckb/network/src/network.rs

Lines 477 to 486 in b8f655a

/// Dial just identify protocol
pub fn dial_identify(&self, p2p_control: &ServiceControl, addr: Multiaddr) {
if let Err(err) = self.dial_inner(
p2p_control,
addr,
TargetProtocol::Single(SupportProtocols::Identify.protocol_id()),
) {
debug!("dial_identify error: {err}");
}
}

  • dial_identify(..) will also dial with feeler protocol, too.

@driftluo
Copy link
Collaborator

driftluo commented Jan 16, 2025

If dial with the feeler is inside the identify protocol, why don't just call dial_identify(..), with error handling.

dial feeler success should mark this dial is feeler, but dial identify not

  self.with_peer_registry_mut(|reg| {
      reg.add_feeler(&addr);
  });

These two dial behaviors are mutually exclusive:

  • dial feeler will open identify and feeler, then close this session
  • dial identify will open identify and all protocols except feeler, and keep connected

@yangby-cryptape
Copy link
Collaborator Author

yangby-cryptape commented Jan 17, 2025

  • dial identify will open identify and all protocols except feeler, and keep connected

If I was correct, it should be:

dial identify will open identify with all supported protocols inside it, then close feeler, and keep session connected

Am I right? (then I will add this comment into the code)

@driftluo
Copy link
Collaborator

driftluo commented Jan 17, 2025

dial identify will open identify with all supported protocols inside it, then close feeler, and keep the session connected

no, feeler never opened on dial identify

id != &SupportProtocols::Feeler.protocol_id()
&& id != &SupportProtocols::RelayV2.protocol_id()
} else {
id != &SupportProtocols::Feeler.protocol_id()

@yangby-cryptape
Copy link
Collaborator Author

yangby-cryptape commented Jan 17, 2025

no, feeler never opened on dial identify

dial_identify:

  • A dial identify to B: connected.

dial_feeler:

  • A dial identify to B: connected.
  • A mark feeler.
  • A dial feeler to B. (or B dial feeler to A in callback?)

Am I correct this time?

@driftluo
Copy link
Collaborator

driftluo commented Jan 17, 2025

dial_identify:

  • A dial identify to B: connected
  • A‘s identify open all protocols except feeler to B

dial_feeler:

  • A mark feeler
  • A dial identify to B: connected
  • A‘s identify open feeler to B

@yangby-cryptape
Copy link
Collaborator Author

yangby-cryptape commented Jan 17, 2025

dial_feeler:

  • A mark feeler
  • A dial identify to B: connected
  • A‘s identify open feeler to B

ckb/network/src/network.rs

Lines 488 to 501 in b8f655a

/// Dial just feeler protocol
pub fn dial_feeler(&self, p2p_control: &ServiceControl, addr: Multiaddr) {
if let Err(err) = self.dial_inner(
p2p_control,
addr.clone(),
TargetProtocol::Single(SupportProtocols::Identify.protocol_id()),
) {
debug!("dial_feeler error {err}");
} else {
self.with_peer_registry_mut(|reg| {
reg.add_feeler(&addr);
});
}
}

But, in above code, dial_inner is before add_feeler.
So, in my understood, dial-identify.is_ok() --> mark-feeler --> dial-feeler.

@driftluo
Copy link
Collaborator

But dial_inner is before add_feeler.

I think a better description here is, "After successfully sending the command, mark the feeler", there is no dial completed at this time

@yangby-cryptape
Copy link
Collaborator Author

yangby-cryptape commented Jan 17, 2025

there is no dial completed at this time

So, can I say:

In theory, the order is not strong (or strict) promised.
Just based on experience in most cases (maybe 99.9999999%).

For example, the return of dial_inner is hanging for a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants