Cleanup on time-relaqted constants. New conventions:

  1) Surround all constants by (parens), whether we'll be using them
     in a denominator or not.
  2) Express all time periods as products (24*60*60), not as multiplied-out
     constants (86400).
  3) Comments like "(60*60) /* one hour */" are as pointless as comments
     like "c = a + b; /* set c to the sum of a and b */".  Remove them.
  4) All time periods should be #defined constants, not given inline.
  5) All time periods should have doxygen comments.
  6) All time periods, unless specified, are in seconds.  It's not necessary
     to say so.

To summarize, the old (lack of) style would allow:

  #define FOO_RETRY_INTERVAL 60*60 /* one hour (seconds) */
  next_try = now + 3600;

The new style is:

  /** How often do we reattempt foo? */
  #define FOO_RETRY_INTERVAL (60*60)

  next_try = now + RETRY_INTERVAL;


svn:r6142

src/or/circuituse.c

@@ -333,7 +333,7 @@ circuit_stream_is_being_handled(connection_t *conn, uint16_t port, int min)
   return 0;
 }
 
-/** Don't keep more than 10 unused open circuits around. */
+/** Don't keep more than this many unused open circuits around. */
 #define MAX_UNUSED_OPEN_CIRCUITS 12
 
 /** Figure out how many circuits we have open that are clean. Make
@@ -547,6 +547,9 @@ circuit_about_to_close_connection(connection_t *conn)
   } /* end switch */
 }
 
+/** How old do we let an unused circuit get before expiring it? */
+#define CIRCUIT_UNUSED_CIRC_TIMEOUT (60*60)
+
 /** Find each circuit that has been dirty for too long, and has
  * no streams on it: mark it for close.
  */
@@ -576,7 +579,6 @@ circuit_expire_old_circuits(void)
     } else if (!circ->timestamp_dirty && CIRCUIT_IS_ORIGIN(circ) &&
                circ->state == CIRCUIT_STATE_OPEN &&
                circ->purpose == CIRCUIT_PURPOSE_C_GENERAL) {
-#define CIRCUIT_UNUSED_CIRC_TIMEOUT 3600 /* an hour */
       if (circ->timestamp_created + CIRCUIT_UNUSED_CIRC_TIMEOUT < now) {
         log_debug(LD_CIRC,
                   "Closing circuit that has been unused for %d seconds.",

src/or/config.c

@@ -1952,6 +1952,24 @@ fascist_firewall_allows_address_dir(uint32_t addr, uint16_t port)
                                           reachable_dir_addr_policy);
 }
 
+/** Lowest allowable value for DirFetchPeriod; if this is too low, clients can
+ * overload the directory system. */
+#define MIN_DIR_FETCH_PERIOD (10*60)
+/** Lowest allowable value for RendPostPeriod; if this is too low, hidden
+ * services can overload the directory system. */
+#define MIN_REND_POST_PERIOD (5*60)
+/** Lowest allowable value for StatusFetchPeriod; if this is too low, clients
+ * can overload the directory system. */
+#define MIN_STATUS_FETCH_PERIOD (5*60)
+
+/** Highest allowable value for DirFetchPeriod, StatusFetchPeriod, and
+ * RendPostPeriod. */
+#define MAX_DIR_PERIOD (MIN_ONION_KEY_LIFETIME/2)
+/** Highest allowable value for DirFetchPeriod for directory caches. */
+#define MAX_CACHE_DIR_FETCH_PERIOD (60*60)
+/** Highest allowable value for StatusFetchPeriod for directory caches. */
+#define MAX_CACHE_STATUS_FETCH_PERIOD (15*60)
+
 /** Return 0 if every setting in <b>options</b> is reasonable.  Else
  * warn and return -1.  Should have no side effects, except for
  * normalizing the contents of <b>options</b>.
@@ -2257,14 +2275,6 @@ options_validate(or_options_t *old_options, or_options_t *options,
       (options->PathlenCoinWeight < 0.0 || options->PathlenCoinWeight >= 1.0))
     REJECT("PathlenCoinWeight option must be >=0.0 and <1.0.");
 
-#define MIN_DIR_FETCH_PERIOD 600
-#define MIN_REND_POST_PERIOD 300
-#define MIN_STATUS_FETCH_PERIOD 60
-
-#define MAX_DIR_PERIOD (MIN_ONION_KEY_LIFETIME/2)
-#define MAX_CACHE_DIR_FETCH_PERIOD 3600
-#define MAX_CACHE_STATUS_FETCH_PERIOD 900
-
   if (options->DirFetchPeriod &&
       options->DirFetchPeriod < MIN_DIR_FETCH_PERIOD) {
     log(LOG_WARN, LD_CONFIG,

src/or/cpuworker.c

@@ -398,12 +398,14 @@ process_pending_task(connection_t *cpuworker)
     log_warn(LD_OR,"assign_to_cpuworker failed. Ignoring.");
 }
 
-#define CPUWORKER_BUSY_TIMEOUT 3600 /* seconds */
+/** How long do we let a cpuworker work before deciding that it's wedged? */
+#define CPUWORKER_BUSY_TIMEOUT (60*60)
 
-/** We have a bug that I can't find. Sometimes, very rarely, cpuworkers
- * get stuck in the 'busy' state, even though the cpuworker process
- * thinks of itself as idle. I don't know why. But here's a workaround
- * to kill any cpuworker that's been busy for more than 3600 seconds. */
+/** We have a bug that I can't find. Sometimes, very rarely, cpuworkers get
+ * stuck in the 'busy' state, even though the cpuworker process thinks of
+ * itself as idle. I don't know why. But here's a workaround to kill any
+ * cpuworker that's been busy for more than CPUWORKER_BUSY_TIMEOUT.
+ */
 static void
 cull_wedged_cpuworkers(void)
 {

src/or/directory.c

@@ -56,7 +56,9 @@ static void note_request(const char *key, size_t bytes);
 
 static addr_policy_t *dir_policy = NULL;
 
-#define ALLOW_DIRECTORY_TIME_SKEW 30*60 /* 30 minutes */
+/** How far in the future do we allow a directory server to tell us it is
+ * before deciding that one of us has the wrong time? */
+#define ALLOW_DIRECTORY_TIME_SKEW (30*60)
 
 /********* END VARIABLES ************/
 

src/or/dirserv.c

@@ -14,7 +14,7 @@ const char dirserv_c_id[] =
  **/
 
 /** How far in the future do we allow a router to get? (seconds) */
-#define ROUTER_ALLOW_SKEW (60*60*12) /* 12 hours */
+#define ROUTER_ALLOW_SKEW (60*60*12)
 /** How many seconds do we wait before regenerating the directory? */
 #define DIR_REGEN_SLACK_TIME 30
 /** If we're a cache, keep this many networkstatuses around from non-trusted

src/or/hibernate.c

@@ -430,6 +430,10 @@ accounting_run_housekeeping(time_t now)
   }
 }
 
+/** When we have no idea how fast we are, how long do we assume it will take
+ * us to exhaust our bandwidth? */
+#define GUESS_TIME_TO_USE_BANDWIDTH (24*60*60)
+
 /** Based on our interval and our estimated bandwidth, choose a
  * deterministic (but random-ish) time to wake up. */
 static void
@@ -463,7 +467,7 @@ accounting_set_wakeup_time(void)
     char buf2[ISO_TIME_LEN+1];
     format_local_iso_time(buf1, interval_start_time);
     format_local_iso_time(buf2, interval_end_time);
-    time_to_exhaust_bw = 24*60*60;
+    time_to_exhaust_bw = GUESS_TIME_TO_USE_BANDWIDTH;
     interval_wakeup_time = interval_start_time;
 
     log_notice(LD_ACCT,

src/or/main.c

@@ -89,13 +89,32 @@ static char* nt_strerror(uint32_t errnum);
 #define nt_service_is_stopped() (0)
 #endif
 
-#define FORCE_REGENERATE_DESCRIPTOR_INTERVAL 18*60*60 /* 18 hours */
-#define CHECK_DESCRIPTOR_INTERVAL 60 /* one minute */
+/** If our router descriptor ever goes this long without being regenerated
+ * because something changed, we force an immediate regenerate-and-upload. */
+#define FORCE_REGENERATE_DESCRIPTOR_INTERVAL (18*60*60)
+/** How often do we check whether part of our router info has changed in a way
+ * that would require an upload? */
+#define CHECK_DESCRIPTOR_INTERVAL (60)
+/** How often do we (as a router) check whether our IP address has changed? */
 #define CHECK_IPADDRESS_INTERVAL (15*60) /* 15 minutes */
-#define BUF_SHRINK_INTERVAL 60 /* one minute */
-#define DESCRIPTOR_RETRY_INTERVAL 10
-#define DESCRIPTOR_FAILURE_RESET_INTERVAL 60*60
-#define ENTROPY_INTERVAL 60*60
+/** How often do we check buffers for empty space that can be deallocated? */
+#define BUF_SHRINK_INTERVAL (60)
+/** How often do we check for router descriptors that we should download? */
+#define DESCRIPTOR_RETRY_INTERVAL (10)
+/** How often do we 'forgive' undownloadable router descriptors and attempt
+ * to download them again? */
+#define DESCRIPTOR_FAILURE_RESET_INTERVAL (60*60)
+/** How often do we add more entropy to OpenSSL's RNG pool? */
+#define ENTROPY_INTERVAL (60*60)
+/** How long do we let a directory connection stall before expiring it? */
+#define DIR_CONN_MAX_STALL (5*60)
+
+/** How old do we let a connection to an OR get before deciding it's
+ * obsolete? */
+#define TIME_BEFORE_OR_CONN_IS_OBSOLETE (60*60*24*7)
+/** How long do we OR connections to handshake before we decide that they
+ * could be obsolete? */
+#define TLS_HANDSHAKE_TIMEOUT           (60)
 
 /********* END VARIABLES ************/
 
@@ -604,7 +623,7 @@ run_connection_housekeeping(int i, time_t now)
 
   /* Expire any directory connections that haven't sent anything for 5 min */
   if (conn->type == CONN_TYPE_DIR &&
-      conn->timestamp_lastwritten + 5*60 < now) {
+      conn->timestamp_lastwritten + DIR_CONN_MAX_STALL < now) {
     log_info(LD_DIR,"Expiring wedged directory conn (fd %d, purpose %d)",
              conn->s, conn->purpose);
     /* This check is temporary; it's to let us know whether we should consider
@@ -623,8 +642,6 @@ run_connection_housekeeping(int i, time_t now)
   if (!connection_speaks_cells(conn))
     return; /* we're all done here, the rest is just for OR conns */
 
-#define TIME_BEFORE_OR_CONN_IS_OBSOLETE (60*60*24*7) /* a week */
-#define TLS_TIMEOUT                             (60) /* a minute */
   if (!conn->is_obsolete) {
     if (conn->timestamp_created + TIME_BEFORE_OR_CONN_IS_OBSOLETE < now) {
       log_info(LD_OR,
@@ -637,10 +654,10 @@ run_connection_housekeeping(int i, time_t now)
         connection_or_get_by_identity_digest(conn->identity_digest);
       if (best && best != conn &&
           (conn->state == OR_CONN_STATE_OPEN ||
-           now > conn->timestamp_created + TLS_TIMEOUT)) {
+           now > conn->timestamp_created + TLS_HANDSHAKE_TIMEOUT)) {
           /* We only mark as obsolete connections that already are in
            * OR_CONN_STATE_OPEN, i.e. that have finished their TLS handshaking.
-           * This is necessay because authorities judge whether a router is
+           * This is necessary because authorities judge whether a router is
            * reachable based on whether they were able to TLS handshake with it
            * recently.  Without this check we would expire connections too
            * early for router->last_reachable to be updated.

src/or/or.h

@@ -187,9 +187,9 @@
 #endif
 
 /** How often do we rotate onion keys? */
-#define MIN_ONION_KEY_LIFETIME (7*24*60*60) /* once a week */
+#define MIN_ONION_KEY_LIFETIME (7*24*60*60)
 /** How often do we rotate TLS contexts? */
-#define MAX_SSL_KEY_LIFETIME (120*60)
+#define MAX_SSL_KEY_LIFETIME (2*60*60)
 
 /** How old do we allow a router to get before removing it
  * from the router list? In seconds. */
@@ -532,7 +532,7 @@ typedef enum {
 #define CELL_CREATED_FAST 6
 
 /** How long to test reachability before complaining to the user. */
-#define TIMEOUT_UNTIL_UNREACHABILITY_COMPLAINT (20*60) /* 20 minutes */
+#define TIMEOUT_UNTIL_UNREACHABILITY_COMPLAINT (20*60)
 
 /* people behind fascist firewalls use only these ports */
 #define REQUIRED_FIREWALL_DIRPORT 80

src/or/rendcommon.c

@@ -231,7 +231,11 @@ rend_get_service_id(crypto_pk_env_t *pk, char *out)
 
 /* ==== Rendezvous service descriptor cache. */
 
+/** How old do we let hidden service descriptors get discarding them as too
+ * old? */
 #define REND_CACHE_MAX_AGE (48*60*60)
+/** How wrong to we assume our clock may be when checking whether hidden
+ * services are too old or too new? */
 #define REND_CACHE_MAX_SKEW (24*60*60)
 
 /** Map from service id (as generated by rend_get_service_id) to

src/or/rephist.c

@@ -382,9 +382,14 @@ rep_history_clean(time_t before)
   }
 }
 
+/** For how many seconds do we keep track of individual per-second bandwidth
+ * totals? */
 #define NUM_SECS_ROLLING_MEASURE 10
-#define NUM_SECS_BW_SUM_IS_VALID (24*60*60) /* one day */
+/** How large are the intervals for with we track and report bandwidth use? */
 #define NUM_SECS_BW_SUM_INTERVAL (15*60)
+/** How far in the past do we remember and publish bandwidth use? */
+#define NUM_SECS_BW_SUM_IS_VALID (24*60*60)
+/** How many bandwidth usage intervals do we remember? (derived.) */
 #define NUM_TOTALS (NUM_SECS_BW_SUM_IS_VALID/NUM_SECS_BW_SUM_INTERVAL)
 
 /**
@@ -818,7 +823,9 @@ rep_hist_note_used_port(uint16_t port, time_t now)
   add_predicted_port(port, now);
 }
 
-#define PREDICTED_CIRCS_RELEVANCE_TIME (3600) /* 1 hour */
+/** For this long after we've seen a request for a given port, assume that
+ * we'll want to make connections to the same port in the future.  */
+#define PREDICTED_CIRCS_RELEVANCE_TIME (60*60)
 
 /** Return a pointer to the list of port numbers that
  * are likely to be asked for in the near future.

src/or/router.c

@@ -913,7 +913,10 @@ mark_my_descriptor_dirty(void)
   desc_clean_since = 0;
 }
 
-#define MAX_BANDWIDTH_CHANGE_FREQ 20*60
+/** How frequently will we republish our descriptor because of large (factor
+ * of 2) shifts in estimated bandwidth? */
+#define MAX_BANDWIDTH_CHANGE_FREQ (20*60)
+
 /** Check whether bandwidth has changed a lot since the last time we announced
  * bandwidth. If so, mark our descriptor dirty. */
 void

src/or/routerlist.c

@@ -2220,8 +2220,15 @@ signed_desc_digest_is_recognized(signed_descriptor_t *desc)
 }
 
 /* XXXX These should be configurable, perhaps? NM */
-#define AUTHORITY_NS_CACHE_INTERVAL 5*60
-#define NONAUTHORITY_NS_CACHE_INTERVAL 15*60
+
+/** How frequently do directory authorities re-download fresh networkstatus
+ * documents? */
+#define AUTHORITY_NS_CACHE_INTERVAL (5*60)
+
+/** How frequently do non-authority directory caches re-download fresh
+ * networkstatus documents? */
+#define NONAUTHORITY_NS_CACHE_INTERVAL (15*60)
+
 /** We are a directory server, and so cache network_status documents.
  * Initiate downloads as needed to update them.  For authorities, this means
  * asking each trusted directory for its network-status.  For caches, this
@@ -2685,7 +2692,7 @@ networkstatus_get_by_digest(const char *digest)
 
 /** We believe networkstatuses more recent than this when they tell us that
  * our server is broken, invalid, obsolete, etc. */
-#define SELF_OPINION_INTERVAL 90*60
+#define SELF_OPINION_INTERVAL (90*60)
 
 /** Return a string naming the versions of Tor recommended by
  * at least n_needed versioning networkstatuses */
@@ -2865,7 +2872,7 @@ routers_update_all_from_networkstatus(void)
 
 /** Allow any network-status newer than this to influence our view of who's
  * running. */
-#define DEFAULT_RUNNING_INTERVAL 60*60
+#define DEFAULT_RUNNING_INTERVAL (60*60)
 /** If possible, always allow at least this many network-statuses to influence
  * our view of who's running. */
 #define MIN_TO_INFLUENCE_RUNNING 3
@@ -3328,14 +3335,16 @@ initiate_descriptor_downloads(routerstatus_t *source,
   tor_free(resource);
 }
 
+/** Clients don't download any descriptor this recent, since it will probably
+ * not have propageted to enough caches. */
+#define ESTIMATED_PROPAGATION_TIME (10*60)
+
 /** Return new list of ID fingerprints for routers that we (as a client) would
  * like to download.
  */
 static smartlist_t *
 router_list_client_downloadable(void)
 {
-#define MAX_OLD_SERVER_DOWNLOAD_RATE 2*60*60
-#define ESTIMATED_PROPAGATION_TIME 10*60
   int n_downloadable = 0;
   smartlist_t *downloadable = smartlist_create();
   digestmap_t *downloading;
@@ -3415,11 +3424,23 @@ update_router_descriptor_client_downloads(time_t now)
    *   So use 96 because it's a nice number.
    */
 #define MAX_DL_PER_REQUEST 96
+  /** Don't split our requests so finely that we are requesting fewer than
+   * this number per server. */
 #define MIN_DL_PER_REQUEST 4
+  /** To prevent a single screwy cache from confusing us by selective reply,
+   * try to split our requests into at least this this many requests. */
 #define MIN_REQUESTS 3
+  /** If we want fewer than this many descriptors, wait until we
+   * want more, or until MAX_(CLIENT|SERVER)_INTERVAL_WITHOUT_REQUEST has
+   * passed. */
 #define MAX_DL_TO_DELAY 16
-#define MAX_CLIENT_INTERVAL_WITHOUT_REQUEST 10*60
-#define MAX_SERVER_INTERVAL_WITHOUT_REQUEST 1*60
+  /** When directory clients have only a few servers to request, they batch
+   * them until they have more, or until this amount of time has passed. */
+#define MAX_CLIENT_INTERVAL_WITHOUT_REQUEST (10*60)
+  /** When directory caches and authorities have only a few servers to
+   * request, they batch them until they have more, or until this amount of
+   * time has passed. */
+#define MAX_SERVER_INTERVAL_WITHOUT_REQUEST (60)
   smartlist_t *downloadable = NULL;
   int should_delay, n_downloadable;
   or_options_t *options = get_options();
@@ -3682,6 +3703,10 @@ router_reset_descriptor_download_failures(void)
   last_routerdesc_download_attempted = 0;
 }
 
+/** Any changes in a router descriptor's publication time larger than this are
+ * automatically non-cosmetic. */
+#define ROUTER_MAX_COSMETIC_TIME_DIFFERENCE (12*60*60)
+
 /** Return true iff the only differences between r1 and r2 are such that
  * would not cause a recent (post 0.1.1.6) dirserver to republish.
  */
@@ -3733,7 +3758,8 @@ router_differences_are_cosmetic(routerinfo_t *r1, routerinfo_t *r2)
     return 0;
 
   /* Did more than 12 hours pass? */
-  if (r1->cache_info.published_on + 12*60*60 < r2->cache_info.published_on)
+  if (r1->cache_info.published_on + ROUTER_MAX_COSMETIC_TIME_DIFFERENCE
+      < r2->cache_info.published_on)
     return 0;
 
   /* Did uptime fail to increase by approximately the amount we would think,