Remove `process_timestamps_` and use `max_receive_timestamps_per_ack` as the control for whether QUIC ACK timestamps are processed. PiperOrigin-RevId: 937984021
diff --git a/quiche/quic/core/quic_framer.cc b/quiche/quic/core/quic_framer.cc index 73f79c1..85b5f0c 100644 --- a/quiche/quic/core/quic_framer.cc +++ b/quiche/quic/core/quic_framer.cc
@@ -432,11 +432,8 @@ alternative_decrypter_latch_(false), perspective_(perspective), validate_flags_(true), - process_timestamps_(false), - peer_max_receive_timestamps_per_ack_( - std::numeric_limits<uint32_t>::max()), - local_max_receive_timestamps_per_ack_( - std::numeric_limits<uint32_t>::max()), + peer_max_receive_timestamps_per_ack_(0), + local_max_receive_timestamps_per_ack_(0), peer_receive_timestamps_exponent_(0), local_receive_timestamps_exponent_(0), process_reset_stream_at_(false), @@ -3105,7 +3102,7 @@ } case IETF_ACK_RECEIVE_TIMESTAMPS: case IETF_ACK_RECEIVE_TIMESTAMPS_ECN: - if (!process_timestamps_) { + if (local_max_receive_timestamps_per_ack_ == 0) { set_detailed_error("Unsupported frame type."); QUIC_DLOG(WARNING) << ENDPOINT << "IETF_ACK_RECEIVE_TIMESTAMPS not supported"; @@ -3835,7 +3832,7 @@ } if (AckHasTimestamps(frame_type)) { - QUICHE_DCHECK(process_timestamps_); + QUICHE_DCHECK_GT(local_max_receive_timestamps_per_ack_, 0u); if (!ProcessIetfTimestampsInAckFrame(ack_frame->largest_acked, reader)) { return false; }
diff --git a/quiche/quic/core/quic_framer.h b/quiche/quic/core/quic_framer.h index c0c9f2f..3ca0db2 100644 --- a/quiche/quic/core/quic_framer.h +++ b/quiche/quic/core/quic_framer.h
@@ -325,11 +325,6 @@ // TODO(b/132632465): Remove the const in timestamp-related setters once // timestamps are negotiated via transport params. - // Allows enabling or disabling of timestamp processing and serialization. - void set_process_timestamps(bool process_timestamps) const { - process_timestamps_ = process_timestamps; - } - // Sets the max number of receive timestamps to parse per ACK frame. void set_local_max_receive_timestamps_per_ack(uint32_t max_timestamps) const { local_max_receive_timestamps_per_ack_ = max_timestamps; @@ -1126,7 +1121,6 @@ // IETF_ACK_RECEIVE_TIMESTAMPS frame type. bool UseIetfAckWithReceiveTimestamp(const QuicAckFrame& frame) const { return VersionIsIetfQuic(version_.transport_version) && - process_timestamps_ && std::min<uint64_t>(peer_max_receive_timestamps_per_ack_, frame.received_packet_times.size()) > 0; } @@ -1167,10 +1161,6 @@ bool validate_flags_; // The diversification nonce from the last received packet. DiversificationNonce last_nonce_; - // If true, send and process timestamps in the ACK frame. - // TODO(ianswett): Remove the mutables once set_process_timestamps and - // set_receive_timestamp_exponent_ aren't const. - mutable bool process_timestamps_; // The max number of receive timestamps to send per ACK frame. mutable uint32_t peer_max_receive_timestamps_per_ack_; // The max number of receive timestamps to allow in an incoming ACK frame. @@ -1183,7 +1173,7 @@ bool process_reset_stream_at_; // The creation time of the connection, used to calculate timestamps. QuicTime creation_time_; - // The last timestamp received if process_timestamps_ is true. + // The last timestamp received if local_max_receive_timestamps_per_ack_ > 0. QuicTime::Delta last_timestamp_; // Whether IETF QUIC Key Update is supported on this connection.
diff --git a/quiche/quic/core/quic_framer_test.cc b/quiche/quic/core/quic_framer_test.cc index 547c70a..b6da809 100644 --- a/quiche/quic/core/quic_framer_test.cc +++ b/quiche/quic/core/quic_framer_test.cc
@@ -3683,7 +3683,7 @@ std::unique_ptr<QuicEncryptedPacket> encrypted( AssemblePacketFromFragments(fragments)); - framer_.set_process_timestamps(true); + framer_.set_local_max_receive_timestamps_per_ack(1000); EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); EXPECT_THAT(framer_.error(), IsQuicNoError()); @@ -3778,7 +3778,7 @@ std::unique_ptr<QuicEncryptedPacket> encrypted( AssemblePacketFromFragments(packet_ietf)); - framer_.set_process_timestamps(true); + framer_.set_local_max_receive_timestamps_per_ack(1000); EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); EXPECT_THAT(framer_.error(), IsQuicNoError()); ASSERT_TRUE(visitor_.header_.get()); @@ -3858,7 +3858,7 @@ std::unique_ptr<QuicEncryptedPacket> encrypted( AssemblePacketFromFragments(packet_ietf)); - framer_.set_process_timestamps(true); + framer_.set_local_max_receive_timestamps_per_ack(1000); EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); EXPECT_THAT(framer_.error(), IsQuicNoError()); ASSERT_TRUE(visitor_.header_.get()); @@ -3930,7 +3930,7 @@ AssemblePacketFromFragments(packet_ietf)); framer_.set_local_receive_timestamps_exponent(3); - framer_.set_process_timestamps(true); + framer_.set_local_max_receive_timestamps_per_ack(1000); EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); EXPECT_THAT(framer_.error(), IsQuicNoError()); ASSERT_TRUE(visitor_.header_.get()); @@ -3993,7 +3993,7 @@ std::unique_ptr<QuicEncryptedPacket> encrypted( AssemblePacketFromFragments(packet_ietf)); - framer_.set_process_timestamps(true); + framer_.set_local_max_receive_timestamps_per_ack(1000); EXPECT_FALSE(framer_.ProcessPacket(*encrypted)); EXPECT_TRUE(absl::StartsWith(framer_.detailed_error(), "Receive delta largest acked too high.")); @@ -4049,7 +4049,7 @@ std::unique_ptr<QuicEncryptedPacket> encrypted( AssemblePacketFromFragments(packet_ietf)); - framer_.set_process_timestamps(true); + framer_.set_local_max_receive_timestamps_per_ack(1000); EXPECT_FALSE(framer_.ProcessPacket(*encrypted)); EXPECT_TRUE(absl::StartsWith(framer_.detailed_error(), "Receive timestamp delta too high.")); @@ -4103,7 +4103,7 @@ std::unique_ptr<QuicEncryptedPacket> encrypted( AssemblePacketFromFragments(packet_ietf)); - framer_.set_process_timestamps(true); + framer_.set_local_max_receive_timestamps_per_ack(1000); EXPECT_FALSE(framer_.ProcessPacket(*encrypted)); EXPECT_TRUE(absl::StartsWith(framer_.detailed_error(), "Receive timestamp count too high.")); @@ -6642,7 +6642,6 @@ }; // clang-format on - framer_.set_process_timestamps(true); framer_.set_peer_max_receive_timestamps_per_ack(8); std::unique_ptr<QuicPacket> data(BuildDataPacket(header, frames)); ASSERT_TRUE(data != nullptr); @@ -6744,7 +6743,6 @@ }; // clang-format on - framer_.set_process_timestamps(true); framer_.set_peer_max_receive_timestamps_per_ack(8); std::unique_ptr<QuicPacket> data(BuildDataPacket(header, frames)); ASSERT_TRUE(data != nullptr); @@ -6838,7 +6836,6 @@ }; // clang-format on - framer_.set_process_timestamps(true); framer_.set_peer_max_receive_timestamps_per_ack(8); std::unique_ptr<QuicPacket> data(BuildDataPacket(header, frames)); ASSERT_TRUE(data != nullptr); @@ -6940,7 +6937,6 @@ }; // clang-format on - framer_.set_process_timestamps(true); framer_.set_peer_max_receive_timestamps_per_ack(4); std::unique_ptr<QuicPacket> data(BuildDataPacket(header, frames)); ASSERT_TRUE(data != nullptr); @@ -7048,7 +7044,6 @@ }; // clang-format on - framer_.set_process_timestamps(true); framer_.set_peer_max_receive_timestamps_per_ack(8); framer_.set_peer_receive_timestamps_exponent(3); std::unique_ptr<QuicPacket> data(BuildDataPacket(header, frames)); @@ -7065,7 +7060,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(8); framer_.set_peer_max_receive_timestamps_per_ack(8); @@ -7119,7 +7113,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(2); framer_.set_peer_max_receive_timestamps_per_ack(2); @@ -7168,7 +7161,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(8); framer_.set_peer_max_receive_timestamps_per_ack(8); framer_.set_local_receive_timestamps_exponent(3); @@ -7218,7 +7210,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(8); framer_.set_peer_max_receive_timestamps_per_ack(8); framer_.set_local_receive_timestamps_exponent(3); @@ -7273,7 +7264,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(8); framer_.set_peer_max_receive_timestamps_per_ack(8); framer_.set_local_receive_timestamps_exponent(3); @@ -7323,7 +7313,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(8); framer_.set_peer_max_receive_timestamps_per_ack(8); @@ -7377,7 +7366,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(8); framer_.set_peer_max_receive_timestamps_per_ack(8); @@ -7424,7 +7412,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_peer_max_receive_timestamps_per_ack(8); framer_.set_peer_receive_timestamps_exponent(3); @@ -7455,7 +7442,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(8); framer_.set_peer_max_receive_timestamps_per_ack(8); framer_.set_local_receive_timestamps_exponent(3); @@ -7501,7 +7487,6 @@ framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); framer_.SetKeyUpdateSupportForConnection(true); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(8192); framer_.set_peer_max_receive_timestamps_per_ack(8192); framer_.set_local_receive_timestamps_exponent(3); @@ -7544,7 +7529,6 @@ return; } SetDecrypterLevel(ENCRYPTION_FORWARD_SECURE); - framer_.set_process_timestamps(true); framer_.set_local_max_receive_timestamps_per_ack(8); framer_.set_peer_max_receive_timestamps_per_ack(8); framer_.set_local_receive_timestamps_exponent(3); @@ -7594,6 +7578,89 @@ EXPECT_TRUE(processed_ack_frame.received_packet_times.empty()); } +TEST_P(QuicFramerTest, AckReceiveTimestampsDisabledByDefault) { + if (!VersionIsIetfQuic(framer_.transport_version())) { + return; + } + SetDecrypterLevel(ENCRYPTION_FORWARD_SECURE); + framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, + std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); + framer_.SetKeyUpdateSupportForConnection(true); + + // Do NOT configure peer_max_receive_timestamps_per_ack or + // local_max_receive_timestamps_per_ack. They should default to 0. + + QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT); + QuicPacketHeader header; + header.destination_connection_id = FramerTestConnectionId(); + header.reset_flag = false; + header.version_flag = false; + header.packet_number = kPacketNumber; + + QuicAckFrame ack_frame = InitAckFrame(kSmallLargestObserved); + ack_frame.received_packet_times = PacketTimeVector{ + {kSmallLargestObserved - 5, CreationTimePlus((0x29ff << 3))}, + {kSmallLargestObserved - 4, CreationTimePlus((0x29ff << 3))}, + }; + ack_frame.ack_delay_time = QuicTime::Delta::Zero(); + QuicFrames frames = {QuicFrame(&ack_frame)}; + + std::unique_ptr<QuicPacket> data(BuildDataPacket(header, frames)); + ASSERT_TRUE(data != nullptr); + std::unique_ptr<QuicEncryptedPacket> encrypted( + EncryptPacketWithTagAndPhase(*data, 0, false)); + ASSERT_TRUE(encrypted); + QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_SERVER); + EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); + EXPECT_THAT(framer_.error(), IsQuicNoError()); + + const QuicAckFrame& frame = *visitor_.ack_frames_[0]; + // Timestamps should not be sent. + EXPECT_TRUE(frame.received_packet_times.empty()); +} + +TEST_P(QuicFramerTest, AckReceiveTimestampsErrorByDefault) { + if (!VersionIsIetfQuic(framer_.transport_version())) { + return; + } + SetDecrypterLevel(ENCRYPTION_FORWARD_SECURE); + framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, + std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); + framer_.SetKeyUpdateSupportForConnection(true); + + // Configure peer_max_receive_timestamps_per_ack on the client side so it + // generates a packet with timestamps. + framer_.set_peer_max_receive_timestamps_per_ack(8); + + QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT); + QuicPacketHeader header; + header.destination_connection_id = FramerTestConnectionId(); + header.reset_flag = false; + header.version_flag = false; + header.packet_number = kPacketNumber; + + QuicAckFrame ack_frame = InitAckFrame(kSmallLargestObserved); + ack_frame.received_packet_times = PacketTimeVector{ + {kSmallLargestObserved - 5, CreationTimePlus((0x29ff << 3))}, + {kSmallLargestObserved - 4, CreationTimePlus((0x29ff << 3))}, + }; + ack_frame.ack_delay_time = QuicTime::Delta::Zero(); + QuicFrames frames = {QuicFrame(&ack_frame)}; + + std::unique_ptr<QuicPacket> data(BuildDataPacket(header, frames)); + ASSERT_TRUE(data != nullptr); + std::unique_ptr<QuicEncryptedPacket> encrypted( + EncryptPacketWithTagAndPhase(*data, 0, false)); + ASSERT_TRUE(encrypted); + + QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_SERVER); + // local_max_receive_timestamps_per_ack should be 0 by default. + + EXPECT_FALSE(framer_.ProcessPacket(*encrypted)); + EXPECT_THAT(framer_.error(), IsError(QUIC_INVALID_FRAME_DATA)); + EXPECT_EQ("Unsupported frame type.", framer_.detailed_error()); +} + TEST_P(QuicFramerTest, BuildAckFramePacketOneAckBlockMaxLength) { QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT); QuicPacketHeader header; @@ -15830,7 +15897,7 @@ // - The timestamp count always has to be included, meaning 1 extra byte. // - The timestamps themselves are truncatable, but the range specified above // is 3 bytes. - framer_.set_process_timestamps(true); + framer_.set_peer_max_receive_timestamps_per_ack(1000); ack_frame.ecn_counters = std::nullopt; EXPECT_THAT(GetAckFrameSizes(ack_frame), ElementsAre(9, 12)); ack_frame.ecn_counters = QuicEcnCounts(1, 2, 3); @@ -15877,7 +15944,7 @@ // - The timestamps themselves are truncatable 3 extra bytes. // This means that the minimum size is now 12 bytes; the extra 3 bytes of // timestamps will be truncated before the extra 2 bytes of ACK ranges. - framer_.set_process_timestamps(true); + framer_.set_peer_max_receive_timestamps_per_ack(1000); ack_frame.received_packet_times.push_back( {QuicPacketNumber(3), CreationTimePlus(11)}); EXPECT_THAT(GetAckFrameSizes(ack_frame), ElementsAre(12, 14, 17));