Merge branch 'bug29085_rebase'

changes/bug29085

@@ -0,0 +1,4 @@
+  o Code simplification and refactoring (circuit padding):
+    - Avoid calling monotime_absolute_usec() in circuit padding machines
+      that do not use token removal or circuit RTT estimation. Fixes bug
+      29085; bugfix on 0.4.0.1-alpha.

scripts/maint/practracker/exceptions.txt

@@ -289,3 +289,4 @@ problem function-size /src/tools/tor-gencert.c:parse_commandline() 111
 problem function-size /src/tools/tor-resolve.c:build_socks5_resolve_request() 104
 problem function-size /src/tools/tor-resolve.c:do_resolve() 175
 problem function-size /src/tools/tor-resolve.c:main() 112
+problem function-size /src/core/or/circuitpadding.c:circpad_machine_schedule_padding() 107

src/core/or/circuitpadding.c

@@ -80,6 +80,9 @@ static void circpad_setup_machine_on_circ(circuit_t *on_circ,
                                         const circpad_machine_spec_t *machine);
 static double circpad_distribution_sample(circpad_distribution_t dist);
 
+static inline void circpad_machine_update_state_length_for_nonpadding(
+        circpad_machine_runtime_t *mi);
+
 /** Cached consensus params */
 static uint8_t circpad_padding_disabled;
 static uint8_t circpad_padding_reduced;
@@ -250,7 +253,7 @@ circpad_histogram_bin_to_usec(const circpad_machine_runtime_t *mi,
 
   /* The infinity bin has an upper bound of infinity, so make sure we return
    * that if they ask for it. */
-  if (bin > CIRCPAD_INFINITY_BIN(mi)) {
+  if (bin > CIRCPAD_INFINITY_BIN(state)) {
     return CIRCPAD_DELAY_INFINITE;
   }
 
@@ -327,6 +330,39 @@ circpad_histogram_usec_to_bin(const circpad_machine_runtime_t *mi,
   return CIRCPAD_INFINITY_BIN(state)-1;
 }
 
+/**
+ * Return true if the machine supports token removal.
+ *
+ * Token removal is equivalent to having a mutable histogram in the
+ * circpad_machine_runtime_t mutable info. So while we're at it,
+ * let's assert that everything is consistent between the mutable
+ * runtime and the readonly machine spec.
+ */
+static inline int
+circpad_is_token_removal_supported(circpad_machine_runtime_t *mi)
+{
+  /* No runtime histogram == no token removal */
+  if (mi->histogram == NULL) {
+    /* Machines that don't want token removal are trying to avoid
+     * potentially expensive mallocs, extra memory accesses, and/or
+     * potentially expensive monotime calls. Let's minimize checks
+     * and keep this path fast. */
+    tor_assert_nonfatal(mi->histogram_len == 0);
+    return 0;
+  } else {
+    /* Machines that do want token removal are less sensitive to performance.
+     * Let's spend some time to check that our state is consistent and sane */
+    const circpad_state_t *state = circpad_machine_current_state(mi);
+    tor_assert_nonfatal(state->token_removal != CIRCPAD_TOKEN_REMOVAL_NONE);
+    tor_assert_nonfatal(state->histogram_len == mi->histogram_len);
+    tor_assert_nonfatal(mi->histogram_len != 0);
+    return 1;
+  }
+
+  tor_assert_nonfatal_unreached();
+  return 0;
+}
+
 /**
  * This function frees any token bins allocated from a previous state
  *
@@ -438,13 +474,7 @@ circpad_machine_sample_delay(circpad_machine_runtime_t *mi)
       mi->rtt_estimate_usec + state->dist_added_shift_usec :
       state->dist_added_shift_usec;
     return circpad_distribution_sample_iat_delay(state, iat_delay_shift);
-  } else if (state->token_removal != CIRCPAD_TOKEN_REMOVAL_NONE) {
+  } else if (circpad_is_token_removal_supported(mi)) {
-    /* We have a mutable histogram. Do basic sanity check and apply: */
-    if (BUG(!mi->histogram) ||
-        BUG(mi->histogram_len != state->histogram_len)) {
-      return CIRCPAD_DELAY_INFINITE;
-    }
-
     histogram = mi->histogram;
     for (circpad_hist_index_t b = 0; b < state->histogram_len; b++)
       histogram_total_tokens += histogram[b];
@@ -809,7 +839,7 @@ check_machine_token_supply(circpad_machine_runtime_t *mi)
    *
    * We also do not count infinity bin in histogram totals.
    */
-  if (mi->histogram_len && mi->histogram) {
+  if (circpad_is_token_removal_supported(mi)) {
     for (circpad_hist_index_t b = 0; b < CIRCPAD_INFINITY_BIN(mi); b++)
       histogram_total_tokens += mi->histogram[b];
 
@@ -828,22 +858,55 @@ check_machine_token_supply(circpad_machine_runtime_t *mi)
 }
 
 /**
- * Remove a token from the bin corresponding to the delta since
+ * Count that a padding packet was sent.
- * last packet. If that bin is empty, choose a token based on
- * the specified removal strategy in the state machine.
  *
- * This function also updates and checks rate limit and state
+ * This updates our state length count, our machine rate limit counts,
- * limit counters.
+ * and if token removal is used, decrements the histogram.
- *
- * Returns 1 if we transition states, 0 otherwise.
  */
-STATIC circpad_decision_t
+static inline void
-circpad_machine_remove_token(circpad_machine_runtime_t *mi)
+circpad_machine_count_padding_sent(circpad_machine_runtime_t *mi)
 {
-  const circpad_state_t *state = NULL;
+  /* If we have a valid state length bound, consider it */
-  circpad_time_t current_time;
+  if (mi->state_length != CIRCPAD_STATE_LENGTH_INFINITE &&
-  circpad_delay_t target_bin_usec;
+      !BUG(mi->state_length <= 0)) {
+    mi->state_length--;
+  }
 
+  /*
+   * Update non-padding counts for rate limiting: We scale at UINT16_MAX
+   * because we only use this for a percentile limit of 2 sig figs, and
+   * space is scare in the machineinfo struct.
+   */
+  mi->padding_sent++;
+  if (mi->padding_sent == UINT16_MAX) {
+    mi->padding_sent /= 2;
+    mi->nonpadding_sent /= 2;
+  }
+
+  circpad_global_padding_sent++;
+
+  /* If we have a mutable histogram, reduce the token count from
+   * the chosen padding bin (this assumes we always send padding
+   * when we intended to). */
+  if (circpad_is_token_removal_supported(mi)) {
+    /* Check array bounds and token count before removing */
+    if (!BUG(mi->chosen_bin >= mi->histogram_len) &&
+        !BUG(mi->histogram[mi->chosen_bin] == 0)) {
+      mi->histogram[mi->chosen_bin]--;
+    }
+  }
+}
+
+/**
+ * Count a nonpadding packet as being sent.
+ *
+ * This function updates our overhead accounting variables, as well
+ * as decrements the state limit packet counter, if the latter was
+ * flagged as applying to non-padding as well.
+ */
+static inline void
+circpad_machine_count_nonpadding_sent(circpad_machine_runtime_t *mi)
+{
   /* Update non-padding counts for rate limiting: We scale at UINT16_MAX
    * because we only use this for a percentile limit of 2 sig figs, and
    * space is scare in the machineinfo struct. */
@@ -853,12 +916,67 @@ circpad_machine_remove_token(circpad_machine_runtime_t *mi)
     mi->nonpadding_sent /= 2;
   }
 
+  /* Update any state packet length limits that apply */
+  circpad_machine_update_state_length_for_nonpadding(mi);
+
+  /* Remove a token from the histogram, if applicable */
+  circpad_machine_remove_token(mi);
+}
+
+/**
+ * Decrement the state length counter for a non-padding packet.
+ *
+ * Only updates the state length if we're using that feature, we
+ * have a state, and the machine wants to count non-padding packets
+ * towards the state length.
+ */
+static inline void
+circpad_machine_update_state_length_for_nonpadding(
+        circpad_machine_runtime_t *mi)
+{
+  const circpad_state_t *state = NULL;
+
+  if (mi->state_length == CIRCPAD_STATE_LENGTH_INFINITE)
+    return;
+
+  state = circpad_machine_current_state(mi);
+
+  /* If we are not in a padding state (like start or end), we're done */
+  if (!state)
+    return;
+
+  /* If we're enforcing a state length on non-padding packets,
+   * decrement it */
+  if (state->length_includes_nonpadding &&
+      mi->state_length > 0) {
+    mi->state_length--;
+  }
+}
+
+/**
+ * When a non-padding packet arrives, remove a token from the bin
+ * corresponding to the delta since last sent packet. If that bin
+ * is empty, choose a token based on the specified removal strategy
+ * in the state machine.
+ */
+STATIC void
+circpad_machine_remove_token(circpad_machine_runtime_t *mi)
+{
+  const circpad_state_t *state = NULL;
+  circpad_time_t current_time;
+  circpad_delay_t target_bin_usec;
+
   /* Dont remove any tokens if there was no padding scheduled */
   if (!mi->padding_scheduled_at_usec) {
-    return CIRCPAD_STATE_UNCHANGED;
+    return;
   }
 
   state = circpad_machine_current_state(mi);
+
+  /* Don't remove any tokens if we're not doing token removal */
+  if (!state || state->token_removal == CIRCPAD_TOKEN_REMOVAL_NONE)
+    return;
+
   current_time = monotime_absolute_usec();
 
   /* If we have scheduled padding some time in the future, we want to see what
@@ -877,20 +995,10 @@ circpad_machine_remove_token(circpad_machine_runtime_t *mi)
 
   /* If we are not in a padding state (like start or end), we're done */
   if (!state)
-    return CIRCPAD_STATE_UNCHANGED;
+    return;
-
-  /* If we're enforcing a state length on non-padding packets,
-   * decrement it */
-  if (mi->state_length != CIRCPAD_STATE_LENGTH_INFINITE &&
-      state->length_includes_nonpadding &&
-      mi->state_length > 0) {
-    mi->state_length--;
-  }
 
   /* Perform the specified token removal strategy */
   switch (state->token_removal) {
-    case CIRCPAD_TOKEN_REMOVAL_NONE:
-      break;
     case CIRCPAD_TOKEN_REMOVAL_CLOSEST_USEC:
       circpad_machine_remove_closest_token(mi, target_bin_usec, 1);
       break;
@@ -906,10 +1014,13 @@ circpad_machine_remove_token(circpad_machine_runtime_t *mi)
     case CIRCPAD_TOKEN_REMOVAL_EXACT:
       circpad_machine_remove_exact(mi, target_bin_usec);
       break;
+    case CIRCPAD_TOKEN_REMOVAL_NONE:
+    default:
+      tor_assert_nonfatal_unreached();
+      log_warn(LD_BUG, "Circpad: Unknown token removal strategy %d",
+               state->token_removal);
+      break;
   }
-
-  /* Check our token and state length limits */
-  return check_machine_token_supply(mi);
 }
 
 /**
@@ -979,34 +1090,7 @@ circpad_send_padding_cell_for_callback(circpad_machine_runtime_t *mi)
     return CIRCPAD_STATE_CHANGED;
   }
 
-  /* If it's a histogram, reduce the token count */
+  circpad_machine_count_padding_sent(mi);
-  if (mi->histogram && mi->histogram_len) {
-    /* Basic sanity check on the histogram before removing anything */
-    if (BUG(mi->chosen_bin >= mi->histogram_len) ||
-        BUG(mi->histogram[mi->chosen_bin] == 0)) {
-      return CIRCPAD_STATE_CHANGED;
-    }
-
-    mi->histogram[mi->chosen_bin]--;
-  }
-
-  /* If we have a valid state length bound, consider it */
-  if (mi->state_length != CIRCPAD_STATE_LENGTH_INFINITE &&
-      !BUG(mi->state_length <= 0)) {
-    mi->state_length--;
-  }
-
-  /*
-   * Update non-padding counts for rate limiting: We scale at UINT16_MAX
-   * because we only use this for a percentile limit of 2 sig figs, and
-   * space is scare in the machineinfo struct.
-   */
-  mi->padding_sent++;
-  if (mi->padding_sent == UINT16_MAX) {
-    mi->padding_sent /= 2;
-    mi->nonpadding_sent /= 2;
-  }
-  circpad_global_padding_sent++;
 
   if (CIRCUIT_IS_ORIGIN(mi->on_circ)) {
     circpad_send_command_to_hop(TO_ORIGIN_CIRCUIT(mi->on_circ),
@@ -1240,7 +1324,17 @@ circpad_machine_schedule_padding,(circpad_machine_runtime_t *mi))
 
   /* in_usec = in microseconds */
   in_usec = circpad_machine_sample_delay(mi);
+  /* If we're using token removal, we need to know when the padding
+   * was scheduled at, so we can remove the appropriate token if
+   * a non-padding cell is sent before the padding timer expires.
+   *
+   * However, since monotime is unpredictably expensive, let's avoid
+   * using it for machines that don't need token removal. */
+  if (circpad_is_token_removal_supported(mi)) {
     mi->padding_scheduled_at_usec = monotime_absolute_usec();
+  } else {
+    mi->padding_scheduled_at_usec = 1;
+  }
   log_fn(LOG_INFO,LD_CIRC,"\tPadding in %u usec", in_usec);
 
   // Don't schedule if we have infinite delay.
@@ -1454,9 +1548,25 @@ circpad_estimate_circ_rtt_on_received(circuit_t *circ,
            circ->n_chan ?  circ->n_chan->global_identifier : 0,
            circ->n_circ_id, mi->rtt_estimate_usec);
       mi->stop_rtt_update = 1;
+
+      if (!mi->rtt_estimate_usec) {
+        static ratelim_t rtt_lim = RATELIM_INIT(600);
+        log_fn_ratelim(&rtt_lim,LOG_NOTICE,LD_BUG,
+          "Circuit got two cells back to back before estimating RTT.");
+      }
     }
   } else {
+    const circpad_state_t *state = circpad_machine_current_state(mi);
+
+    /* Since monotime is unpredictably expensive, only update this field
+     * if rtt estimates are needed. Otherwise, stop the rtt update. */
+    if (state->use_rtt_estimate) {
       mi->last_received_time_usec = monotime_absolute_usec();
+    } else {
+      /* Let's fast-path future decisions not to update rtt if the
+       * feature is not in use. */
+      mi->stop_rtt_update = 1;
+    }
   }
 }
 
@@ -1516,8 +1626,9 @@ circpad_estimate_circ_rtt_on_send(circuit_t *circ,
     mi->stop_rtt_update = 1;
 
     if (!mi->rtt_estimate_usec) {
-      log_fn(LOG_NOTICE, LD_CIRC,
+      static ratelim_t rtt_lim = RATELIM_INIT(600);
-             "Got two cells back to back on a circuit before estimating RTT.");
+      log_fn_ratelim(&rtt_lim,LOG_NOTICE,LD_BUG,
+        "Circuit sent two cells back to back before estimating RTT.");
     }
   }
 }
@@ -1541,9 +1652,13 @@ circpad_cell_event_nonpadding_sent(circuit_t *on_circ)
     /* First, update any RTT estimate */
     circpad_estimate_circ_rtt_on_send(on_circ, on_circ->padding_info[i]);
 
-    /* Remove a token: this is the idea of adaptive padding, since we have an
+    /* Then, do accounting */
-     * ideal distribution that we want our distribution to look like. */
+    circpad_machine_count_nonpadding_sent(on_circ->padding_info[i]);
-    if (!circpad_machine_remove_token(on_circ->padding_info[i])) {
+
+    /* Check to see if we've run out of tokens for this state already,
+     * and if not, check for other state transitions */
+    if (check_machine_token_supply(on_circ->padding_info[i])
+        == CIRCPAD_STATE_UNCHANGED) {
       /* If removing a token did not cause a transition, check if
        * non-padding sent event should */
       circpad_machine_spec_transition(on_circ->padding_info[i],
@@ -1584,8 +1699,16 @@ void
 circpad_cell_event_padding_sent(circuit_t *on_circ)
 {
   FOR_EACH_ACTIVE_CIRCUIT_MACHINE_BEGIN(i, on_circ) {
+    /* Check to see if we've run out of tokens for this state already,
+     * and if not, check for other state transitions */
+    if (check_machine_token_supply(on_circ->padding_info[i])
+        == CIRCPAD_STATE_UNCHANGED) {
+      /* If removing a token did not cause a transition, check if
+       * non-padding sent event should */
+
       circpad_machine_spec_transition(on_circ->padding_info[i],
                              CIRCPAD_EVENT_PADDING_SENT);
+    }
   } FOR_EACH_ACTIVE_CIRCUIT_MACHINE_END;
 }
 

src/core/or/circuitpadding.h

@@ -712,9 +712,6 @@ circpad_machine_sample_delay(circpad_machine_runtime_t *mi);
 STATIC bool
 circpad_machine_reached_padding_limit(circpad_machine_runtime_t *mi);
 
-STATIC
-circpad_decision_t circpad_machine_remove_token(circpad_machine_runtime_t *mi);
-
 STATIC circpad_delay_t
 circpad_histogram_bin_to_usec(const circpad_machine_runtime_t *mi,
                               circpad_hist_index_t bin);
@@ -722,6 +719,8 @@ circpad_histogram_bin_to_usec(const circpad_machine_runtime_t *mi,
 STATIC const circpad_state_t *
 circpad_machine_current_state(const circpad_machine_runtime_t *mi);
 
+STATIC void circpad_machine_remove_token(circpad_machine_runtime_t *mi);
+
 STATIC circpad_hist_index_t circpad_histogram_usec_to_bin(
                                        const circpad_machine_runtime_t *mi,
                                        circpad_delay_t us);

src/test/test_circuitpadding.c

@@ -55,6 +55,7 @@ void test_circuitpadding_conditions(void *arg);
 void test_circuitpadding_serialize(void *arg);
 void test_circuitpadding_rtt(void *arg);
 void test_circuitpadding_tokens(void *arg);
+void test_circuitpadding_state_length(void *arg);
 
 static void
 simulate_single_hop_extend(circuit_t *client, circuit_t *mid_relay,
@@ -329,12 +330,12 @@ test_circuitpadding_rtt(void *arg)
   relay_side->padding_info[0] = circpad_circuit_machineinfo_new(client_side,0);
 
   /* Test 1: Test measuring RTT */
-  circpad_cell_event_nonpadding_received((circuit_t*)relay_side);
+  circpad_cell_event_nonpadding_received(relay_side);
   tt_u64_op(relay_side->padding_info[0]->last_received_time_usec, OP_NE, 0);
 
   timers_advance_and_run(20);
 
-  circpad_cell_event_nonpadding_sent((circuit_t*)relay_side);
+  circpad_cell_event_nonpadding_sent(relay_side);
   tt_u64_op(relay_side->padding_info[0]->last_received_time_usec, OP_EQ, 0);
 
   tt_int_op(relay_side->padding_info[0]->rtt_estimate_usec, OP_GE, 19000);
@@ -345,12 +346,12 @@ test_circuitpadding_rtt(void *arg)
             circpad_machine_current_state(
              relay_side->padding_info[0])->histogram_edges[0]);
 
-  circpad_cell_event_nonpadding_received((circuit_t*)relay_side);
+  circpad_cell_event_nonpadding_received(relay_side);
-  circpad_cell_event_nonpadding_received((circuit_t*)relay_side);
+  circpad_cell_event_nonpadding_received(relay_side);
   tt_u64_op(relay_side->padding_info[0]->last_received_time_usec, OP_NE, 0);
   timers_advance_and_run(20);
-  circpad_cell_event_nonpadding_sent((circuit_t*)relay_side);
+  circpad_cell_event_nonpadding_sent(relay_side);
-  circpad_cell_event_nonpadding_sent((circuit_t*)relay_side);
+  circpad_cell_event_nonpadding_sent(relay_side);
   tt_u64_op(relay_side->padding_info[0]->last_received_time_usec, OP_EQ, 0);
 
   tt_int_op(relay_side->padding_info[0]->rtt_estimate_usec, OP_GE, 20000);
@@ -365,9 +366,9 @@ test_circuitpadding_rtt(void *arg)
   tt_int_op(relay_side->padding_info[0]->stop_rtt_update, OP_EQ, 1);
   rtt_estimate = relay_side->padding_info[0]->rtt_estimate_usec;
 
-  circpad_cell_event_nonpadding_received((circuit_t*)relay_side);
+  circpad_cell_event_nonpadding_received(relay_side);
   timers_advance_and_run(4);
-  circpad_cell_event_nonpadding_sent((circuit_t*)relay_side);
+  circpad_cell_event_nonpadding_sent(relay_side);
 
   tt_int_op(relay_side->padding_info[0]->rtt_estimate_usec, OP_EQ,
             rtt_estimate);
@@ -380,11 +381,11 @@ test_circuitpadding_rtt(void *arg)
              relay_side->padding_info[0])->histogram_edges[0]);
 
   /* Test 3: Make sure client side machine properly ignores RTT */
-  circpad_cell_event_nonpadding_received((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_received(client_side);
   tt_u64_op(client_side->padding_info[0]->last_received_time_usec, OP_EQ, 0);
 
   timers_advance_and_run(20);
-  circpad_cell_event_nonpadding_sent((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_sent(client_side);
   tt_u64_op(client_side->padding_info[0]->last_received_time_usec, OP_EQ, 0);
 
   tt_int_op(client_side->padding_info[0]->rtt_estimate_usec, OP_EQ, 0);
@@ -415,6 +416,7 @@ helper_create_basic_machine(void)
 
   circ_client_machine.states[CIRCPAD_STATE_START].
       next_state[CIRCPAD_EVENT_NONPADDING_RECV] = CIRCPAD_STATE_BURST;
+  circ_client_machine.states[CIRCPAD_STATE_START].use_rtt_estimate = 1;
 
   circ_client_machine.states[CIRCPAD_STATE_BURST].
       next_state[CIRCPAD_EVENT_PADDING_RECV] = CIRCPAD_STATE_BURST;
@@ -526,7 +528,7 @@ test_circuitpadding_token_removal_higher(void *arg)
   MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock);
 
   /* Setup test environment (time etc.) */
-  client_side = (circuit_t *)origin_circuit_new();
+  client_side = TO_CIRCUIT(origin_circuit_new());
   client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL;
   monotime_enable_test_mocking();
 
@@ -537,7 +539,7 @@ test_circuitpadding_token_removal_higher(void *arg)
     circpad_circuit_machineinfo_new(client_side, 0);
 
   /* move the machine to the right state */
-  circpad_cell_event_nonpadding_received((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_received(client_side);
   tt_int_op(client_side->padding_info[0]->current_state, OP_EQ,
             CIRCPAD_STATE_BURST);
 
@@ -586,12 +588,12 @@ test_circuitpadding_token_removal_higher(void *arg)
     tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2);
 
     mi->padding_scheduled_at_usec = current_time - 57;
-    circpad_machine_remove_token(mi);
+    circpad_cell_event_nonpadding_sent(client_side);
 
     tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1);
 
     mi->padding_scheduled_at_usec = current_time - 57;
-    circpad_machine_remove_token(mi);
+    circpad_cell_event_nonpadding_sent(client_side);
 
     /* Test that we cleaned out this bin. Don't do this in the case of the last
        bin since the tokens will get refilled */
@@ -610,7 +612,7 @@ test_circuitpadding_token_removal_higher(void *arg)
             CIRCPAD_STATE_BURST);
   circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[0] = 100;
   mi->padding_scheduled_at_usec = current_time;
-  circpad_machine_remove_token(mi);
+  circpad_cell_event_nonpadding_sent(client_side);
   tt_int_op(mi->histogram[0], OP_EQ, 1);
 
  done:
@@ -631,7 +633,7 @@ test_circuitpadding_token_removal_lower(void *arg)
   MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock);
 
   /* Setup test environment (time etc.) */
-  client_side = (circuit_t *)origin_circuit_new();
+  client_side = TO_CIRCUIT(origin_circuit_new());
   client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL;
   monotime_enable_test_mocking();
 
@@ -642,7 +644,7 @@ test_circuitpadding_token_removal_lower(void *arg)
     circpad_circuit_machineinfo_new(client_side, 0);
 
   /* move the machine to the right state */
-  circpad_cell_event_nonpadding_received((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_received(client_side);
   tt_int_op(client_side->padding_info[0]->current_state, OP_EQ,
             CIRCPAD_STATE_BURST);
 
@@ -683,12 +685,12 @@ test_circuitpadding_token_removal_lower(void *arg)
     tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2);
 
     mi->padding_scheduled_at_usec = current_time - 57;
-    circpad_machine_remove_token(mi);
+    circpad_cell_event_nonpadding_sent(client_side);
 
     tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1);
 
     mi->padding_scheduled_at_usec = current_time - 57;
-    circpad_machine_remove_token(mi);
+    circpad_cell_event_nonpadding_sent(client_side);
 
     /* Test that we cleaned out this bin. Don't do this in the case of the last
        bin since the tokens will get refilled */
@@ -708,7 +710,7 @@ test_circuitpadding_token_removal_lower(void *arg)
   circ_client_machine.states[CIRCPAD_STATE_BURST].
     histogram_edges[BIG_HISTOGRAM_LEN-2] = 100;
   mi->padding_scheduled_at_usec = current_time - 29202;
-  circpad_machine_remove_token(mi);
+  circpad_cell_event_nonpadding_sent(client_side);
   tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1);
 
  done:
@@ -729,7 +731,7 @@ test_circuitpadding_closest_token_removal(void *arg)
   MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock);
 
   /* Setup test environment (time etc.) */
-  client_side = (circuit_t *)origin_circuit_new();
+  client_side = TO_CIRCUIT(origin_circuit_new());
   client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL;
   monotime_enable_test_mocking();
 
@@ -740,7 +742,7 @@ test_circuitpadding_closest_token_removal(void *arg)
     circpad_circuit_machineinfo_new(client_side, 0);
 
   /* move the machine to the right state */
-  circpad_cell_event_nonpadding_received((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_received(client_side);
   tt_int_op(client_side->padding_info[0]->current_state, OP_EQ,
             CIRCPAD_STATE_BURST);
 
@@ -780,12 +782,12 @@ test_circuitpadding_closest_token_removal(void *arg)
     tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2);
 
     mi->padding_scheduled_at_usec = current_time - 57;
-    circpad_machine_remove_token(mi);
+    circpad_cell_event_nonpadding_sent(client_side);
 
     tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1);
 
     mi->padding_scheduled_at_usec = current_time - 57;
-    circpad_machine_remove_token(mi);
+    circpad_cell_event_nonpadding_sent(client_side);
 
     /* Test that we cleaned out this bin. Don't do this in the case of the last
        bin since the tokens will get refilled */
@@ -807,14 +809,14 @@ test_circuitpadding_closest_token_removal(void *arg)
   circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[2] = 120;
   mi->padding_scheduled_at_usec = current_time - 102;
   mi->histogram[0] = 0;
-  circpad_machine_remove_token(mi);
+  circpad_cell_event_nonpadding_sent(client_side);
   tt_int_op(mi->histogram[1], OP_EQ, 1);
 
   /* Test above the highest bin, for coverage */
   tt_int_op(client_side->padding_info[0]->current_state, OP_EQ,
             CIRCPAD_STATE_BURST);
   mi->padding_scheduled_at_usec = current_time - 29202;
-  circpad_machine_remove_token(mi);
+  circpad_cell_event_nonpadding_sent(client_side);
   tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1);
 
  done:
@@ -835,7 +837,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg)
   MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock);
 
   /* Setup test environment (time etc.) */
-  client_side = (circuit_t *)origin_circuit_new();
+  client_side = TO_CIRCUIT(origin_circuit_new());
   client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL;
   monotime_enable_test_mocking();
 
@@ -846,7 +848,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg)
     circpad_circuit_machineinfo_new(client_side, 0);
 
   /* move the machine to the right state */
-  circpad_cell_event_nonpadding_received((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_received(client_side);
   tt_int_op(client_side->padding_info[0]->current_state, OP_EQ,
             CIRCPAD_STATE_BURST);
 
@@ -889,12 +891,12 @@ test_circuitpadding_closest_token_removal_usec(void *arg)
     tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2);
 
     mi->padding_scheduled_at_usec = current_time - 57;
-    circpad_machine_remove_token(mi);
+    circpad_cell_event_nonpadding_sent(client_side);
 
     tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1);
 
     mi->padding_scheduled_at_usec = current_time - 57;
-    circpad_machine_remove_token(mi);
+    circpad_cell_event_nonpadding_sent(client_side);
 
     /* Test that we cleaned out this bin. Don't do this in the case of the last
        bin since the tokens will get refilled */
@@ -916,7 +918,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg)
   circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[2] = 120;
   mi->padding_scheduled_at_usec = current_time - 102;
   mi->histogram[0] = 0;
-  circpad_machine_remove_token(mi);
+  circpad_cell_event_nonpadding_sent(client_side);
   tt_int_op(mi->histogram[1], OP_EQ, 1);
 
   /* Test above the highest bin, for coverage */
@@ -925,7 +927,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg)
   circ_client_machine.states[CIRCPAD_STATE_BURST].
     histogram_edges[BIG_HISTOGRAM_LEN-2] = 100;
   mi->padding_scheduled_at_usec = current_time - 29202;
-  circpad_machine_remove_token(mi);
+  circpad_cell_event_nonpadding_sent(client_side);
   tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1);
 
  done:
@@ -946,7 +948,7 @@ test_circuitpadding_token_removal_exact(void *arg)
   MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock);
 
   /* Setup test environment (time etc.) */
-  client_side = (circuit_t *)origin_circuit_new();
+  client_side = TO_CIRCUIT(origin_circuit_new());
   client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL;
   monotime_enable_test_mocking();
 
@@ -957,7 +959,7 @@ test_circuitpadding_token_removal_exact(void *arg)
     circpad_circuit_machineinfo_new(client_side, 0);
 
   /* move the machine to the right state */
-  circpad_cell_event_nonpadding_received((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_received(client_side);
   tt_int_op(client_side->padding_info[0]->current_state, OP_EQ,
             CIRCPAD_STATE_BURST);
 
@@ -971,16 +973,16 @@ test_circuitpadding_token_removal_exact(void *arg)
   /* Ensure that we will clear out bin #4 with this usec */
   mi->padding_scheduled_at_usec = current_time - 57;
   tt_int_op(mi->histogram[4], OP_EQ, 2);
-  circpad_machine_remove_token(mi);
+  circpad_cell_event_nonpadding_sent(client_side);
   mi->padding_scheduled_at_usec = current_time - 57;
   tt_int_op(mi->histogram[4], OP_EQ, 1);
-  circpad_machine_remove_token(mi);
+  circpad_cell_event_nonpadding_sent(client_side);
   tt_int_op(mi->histogram[4], OP_EQ, 0);
 
   /* Ensure that we will not remove any other tokens even tho we try to, since
    * this is what the exact strategy dictates */
   mi->padding_scheduled_at_usec = current_time - 57;
-  circpad_machine_remove_token(mi);
+  circpad_cell_event_nonpadding_sent(client_side);
   for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) {
     if (i != 4) {
       tt_int_op(mi->histogram[i], OP_EQ, 2);
@@ -1046,8 +1048,8 @@ test_circuitpadding_tokens(void *arg)
   mi = client_side->padding_info[0];
 
   // Pretend a non-padding cell was sent
-  circpad_cell_event_nonpadding_received((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_received(client_side);
-  circpad_cell_event_nonpadding_sent((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_sent(client_side);
   /* We have to save the infinity bin because one inf delay
    * could have been chosen when we transition to burst */
   circpad_hist_token_t inf_bin = mi->histogram[4];
@@ -1156,11 +1158,11 @@ test_circuitpadding_tokens(void *arg)
   /* Drain the infinity bin and cause a refill */
   while (inf_bin != 0) {
     tt_int_op(mi->histogram[4], OP_EQ, inf_bin);
-    circpad_cell_event_nonpadding_received((circuit_t*)client_side);
+    circpad_cell_event_nonpadding_received(client_side);
     inf_bin--;
   }
 
-  circpad_cell_event_nonpadding_sent((circuit_t*)client_side);
+  circpad_cell_event_nonpadding_sent(client_side);
 
   // We should have refilled here.
   tt_int_op(mi->histogram[4], OP_EQ, 2);
@@ -1284,10 +1286,10 @@ test_circuitpadding_wronghop(void *arg)
    * padding that gets sent by scheduled timers. */
   MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock);
 
-  client_side = (circuit_t *)origin_circuit_new();
+  client_side = TO_CIRCUIT(origin_circuit_new());
   dummy_channel.cmux = circuitmux_alloc();
-  relay_side = (circuit_t *)new_fake_orcirc(&dummy_channel,
+  relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel,
-                                            &dummy_channel);
+                                            &dummy_channel));
   orig_client = TO_ORIGIN_CIRCUIT(client_side);
 
   relay_side->purpose = CIRCUIT_PURPOSE_OR;
@@ -1405,9 +1407,9 @@ test_circuitpadding_wronghop(void *arg)
   free_fake_origin_circuit(TO_ORIGIN_CIRCUIT(client_side));
   free_fake_orcirc(relay_side);
 
-  client_side = (circuit_t *)origin_circuit_new();
+  client_side = TO_CIRCUIT(origin_circuit_new());
-  relay_side = (circuit_t *)new_fake_orcirc(&dummy_channel,
+  relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel,
-                                            &dummy_channel);
+                                            &dummy_channel));
   relay_side->purpose = CIRCUIT_PURPOSE_OR;
   client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL;
 
@@ -1601,10 +1603,10 @@ simulate_single_hop_extend(circuit_t *client, circuit_t *mid_relay,
   tor_addr_t addr;
 
   // Pretend a non-padding cell was sent
-  circpad_cell_event_nonpadding_sent((circuit_t*)client);
+  circpad_cell_event_nonpadding_sent(client);
 
   // Receive extend cell at middle
-  circpad_cell_event_nonpadding_received((circuit_t*)mid_relay);
+  circpad_cell_event_nonpadding_received(mid_relay);
 
   // Advance time a tiny bit so we can calculate an RTT
   curr_mocked_time += 10 * TOR_NSEC_PER_MSEC;
@@ -1612,10 +1614,10 @@ simulate_single_hop_extend(circuit_t *client, circuit_t *mid_relay,
   monotime_set_mock_time_nsec(curr_mocked_time);
 
   // Receive extended cell at middle
-  circpad_cell_event_nonpadding_sent((circuit_t*)mid_relay);
+  circpad_cell_event_nonpadding_sent(mid_relay);
 
   // Receive extended cell at first hop
-  circpad_cell_event_nonpadding_received((circuit_t*)client);
+  circpad_cell_event_nonpadding_received(client);
 
   // Add a hop to cpath
   crypt_path_t *hop = tor_malloc_zero(sizeof(crypt_path_t));
@@ -1642,6 +1644,55 @@ simulate_single_hop_extend(circuit_t *client, circuit_t *mid_relay,
   circpad_machine_event_circ_added_hop(TO_ORIGIN_CIRCUIT(client));
 }
 
+static circpad_machine_spec_t *
+helper_create_length_machine(void)
+{
+  circpad_machine_spec_t *ret =
+    tor_malloc_zero(sizeof(circpad_machine_spec_t));
+
+  /* Start, burst */
+  circpad_machine_states_init(ret, 2);
+
+  ret->states[CIRCPAD_STATE_START].
+      next_state[CIRCPAD_EVENT_PADDING_SENT] = CIRCPAD_STATE_BURST;
+
+  ret->states[CIRCPAD_STATE_BURST].
+      next_state[CIRCPAD_EVENT_PADDING_SENT] = CIRCPAD_STATE_BURST;
+
+  ret->states[CIRCPAD_STATE_BURST].
+      next_state[CIRCPAD_EVENT_LENGTH_COUNT] = CIRCPAD_STATE_END;
+
+  ret->states[CIRCPAD_STATE_BURST].
+      next_state[CIRCPAD_EVENT_BINS_EMPTY] = CIRCPAD_STATE_END;
+
+  /* No token removal.. end via state_length only */
+  ret->states[CIRCPAD_STATE_BURST].token_removal =
+      CIRCPAD_TOKEN_REMOVAL_NONE;
+
+  /* Let's have this one end after 12 packets */
+  ret->states[CIRCPAD_STATE_BURST].length_dist.type = CIRCPAD_DIST_UNIFORM;
+  ret->states[CIRCPAD_STATE_BURST].length_dist.param1 = 12;
+  ret->states[CIRCPAD_STATE_BURST].length_dist.param2 = 13;
+  ret->states[CIRCPAD_STATE_BURST].max_length = 12;
+
+  ret->states[CIRCPAD_STATE_BURST].histogram_len = 4;
+
+  ret->states[CIRCPAD_STATE_BURST].histogram_edges[0] = 0;
+  ret->states[CIRCPAD_STATE_BURST].histogram_edges[1] = 1;
+  ret->states[CIRCPAD_STATE_BURST].histogram_edges[2] = 1000000;
+  ret->states[CIRCPAD_STATE_BURST].histogram_edges[3] = 10000000;
+
+  ret->states[CIRCPAD_STATE_BURST].histogram[0] = 0;
+  ret->states[CIRCPAD_STATE_BURST].histogram[1] = 0;
+  ret->states[CIRCPAD_STATE_BURST].histogram[2] = 6;
+
+  ret->states[CIRCPAD_STATE_BURST].histogram_total_tokens = 6;
+  ret->states[CIRCPAD_STATE_BURST].use_rtt_estimate = 0;
+  ret->states[CIRCPAD_STATE_BURST].length_includes_nonpadding = 0;
+
+  return ret;
+}
+
 static circpad_machine_spec_t *
 helper_create_conditional_machine(void)
 {
@@ -1737,6 +1788,135 @@ helper_create_conditional_machines(void)
   register_padding_machine(add, relay_padding_machines);
 }
 
+void
+test_circuitpadding_state_length(void *arg)
+{
+  /**
+   * Test plan:
+   *  * Explicitly test that with no token removal enabled, we hit
+   *    the state length limit due to either padding, or non-padding.
+   *  * Repeat test with an arbitrary token removal strategy, and
+   *    verify that if we run out of tokens due to padding before we
+   *    hit the state length, we still go to state end (all our
+   *    token removal tests only test nonpadding token removal).
+   */
+  int64_t actual_mocked_monotime_start;
+  (void)arg;
+  MOCK(circuitmux_attach_circuit, circuitmux_attach_circuit_mock);
+  MOCK(circpad_send_command_to_hop, circpad_send_command_to_hop_mock);
+
+  nodes_init();
+  dummy_channel.cmux = circuitmux_alloc();
+  relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel,
+                                            &dummy_channel));
+  client_side = TO_CIRCUIT(origin_circuit_new());
+  relay_side->purpose = CIRCUIT_PURPOSE_OR;
+  client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL;
+
+  monotime_init();
+  monotime_enable_test_mocking();
+  actual_mocked_monotime_start = MONOTIME_MOCK_START;
+  monotime_set_mock_time_nsec(actual_mocked_monotime_start);
+  monotime_coarse_set_mock_time_nsec(actual_mocked_monotime_start);
+  curr_mocked_time = actual_mocked_monotime_start;
+
+  /* This is needed so that we are not considered to be dormant */
+  note_user_activity(20);
+
+  timers_initialize();
+  circpad_machine_spec_t *client_machine =
+      helper_create_length_machine();
+
+  MOCK(circuit_package_relay_cell,
+       circuit_package_relay_cell_mock);
+  MOCK(node_get_by_id,
+       node_get_by_id_mock);
+
+  client_side->padding_machine[0] = client_machine;
+  client_side->padding_info[0] =
+    circpad_circuit_machineinfo_new(client_side, 0);
+  circpad_machine_runtime_t *mi = client_side->padding_info[0];
+
+  circpad_cell_event_padding_sent(client_side);
+  tt_int_op(mi->state_length, OP_EQ, 12);
+  tt_ptr_op(mi->histogram, OP_EQ, NULL);
+
+  /* Verify that non-padding does not change our state length */
+  circpad_cell_event_nonpadding_sent(client_side);
+  tt_int_op(mi->state_length, OP_EQ, 12);
+
+  /* verify that sending padding changes our state length */
+  for (uint64_t i = mi->state_length-1; i > 0; i--) {
+    circpad_send_padding_cell_for_callback(mi);
+    tt_int_op(mi->state_length, OP_EQ, i);
+  }
+  circpad_send_padding_cell_for_callback(mi);
+
+  tt_int_op(mi->state_length, OP_EQ, -1);
+  tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_END);
+
+  /* Restart machine */
+  mi->current_state = CIRCPAD_STATE_START;
+
+  /* Now, count nonpadding as part of the state length */
+  client_machine->states[CIRCPAD_STATE_BURST].length_includes_nonpadding = 1;
+
+  circpad_cell_event_padding_sent(client_side);
+  tt_int_op(mi->state_length, OP_EQ, 12);
+
+  /* Verify that non-padding does change our state length now */
+  for (uint64_t i = mi->state_length-1; i > 0; i--) {
+    circpad_cell_event_nonpadding_sent(client_side);
+    tt_int_op(mi->state_length, OP_EQ, i);
+  }
+
+  circpad_cell_event_nonpadding_sent(client_side);
+  tt_int_op(mi->state_length, OP_EQ, -1);
+  tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_END);
+
+  /* Now, just test token removal when we send padding */
+  client_machine->states[CIRCPAD_STATE_BURST].token_removal =
+      CIRCPAD_TOKEN_REMOVAL_EXACT;
+
+  /* Restart machine */
+  mi->current_state = CIRCPAD_STATE_START;
+  circpad_cell_event_padding_sent(client_side);
+  tt_int_op(mi->state_length, OP_EQ, 12);
+  tt_ptr_op(mi->histogram, OP_NE, NULL);
+  tt_int_op(mi->chosen_bin, OP_EQ, 2);
+
+  /* verify that sending padding changes our state length and
+   * our histogram now */
+  for (uint32_t i = mi->histogram[2]-1; i > 0; i--) {
+    circpad_send_padding_cell_for_callback(mi);
+    tt_int_op(mi->chosen_bin, OP_EQ, 2);
+    tt_int_op(mi->histogram[2], OP_EQ, i);
+  }
+
+  tt_int_op(mi->state_length, OP_EQ, 7);
+  tt_int_op(mi->histogram[2], OP_EQ, 1);
+
+  circpad_send_padding_cell_for_callback(mi);
+  tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_END);
+
+ done:
+  tor_free(client_machine->states);
+  tor_free(client_machine);
+
+  free_fake_origin_circuit(TO_ORIGIN_CIRCUIT(client_side));
+  free_fake_orcirc(relay_side);
+
+  circuitmux_detach_all_circuits(dummy_channel.cmux, NULL);
+  circuitmux_free(dummy_channel.cmux);
+  timers_shutdown();
+  monotime_disable_test_mocking();
+  UNMOCK(circuit_package_relay_cell);
+  UNMOCK(circuitmux_attach_circuit);
+  UNMOCK(node_get_by_id);
+
+  return;
+}
+
 void
 test_circuitpadding_conditions(void *arg)
 {
@@ -1761,9 +1941,9 @@ test_circuitpadding_conditions(void *arg)
 
   nodes_init();
   dummy_channel.cmux = circuitmux_alloc();
-  relay_side = (circuit_t *)new_fake_orcirc(&dummy_channel,
+  relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel,
-                                            &dummy_channel);
+                                            &dummy_channel));
-  client_side = (circuit_t *)origin_circuit_new();
+  client_side = TO_CIRCUIT(origin_circuit_new());
   relay_side->purpose = CIRCUIT_PURPOSE_OR;
   client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL;
 
@@ -2217,7 +2397,7 @@ test_circuitpadding_sample_distribution(void *arg)
     }
 
     /* send a non-padding cell to move to the next machine state */
-    circpad_cell_event_nonpadding_received((circuit_t*)client_side);
+    circpad_cell_event_nonpadding_received(client_side);
   }
 
  done:
@@ -2329,12 +2509,12 @@ test_circuitpadding_global_rate_limiting(void *arg)
   curr_mocked_time = actual_mocked_monotime_start;
   timers_initialize();
 
-  client_side = (circuit_t *)origin_circuit_new();
+  client_side = TO_CIRCUIT(origin_circuit_new());
   client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL;
   dummy_channel.cmux = circuitmux_alloc();
 
   /* Setup machine and circuits */
-  relay_side = (circuit_t *)new_fake_orcirc(&dummy_channel, &dummy_channel);
+  relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel, &dummy_channel));
   relay_side->purpose = CIRCUIT_PURPOSE_OR;
   helper_create_basic_machine();
   relay_side->padding_machine[0] = &circ_client_machine;
@@ -2555,6 +2735,7 @@ test_circuitpadding_reduce_disable(void *arg)
 
 struct testcase_t circuitpadding_tests[] = {
   TEST_CIRCUITPADDING(circuitpadding_tokens, TT_FORK),
+  TEST_CIRCUITPADDING(circuitpadding_state_length, TT_FORK),
   TEST_CIRCUITPADDING(circuitpadding_negotiation, TT_FORK),
   TEST_CIRCUITPADDING(circuitpadding_wronghop, TT_FORK),
   /** Disabled unstable test until #29298 is implemented (see #29122) */