From b72b3bae2983b19d19c7b483f6d08db45ef4a1ed Mon Sep 17 00:00:00 2001
From: Lars Eggert <lars@eggert.org>
Date: Tue, 17 Sep 2024 06:03:18 -0700
Subject: [PATCH 1/3] fix: Don't encode large RTT guesses in tickets (#2114)

* fix: Don't encode large RTT guesses in tickets

Because under lossy conditions (e.g., QNS `handshakeloss` test), the
guess can be multiple times the actual RTT, which when encoded in the
resumption ticket will cause an extremely slow second handshake, often
causing the test to time out.

Broken out of #1998
Fixes #2088

* Fixes & tests

* Suggestion from @mxinden

* Fix
---
 neqo-transport/src/connection/mod.rs          |  22 +++-
 .../src/connection/tests/resumption.rs        | 124 +++++++++++++++++-
 neqo-transport/src/path.rs                    |   4 +-
 neqo-transport/src/recovery/mod.rs            |  10 +-
 neqo-transport/src/rtt.rs                     |  25 +++-
 5 files changed, 173 insertions(+), 12 deletions(-)

diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs
index 29bc85ff73..577a93277c 100644
--- a/neqo-transport/src/connection/mod.rs
+++ b/neqo-transport/src/connection/mod.rs
@@ -48,7 +48,7 @@ use crate::{
     quic_datagrams::{DatagramTracking, QuicDatagrams},
     recovery::{LossRecovery, RecoveryToken, SendProfile, SentPacket},
     recv_stream::RecvStreamStats,
-    rtt::{RttEstimate, GRANULARITY},
+    rtt::{RttEstimate, GRANULARITY, INITIAL_RTT},
     send_stream::SendStream,
     stats::{Stats, StatsCell},
     stream_id::StreamType,
@@ -594,9 +594,25 @@ impl Connection {
     fn make_resumption_token(&mut self) -> ResumptionToken {
         debug_assert_eq!(self.role, Role::Client);
         debug_assert!(self.crypto.has_resumption_token());
+        // Values less than GRANULARITY are ignored when using the token, so use 0 where needed.
         let rtt = self.paths.primary().map_or_else(
-            || RttEstimate::default().estimate(),
-            |p| p.borrow().rtt().estimate(),
+            // If we don't have a path, we don't have an RTT.
+            || Duration::from_millis(0),
+            |p| {
+                let rtt = p.borrow().rtt().estimate();
+                if p.borrow().rtt().is_guesstimate() {
+                    // When we have no actual RTT sample, do not encode a guestimated RTT larger
+                    // than the default initial RTT. (The guess can be very large under lossy
+                    // conditions.)
+                    if rtt < INITIAL_RTT {
+                        rtt
+                    } else {
+                        Duration::from_millis(0)
+                    }
+                } else {
+                    rtt
+                }
+            },
         );
 
         self.crypto
diff --git a/neqo-transport/src/connection/tests/resumption.rs b/neqo-transport/src/connection/tests/resumption.rs
index 7410e76ef8..01e977e506 100644
--- a/neqo-transport/src/connection/tests/resumption.rs
+++ b/neqo-transport/src/connection/tests/resumption.rs
@@ -6,7 +6,16 @@
 
 use std::{cell::RefCell, mem, rc::Rc, time::Duration};
 
-use test_fixture::{assertions, now};
+use neqo_common::{Datagram, Decoder, Role};
+use neqo_crypto::AuthenticationStatus;
+use test_fixture::{
+    assertions,
+    header_protection::{
+        apply_header_protection, decode_initial_header, initial_aead_and_hp,
+        remove_header_protection,
+    },
+    now, split_datagram,
+};
 
 use super::{
     connect, connect_with_rtt, default_client, default_server, exchange_ticket, get_tokens,
@@ -14,7 +23,9 @@ use super::{
 };
 use crate::{
     addr_valid::{AddressValidation, ValidateAddress},
-    ConnectionParameters, Error, Version,
+    frame::FRAME_TYPE_PADDING,
+    rtt::INITIAL_RTT,
+    ConnectionParameters, Error, State, Version, MIN_INITIAL_PACKET_SIZE,
 };
 
 #[test]
@@ -75,6 +86,115 @@ fn remember_smoothed_rtt() {
     );
 }
 
+fn ticket_rtt(rtt: Duration) -> Duration {
+    // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark.
+    const ACK_FRAME_1: &[u8] = &[0x03, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00];
+
+    let mut client = new_client(
+        ConnectionParameters::default().versions(Version::Version1, vec![Version::Version1]),
+    );
+    let mut server = default_server();
+    let mut now = now();
+
+    let client_initial = client.process_output(now);
+    let (_, client_dcid, _, _) =
+        decode_initial_header(client_initial.as_dgram_ref().unwrap(), Role::Client).unwrap();
+    let client_dcid = client_dcid.to_owned();
+
+    now += rtt / 2;
+    let server_packet = server.process(client_initial.as_dgram_ref(), now).dgram();
+    let (server_initial, server_hs) = split_datagram(server_packet.as_ref().unwrap());
+    let (protected_header, _, _, payload) =
+        decode_initial_header(&server_initial, Role::Server).unwrap();
+
+    // Now decrypt the packet.
+    let (aead, hp) = initial_aead_and_hp(&client_dcid, Role::Server);
+    let (header, pn) = remove_header_protection(&hp, protected_header, payload);
+    assert_eq!(pn, 0);
+    let pn_len = header.len() - protected_header.len();
+    let mut buf = vec![0; payload.len()];
+    let mut plaintext = aead
+        .decrypt(pn, &header, &payload[pn_len..], &mut buf)
+        .unwrap()
+        .to_owned();
+
+    // Now we need to find the frames.  Make some really strong assumptions.
+    let mut dec = Decoder::new(&plaintext[..]);
+    assert_eq!(dec.decode(ACK_FRAME_1.len()), Some(ACK_FRAME_1));
+    assert_eq!(dec.decode_varint(), Some(0x06)); // CRYPTO
+    assert_eq!(dec.decode_varint(), Some(0x00)); // offset
+    dec.skip_vvec(); // Skip over the payload.
+
+    // Replace the ACK frame with PADDING.
+    plaintext[..ACK_FRAME_1.len()].fill(FRAME_TYPE_PADDING.try_into().unwrap());
+
+    // And rebuild a packet.
+    let mut packet = header.clone();
+    packet.resize(MIN_INITIAL_PACKET_SIZE, 0);
+    aead.encrypt(pn, &header, &plaintext, &mut packet[header.len()..])
+        .unwrap();
+    apply_header_protection(&hp, &mut packet, protected_header.len()..header.len());
+    let si = Datagram::new(
+        server_initial.source(),
+        server_initial.destination(),
+        server_initial.tos(),
+        packet,
+    );
+
+    // Now a connection can be made successfully.
+    now += rtt / 2;
+    client.process_input(&si, now);
+    client.process_input(&server_hs.unwrap(), now);
+    client.authenticated(AuthenticationStatus::Ok, now);
+    let finished = client.process_output(now);
+
+    assert_eq!(*client.state(), State::Connected);
+
+    now += rtt / 2;
+    _ = server.process(finished.as_dgram_ref(), now);
+    assert_eq!(*server.state(), State::Confirmed);
+
+    // Don't deliver the server's handshake finished, it has ACKs.
+    // Now get a ticket.
+    let validation = AddressValidation::new(now, ValidateAddress::NoToken).unwrap();
+    let validation = Rc::new(RefCell::new(validation));
+    server.set_validation(&validation);
+    send_something(&mut server, now);
+    server.send_ticket(now, &[]).expect("can send ticket");
+    let ticket = server.process_output(now).dgram();
+    assert!(ticket.is_some());
+    now += rtt / 2;
+    client.process_input(&ticket.unwrap(), now);
+    let token = get_tokens(&mut client).pop().unwrap();
+
+    // And connect again.
+    let mut client = default_client();
+    client.enable_resumption(now, token).unwrap();
+    let ticket_rtt = client.paths.rtt();
+    let mut server = resumed_server(&client);
+
+    connect_with_rtt(&mut client, &mut server, now, Duration::from_millis(50));
+    assert_eq!(
+        client.paths.rtt(),
+        Duration::from_millis(50),
+        "previous RTT should be completely erased"
+    );
+    ticket_rtt
+}
+
+#[test]
+fn ticket_rtt_less_than_default() {
+    assert_eq!(
+        ticket_rtt(Duration::from_millis(10)),
+        Duration::from_millis(10)
+    );
+}
+
+#[test]
+fn ticket_rtt_larger_than_default() {
+    assert_eq!(ticket_rtt(Duration::from_millis(500)), INITIAL_RTT);
+}
+
 /// Check that a resumed connection uses a token on Initial packets.
 #[test]
 fn address_validation_token_resume() {
diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs
index 35d29f0253..49c289f60b 100644
--- a/neqo-transport/src/path.rs
+++ b/neqo-transport/src/path.rs
@@ -27,7 +27,7 @@ use crate::{
     packet::PacketBuilder,
     pmtud::Pmtud,
     recovery::{RecoveryToken, SentPacket},
-    rtt::RttEstimate,
+    rtt::{RttEstimate, RttSource},
     sender::PacketSender,
     stats::FrameStats,
     tracking::PacketNumberSpace,
@@ -984,7 +984,7 @@ impl Path {
                 &self.qlog,
                 now - sent.time_sent(),
                 Duration::new(0, 0),
-                false,
+                RttSource::Guesstimate,
                 now,
             );
         }
diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs
index 5820dc07a0..f2c3e8e298 100644
--- a/neqo-transport/src/recovery/mod.rs
+++ b/neqo-transport/src/recovery/mod.rs
@@ -27,7 +27,7 @@ use crate::{
     packet::PacketNumber,
     path::{Path, PathRef},
     qlog::{self, QlogMetric},
-    rtt::RttEstimate,
+    rtt::{RttEstimate, RttSource},
     stats::{Stats, StatsCell},
     tracking::{PacketNumberSpace, PacketNumberSpaceSet},
 };
@@ -558,9 +558,13 @@ impl LossRecovery {
         now: Instant,
         ack_delay: Duration,
     ) {
-        let confirmed = self.confirmed_time.map_or(false, |t| t < send_time);
+        let source = if self.confirmed_time.map_or(false, |t| t < send_time) {
+            RttSource::AckConfirmed
+        } else {
+            RttSource::Ack
+        };
         if let Some(sample) = now.checked_duration_since(send_time) {
-            rtt.update(&self.qlog, sample, ack_delay, confirmed, now);
+            rtt.update(&self.qlog, sample, ack_delay, source, now);
         }
     }
 
diff --git a/neqo-transport/src/rtt.rs b/neqo-transport/src/rtt.rs
index 7bfbbe4aaa..027b574aad 100644
--- a/neqo-transport/src/rtt.rs
+++ b/neqo-transport/src/rtt.rs
@@ -28,6 +28,17 @@ pub const GRANULARITY: Duration = Duration::from_millis(1);
 // Defined in -recovery 6.2 as 333ms but using lower value.
 pub const INITIAL_RTT: Duration = Duration::from_millis(100);
 
+/// The source of the RTT measurement.
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)]
+pub enum RttSource {
+    /// RTT guess from a retry or dropping a packet number space.
+    Guesstimate,
+    /// Ack on an unconfirmed connection.
+    Ack,
+    /// Ack on a confirmed connection.
+    AckConfirmed,
+}
+
 #[derive(Debug)]
 #[allow(clippy::module_name_repetitions)]
 pub struct RttEstimate {
@@ -37,6 +48,7 @@ pub struct RttEstimate {
     rttvar: Duration,
     min_rtt: Duration,
     ack_delay: PeerAckDelay,
+    best_source: RttSource,
 }
 
 impl RttEstimate {
@@ -58,6 +70,7 @@ impl RttEstimate {
             rttvar: Duration::from_millis(0),
             min_rtt: rtt,
             ack_delay: PeerAckDelay::Fixed(Duration::from_millis(25)),
+            best_source: RttSource::Ack,
         }
     }
 
@@ -83,17 +96,24 @@ impl RttEstimate {
         self.ack_delay.update(cwnd, mtu, self.smoothed_rtt);
     }
 
+    pub fn is_guesstimate(&self) -> bool {
+        self.best_source == RttSource::Guesstimate
+    }
+
     pub fn update(
         &mut self,
         qlog: &NeqoQlog,
         mut rtt_sample: Duration,
         ack_delay: Duration,
-        confirmed: bool,
+        source: RttSource,
         now: Instant,
     ) {
+        debug_assert!(source >= self.best_source);
+        self.best_source = max(self.best_source, source);
+
         // Limit ack delay by max_ack_delay if confirmed.
         let mad = self.ack_delay.max();
-        let ack_delay = if confirmed && ack_delay > mad {
+        let ack_delay = if self.best_source == RttSource::AckConfirmed && ack_delay > mad {
             mad
         } else {
             ack_delay
@@ -205,6 +225,7 @@ impl Default for RttEstimate {
             rttvar: INITIAL_RTT / 2,
             min_rtt: INITIAL_RTT,
             ack_delay: PeerAckDelay::default(),
+            best_source: RttSource::Guesstimate,
         }
     }
 }

From 0e40d362a94a106d60c43e08d7d9eef5ce2838d2 Mon Sep 17 00:00:00 2001
From: Lars Eggert <lars@eggert.org>
Date: Tue, 17 Sep 2024 23:28:13 -0700
Subject: [PATCH 2/3] ci: Run more things in parallel (#2125)

* `cargo machete`
* `cargo fmt`
* `cargo clippy`
---
 .github/workflows/check.yml   | 59 +++++++----------------------------
 .github/workflows/clippy.yml  | 47 ++++++++++++++++++++++++++++
 .github/workflows/machete.yml | 34 ++++++++++++++++++++
 .github/workflows/mutants.yml | 19 +++++------
 .github/workflows/rustfmt.yml | 32 +++++++++++++++++++
 5 files changed, 133 insertions(+), 58 deletions(-)
 create mode 100644 .github/workflows/clippy.yml
 create mode 100644 .github/workflows/machete.yml
 create mode 100644 .github/workflows/rustfmt.yml

diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml
index e2861e3400..49dfb8ec80 100644
--- a/.github/workflows/check.yml
+++ b/.github/workflows/check.yml
@@ -7,6 +7,7 @@ on:
     branches: ["main"]
     paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
   merge_group:
+  workflow_dispatch:
 env:
   CARGO_TERM_COLOR: always
   RUST_BACKTRACE: 1
@@ -20,7 +21,6 @@ permissions:
 
 jobs:
   check:
-    name: Build & test
     strategy:
       fail-fast: false
       matrix:
@@ -42,23 +42,19 @@ jobs:
         shell: bash
 
     steps:
-      - name: Checkout
-        uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
+      - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
 
-      - name: Install Rust
-        uses: ./.github/actions/rust
+      - uses: ./.github/actions/rust
         with:
           version: ${{ matrix.rust-toolchain }}
-          components: rustfmt, clippy, llvm-tools-preview
-          tools: cargo-llvm-cov, cargo-nextest, cargo-hack, cargo-fuzz, cargo-machete
+          components: clippy, llvm-tools-preview
+          tools: cargo-llvm-cov, cargo-nextest, cargo-hack, cargo-fuzz
           token: ${{ secrets.GITHUB_TOKEN }}
 
-      - name: Get minimum NSS version
-        id: nss-version
+      - id: nss-version
         run: echo "minimum=$(cat neqo-crypto/min_version.txt)" >> "$GITHUB_OUTPUT"
 
-      - name: Install NSS
-        uses: ./.github/actions/nss
+      - uses: ./.github/actions/nss
         with:
           minimum-version: ${{ steps.nss-version.outputs.minimum }}
 
@@ -66,6 +62,10 @@ jobs:
         run: |
           # shellcheck disable=SC2086
           cargo +${{ matrix.rust-toolchain }} build $BUILD_TYPE --all-targets --features ci
+          # Check that the fuzz targets also build
+          if [ ${{ startsWith(matrix.rust-toolchain, 'nightly') && 'nightly' }} == 'nightly' ]; then
+            cargo +${{ matrix.rust-toolchain }} fuzz check
+          fi
 
       - name: Run tests and determine coverage
         run: |
@@ -90,41 +90,7 @@ jobs:
           RUST_LOG: warn
           BUILD_DIR: ${{ matrix.type == 'release' && 'release' || 'debug' }}
 
-      - name: Check formatting
-        run: |
-          if [ "${{ startsWith(matrix.rust-toolchain, 'nightly') && 'nightly' }}" != "nightly" ]; then
-            CONFIG_PATH="--config-path=$(mktemp)"
-          fi
-          # shellcheck disable=SC2086
-          cargo +${{ matrix.rust-toolchain }} fmt --all -- --check $CONFIG_PATH
-        if: success() || failure()
-
-      - name: Check for unused dependencies
-        run: |
-          #  --with-metadata has false positives, see https://github.com/bnjbvr/cargo-machete/issues/127
-          cargo +${{ matrix.rust-toolchain }} machete
-
-      - name: Clippy
-        run: |
-          # Use cargo-hack to run clippy on each crate individually with its
-          # respective default features only. Can reveal warnings otherwise
-          # hidden given that a plain cargo clippy combines all features of the
-          # workspace. See e.g. https://github.com/mozilla/neqo/pull/1695.
-          cargo +${{ matrix.rust-toolchain }} hack clippy --all-targets --feature-powerset --exclude-features gecko -- -D warnings || ${{ matrix.rust-toolchain == 'nightly' }}
-          # Check that the fuzz targets also build
-          if [ ${{ startsWith(matrix.rust-toolchain, 'nightly') && 'nightly' }} == 'nightly' ]; then
-            cargo +${{ matrix.rust-toolchain }} fuzz check
-          fi
-        if: success() || failure()
-
-      - name: Check rustdoc links
-        run: cargo +${{ matrix.rust-toolchain }} doc --workspace --no-deps --document-private-items
-        env:
-          RUSTDOCFLAGS: "--deny rustdoc::broken_intra_doc_links --deny warnings"
-        if: success() || failure()
-
-      - name: Upload coverage reports to Codecov
-        uses: codecov/codecov-action@e28ff129e5465c2c0dcc6f003fc735cb6ae0c673 # v4.5.0
+      - uses: codecov/codecov-action@e28ff129e5465c2c0dcc6f003fc735cb6ae0c673 # v4.5.0
         with:
           file: lcov.info
           fail_ci_if_error: false
@@ -135,6 +101,5 @@ jobs:
         if: matrix.type == 'debug' && matrix.rust-toolchain == 'stable'
 
   bench:
-    name: "Benchmark"
     needs: [check]
     uses: ./.github/workflows/bench.yml
diff --git a/.github/workflows/clippy.yml b/.github/workflows/clippy.yml
new file mode 100644
index 0000000000..a1ef1ed6ba
--- /dev/null
+++ b/.github/workflows/clippy.yml
@@ -0,0 +1,47 @@
+name: Clippy
+on:
+  push:
+    branches: ["main"]
+    paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
+  pull_request:
+    branches: ["main"]
+    paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
+  merge_group:
+  workflow_dispatch:
+env:
+  CARGO_TERM_COLOR: always
+  RUST_BACKTRACE: 1
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref_name }}
+  cancel-in-progress: true
+
+permissions:
+  contents: read
+
+jobs:
+  clippy:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
+      - uses: ./.github/actions/rust
+        with:
+          components: clippy
+          tools: cargo-hack, cargo-fuzz
+          token: ${{ secrets.GITHUB_TOKEN }}
+
+      - id: nss-version
+        run: echo "minimum=$(cat neqo-crypto/min_version.txt)" >> "$GITHUB_OUTPUT"
+
+      - uses: ./.github/actions/nss
+        with:
+          minimum-version: ${{ steps.nss-version.outputs.minimum }}
+
+      # Use cargo-hack to run clippy on each crate individually with its
+      # respective default features only. Can reveal warnings otherwise
+      # hidden given that a plain cargo clippy combines all features of the
+      # workspace. See e.g. https://github.com/mozilla/neqo/pull/1695.
+      - run: cargo hack clippy --all-targets --feature-powerset --exclude-features gecko -- -D warnings
+      - run: cargo doc --workspace --no-deps --document-private-items
+        env:
+          RUSTDOCFLAGS: "--deny rustdoc::broken_intra_doc_links --deny warnings"
diff --git a/.github/workflows/machete.yml b/.github/workflows/machete.yml
new file mode 100644
index 0000000000..3e7404bba1
--- /dev/null
+++ b/.github/workflows/machete.yml
@@ -0,0 +1,34 @@
+name: Machete
+on:
+  workflow_dispatch:
+  pull_request:
+    branches: ["main"]
+    paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
+  merge_group:
+env:
+  CARGO_TERM_COLOR: always
+  RUST_BACKTRACE: 1
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref_name }}
+  cancel-in-progress: true
+
+permissions:
+  contents: read
+
+jobs:
+  machete:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
+
+      - name: Install Rust
+        uses: ./.github/actions/rust
+        with:
+          tools: cargo-machete
+          token: ${{ secrets.GITHUB_TOKEN }}
+
+      - name: Check for unused dependencies
+        run: |
+          #  --with-metadata has false positives, see https://github.com/bnjbvr/cargo-machete/issues/127
+          cargo machete
diff --git a/.github/workflows/mutants.yml b/.github/workflows/mutants.yml
index 202f53d652..e1e0314ea3 100644
--- a/.github/workflows/mutants.yml
+++ b/.github/workflows/mutants.yml
@@ -1,11 +1,12 @@
 name: Find mutants
 on:
-  schedule:
-    - cron: '42 3 * * 2,5' # Runs at 03:42 UTC (m and h chosen arbitrarily) twice a week.
-  workflow_dispatch:
+  push:
+    branches: ["main"]
+    paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
   pull_request:
     branches: ["main"]
     paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
+  workflow_dispatch:
 
 concurrency:
   group: ${{ github.workflow }}-${{ github.ref_name }}
@@ -22,17 +23,14 @@ jobs:
         with:
           fetch-depth: 0
 
-      - name: Get minimum NSS version
-        id: nss-version
+      - id: nss-version
         run: echo "minimum=$(cat neqo-crypto/min_version.txt)" >> "$GITHUB_OUTPUT"
 
-      - name: Install NSS
-        uses: ./.github/actions/nss
+      - uses: ./.github/actions/nss
         with:
           minimum-version: ${{ steps.nss-version.outputs.minimum }}
 
-      - name: Install Rust
-        uses: ./.github/actions/rust
+      - uses: ./.github/actions/rust
         with:
           tools: cargo-mutants
           token: ${{ secrets.GITHUB_TOKEN }}
@@ -63,8 +61,7 @@ jobs:
             echo '```'
           } > "$GITHUB_STEP_SUMMARY"
 
-      - name: Archive mutants.out
-        uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0
+      - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0
         if: always()
         with:
           name: mutants.out
diff --git a/.github/workflows/rustfmt.yml b/.github/workflows/rustfmt.yml
new file mode 100644
index 0000000000..0f48b029a0
--- /dev/null
+++ b/.github/workflows/rustfmt.yml
@@ -0,0 +1,32 @@
+name: Format
+on:
+  push:
+    branches: ["main"]
+    paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
+  pull_request:
+    branches: ["main"]
+    paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
+  merge_group:
+  workflow_dispatch:
+env:
+  CARGO_TERM_COLOR: always
+  RUST_BACKTRACE: 1
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref_name }}
+  cancel-in-progress: true
+
+permissions:
+  contents: read
+
+jobs:
+  format:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
+      - uses: ./.github/actions/rust
+        with:
+          version: nightly
+          components: rustfmt
+          token: ${{ secrets.GITHUB_TOKEN }}
+      - run: cargo fmt --all -- --check

From b780e5331e6e61a87b05d342128fdf4d6f7e06b4 Mon Sep 17 00:00:00 2001
From: Lars Eggert <lars@eggert.org>
Date: Wed, 18 Sep 2024 03:00:05 -0700
Subject: [PATCH 3/3] ci: More parallelization and caching (#2124)

* ci: Try and use artifacts to cache prebuilt NSS

* needs

* actions/cache

* Fix

* Fix

* no sccache

* sccache is killing our cache

* Set env

* Env

* NSS_PREBUILT

* Check if set

* Cache on self-hosted runner

* Fixes

* Fixes

* Fixes

* Run fuzz in parallel

* Invert

* fuzz-bench

* SCCACHE_INSTALLED && build -> check

* Fixes

* Fixes

* Don't update rustup

* Compile less
---
 .github/actions/nss/action.yml   | 66 ++++++++++++++++++++++++--------
 .github/actions/rust/action.yml  | 22 +++++------
 .github/workflows/bench.yml      |  4 +-
 .github/workflows/check.yml      | 19 +++++----
 .github/workflows/clippy.yml     |  2 +-
 .github/workflows/fuzz-bench.yml | 39 +++++++++++++++++++
 Cargo.toml                       |  5 +++
 neqo-crypto/build.rs             | 13 ++++---
 8 files changed, 125 insertions(+), 45 deletions(-)
 create mode 100644 .github/workflows/fuzz-bench.yml

diff --git a/.github/actions/nss/action.yml b/.github/actions/nss/action.yml
index b5989476f0..b8f7470f38 100644
--- a/.github/actions/nss/action.yml
+++ b/.github/actions/nss/action.yml
@@ -61,14 +61,19 @@ runs:
 
     - name: Use sccache
       # Apparently the action can't be installed twice in the same workflow, so check if
-      # it's already installed by checking if the RUSTC_WRAPPER environment variable is set
+      # it's already installed by checking if the SCCACHE_ENABLED environment variable is set
       # (which every "use" of this action needs to therefore set)
-      if: env.RUSTC_WRAPPER != 'sccache'
+      #
+      # Also, only enable sscache on our self-hosted runner, because the GitHub cache limit
+      # is too small for this to be effective there.
+      if: env.SCCACHE_ENABLED != '1' && env.BUILD_NSS == '1' && runner.environment != 'github-hosted'
       uses: mozilla-actions/sccache-action@2e7f9ec7921547d4b46598398ca573513895d0bd # v0.0.4
 
     - name: Enable sscache
+      if: env.BUILD_NSS == '1' && runner.environment != 'github-hosted'
       shell: bash
       run: |
+        echo "SCCACHE_ENABLED=1" >> "$GITHUB_ENV"
         if [ "${{ runner.os }}" != "Windows" ]; then
           # TODO: Figure out how to make this work on Windows
           echo "SCCACHE_CC=sccache cc" >> "$GITHUB_ENV"
@@ -76,11 +81,9 @@ runs:
         fi
         echo "CMAKE_C_COMPILER_LAUNCHER=sccache" >> "$GITHUB_ENV"
         echo "CMAKE_CXX_COMPILER_LAUNCHER=sccache" >> "$GITHUB_ENV"
-        if [ "$GITHUB_WORKFLOW" ]; then
+        if [ "${{ runner.environment }}" == "github-hosted" ]; then
           echo "SCCACHE_GHA_ENABLED=true" >> "$GITHUB_ENV"
         fi
-        echo "RUSTC_WRAPPER=sccache" >> "$GITHUB_ENV"
-        echo "CARGO_INCREMENTAL=0" >> "$GITHUB_ENV"
 
     - name: Checkout NSS
       if: env.BUILD_NSS == '1'
@@ -96,6 +99,34 @@ runs:
         repository: nss-dev/nspr
         path: nspr
 
+    - name: Get head revisions
+      if: env.BUILD_NSS == '1'
+      shell: bash
+      run: |
+        NSS_HEAD=$(git -C nss rev-parse HEAD)
+        NSPR_HEAD=$(git -C nspr rev-parse HEAD)
+        echo "NSS_HEAD=$NSS_HEAD" >> "$GITHUB_ENV"
+        echo "NSPR_HEAD=$NSPR_HEAD" >> "$GITHUB_ENV"
+
+    - name: Cache NSS
+      id: cache
+      if: env.BUILD_NSS == '1' && runner.environment == 'github-hosted'
+      uses: actions/cache@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4.0.2
+      with:
+        path: dist
+        key: nss-${{ runner.os }}-${{ inputs.type }}-${{ env.NSS_HEAD }}-${{ env.NSPR_HEAD }}
+
+    - name: Check if build is needed
+      if: env.BUILD_NSS == '1' && runner.environment == 'github-hosted'
+      shell: bash
+      run: |
+        if [ "${{ steps.cache.outputs.cache-hit }}" == "true" ]; then
+          echo "Using cached prebuilt NSS"
+          echo "BUILD_NSS=0" >> "$GITHUB_ENV"
+        else
+          echo "Building NSS from source"
+        fi
+
     - name: Install build dependencies (Linux)
       shell: bash
       if: runner.os == 'Linux' && env.BUILD_NSS == '1' && runner.environment == 'github-hosted'
@@ -143,6 +174,21 @@ runs:
         # See https://github.com/ilammy/msvc-dev-cmd#name-conflicts-with-shell-bash
         rm /usr/bin/link.exe || true
 
+    - name: Set up environment
+      shell: bash
+      run: |
+        NSS_TARGET="${{ inputs.type }}"
+        echo "NSS_TARGET=$NSS_TARGET" >> "$GITHUB_ENV"
+        NSS_OUT="$NSS_DIR/../dist/$NSS_TARGET"
+        echo "LD_LIBRARY_PATH=$NSS_OUT/lib" >> "$GITHUB_ENV"
+        echo "DYLD_FALLBACK_LIBRARY_PATH=$NSS_OUT/lib" >> "$GITHUB_ENV"
+        echo "$NSS_OUT/lib" >> "$GITHUB_PATH"
+        echo "NSS_DIR=$NSS_DIR" >> "$GITHUB_ENV"
+        echo "NSS_PREBUILT=1" >> "$GITHUB_ENV"
+      env:
+        NSS_DIR: ${{ github.workspace }}/nss
+        NSPR_DIR: ${{ github.workspace }}/nspr
+
     - name: Build
       shell: bash
       if: env.BUILD_NSS == '1'
@@ -154,15 +200,5 @@ runs:
           OPT="-o"
           [ "${{ runner.os }}" != "Windows" ] && export CFLAGS="-ggdb3 -fno-omit-frame-pointer"
         fi
-        NSS_TARGET="${{ inputs.type }}"
-        echo "NSS_TARGET=$NSS_TARGET" >> "$GITHUB_ENV"
-        NSS_OUT="$NSS_DIR/../dist/$NSS_TARGET"
-        echo "LD_LIBRARY_PATH=$NSS_OUT/lib" >> "$GITHUB_ENV"
-        echo "DYLD_FALLBACK_LIBRARY_PATH=$NSS_OUT/lib" >> "$GITHUB_ENV"
-        echo "$NSS_OUT/lib" >> "$GITHUB_PATH"
-        echo "NSS_DIR=$NSS_DIR" >> "$GITHUB_ENV"
         [ "$SCCACHE_CC" ] && [ "$SCCACHE_CXX" ] && export CC="$SCCACHE_CC" CXX="$SCCACHE_CXX"
         $NSS_DIR/build.sh -g -Ddisable_tests=1 $OPT --static
-      env:
-        NSS_DIR: ${{ github.workspace }}/nss
-        NSPR_DIR: ${{ github.workspace }}/nspr
diff --git a/.github/actions/rust/action.yml b/.github/actions/rust/action.yml
index c96ec7b269..0f47e8fb2b 100644
--- a/.github/actions/rust/action.yml
+++ b/.github/actions/rust/action.yml
@@ -21,11 +21,6 @@ inputs:
 runs:
   using: composite
   steps:
-    - name: Upgrade rustup (MacOS)
-      shell: bash
-      if: runner.os == 'MacOS'
-      run: brew update && brew upgrade rustup
-
     - name: Install Rust
       uses: dtolnay/rust-toolchain@21dc36fb71dd22e3317045c0c31a3f4249868b17 # master
       with:
@@ -35,21 +30,24 @@ runs:
 
     - name: Use sccache
       # Apparently the action can't be installed twice in the same workflow, so check if
-      # it's already installed by checking if the RUSTC_WRAPPER environment variable is set
+      # it's already installed by checking if the SCCACHE_ENABLED environment variable is set
       # (which every "use" of this action needs to therefore set)
-      if: env.RUSTC_WRAPPER != 'sccache'
+      #
+      # Also, only enable sscache on our self-hosted runner, because the GitHub cache limit
+      # is too small for this to be effective there.
+      if: env.SCCACHE_ENABLED != '1' && runner.environment != 'github-hosted'
       uses: mozilla-actions/sccache-action@2e7f9ec7921547d4b46598398ca573513895d0bd # v0.0.4
 
     - name: Enable sscache
+      if: runner.environment != 'github-hosted'
       shell: bash
       run: |
-        echo "CMAKE_C_COMPILER_LAUNCHER=sccache" >> "$GITHUB_ENV"
-        echo "CMAKE_CXX_COMPILER_LAUNCHER=sccache" >> "$GITHUB_ENV"
-        if [ "$GITHUB_WORKFLOW" ]; then
-          echo "SCCACHE_GHA_ENABLED=true" >> "$GITHUB_ENV"
-        fi
+        echo "SCCACHE_ENABLED=1" >> "$GITHUB_ENV"
         echo "RUSTC_WRAPPER=sccache" >> "$GITHUB_ENV"
         echo "CARGO_INCREMENTAL=0" >> "$GITHUB_ENV"
+        if [ "${{ runner.environment }}" == "github-hosted" ]; then
+          echo "SCCACHE_GHA_ENABLED=true" >> "$GITHUB_ENV"
+        fi
 
     - name: Set up MSVC (Windows)
       if: runner.os == 'Windows'
diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml
index d758aff65b..8b959fb2f2 100644
--- a/.github/workflows/bench.yml
+++ b/.github/workflows/bench.yml
@@ -58,8 +58,8 @@ jobs:
 
       - name: Build neqo
         run: |
-          cargo "+$TOOLCHAIN" bench --features bench --no-run
-          cargo "+$TOOLCHAIN" build --release
+          cargo "+$TOOLCHAIN" bench --workspace --features bench --no-run
+          cargo "+$TOOLCHAIN" build --release --bin neqo-client --bin neqo-server
 
       - name: Build msquic
         run: |
diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml
index 49dfb8ec80..fb7877b9ee 100644
--- a/.github/workflows/check.yml
+++ b/.github/workflows/check.yml
@@ -47,8 +47,8 @@ jobs:
       - uses: ./.github/actions/rust
         with:
           version: ${{ matrix.rust-toolchain }}
-          components: clippy, llvm-tools-preview
-          tools: cargo-llvm-cov, cargo-nextest, cargo-hack, cargo-fuzz
+          components: ${{ matrix.rust-toolchain == 'stable' && 'llvm-tools-preview' || '' }}
+          tools: ${{ matrix.rust-toolchain == 'stable' && 'cargo-llvm-cov, ' || '' }} cargo-nextest
           token: ${{ secrets.GITHUB_TOKEN }}
 
       - id: nss-version
@@ -58,20 +58,19 @@ jobs:
         with:
           minimum-version: ${{ steps.nss-version.outputs.minimum }}
 
-      - name: Build
+      - name: Check
         run: |
           # shellcheck disable=SC2086
-          cargo +${{ matrix.rust-toolchain }} build $BUILD_TYPE --all-targets --features ci
-          # Check that the fuzz targets also build
-          if [ ${{ startsWith(matrix.rust-toolchain, 'nightly') && 'nightly' }} == 'nightly' ]; then
-            cargo +${{ matrix.rust-toolchain }} fuzz check
-          fi
+          cargo +${{ matrix.rust-toolchain }} check $BUILD_TYPE --all-targets --features ci
 
       - name: Run tests and determine coverage
         run: |
           # shellcheck disable=SC2086
-          RUST_LOG=trace cargo +${{ matrix.rust-toolchain }} llvm-cov nextest $BUILD_TYPE --features ci --no-fail-fast --lcov --output-path lcov.info
-          cargo +${{ matrix.rust-toolchain }} bench --features bench --no-run
+          if [ "${{ matrix.rust-toolchain }}" == "stable" ]; then
+            RUST_LOG=trace cargo +${{ matrix.rust-toolchain }} llvm-cov nextest $BUILD_TYPE --features ci --no-fail-fast --lcov --output-path lcov.info
+          else
+            RUST_LOG=trace cargo +${{ matrix.rust-toolchain }} nextest run $BUILD_TYPE --features ci --no-fail-fast
+          fi
 
       - name: Run client/server transfer
         run: |
diff --git a/.github/workflows/clippy.yml b/.github/workflows/clippy.yml
index a1ef1ed6ba..c323f79048 100644
--- a/.github/workflows/clippy.yml
+++ b/.github/workflows/clippy.yml
@@ -27,7 +27,7 @@ jobs:
       - uses: ./.github/actions/rust
         with:
           components: clippy
-          tools: cargo-hack, cargo-fuzz
+          tools: cargo-hack
           token: ${{ secrets.GITHUB_TOKEN }}
 
       - id: nss-version
diff --git a/.github/workflows/fuzz-bench.yml b/.github/workflows/fuzz-bench.yml
new file mode 100644
index 0000000000..6ffb3d1cbb
--- /dev/null
+++ b/.github/workflows/fuzz-bench.yml
@@ -0,0 +1,39 @@
+name: Fuzz & Bench
+on:
+  workflow_dispatch:
+  pull_request:
+    branches: ["main"]
+    paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
+  merge_group:
+env:
+  CARGO_TERM_COLOR: always
+  RUST_BACKTRACE: 1
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref_name }}
+  cancel-in-progress: true
+
+permissions:
+  contents: read
+
+jobs:
+  fuzz-bench:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
+      - uses: ./.github/actions/rust
+        with:
+          version: nightly
+          tools: cargo-fuzz
+          token: ${{ secrets.GITHUB_TOKEN }}
+
+      - id: nss-version
+        run: echo "minimum=$(cat neqo-crypto/min_version.txt)" >> "$GITHUB_OUTPUT"
+
+      - uses: ./.github/actions/nss
+        with:
+          minimum-version: ${{ steps.nss-version.outputs.minimum }}
+
+      # Check that the fuzz and bench targets build
+      - run: cargo fuzz check
+      - run: cargo bench --features bench --no-run
diff --git a/Cargo.toml b/Cargo.toml
index 815470bddd..8c7f0e1f6a 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -40,6 +40,11 @@ nursery = { level = "warn", priority = -1 }
 pedantic = { level = "warn", priority = -1 }
 multiple_crate_versions = "allow"
 
+# Optimize build dependencies, because bindgen and proc macros / style
+# compilation take more to run than to build otherwise.
+[profile.dev.build-override]
+opt-level = 1
+
 [profile.release]
 lto = "fat"
 
diff --git a/neqo-crypto/build.rs b/neqo-crypto/build.rs
index 34cc842b5e..b61a8e92af 100644
--- a/neqo-crypto/build.rs
+++ b/neqo-crypto/build.rs
@@ -109,14 +109,14 @@ fn get_bash() -> PathBuf {
     )
 }
 
-fn build_nss(dir: PathBuf) {
+fn build_nss(dir: PathBuf, nsstarget: &str) {
     let mut build_nss = vec![
         String::from("./build.sh"),
         String::from("-Ddisable_tests=1"),
         // Generate static libraries in addition to shared libraries.
         String::from("--static"),
     ];
-    if !is_debug() {
+    if nsstarget == "Release" {
         build_nss.push(String::from("-o"));
     }
     if let Ok(d) = env::var("NSS_JOBS") {
@@ -317,15 +317,18 @@ fn setup_standalone(nss: &str) -> Vec<String> {
         "The NSS_DIR environment variable is expected to be an absolute path."
     );
 
-    build_nss(nss.clone());
-
     // $NSS_DIR/../dist/
     let nssdist = nss.parent().unwrap().join("dist");
     println!("cargo:rerun-if-env-changed=NSS_TARGET");
     let nsstarget = env::var("NSS_TARGET")
         .unwrap_or_else(|_| fs::read_to_string(nssdist.join("latest")).unwrap());
-    let nsstarget = nssdist.join(nsstarget.trim());
 
+    // If NSS_PREBUILT is set, we assume that the NSS libraries are already built.
+    if env::var("NSS_PREBUILT").is_err() {
+        build_nss(nss, &nsstarget);
+    }
+
+    let nsstarget = nssdist.join(nsstarget.trim());
     let includes = get_includes(&nsstarget, &nssdist);
 
     let nsslibdir = nsstarget.join("lib");