Opened 6 years ago

Closed 5 years ago

#11679 closed defect (fixed)

Download status for consensus scheduled as generic

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client easy 026-triaged-1
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Currently download status scheduled as generic (DL_SCHED_GENERIC = 0);

/** Download status for the current consensus networkstatus. */
static download_status_t consensus_dl_status[N_CONSENSUS_FLAVORS];

Before implementing of consensus for md it was DL_SCHED_CONSENSUS:

static download_status_t consensus_dl_status = { 0, 0, DL_SCHED_CONSENSUS };

That means unfriendly schedule for client with fragile network connection.

/** Schedule for when clients should download things in general. */
static const int client_dl_schedule[] = {
  0, 0, 60, 60*5, 60*10, INT_MAX
};

vs.

/** Schedule for when clients should download consensuses. */
static const int client_consensus_dl_schedule[] = {
  0, 0, 60, 60*5, 60*10, 60*30, 60*60, 60*60, 60*60, 60*60*3, 60*60*6, 60*60*12
};

Tor will reset download status by networkstatus_reset_download_failures once a hour so client will have a chance to retry, so no unresolved state actually.

Is it worth to initialize download status for consensus schedule? Or to rename schedule if planned to use it for certs only?

Child Tickets

Change History (8)

comment:1 Changed 6 years ago by nickm

Keywords: tor-client added
Milestone: Tor: 0.2.5.x-final

Should at least look at this for 0.2.5, though it's likely safe to do in 0.2.6.

comment:2 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

I think because of the reset-once-an-hour thing from networkstatus_reset_download_failures, it's okay to do this in 0.2.6.

comment:3 Changed 5 years ago by nickm

Keywords: easy 026-triaged added

comment:4 Changed 5 years ago by nickm

Keywords: 026-triaged-1 added; 026-triaged removed

comment:5 Changed 5 years ago by arma

Right, this is quite clearly a bug.

I just looked through the rest of the code and I don't think there are any other instances of it -- though we should probably document in or.h when we say DL_SCHED_CONSENSUS that we include cert fetches.

Seems to me that the fix is to either initialize consensus_dl_status with

static download_status_t consensus_dl_status[N_CONSENSUS_FLAVORS] =
  { {0,0,DL_SCHED_CONSENSUS}, {0,0,DL_SCHED_CONSENSUS} };

which will silently become wrong again when we increment N_CONSENSUS_FLAVORS and forget to fix it, or have an initialization function that walks the array to initialize it. The latter seems wiser?

comment:6 Changed 5 years ago by arma

What would you think of this patch?

diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index 95eb320..2f22eb1 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -754,6 +754,7 @@ update_consensus_networkstatus_downloads(time_t now)
 
     resource = networkstatus_get_flavor_name(i);
 
+    consensus_dl_status[i].schedule = DL_SCHED_CONSENSUS;
     if (!download_status_is_ready(&consensus_dl_status[i], now,
                              options->TestingConsensusMaxDownloadTries))
       continue; /* We failed downloading a consensus too recently. */

It sets the schedule every time it goes through, which should be cheap to do, and is maybe a bit less intuitive than having a separate initialization function, but makes up for it in simplicity.

Here's the more complicated version:

diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index 95eb320..e2e5a95 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -722,6 +722,14 @@ we_want_to_fetch_flavor(const or_options_t *options, int f
   return flavor == usable_consensus_flavor();
 }
 
+static void
+initialize_dl_status_schedule(void)
+{
+  int i;
+  for (i=0; i < N_CONSENSUS_FLAVORS; ++i)
+    consensus_dl_status[i].schedule = DL_SCHED_CONSENSUS;
+}
+
 /** How long will we hang onto a possibly live consensus for which we're
  * fetching certs before we check whether there is a better one? */
 #define DELAY_WHILE_FETCHING_CERTS (20*60)
@@ -734,6 +742,8 @@ update_consensus_networkstatus_downloads(time_t now)
   int i;
   const or_options_t *options = get_options();
 
+  initialize_dl_status_schedule();
+
   for (i=0; i < N_CONSENSUS_FLAVORS; ++i) {
     /* XXXX need some way to download unknown flavors if we are caching. */
     const char *resource;

and we could make it even more complicated still by having a
static int have_initialized_dl_status_schedule = 0;
which we check in update_consensus_networkstatus_downloads(), and call initialize_dl_status_schedule() if needed and then set the value to 1.

I'd argue for the simple approach, but this is a fine Chief Architect taste question. :)

comment:7 Changed 5 years ago by nickm

Status: newneeds_review

comment:8 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I've done a none-of-the-above-but-still-quite-simple approach as d14127eb7adfb92b1790f0aaa65089cfb6c71094: An initializer to start it out right, and an assert to make sure we didn't forget to make the initializer match the number of flavors.

Note: See TracTickets for help on using tickets.