remove setCustomTransportParameter helper function

Summary:
- Remove setCustomTransportParameter, which (based on the quic v19 rfc), verifies whether a parameter is within the private range [0xff00, 0xffff]

> Values with the first byte in the range 0x00 to 0xfe (in hexadecimal) are assigned via the Specification Required policy [RFC8126].

- Consolidating adding MaxStreamGroups transport parameter into all other transport parameters extension.

More specifically, `QuicClientTransport::maybeEnableStreamGroups()` logic is now moved into `QuicClientTransport::setSupportedExtensionTransportParameters()`

Reviewed By: mjoras

Differential Revision: D50461610

fbshipit-source-id: 802b546c8364586cdcf36a230b156ca140c57ce4

quic/QuicConstants.h

@@ -77,9 +77,6 @@ constexpr uint16_t kMaxNumGROBuffers = 64;
 constexpr uint16_t kDefaultNumGROBuffers = kMinNumGROBuffers;
 
 constexpr uint16_t kMaxNumCoalescedPackets = 5;
-// As per version 20 of the spec, transport parameters for private use must
-// have ids with first byte being 0xff.
-constexpr uint16_t kCustomTransportParameterThreshold = 0xff00;
 
 // The length of the integrity tag present in a retry packet.
 constexpr uint32_t kRetryIntegrityTagLen = 16;

quic/api/QuicTransportFunctions.cpp

@@ -1884,36 +1884,6 @@ bool hasInitialOrHandshakeCiphers(QuicConnectionStateBase& conn) {
       conn.readCodec->getHandshakeReadCipher();
 }
 
-bool setCustomTransportParameter(
-    const CustomTransportParameter& customParam,
-    std::vector<TransportParameter>& customTransportParameters) {
-  // Check that the parameter id is in the "private parameter" range, as
-  // described by the spec.
-  if (static_cast<uint16_t>(customParam.getParameterId()) <
-      kCustomTransportParameterThreshold) {
-    LOG(ERROR) << "invalid parameter id";
-    return false;
-  }
-
-  // check to see that we haven't already added in a parameter with the
-  // specified parameter id
-  auto it = std::find_if(
-      customTransportParameters.begin(),
-      customTransportParameters.end(),
-      [&customParam](const TransportParameter& param) {
-        return param.parameter == customParam.getParameterId();
-      });
-
-  // if a match has been found, we return failure
-  if (it != customTransportParameters.end()) {
-    LOG(ERROR) << "transport parameter already present";
-    return false;
-  }
-
-  customTransportParameters.push_back(customParam.encode());
-  return true;
-}
-
 bool toWriteInitialAcks(const quic::QuicConnectionStateBase& conn) {
   return (
       conn.initialWriteCipher && conn.ackStates.initialAckState &&

quic/api/QuicTransportFunctions.h

@@ -325,16 +325,6 @@ bool writeLoopTimeLimit(
     TimePoint loopBeginTime,
     const QuicConnectionStateBase& connection);
 
-/**
- * Sets private transport parameters that are not in the TransportParameterId
- * enum. See kCustomTransportParameterThreshold in QuicConstants.h
- * - checks that the custom param id complies to the spec
- * - checks that the param is not in the provided vector already
- */
-bool setCustomTransportParameter(
-    const CustomTransportParameter& customParam,
-    std::vector<TransportParameter>& customTransportParameters);
-
 bool toWriteInitialAcks(const quic::QuicConnectionStateBase& conn);
 bool toWriteHandshakeAcks(const quic::QuicConnectionStateBase& conn);
 bool toWriteAppDataAcks(const quic::QuicConnectionStateBase& conn);

quic/api/test/QuicTransportFunctionsTest.cpp

@@ -4731,36 +4731,6 @@ TEST_F(QuicTransportFunctionsTest, MissingStreamFrameBytesSingleByteWrite) {
   }
 }
 
-TEST_F(QuicTransportFunctionsTest, CustomTransportParamTest) {
-  std::vector<TransportParameter> customTransportParameters;
-
-  // Add new param.
-  EXPECT_TRUE(setCustomTransportParameter(
-      CustomIntegralTransportParameter(kCustomTransportParameterThreshold, 0),
-      customTransportParameters));
-  EXPECT_EQ(customTransportParameters.size(), 1);
-
-  // Existing param not added.
-  EXPECT_FALSE(setCustomTransportParameter(
-      CustomIntegralTransportParameter(kCustomTransportParameterThreshold, 1),
-      customTransportParameters));
-  EXPECT_EQ(customTransportParameters.size(), 1);
-
-  // Bad param id is not added.
-  EXPECT_FALSE(setCustomTransportParameter(
-      CustomIntegralTransportParameter(
-          kCustomTransportParameterThreshold - 1, 2),
-      customTransportParameters));
-  EXPECT_EQ(customTransportParameters.size(), 1);
-
-  // Another valid param added.
-  EXPECT_TRUE(setCustomTransportParameter(
-      CustomIntegralTransportParameter(
-          kCustomTransportParameterThreshold + 1, 0),
-      customTransportParameters));
-  EXPECT_EQ(customTransportParameters.size(), 2);
-}
-
 TEST_F(
     QuicTransportFunctionsTest,
     StaticCapOnWritableBytesFromThrottlingSignalProvider) {

quic/client/QuicClientTransport.cpp

@@ -1017,7 +1017,7 @@ void QuicClientTransport::startCryptoHandshake() {
       *clientConn_->initialDestinationConnectionId, version);
 
   setSupportedExtensionTransportParameters();
-  maybeEnableStreamGroups();
+
   auto paramsExtension = std::make_shared<ClientTransportParametersExtension>(
       conn_->originalVersion.value(),
       conn_->transportSettings.advertisedInitialConnectionFlowControlWindow,
@@ -1666,6 +1666,13 @@ void QuicClientTransport::setSupportedExtensionTransportParameters() {
         static_cast<uint64_t>(TransportParameterId::knob_frames_supported), 1);
     customTransportParameters_.push_back(knobFrameSupport.encode());
   }
+
+  if (ts.advertisedMaxStreamGroups > 0) {
+    CustomIntegralTransportParameter streamGroupsEnabledParam(
+        static_cast<uint64_t>(TransportParameterId::stream_groups_enabled),
+        conn_->transportSettings.advertisedMaxStreamGroups);
+    customTransportParameters_.push_back(streamGroupsEnabledParam.encode());
+  }
 }
 
 void QuicClientTransport::adjustGROBuffers() {
@@ -1769,21 +1776,6 @@ void QuicClientTransport::maybeSendTransportKnobs() {
   }
 }
 
-void QuicClientTransport::maybeEnableStreamGroups() {
-  if (conn_->transportSettings.advertisedMaxStreamGroups == 0) {
-    return;
-  }
-
-  CustomIntegralTransportParameter streamGroupsEnabledParam(
-      static_cast<uint64_t>(TransportParameterId::stream_groups_enabled),
-      conn_->transportSettings.advertisedMaxStreamGroups);
-
-  if (!setCustomTransportParameter(
-          streamGroupsEnabledParam, customTransportParameters_)) {
-    LOG(ERROR) << "failed to set stream groups enabled transport parameter";
-  }
-}
-
 void QuicClientTransport::RecvmmsgStorage::resize(size_t numPackets) {
   if (msgs.size() != numPackets) {
     msgs.resize(numPackets);

quic/client/QuicClientTransport.h

@@ -282,8 +282,6 @@ class QuicClientTransport
    */
   void maybeSendTransportKnobs();
 
-  void maybeEnableStreamGroups();
-
   bool replaySafeNotified_{false};
   // Set it QuicClientTransport is in a self owning mode. This will be cleaned
   // up when the caller invokes a terminal call to the transport.

quic/client/test/ClientStateMachineTest.cpp

@@ -242,8 +242,7 @@ TEST_P(
     CustomIntegralTransportParameter streamGroupsEnabledParam(
         static_cast<uint64_t>(TransportParameterId::stream_groups_enabled),
         GetParam().peerMaxGroupsIn);
-    CHECK(
+    transportParams.push_back(streamGroupsEnabledParam.encode());
-        setCustomTransportParameter(streamGroupsEnabledParam, transportParams));
   }
   ServerTransportParameters serverTransportParams = {
       std::move(transportParams)};

quic/server/state/ServerStateMachine.cpp

@@ -1591,11 +1591,7 @@ std::vector<TransportParameter> setSupportedExtensionTransportParameters(
     CustomIntegralTransportParameter streamGroupsEnabledParam(
         static_cast<uint64_t>(TransportParameterId::stream_groups_enabled),
         ts.advertisedMaxStreamGroups);
-
+    customTransportParams.push_back(streamGroupsEnabledParam.encode());
-    if (!setCustomTransportParameter(
-            streamGroupsEnabledParam, customTransportParams)) {
-      LOG(ERROR) << "failed to set stream groups enabled transport parameter";
-    }
   }
 
   CustomIntegralTransportParameter ackReceiveTimestampsEnabled(

quic/server/test/ServerStateMachineTest.cpp

@@ -361,8 +361,7 @@ TEST_P(
     CustomIntegralTransportParameter streamGroupsEnabledParam(
         static_cast<uint64_t>(TransportParameterId::stream_groups_enabled),
         GetParam().peerMaxGroupsIn);
-    CHECK(
+    transportParams.push_back(streamGroupsEnabledParam.encode());
-        setCustomTransportParameter(streamGroupsEnabledParam, transportParams));
   }
   ClientTransportParameters clientTransportParams = {
       std::move(transportParams)};