feat(wstransport): add autotls support #1535

gmelodie · 2025-07-14T14:37:32Z

Use autotls installed certificates in wstransport.

tests/testwstransport.nim

libp2p/autotls/mockservice.nim

libp2p/transports/wstransport.nim

richard-ramos · 2025-07-16T20:30:41Z

libp2p/transports/wstransport.nim

+  var retries = 0
+  while not self.running and retries < DefaultAutotlsRetries:
+    retries += 1
+    await sleepAsync(DefaultAutotlsWaitTimeout)


This loop is a bit ugly.

Wdyt of having an async event that is fired when running is set to true, and then here the code could look like

await self.onRunning.wait().withTimeout(someTimeout)

I think this could be a change introduced in a separate PR like so:

add onRunning asyncEvent field to base Transport type

make Transport.start also fire onRunning

change other transports to benefit from this too

In a note, I saw that many transports are redundant in the way they set self.running (i.e. they set self.running = true and also call the base Transport.start method, which does the same thing). We could clean that up as well in this PR.

This seems like a good thing to do in that separate PR, yes!

libp2p/transports/wstransport.nim

richard-ramos · 2025-07-16T20:38:06Z

libp2p/switch.nim

+      if t of TcpTransport:
+        await fut
+        s.acceptFuts.add(s.accept(t))
+        s.peerInfo.listenAddrs &= t.addrs


Instead of having this special condition for TcpTransport here and in line 380, wouldn't it be possible for AutoTLSService to asynsspawn a routine that will check the switch for the listenAddrs it has, and request the certificate when the switch has an available TCP addrs?

The problem here runs a bit deeper. In line 369 we await all transports' futures, which include wstransport. For wstransport to be finished it needs certs from autotls, but autotls needs tcptransport to be running and accepting connections, which only happens in 382 if we don't have this condition here.

ws transports -> accept blocked until auto tls is ready -> blocked until tcp transport is accepting
tcp transport -> no dependencies for accepting

does this graph represent what start dependency should be? if so, then these are separate processes that can happen in separate futures?

Pretty much yes. The only thing missing there is the start functions:

ws transports -> start blocked until auto tls is ready -> blocked until tcp transport is accepting
tcp transport -> accept blocked until trasport started -> no dependencies to start

ws transports -> start blocked until auto tls is ready -> blocked until tcp transport is accepting
tcp transport -> accept blocked until trasport started -> no dependencies to start

we just need to collect all futures to single wait:
wait allFutures = transport.start() + transport.accept()

ws started (gets blocked on auto tls) -> tcp started -> ws accept started (gets blocked because not started) -> tcp accept started -> ws start gets unblocked auto tls complets -> ws accept completes

having transports [ws, tc], code would call in this order:
ws start
tcp start
ws accept
tcp accept

since tcp is not depended on anything it is guaranteed to complete first (tcp start future completes -> then tcp accept completes).
ws will complete with delay after tcp completes.

Hmmm I don't know if that's not going to break anything. Accept futures require self.running == true otherwise they fail (e.g. tcpTransport here). I think we should add a separate task to create asyncEvents that fire when running == true, making async dependencies cleaner and easier to work with (i suggested this here).

it definitely would, but that what's coming should be fixable for wizards :)

libp2p/autotls/service.nim

vladopajic · 2025-07-17T08:08:57Z

libp2p/autotls/service.nim

+    let tcpTransports = switch.transports
+      .filterIt(it of TcpTransport and it.running)
+      .mapIt(TcpTransport(it))
+
+    if tcpTransports.len == 0:
+      error "Could not find a running TcpTransport in switch"
+      return
+
+    let tcpTransport = tcpTransports[0]
+


why is this code needed? why is running tcp transport needed here? what happens with tcpTransport variable?

what happens with tcpTransport variable?

Oops! My bad there.

why is this code needed? why is running tcp transport needed here?

We need a tcpTransport to receive a dial from the autotls broker. I remember we talked about having autotls spawn its own tcp server on DMs but I spoke to @richard-ramos and we decided to go with the other approach (i.e. using a tcpTransport from the switch). Edited the code to be more straightforward.

so point of this code is to return error if tcp transport is not started ?
if so please consider improving understanding what's happening here and why. because looking at the code is not obvious.

Agreed! Can you give me some pointers on how you'd do that?

create proc hasTcpTransportStarted that returns bool

call proc in this place:

# starting auto tls requires tcp transport to be running if not hasTcpStarted(): error "Could not find a running TcpTransport in switch" return

Better now?

i dont see any change

libp2p/switch.nim

tests/testwstransport.nim

codecov-commenter · 2025-07-17T13:09:06Z

Codecov Report

Attention: Patch coverage is 56.68790% with 68 lines in your changes missing coverage. Please review.

Project coverage is 82.59%. Comparing base (b1dd0a2) to head (9f03cdb).
Report is 13 commits behind head on master.

Files with missing lines	Patch %	Lines
libp2p/autotls/service.nim	7.01%	53 Missing ⚠️
libp2p/peerinfo.nim	12.50%	7 Missing ⚠️
libp2p/autotls/mockservice.nim	84.21%	3 Missing ⚠️
libp2p/transports/wstransport.nim	90.90%	3 Missing ⚠️
libp2p/autotls/acme/client.nim	80.00%	1 Missing ⚠️
libp2p/autotls/acme/utils.nim	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1535      +/-   ##
==========================================
- Coverage   82.91%   82.59%   -0.32%     
==========================================
  Files         112      114       +2     
  Lines       18440    18672     +232     
==========================================
+ Hits        15289    15422     +133     
- Misses       3151     3250      +99

Files with missing lines	Coverage Δ
libp2p/autotls/acme/api.nim	`83.84% <100.00%> (+0.72%)`	⬆️
libp2p/autotls/utils.nim	`1.47% <ø> (ø)`
libp2p/builders.nim	`93.97% <ø> (ø)`
libp2p/dialer.nim	`87.36% <100.00%> (-0.18%)`	⬇️
libp2p/protocols/connectivity/relay/rtransport.nim	`87.62% <100.00%> (ø)`
libp2p/switch.nim	`90.86% <100.00%> (-2.18%)`	⬇️
libp2p/transports/memorytransport.nim	`89.87% <100.00%> (ø)`
libp2p/transports/quictransport.nim	`82.94% <100.00%> (+0.20%)`	⬆️
libp2p/transports/tcptransport.nim	`84.18% <100.00%> (ø)`
libp2p/transports/tortransport.nim	`84.21% <100.00%> (ø)`
... and 7 more

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

vladopajic · 2025-07-17T13:32:10Z

libp2p/switch.nim


    s.peerInfo.listenAddrs.keepItIf(it notin addrs)

+    debug "addresses", addrs = addrs


Suggested change

debug "addresses", addrs = addrs

debug "listen addresses", addrs = addrs

I don't quite understand the difference between addrs and listenAddrs, but apparently there is one. So addrs is technically not listenAddrs. I'm fine with whatever though. lmk what you think

i wanted to be more descriptive, what are these addresses used for? if you see log in in terminal:

DEBUG [TIME] addresses addresses={0x111, 0x222, 0x333}

what do we know here?
addresses for what?

current log message only looks usefully if you really know what are you look at...

if there is better log text then "listen addresses" consider using it

vladopajic · 2025-07-17T13:41:42Z

libp2p/switch.nim

+      if t of TcpTransport:
+        await fut
+        s.acceptFuts.add(s.accept(t))
+        s.peerInfo.listenAddrs &= t.addrs


ws transports -> accept blocked until auto tls is ready -> blocked until tcp transport is accepting
tcp transport -> no dependencies for accepting

does this graph represent what start dependency should be? if so, then these are separate processes that can happen in separate futures?

vladopajic

this pr is still considered big +863 −423
changes in switch.start proc are manually fixing problem of asynchronous start of transports, which works but is hacky and could bites us down the road; in other words it doesn't appear to be proper solution

richard-ramos · 2025-07-18T15:56:29Z

libp2p/switch.nim

+        await fut
+        s.acceptFuts.add(s.accept(t))
+        s.peerInfo.listenAddrs &= t.addrs
+  debug "switch addresses",


This is probably unneeded as later in the start() we have

debug "Started libp2p node", peer = s.peerInfo

which will log the same information

gmelodie self-assigned this Jul 14, 2025

github-project-automation bot added this to nim-libp2p Jul 14, 2025

github-project-automation bot moved this to new in nim-libp2p Jul 14, 2025

gmelodie mentioned this pull request Jul 14, 2025

feat(autotls): make wstransport use autotls #1500

Closed

gmelodie force-pushed the autotls-wstransport branch 6 times, most recently from 0b12a03 to 5d34ffb Compare July 14, 2025 15:09

gmelodie changed the title ~~feat(websocket): add autotls support to wstransport~~ feat(wstransport): add autotls support Jul 14, 2025

gmelodie force-pushed the autotls-wstransport branch 19 times, most recently from d84ec83 to fd441f4 Compare July 15, 2025 15:43

gmelodie force-pushed the autotls-wstransport branch 2 times, most recently from 8950e08 to f751324 Compare July 16, 2025 17:30

gmelodie and others added 2 commits July 16, 2025 14:30

feat(wstransport): add autotls support

f751324

Merge branch 'master' into autotls-wstransport

d80a777

gmelodie marked this pull request as ready for review July 16, 2025 17:59

gmelodie requested a review from a team as a code owner July 16, 2025 17:59

gmelodie requested review from richard-ramos and vladopajic July 16, 2025 17:59

richard-ramos reviewed Jul 16, 2025

View reviewed changes

vladopajic reviewed Jul 17, 2025

View reviewed changes

gmelodie force-pushed the autotls-wstransport branch 2 times, most recently from 695d2da to 3b87c43 Compare July 17, 2025 13:31

vladopajic reviewed Jul 17, 2025

View reviewed changes

gmelodie force-pushed the autotls-wstransport branch 2 times, most recently from 0bd1a4b to 4dee37c Compare July 17, 2025 18:15

gmelodie mentioned this pull request Jul 17, 2025

Event-based transport and service initialization #1553

Closed

gmelodie force-pushed the autotls-wstransport branch from 4dee37c to 2aeacd6 Compare July 18, 2025 12:07

gmelodie requested review from richard-ramos and vladopajic July 18, 2025 12:23

gmelodie enabled auto-merge (squash) July 18, 2025 12:23

vladopajic approved these changes Jul 18, 2025

View reviewed changes

gmelodie force-pushed the autotls-wstransport branch from 9f92913 to c606ac4 Compare July 18, 2025 13:37

gmelodie added 2 commits July 18, 2025 10:37

Merge branch 'master' into autotls-wstransport

c606ac4

Merge branch 'master' into autotls-wstransport

def9150

richard-ramos moved this from new to In Progress in nim-libp2p Jul 18, 2025

richard-ramos approved these changes Jul 18, 2025

View reviewed changes

gmelodie merged commit 2e6b1d2 into master Jul 18, 2025
56 of 67 checks passed

gmelodie deleted the autotls-wstransport branch July 18, 2025 15:58

github-project-automation bot moved this from In Progress to done in nim-libp2p Jul 18, 2025


		s.peerInfo.listenAddrs.keepItIf(it notin addrs)

		debug "addresses", addrs = addrs

	debug "addresses", addrs = addrs
	debug "listen addresses", addrs = addrs

feat(wstransport): add autotls support #1535

feat(wstransport): add autotls support #1535

Uh oh!

Conversation

gmelodie commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmelodie Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladopajic Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladopajic Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladopajic Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jul 17, 2025

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladopajic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmelodie commented Jul 14, 2025 •

edited

Loading

gmelodie Jul 17, 2025 •

edited

Loading

vladopajic Jul 17, 2025 •

edited

Loading

vladopajic Jul 17, 2025 •

edited

Loading

vladopajic Jul 17, 2025 •

edited

Loading