prop224: Refactor descriptor reupload logic.

We refactor the descriptor reupload logic to be similar to the v2 logic
where we update a global 'consider_republishing_rend_descriptors' flag
and then we use that to check for hash ring changes during the global
hidden service callbacks.

This fixes bugs where we would inspect the hash ring immediately as we
receive new dirinfo (e.g. consensus) but before running the hidden
service housekeeping events. That was leaving us in an inconsistent
state wrt hsdir indices and causing bugs all around.

src/or/hs_service.c

@@ -72,6 +72,11 @@ static const char address_tld[] = "onion";
  * loading keys requires that we are an actual running tor process. */
 static smartlist_t *hs_service_staging_list;
 
+/** True if the list of available router descriptors might have changed which
+ *  might result in an altered hash ring. Check if the hash ring changed and
+ *  reupload if needed */
+static int consider_republishing_hs_descriptors = 0;
+
 static void set_descriptor_revision_counter(hs_descriptor_t *hs_desc);
 
 /* Helper: Function to compare two objects in the service map. Return 1 if the
@@ -2429,7 +2434,14 @@ run_upload_descriptor_event(time_t now)
     FOR_EACH_DESCRIPTOR_BEGIN(service, desc) {
       int for_next_period = 0;
 
-      /* Can this descriptor be uploaed? */
+      /* If we were asked to re-examine the hash ring, and it changed, then
+         schedule an upload */
+      if (consider_republishing_hs_descriptors &&
+          service_desc_hsdirs_changed(service, desc)) {
+        service_desc_schedule_upload(desc, now, 0);
+      }
+
+      /* Can this descriptor be uploaded? */
       if (!should_service_upload_descriptor(service, desc, now)) {
         continue;
       }
@@ -2456,6 +2468,9 @@ run_upload_descriptor_event(time_t now)
       upload_descriptor_to_all(service, desc, for_next_period);
     } FOR_EACH_DESCRIPTOR_END;
   } FOR_EACH_SERVICE_END;
+
+  /* We are done considering whether to republish rend descriptors */
+  consider_republishing_hs_descriptors = 0;
 }
 
 /* Called when the introduction point circuit is done building and ready to be
@@ -2738,7 +2753,7 @@ service_add_fnames_to_list(const hs_service_t *service, smartlist_t *list)
 
 /** The set of HSDirs have changed: check if the change affects our descriptor
  *  HSDir placement, and if it does, reupload the desc. */
-static int
+int
 service_desc_hsdirs_changed(const hs_service_t *service,
                             const hs_service_descriptor_t *desc)
 {
@@ -2788,34 +2803,13 @@ service_desc_hsdirs_changed(const hs_service_t *service,
 /* ========== */
 
 /* We just received a new batch of descriptors which might affect the shape of
- * the HSDir hash ring. Signal that we should re-upload our HS descriptors. */
+ * the HSDir hash ring. Signal that we should reexamine the hash ring and
+ * re-upload our HS descriptors if needed. */
 void
 hs_hsdir_set_changed_consider_reupload(void)
 {
-  time_t now = approx_time();
+  log_info(LD_REND, "New dirinfo arrived: consider reuploading descriptor");
-
+  consider_republishing_hs_descriptors = 1;
-  /* Check if HS subsystem is initialized */
-  if (!hs_service_map) {
-    return;
-  }
-
-  /* Basic test: If we have not bootstrapped 100% yet, no point in even trying
-     to upload descriptor. */
-  if (!router_have_minimum_dir_info()) {
-    return;
-  }
-
-  log_info(LD_GENERAL, "Received new dirinfo: Checking hash ring for changes");
-
-  /* Go over all descriptors and check if the set of HSDirs changed for any of
-   * them. Schedule reupload if so. */
-  FOR_EACH_SERVICE_BEGIN(service) {
-    FOR_EACH_DESCRIPTOR_BEGIN(service, desc) {
-      if (service_desc_hsdirs_changed(service, desc)) {
-        service_desc_schedule_upload(desc, now, 0);
-      }
-    } FOR_EACH_DESCRIPTOR_END;
-  } FOR_EACH_SERVICE_END;
 }
 
 /* Return the number of service we have configured and usable. */

src/or/hs_service.h

@@ -280,6 +280,10 @@ int hs_service_receive_introduce2(origin_circuit_t *circ,
 
 void hs_service_intro_circ_has_closed(origin_circuit_t *circ);
 
+int service_desc_hsdirs_changed(const hs_service_t *service,
+                                const hs_service_descriptor_t *desc);
+
+
 #ifdef HS_SERVICE_PRIVATE
 
 #ifdef TOR_UNIT_TESTS

src/test/test_hs_common.c

@@ -626,12 +626,11 @@ test_desc_reupload_logic(void *arg)
                                       curr_hsdir_index, nickname, 1);
   }
 
-  /* Now call router_dir_info_changed() again and see that it detected the hash
+  /* Now call service_desc_hsdirs_changed() and see that it detected the hash
-     ring change and updated the upload time */
+     ring change */
   time_t now = approx_time();
   tt_assert(now);
-  router_dir_info_changed();
+  tt_int_op(service_desc_hsdirs_changed(service, desc), OP_EQ, 1);
-  tt_int_op(desc->next_upload_time, OP_EQ, now);
 
   /* Now pretend that the descriptor changed, and order a reupload to all
      HSDirs. Make sure that the set of previous HSDirs was cleared. */

src/test/test_hs_service.c

@@ -1005,6 +1005,13 @@ test_rotate_descriptors(void *arg)
             OP_EQ, 0);
   tt_assert(service->desc_next == NULL);
 
+  /* Now let's re-create desc_next and get out of overlap period. We should
+     test that desc_current gets replaced by desc_next, and desc_next becomes
+     NULL. */
+  desc_next = service_descriptor_new();
+  desc_next->next_upload_time = 240; /* Our marker to recognize it. */
+  service->desc_next = desc_next;
+
   /* Going out of the overlap period. */
   ret = parse_rfc1123_time("Sat, 26 Oct 1985 12:00:00 UTC",
                            &mock_ns.valid_after);
@@ -1017,6 +1024,12 @@ test_rotate_descriptors(void *arg)
   tt_mem_op(service->desc_current, OP_EQ, desc_next, sizeof(*desc_next));
   tt_assert(service->desc_next == NULL);
 
+  /* Calling rotate_all_descriptors() another time should do nothing */
+  rotate_all_descriptors(now);
+  tt_int_op(service->state.in_overlap_period, OP_EQ, 0);
+  tt_mem_op(service->desc_current, OP_EQ, desc_next, sizeof(*desc_next));
+  tt_assert(service->desc_next == NULL);
+
  done:
   hs_free_all();
   UNMOCK(circuit_mark_for_close_);