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 e2861e3400..fb7877b9ee 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,36 +42,35 @@ 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: ${{ matrix.rust-toolchain == 'stable' && 'llvm-tools-preview' || '' }} + tools: ${{ matrix.rust-toolchain == 'stable' && 'cargo-llvm-cov, ' || '' }} cargo-nextest 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 }} - - name: Build + - name: Check run: | # shellcheck disable=SC2086 - cargo +${{ matrix.rust-toolchain }} build $BUILD_TYPE --all-targets --features ci + 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: | @@ -90,41 +89,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 +100,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..c323f79048 --- /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 + 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/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/.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 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 { "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"); diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 53264df694..8dc44803ea 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, } } }