quic: require address validation when under load

[fulviodenza]

This PR adds the address validation when under load, referred to the issue #1535

[ErikPelli]

I recommend reviewers to squash the commits

[ErikPelli]

It's probably easier if you use a slice of bufferEntrys. That slice could then start at length 1, and we'd append every minute, until we reach the maximum length (5). As we append to that slice once per minute, so there's no need to worried about the cost of allocations.

When you finish the minutes what do you do? Overwrite starting from the first value? Anyway i don't think that append is necessary, as you can simply do a make([]bufferEntry, minutes) in the constructor, because the allocated space is very small

[marten-seemann]

It's probably easier if you use a slice of bufferEntrys. That slice could then start at length 1, and we'd append every minute, until we reach the maximum length (5). As we append to that slice once per minute, so there's no need to worried about the cost of allocations.

When you finish the minutes what do you do? Overwrite starting from the first value? Anyway i don't think that append is necessary, as you can simply do a make([]bufferEntry, minutes) in the constructor, because the allocated space is very small

Yes exactly, you add a new bufferEntry at the end. And if you reach a length of 5, you drop the first entry. This way you avoid the index calculations that you need when using a ring buffer, and you avoid special cases when you don't have 5 entries yet because you just booted the node.

[ErikPelli]

5 is fixed or is the number of minutes chosen by the user?

[marten-seemann]

I think it can be a constant, the user chooses the duration (and that duration could, in principle, even be less than a minute).

[ErikPelli]

In the fraction, what represents token.IsRetryToken == false? From the name I think that it's a boolean that says if the user sent the correct retry token, but in that case go-quic doesn't even call the function I think.

With duration what do you mean? The time to go to next buffer item (so instead of 1 minute this value is changeable)?

[marten-seemann]

In the fraction, what represents token.IsRetryToken == false? From the name I think that it's a boolean that says if the user sent the correct retry token, but in that case go-quic doesn't even call the function I think.

There are two types of tokens, both used in the same header field of the Initial packet. Retry tokens, as the name says, are tokens that are echoed from a Retry packet. There's also tokens generated on previous connections (see https://datatracker.ietf.org/doc/html/rfc9000#section-8.1.3). You don't need to worry about them too much.

[fulviodenza]

Hi @marten-seemann, we finished the implementation and were working on testing the new feature. We are a bit confused about the testing part, mainly on these points:

1. Should we mock the two peers? We found createPeer function to instantiate the peers that need to connect with each other but we're not sure about the correctness of this approach.
2. Should we create a mock for some component with the clock like in this function? https://github.com/libp2p/go-libp2p/blob/bedce2290de15f7f6150ea1740a59d07b78a34f4/p2p/discovery/mocks/mocks.go#L28-L33 If yes what component should we add the clock to?
3. How do we interrupt handshakes after the accept function and before the conclusion?

[ErikPelli]

Hi @marten-seemann, we finished the implementation and were working on testing the new feature. We are a bit confused about the testing part, mainly on these points:

1. Should we mock the two peers? We found `createPeer` function to instantiate the peers that need to connect with each other but we're not sure about the correctness of this approach.

2. Should we create a mock for some component with the clock like in this function? https://github.com/libp2p/go-libp2p/blob/bedce2290de15f7f6150ea1740a59d07b78a34f4/p2p/discovery/mocks/mocks.go#L28-L33
    If yes what component should we add the clock to?

3. How do we interrupt handshakes after the accept function and before the conclusion?

Any news about that? There hasn't been an answer yet.

[marten-seemann]

1. Should we mock the two peers? We found createPeer function to instantiate the peers that need to connect with each other but we're not sure about the correctness of this approach.

Yes, that sounds reasonable.

2. Should we create a mock for some component with the clock like in this function? https://github.com/libp2p/go-libp2p/blob/bedce2290de15f7f6150ea1740a59d07b78a34f4/p2p/discovery/mocks/mocks.go#L28-L33

Yes. Having a WithClock Option is probably the best way to go.

3. How do we interrupt handshakes after the accept function and before the conclusion?

Probably the easiest way will be to give the client an invalid certificate, like we do in the TLS tests: https://github.com/libp2p/go-libp2p/blob/master/p2p/security/tls/transport_test.go#L281. That will then lead to a failure of the QUIC handshake.

[fulviodenza]

Hi @marten-seemann, I think we finished the feature itself but we can't get out of the test question. We struggled on tests but cannot figure out how to write them. Would you be available to give us a help/hint to implement them? Thank you

[BigLep]

2022-08-26 conversation: @marten-seemann has done some work to cleanup the quic-go API. @marten-seemann needs to release this and then will circle back here to explain the changes so the PR can be updated.

[BigLep]

Status: blocked until the quic-go issue is addressed: https://github.com/lucas-clemente/quic-go/issues/3549