fix: don't form new pairs for invalidated candidates

[thomaseizinger]

When a candidate gets invalidated, we don't want it to be used anymore. At the moment, str0m will form new pairs upon receiving a STUN request for an invalidated candidate. With the new pair being created, str0m will restart sending binding requests for this candidate pair.

[thomaseizinger]

I've stumbled over this because I am using invalidate_candidate to tell str0m that I no longer want certain candidate pairs being tested (i.e. relay pairs when I've found a direct connection).

Now, one could argue that the current behaviour is also correct because we are apparently still receiving messages on this candidate so it must be valid. If that is how it is meant to work, then we'll have to find another way of filtering this traffic by perhaps discarding it on the application layer and not feeding it to str0m. That feels a bit hacky though. I'd expect that calling invalidate_candidate will disable the candidate, even if we receive binding requests for it.

[algesten]

This took me down a rabbit hole. Reading the code it's not clear to me what the exact semantics of discarding a client is. I was wondering whether this change is correct, or whether the check should be in the let local_idx = … above.

I'm leaning towards that your change is correct – this leaves the candidate as known for incoming traffic (not triggering the STUN request for socket that is neither a host nor a relay candidate. expect).

[thomaseizinger]

This took me down a rabbit hole. Reading the code it's not clear to me what the exact semantics of discarding a client is. I was wondering whether this change is correct, or whether the check should be in the let local_idx = … above.

I'm leaning towards that your change is correct – this leaves the candidate as known for incoming traffic (not triggering the STUN request for socket that is neither a host nor a relay candidate. expect).

When reading the code, I noticed that discarded has a double meaning. It means both invalidated and redundant. Perhaps that can be clarified at some point by just not adding redundant candidates at all?

Also, the panic you are talking about is actually something that I think we should fix. I don't think we should put it on the application to sanitise these inputs. Instead, str0m should just discard the message I think.

[algesten]

Also, the panic you are talking about is actually something that I think we should fix. I don't think we should put it on the application to sanitise these inputs. Instead, str0m should just discard the message I think.

You might be right. Maybe raise that as a separate issue/PR?