Skip to content

go/p2p: Ensure only server providers advertise themselves#6270

Merged
martintomazic merged 2 commits intomasterfrom
martin/feature/p2p-advertize-only-servers
Jul 23, 2025
Merged

go/p2p: Ensure only server providers advertise themselves#6270
martintomazic merged 2 commits intomasterfrom
martin/feature/p2p-advertize-only-servers

Conversation

@martintomazic
Copy link
Copy Markdown
Contributor

@martintomazic martintomazic commented Jul 18, 2025

Tackles #6262 (comment), see existing slack discussion.

Only hosts that serve p2p protocol should advertise themselves via seed nodes for peer discovery.

@netlify
Copy link
Copy Markdown

netlify bot commented Jul 18, 2025

Deploy Preview for oasisprotocol-oasis-core canceled.

Name Link
🔨 Latest commit 2068173
🔍 Latest deploy log https://app.netlify.com/projects/oasisprotocol-oasis-core/deploys/687e2e60f106430008f111c1

@martintomazic martintomazic force-pushed the martin/feature/p2p-advertize-only-servers branch from a6c7baa to 9b02396 Compare July 18, 2025 13:22
@martintomazic martintomazic force-pushed the martin/feature/p2p-advertize-only-servers branch from 9b02396 to 4fb3d38 Compare July 18, 2025 13:27
@martintomazic martintomazic marked this pull request as ready for review July 18, 2025 14:23
Copy link
Copy Markdown
Collaborator

@peternose peternose left a comment

Choose a reason for hiding this comment

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

This is not optimal: before that we should check all connected peers via m.host.Peerstore().GetProtocols(remoteID). This way we avoid round-trip to seed nodes. More importantly, we can reuse existing connections and only open a new stream for a new protocol.

Once we connect to a node, we always take into account all its protocols before connecting new ones. See NumProtocolPeers. So when we are out of nodes, we should discover them via seed nodes.

@martintomazic martintomazic force-pushed the martin/feature/p2p-advertize-only-servers branch from 97636bf to da92e5c Compare July 20, 2025 20:53
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.83%. Comparing base (bf911d7) to head (da92e5c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6270      +/-   ##
==========================================
+ Coverage   64.50%   64.83%   +0.33%     
==========================================
  Files         689      689              
  Lines       67114    67127      +13     
==========================================
+ Hits        43290    43525     +235     
+ Misses      18837    18602     -235     
- Partials     4987     5000      +13     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@martintomazic martintomazic requested a review from peternose July 20, 2025 22:17
@peternose
Copy link
Copy Markdown
Collaborator

^^ not an issue basic host runs identity protocol automatically.

What is identity protocol?

This unifies style with light client protocol.

In the future we may want to make protocol id and versions
private and only keep the helper function.
@martintomazic martintomazic force-pushed the martin/feature/p2p-advertize-only-servers branch from da92e5c to 99005c5 Compare July 21, 2025 09:23
Copy link
Copy Markdown
Collaborator

@peternose peternose left a comment

Choose a reason for hiding this comment

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

Looks good, will approve once commits are merged.

@martintomazic
Copy link
Copy Markdown
Contributor Author

What is identity protocol?

https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/identify/id.go - responsible for keeping protobook up to date.

This is complementary to your answer #6270 (review) as to how seed nodes are last resource.

@martintomazic martintomazic force-pushed the martin/feature/p2p-advertize-only-servers branch from 99005c5 to ebff391 Compare July 21, 2025 12:04
@martintomazic martintomazic force-pushed the martin/feature/p2p-advertize-only-servers branch from ebff391 to 2068173 Compare July 21, 2025 12:11
@martintomazic martintomazic requested a review from peternose July 21, 2025 13:23
default:
}

d.logger.Debug("triggered protocol advertisement",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This could also be optional as I just saw that we log started advertising when the running thread picks up a new namespace.

@martintomazic martintomazic merged commit 6fe595f into master Jul 23, 2025
5 checks passed
@martintomazic martintomazic deleted the martin/feature/p2p-advertize-only-servers branch July 23, 2025 17:43
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