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

Add GET /register/:L2_ID #37

Closed
juliangruber opened this issue Aug 3, 2022 · 6 comments · Fixed by #40
Closed

Add GET /register/:L2_ID #37

juliangruber opened this issue Aug 3, 2022 · 6 comments · Fixed by #40
Assignees
Labels
enhancement New feature or request

Comments

@juliangruber
Copy link
Contributor

juliangruber commented Aug 3, 2022

L2 nodes will call this route to establish a long-lived connection, so that L1 can on demand make requests to it.

Protocol see https://www.notion.so/pl-strflt/Elaborate-L1-L2-MVP-flow-and-mechanics-793d381d04974a6b903078bf10e72b2a.

For filecoin-saturn/L2-node#48.

@juliangruber juliangruber added the enhancement New feature or request label Aug 3, 2022
@juliangruber juliangruber self-assigned this Aug 3, 2022
@aarshkshah1992
Copy link

aarshkshah1992 commented Aug 9, 2022

@juliangruber

  • The L1 registration endpoint should be idempotent for L2 IDs to handle L2 restarts, disconnects, re-boots etc.
  • The L2 will use https to talk to L1s to create a shared encrypted channel. Kinda how libp2p uses Noise.
  • For using TLS above, I believe the L1s already have certs from trusted CAs and NOT self signed certs. But, need to confirm with @DiegoRBaquero .
  • The L2s will only ever bind to localhost and so all L1 <-> L2 interop needs to happen over connections created by L2s to L1s.
  • Given that making a connection will need a TCP + TLS handshake (6 sequential hops !!), the L1 <-> L2 protocol we develop should minimze the number of connections we make here and connection making should be outside the critical path of transferring CAR files for client requests.
  • I think the server side events + http post protocol we've come up with should take care of all this but wanted to confirm.

@gruns
Copy link
Contributor

gruns commented Aug 9, 2022

The L2s will only ever bind to localhost and so all L1 <-> L2 interop needs to happen over connections created by L2s to L1s.

yep! l2s will not be listening for any inbound connections from l1s. only l1s listening for inbound connections from l2s

For using TLS above, I believe the L1s already have certs from trusted CAs. But, need to confirm with @DiegoRBaquero .

not to steal any of diego's thunder, but they do indeed 🙂

Given that making a connection will need a TCP + TLS handshake (6 sequential hops !!)

ya. painful

  1. diego shipped ocsp stapling last week, which helps with tls conn setup time (go @DiegoRBaquero 🙌)
  2. in the future, we should/will use quic/http3 over tcp+tls. it remains to be seen how quickly (terrible pun intended...) we can ship quic/http3 support. nginx doesnt yet support it, so it's non-trivial to add. but its bang for the buck could/would be huge -- both for l2s and, more importantly, retrieval clients

that said, initial connection setup time between l1<->l2 shouldnt matter too much, as that connection remains open once established. this cost only has to be paid once for every l1<->l2 pairing

I think the server side events + http post protocol we've come up with should take care of all this but wanted to confirm.

if both the l1 and l2 talk http2, which was my original thought, http2's multiplexing takes care of this and l1s can just issue plain jane http requests for every piece of content they want and l2s can respond with plain jane http responses. no SSE, or HTTP POST requests from l2 to l1, required

on the l1 side, nginx and node both support http2 out of the box

the golang http2 story looks 'solved', too. eg https://pkg.go.dev/golang.org/x/net/http2

@aarshkshah1992
Copy link

@gruns

I think @juliangruber and @DiegoRBaquero had a discussion where they arrived at the conclusion that http2 does NOT work well with nginx or something ?

@gruns
Copy link
Contributor

gruns commented Aug 9, 2022

ah. i wasnt in that convo

if that's the case, im unaware of how/where http2 fails us here

@juliangruber
Copy link
Contributor Author

juliangruber commented Aug 9, 2022

While both nginx and nodejs support http/2, nginx doesn't support upstream server push. This means that l1 can't send a
http request to l2 over the existing connection, asking for a new CID. So, unfortunately http/2 server push is out of the question.

It was just discussed in Slack whether we could start off by l2 creating a tcp connection to l1, and both parties then sending http messages over it. We're postponing this idea for now, while it could give great performance it will require some experimentation with the http language implementations, as this is not a common use case.

The current plan is SSE-ish for CID requests and POST for CAR responses. This will create 2 connections, one for CID requests and the other for POST responses. We're going to experiment afterwards with upgrading to HTTP/2, which will hopefully bring down the connection count to 1.

As an alternative for SSE-ish, we discussed websockets, because its framing layer is more sturdy than NLJSON. It should be a great alternative if SSE-ish doesn't work, however brings its own complexity so IMO isn't the obvious way to go.

@juliangruber juliangruber changed the title Add POST /register Add GET /register/:L2_ID Aug 9, 2022
@juliangruber
Copy link
Contributor Author

I've changed this method from POST /register to GET /register/:L2_ID, since it should be idempotent and we can just submit the l2's id as part of the url

juliangruber added a commit that referenced this issue Aug 30, 2022
* add basic GET /register/:l2id

* FIXME

* flow works if we disable forking

* clean up

* lint

* fix demo

* block cid

* fix write after end

* add cluster logging

* enable cluster again

* shorten heartbeat interval for testing

* add Cache-Control header

* remove node:cluster

* fix type error

* run multiple node processes

* refactor

* allow testing cid again

* Revert "run multiple node processes"

This reverts commit bffdce2.

* Revert "remove node:cluster"

This reverts commit e9dfbbd.

* clean up

* only one fork for now

* websocket implementation with flaws

* fix consistent hashing

* fix timeout error handling

* enable cluster again

* reduce to one worker again

* Revert "websocket implementation with flaws"

This reverts commit 034e0f8.

* heartbeat is just an empty line

* clean up

* pass on requestId from nginx

* remove node:cluster again

* clean up

* send heartbeat immediately

* fix inconsistent naming

* refactor using async functions

* clean up

* add CAR validation

* fix debug

* disable l2 integration in main network

* refactor

* refactor

* enable l2 fire and forget

* add car extraction

* disable l2 fallback on file path req

* use asyncHandler

* remove cluster again (merge error)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants