Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

Relay reservations #67

Closed
wants to merge 5 commits into from
Closed

Relay reservations #67

wants to merge 5 commits into from

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Apr 6, 2019

  • Adds a new message, RESERVE, which signals to a hop relay that the peer wants to reserve the connection for relay.
  • Adds a new error code, RESERVATION_REFUSED, which can signal refusal to reserve.
  • Adds a new exported procudure, Reserve, used to request a reservation in a relay.

This initial implementation handles RESERVE trivially: it tags the connection in the connection manager and always succeeds.

@vyzo vyzo requested a review from Stebalien April 6, 2019 15:02
@ghost ghost assigned vyzo Apr 6, 2019
@ghost ghost added the status/in-progress In progress label Apr 6, 2019
@vyzo
Copy link
Contributor Author

vyzo commented Apr 6, 2019

cc @raulk @whyrusleeping

@vyzo
Copy link
Contributor Author

vyzo commented Apr 6, 2019

I think we want to track reservations regardless of whether imposing limits and untag peers when they disconnect.

edit: done.

@raulk
Copy link
Member

raulk commented Apr 6, 2019

Such a quick turnaround ;-) I'm not so sure we need a RESERVE message so much as we need to segregate protocols, as per #68. If we segregate protocols, the reserve petition would be implicit by way of opening a stream for the hop subprotocol.

@Stebalien
Copy link
Member

So, the issue here is that there's no way to tell the relay "I'm using you as a relay for inbound connections, please don't kill this connection".

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

many of these concepts already have precedent in the TURN spec (RFC5766) -- i think we should strive to align where possible, there's no reason to ignore prior art.

  • RFC5766 calls this action an "Allocation".
  • We should evaluate if we need a "refresh" message. We're primarily connection-oriented now and it may seem overkill, but that will change down the line, so let's plan for it now.
  • We need a way for the server to terminate an allocation gracefully.
  • We might want to introduce the concept of time -- leasing an allocation and renewing it.

We have agreed to be spec-first. We have to discuss this change in protocol in libp2p/specs. Note that other implementations are affected.

I personally think we need to revisit this protocol in several ways, including segregation in two:

  • A /libp2p/relay/allocate protocol that NAT'ted peers use to acquire and renew a slot in a relay, enabling them to announce a relayed address for themselves.
  • A /libp2p/relay/channel protocol, used to open streams against NAT'ted peers.

For now, I suggest we recognise peers who need tagging by identifying HOP targets. Every time we process a valid HOP message, we increment the score of the target. Or we can enforce a ceiling of 1, if we want all NAT'ted peers to be treated equally.

^^ This approach does not require a change in protocol, and has identical effect, except that it leaves the connection vulnerable between its opening, and the time it receives its first incoming HOP. I think that's fair.

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

I think it is better to reserve with RESERVE than try to deduce the reservation.
But yeah, it's very similar to TURN allocations.

@raulk
Copy link
Member

raulk commented Apr 7, 2019

But what the reservation is doing in this PR (tagging in the conn manager) is achievable without a change in protocol. Protocol change here is overkill IMO. And it requires users to adopt this change, which won’t happen immediately.

I suggest we tag peers like I suggested above, and defer allocations to a v2 of circuit relay, which we can start drafting in libp2p/specs straightaway.

With a protocol this important, I prefer to take heavy steps than introduce piecemeal changes.

@raulk
Copy link
Member

raulk commented Apr 7, 2019

Actually, the demand-based variant is more secure than the explicit RESERVE message in this PR. Isn’t it straightforward to max out a relay by creating a bunch of Sybils and sending RESERVE requests?

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

The idea is to extend the implementation and impose resource limits, so that we can refuse reservations if we are over capacity. I am just not sure how to do it yet so deferred to simply tagging.

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

Deployment should be pretty straightforward, we can have it to the majority of relay users with the next version of ipfs (and the relays can be deployed immediately).

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

Isn’t it straightforward to max out a relay by creating a bunch of Sybils and sending RESERVE requests?

That's true, yet another instance of susceptibility to sybil attacks.

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

Thinking this more through, the idea of demand-based tagging is not so bad.
So you want to tag a peer as soon as there is a hop connection to it?

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

extra points of not needing client-side deployment.

@raulk
Copy link
Member

raulk commented Apr 7, 2019

Exactly! We can increment the tag value every time the peer receives a new hop, with a ceiling to not overskew scoring. We’d still need to track which peers are targets (if we want to deny new hops by quota or something), but we need to do that anyway for observability purposes.

@raulk
Copy link
Member

raulk commented Apr 7, 2019

BTW – I’d consider adding IncrTag and DecrTag in the connection manager so we can do that atomically. Although we can emulate the atomicity here with a global lock, assuming we’re the only ones using the tag (which we are).

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

Yeah, been thinking about it.
We also need to increment the other side of the connection I think, so that when we prune we kill the connection with the lowest stream count.

I don't think we need IncrTag in the connection manager, we can simply keep track of the counts in an instance table protected by a lock and SetTag directly -- without doing a GetTag first.

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

I am going to close this in favor of an on-demand based tagging approach.

@vyzo vyzo closed this Apr 7, 2019
@ghost ghost removed the status/in-progress In progress label Apr 7, 2019
@Stebalien
Copy link
Member

@vyzo what if we had a separate reserve protocol? We've talked about having a "go away" protocol and we could bake that in.

@vyzo
Copy link
Contributor Author

vyzo commented Apr 8, 2019

Yeah, I think we do need a reservation protocol regardless, as it can provide immediate feedback about relay load; but let's discuss this in specs.

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

Successfully merging this pull request may close these issues.

3 participants