In quic, consider discarding keys as forward progress. protected by gfe2_reloadable_flag_quic_default_enable_5rto_blackhole_detection2 which replaces gfe2_reloadable_flag_quic_default_enable_5rto_blackhole_detection. PiperOrigin-RevId: 315548676 Change-Id: Ia3db7908a74941108c20fa31d5cfe0c21e252bb9
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index e332a89..e7990d9 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2338,6 +2338,13 @@ sent_packet_manager_.NeuterUnencryptedPackets(); // This may have changed the retransmission timer, so re-arm it. SetRetransmissionAlarm(); + if (default_enable_5rto_blackhole_detection_) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_default_enable_5rto_blackhole_detection2, + 1, 3); + // Consider this as forward progress since this is called when initial key + // gets discarded (or previous unencrypted data is not needed anymore). + OnForwardProgressMade(); + } if (SupportsMultiplePacketNumberSpaces()) { // Stop sending ack of initial packet number space. uber_received_packet_manager_.ResetAckStates(ENCRYPTION_INITIAL); @@ -2889,6 +2896,11 @@ sent_packet_manager_.SetHandshakeConfirmed(); // This may have changed the retransmission timer, so re-arm it. SetRetransmissionAlarm(); + if (default_enable_5rto_blackhole_detection_) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_default_enable_5rto_blackhole_detection2, + 2, 3); + OnForwardProgressMade(); + } if (!SupportsMultiplePacketNumberSpaces()) { // The client should immediately ack the SHLO to confirm the handshake is // complete with the server. @@ -4040,15 +4052,7 @@ // have a better estimate of the current rtt than when it was set. SetRetransmissionAlarm(); if (acked_new_packet) { - is_path_degrading_ = false; - if (sent_packet_manager_.HasInFlightPackets()) { - // Restart detections if forward progress has been made. - blackhole_detector_.RestartDetection(GetPathDegradingDeadline(), - GetNetworkBlackholeDeadline()); - } else { - // Stop detections in quiecense. - blackhole_detector_.StopDetection(); - } + OnForwardProgressMade(); } if (send_stop_waiting) { @@ -4346,6 +4350,18 @@ } } +void QuicConnection::OnForwardProgressMade() { + is_path_degrading_ = false; + if (sent_packet_manager_.HasInFlightPackets()) { + // Restart detections if forward progress has been made. + blackhole_detector_.RestartDetection(GetPathDegradingDeadline(), + GetNetworkBlackholeDeadline()); + } else { + // Stop detections in quiecense. + blackhole_detector_.StopDetection(); + } +} + QuicPacketNumber QuicConnection::GetLargestReceivedPacketWithAck() const { if (SupportsMultiplePacketNumberSpaces()) { return largest_seen_packets_with_ack_[QuicUtils::GetPacketNumberSpace( @@ -4545,7 +4561,8 @@ } // No blackhole detection before handshake completes. if (default_enable_5rto_blackhole_detection_) { - QUIC_RELOADABLE_FLAG_COUNT(quic_default_enable_5rto_blackhole_detection); + QUIC_RELOADABLE_FLAG_COUNT_N(quic_default_enable_5rto_blackhole_detection2, + 3, 3); return IsHandshakeComplete(); }
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 5d149b4..56f84c2 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -1261,6 +1261,10 @@ // received packet number which contains an ACK frame. void SetLargestReceivedPacketWithAck(QuicPacketNumber new_value); + // Called when new packets have been acknowledged or old keys have been + // discarded. + void OnForwardProgressMade(); + // Returns largest received packet number which contains an ACK frame. QuicPacketNumber GetLargestReceivedPacketWithAck() const; @@ -1701,7 +1705,7 @@ : GetQuicFlag(FLAGS_quic_anti_amplification_factor); const bool default_enable_5rto_blackhole_detection_ = - GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection); + GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2); }; } // namespace quic
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 8d096eb..6602a8c 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -6117,7 +6117,7 @@ connection_options.push_back(k5RTO); config.SetConnectionOptionsToSend(connection_options); QuicConfigPeer::SetNegotiated(&config, true); - if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { EXPECT_CALL(visitor_, GetHandshakeState()) .WillRepeatedly(Return(HANDSHAKE_COMPLETE)); } @@ -9930,7 +9930,7 @@ } EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { EXPECT_CALL(visitor_, GetHandshakeState()) .WillRepeatedly(Return(HANDSHAKE_COMPLETE)); } @@ -9981,7 +9981,7 @@ } EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { EXPECT_CALL(visitor_, GetHandshakeState()) .WillRepeatedly(Return(HANDSHAKE_COMPLETE)); } @@ -10031,7 +10031,7 @@ config.SetConnectionOptionsToSend(connection_options); EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { EXPECT_CALL(visitor_, GetHandshakeState()) .WillRepeatedly(Return(HANDSHAKE_COMPLETE)); } @@ -10962,7 +10962,7 @@ } TEST_P(QuicConnectionTest, ClientOnlyBlackholeDetectionClient) { - if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { return; } QuicConfig config; @@ -10983,7 +10983,7 @@ } TEST_P(QuicConnectionTest, ClientOnlyBlackholeDetectionServer) { - if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { return; } set_perspective(Perspective::IS_SERVER); @@ -11009,7 +11009,7 @@ } TEST_P(QuicConnectionTest, 2RtoBlackholeDetection) { - if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { return; } QuicConfig config; @@ -11036,7 +11036,7 @@ } TEST_P(QuicConnectionTest, 3RtoBlackholeDetection) { - if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { return; } QuicConfig config; @@ -11063,7 +11063,7 @@ } TEST_P(QuicConnectionTest, 4RtoBlackholeDetection) { - if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { return; } QuicConfig config; @@ -11090,7 +11090,7 @@ } TEST_P(QuicConnectionTest, 6RtoBlackholeDetection) { - if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection)) { + if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { return; } QuicConfig config; @@ -11116,6 +11116,49 @@ QuicConnectionPeer::GetBlackholeDetectionDeadline(&connection_)); } +// Regresstion test for b/158491591. +TEST_P(QuicConnectionTest, MadeForwardProgressOnDiscardingKeys) { + if (!connection_.SupportsMultiplePacketNumberSpaces()) { + return; + } + use_tagging_decrypter(); + // Send handshake packet. + connection_.SetEncrypter(ENCRYPTION_HANDSHAKE, + std::make_unique<TaggingEncrypter>(0x02)); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE); + EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(1); + QuicConfig config; + QuicTagVector connection_options; + connection_options.push_back(k5RTO); + config.SetConnectionOptionsToSend(connection_options); + QuicConfigPeer::SetNegotiated(&config, true); + if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { + EXPECT_CALL(visitor_, GetHandshakeState()) + .WillRepeatedly(Return(HANDSHAKE_COMPLETE)); + } + if (connection_.version().AuthenticatesHandshakeConnectionIds()) { + QuicConfigPeer::SetReceivedOriginalConnectionId( + &config, connection_.connection_id()); + QuicConfigPeer::SetReceivedInitialSourceConnectionId( + &config, connection_.connection_id()); + } + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); + connection_.SetFromConfig(config); + + connection_.SendCryptoDataWithString("foo", 0, ENCRYPTION_HANDSHAKE); + EXPECT_TRUE(connection_.BlackholeDetectionInProgress()); + // Discard handshake keys. + connection_.OnHandshakeComplete(); + if (GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) { + // Verify blackhole detection stops. + EXPECT_FALSE(connection_.BlackholeDetectionInProgress()); + } else { + // Problematic: although there is nothing in flight, blackhole detection is + // still in progress. + EXPECT_TRUE(connection_.BlackholeDetectionInProgress()); + } +} + } // namespace } // namespace test } // namespace quic