HTTP retrieval proposal #747

hsanjuan · 2024-12-09T19:44:09Z

This is a proposal to add HTTP retrieval to Boxo. The current state is highly WIP, but I successfully retrieved something over HTTP, so posting to initiate a discussion over the approach and if we want to pursue it until the end.

Approach

The high-level idea is that most of what lives in bitswap/client is actually an "exchange" implementation, with the only real "Bitswap" thing being that bitswap/network sends HAS/GET requests over bitswap-protocol streams. As such, we should be able to complement bitswap/network with an HTTP-retrieval implementation which, instead of fetching things over the bitswap protocol, calls HTTP endpoints as indicated by the provider's /http addresses entries.

Note that conceptually at least, this is not adding HTTP retrieval into bitswap, but promoting most of the bitswap code to be a reference "Exchange" implementation, which is re-usable for different retrieval protocols (bitswap, http...). That is, we would be talking of an "exchange network" component and not a "bitswap network" component. Renames to this extent are still missing.

Implementation

In order to introduce an http-retrieval "exchange network" we need to:

Know when something should be retrieved via HTTP - that is, an item has an /http provider.
Use HTTP network for that.

To this end:

We have a router which select the http-network or the bitswap-network (or both) based on the existance of /http addresses in the peerstore of the given peer.
We have implemented an http-network as a PoC that performs GET requests to /http endpoints when handling a WANT.

In my tests plugging it to Kubo, the http-network can be used to retrieve content from a gateway over http. 🥳

The main advantange to this approach is that it is relatively clean to incorporate to the codebase, and keeps most of the code untouched, without having to duplicate any of the complex areas.

Challenges

Connectivity tracking is not implemented yet and we will have to see to what extent it can be implemented (I'm guessing we can plug into the TCP dialer directly).
Options like timeouts etc. are not implemented
We use a single HTTP client rather than a pool
Of course testing is fully lacking.

Bitswap places a lot of importance on managing connectivity events to peers. We avoid requesting things from peers that have not signaled connectivity, we clean peers that have disconnected and re-queue things for peers that disconnect. Thus it seems we must support http-connectivity events. When a libp2p peer connects for bitswap, we know that the connection is setup, handshake has been performed and protocol negotiation has happened. For HTTP these things may not exist so we need to define what means "Connected" (i.e. in the case of https it would mean we have completed SSL handshakes).

Apart from that, the question is what are the elements in the current bitswap/client stack that do not apply to HTTP (peerqueues, messagequeues, broadcast, wantsending, prioritization etc.)... and why not? What if a peer disconnects from bitswap but not from http or vice-versa? What if Latency is much worse for bitswap than for http? Perhaps this is all logic for the network-router to know how to choose which network to use to send messages.

Otherwise perhaps it is not possible to have a satisfactory implementation this way and we need to start thinking what to copy-paste into a separate "http-exchange" (at least the client part).

Related: #608

lidel

Thank you @hsanjuan, would be extremely nice if we can pull it off with such small set of changes.

Once we have HTTP basics like user-agent, status code metrics, 503/429/Retry-After (details inline), this is worth testing on Rainbow staging (do A/B test with bitswap-only box and bitswap+http).

ps. Whatever we do, HTTP should be opt-in, with a big EXPERIMENTAL warning.

lidel · 2024-12-13T19:25:47Z

bitswap/network/http_multiaddr.go

+				return nil, fmt.Errorf("failed to extract host: %w", err)
+			}
+			host = hostVal
+		case "tcp":


nit: include "udp" to future proof HTTP/3 deployments?

lidel · 2024-12-13T19:32:19Z

bitswap/network/http_multiaddr.go

+		case "http", "https":
+			schema = comp.Name


This may not be enough due to resolved multiaddrs that have explicit /tls/http or even /tls/sni/../

/ip4/..../tcp/../tls/http

/ip4/..../tcp/../tls/sni/example.com/http (for example, libp2p.direct peers could announce HTTP capability this way)

here we need to construct https://example.com (or manually ensure request is sent with correct SNI and Host header)

mm to clarify... why wouldn't libp2p direct use a /dns/<libp2p-direct-hostname>/tcp/.../https address?

lidel · 2024-12-13T19:37:46Z

bitswap/network/httpnet/httpnet.go

+	// pick the first one. In general there should not be more than one
+	// url per peer. FIXME: right?
+	pingURL := urls[0]


Hm.. there could be ip4 and ip6 and if we are ip4-only trying ip6 will fail.
We should probe them in order and use first one that passed ping.

I'd expect dns resolution chooses which one to try in that case... but yeah we should try what we have. I would not ping though, just try to make the request directly, probably randomizing the list order.

I thought this code was somewhere else. IIRC ping is only used to record latency to prioritize...

The pinger code prioritizes ipv4 over ipv6 and chooses the first resolved-entry... I think it may be a bit too much to consider hosts that have significantly different latencies depending on ipv4 or 6.

Ultimately the question is : given a provider URL example.com. How many providers is that? One or N?

i.e. example.com can resolve to multiple ipv4/ipv6s. Are each of them meant to be a different provider URL to which we trigger requests? or do we assume the request should go to the first result?

I think we should consider it a single provider. If they want to be considered separate providers they can by providing on different urls.

bitswap/network/httpnet/httpnet.go

lidel · 2024-12-13T20:09:17Z

bitswap/network/httpnet/httpnet.go

+		if resp.StatusCode == http.StatusNotFound {
+			if entry.SendDontHave {
+				bsresp.AddDontHave(entry.Cid)
+			}
+			continue


I'd say we should add 403, 410, and 451 here as well.

lidel · 2024-12-13T20:17:25Z

bitswap/network/httpnet/httpnet.go

+		}
+
+		if resp.StatusCode != http.StatusOK {
+			err := fmt.Errorf("%s -> %d: %s", sendURL, resp.StatusCode, string(body))


nit: escape random bytes from random people

Suggested change

err := fmt.Errorf("%s -> %d: %s", sendURL, resp.StatusCode, string(body))

err := fmt.Errorf("%s -> %d: %q", sendURL, resp.StatusCode, string(body))

lidel · 2024-12-13T20:20:52Z

bitswap/network/httpnet/httpnet.go

+			break
+		}
+
+		if resp.StatusCode != http.StatusOK {


nit: we need special handling of 429 Too Many Requests and 503 Service Unavailable responses, and stop hammering server if they send us such code.

We should check if response has Retry-After and backoff.

If Retry-After is missing, have some default backoff, rather than hammering server that is blocking us.

lidel · 2024-12-13T20:42:40Z

bitswap/network/router.go

nit: have HTTP disabled by default and behind opt-in config flag (or env variable)

lidel · 2024-12-13T20:49:08Z

bitswap/network/httpnet/httpnet.go

We need http-specific metrics for response size, duration and status codes to see if there are any issues.

* Support limit when reading blocks * Support using a user-agent (+default) * Support cooldown periods for URLs on error * Make use of connectEventManager * Detect connect/disconnect events * Better error handlinga and refactoring of SendMsg code. * Prioritize HTTP in router when possible

hsanjuan self-assigned this Dec 9, 2024

hsanjuan requested a review from a team as a code owner December 9, 2024 19:44

lidel requested changes Dec 13, 2024

View reviewed changes

lidel mentioned this pull request Dec 17, 2024

Add support for additional protocols, e..g transport-ipfs-gateway-http ipfs/ipfs-check#73

Open

hsanjuan added 7 commits December 20, 2024 16:41

IWIP WIP WIP

3d43bef

wip

ea209f3

wip

c917140

wip

ab94550

mm

9f3aad6

bitswap/network: extract and share connect event manager

8b7bc71

hsanjuan force-pushed the http-retr2 branch from d57839b to e2f169b Compare December 20, 2024 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP retrieval proposal #747

HTTP retrieval proposal #747

hsanjuan commented Dec 9, 2024 •

edited

Loading

lidel left a comment

lidel Dec 13, 2024

lidel Dec 13, 2024

hsanjuan Dec 16, 2024

lidel Dec 13, 2024

hsanjuan Dec 16, 2024

hsanjuan Dec 16, 2024

lidel Dec 13, 2024

lidel Dec 13, 2024

lidel Dec 13, 2024

lidel Dec 13, 2024

lidel Dec 13, 2024 •

edited

Loading

	err := fmt.Errorf("%s -> %d: %s", sendURL, resp.StatusCode, string(body))
	err := fmt.Errorf("%s -> %d: %q", sendURL, resp.StatusCode, string(body))

HTTP retrieval proposal #747

Are you sure you want to change the base?

HTTP retrieval proposal #747

Conversation

hsanjuan commented Dec 9, 2024 • edited Loading

Approach

Implementation

Challenges

lidel left a 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

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

lidel Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

hsanjuan commented Dec 9, 2024 •

edited

Loading

lidel Dec 13, 2024 •

edited

Loading