From fac272da31554a8ce1095973ac7a08b3028f799f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 31 Jul 2010 16:33:45 -0400 Subject: [PATCH 1/7] If a router is hibernating, never vote that it is Running. Also, clean up and comment some of the logic in dirserv_set_router_is_running. --- changes/bug911_hibernate_precludes_Running | 5 +++++ src/or/dirserv.c | 16 +++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) create mode 100644 changes/bug911_hibernate_precludes_Running diff --git a/changes/bug911_hibernate_precludes_Running b/changes/bug911_hibernate_precludes_Running new file mode 100644 index 0000000000..e8f279223c --- /dev/null +++ b/changes/bug911_hibernate_precludes_Running @@ -0,0 +1,5 @@ + o Minor bugfixes: + - Never vote for a server as "Running" if we have a descriptor for it + claiming to be hibernating, and that descriptor was published more + recently than our last contact with the server. + diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 86cd186111..7b469ce54b 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -935,11 +935,21 @@ dirserv_set_router_is_running(routerinfo_t *router, time_t now) */ int answer; - if (router_is_me(router) && !we_are_hibernating()) + if (router_is_me(router)) + /* We always know if we are down ourselves. */ + answer = ! we_are_hibernating(); + else if (router->is_hibernating && + router->cache_info.published_on > router->last_reachable) + /* A hibernating router is down unless we (somehow) had contact with it + * since it declared itself to be hibernating. */ + answer = 0; + else if (get_options()->AssumeReachable) + /* If AssumeReachable, everybody is up! */ answer = 1; else - answer = get_options()->AssumeReachable || - now < router->last_reachable + REACHABLE_TIMEOUT; + /* Otherwise, a router counts as up if we found it reachable in the last + REACHABLE_TIMEOUT seconds. */ + answer = (now < router->last_reachable + REACHABLE_TIMEOUT); if (!answer && running_long_enough_to_decide_unreachable()) { /* not considered reachable. tell rephist. */ From 4c941920331ba17b4f245110f056cd60616c95b7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 10 Aug 2010 15:32:56 -0400 Subject: [PATCH 2/7] Add missing info to changes file --- changes/bug911_hibernate_precludes_Running | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changes/bug911_hibernate_precludes_Running b/changes/bug911_hibernate_precludes_Running index e8f279223c..5e0168d545 100644 --- a/changes/bug911_hibernate_precludes_Running +++ b/changes/bug911_hibernate_precludes_Running @@ -1,5 +1,6 @@ o Minor bugfixes: - Never vote for a server as "Running" if we have a descriptor for it claiming to be hibernating, and that descriptor was published more - recently than our last contact with the server. + recently than our last contact with the server. Bugfix on + 0.2.0.3-alpha; fixes bug 911. From 5063a1c538739e56a38605b2d8ddba3164e1aa73 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 18 Aug 2010 11:28:51 -0400 Subject: [PATCH 3/7] Add some braces to make arma happy --- src/or/dirserv.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 7b469ce54b..4d4d8a22c7 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -935,21 +935,22 @@ dirserv_set_router_is_running(routerinfo_t *router, time_t now) */ int answer; - if (router_is_me(router)) + if (router_is_me(router)) { /* We always know if we are down ourselves. */ answer = ! we_are_hibernating(); - else if (router->is_hibernating && - router->cache_info.published_on > router->last_reachable) + } else if (router->is_hibernating && + router->cache_info.published_on > router->last_reachable) { /* A hibernating router is down unless we (somehow) had contact with it * since it declared itself to be hibernating. */ answer = 0; - else if (get_options()->AssumeReachable) + } else if (get_options()->AssumeReachable) { /* If AssumeReachable, everybody is up! */ answer = 1; - else + } else { /* Otherwise, a router counts as up if we found it reachable in the last REACHABLE_TIMEOUT seconds. */ answer = (now < router->last_reachable + REACHABLE_TIMEOUT); + } if (!answer && running_long_enough_to_decide_unreachable()) { /* not considered reachable. tell rephist. */ From a4c5287e1b8fbb132cefc834ee717f6c21953e0d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 18 Aug 2010 11:31:19 -0400 Subject: [PATCH 4/7] Clarify AssumeReachable semantics wrt hibernation --- src/or/dirserv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 4d4d8a22c7..dd9026758c 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -944,7 +944,7 @@ dirserv_set_router_is_running(routerinfo_t *router, time_t now) * since it declared itself to be hibernating. */ answer = 0; } else if (get_options()->AssumeReachable) { - /* If AssumeReachable, everybody is up! */ + /* If AssumeReachable, everybody is up unless they say they are down! */ answer = 1; } else { /* Otherwise, a router counts as up if we found it reachable in the last From 23fdf0b30fd9fdfe1f82e5aa1b8a196c3ca68575 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 18 Aug 2010 11:38:41 -0400 Subject: [PATCH 5/7] Allow some skew in checking when a router said it was hibernating This solves the problem Roger noted as: What if the router has a clock that's 5 minutes off, so it publishes a descriptor for 5 minutes in the future, and we test it three minutes in. In this edge case, we will continue to advertise it as Running for the full 45 minute period. --- src/or/dirserv.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/or/dirserv.c b/src/or/dirserv.c index dd9026758c..80831b5be0 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -923,6 +923,11 @@ running_long_enough_to_decide_unreachable(void) * the directory. */ #define REACHABLE_TIMEOUT (45*60) +/** If we tested a router and found it reachable _at least this long_ after it + * declared itself hibernating, it is probably done hibernating and we just + * missed a descriptor from it. */ +#define ALLOW_REACHABILITY_PUBLICATION_SKEW (60*60) + /** Treat a router as alive if * - It's me, and I'm not hibernating. * or - We've found it reachable recently. */ @@ -939,7 +944,8 @@ dirserv_set_router_is_running(routerinfo_t *router, time_t now) /* We always know if we are down ourselves. */ answer = ! we_are_hibernating(); } else if (router->is_hibernating && - router->cache_info.published_on > router->last_reachable) { + (router->cache_info.published_on + + ALLOW_REACHABILITY_PUBLICATION_SKEW) > router->last_reachable) { /* A hibernating router is down unless we (somehow) had contact with it * since it declared itself to be hibernating. */ answer = 0; From 5926d9cfccccfca19895522ed7a445626be8cc79 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 18 Aug 2010 13:36:09 -0400 Subject: [PATCH 6/7] Move code for launching tests out of router_add_to_routerlist() router_add_to_routerlist() is supposed to be a nice minimal function that only touches the routerlist structures, but it included a call to dirserv_single_reachability_test(). We have a function that gets called _after_ adding descriptors successfully: routerlist_descriptors_added. This patch moves the responsibility for testing there. Because the decision of whether to test or not depends on whether there was an old routerinfo for this router or not, we have to first detect whether we _will_ want to run the tests if the router is added. We make this the job of routers_update_status_from_consensus_networkstatus(). Finally, this patch makes the code notice if a router is going from hibernating to non-hibernating, and if so causes a reachability test to get launched. --- src/or/dirserv.c | 26 +++++++++++++++++++++++++- src/or/dirserv.h | 2 ++ src/or/networkstatus.c | 9 +++++++++ src/or/or.h | 3 +++ src/or/routerlist.c | 15 +++++++-------- src/or/routerlist.h | 1 + 6 files changed, 47 insertions(+), 9 deletions(-) diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 80831b5be0..523a92178e 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -730,6 +730,10 @@ dirserv_add_descriptor(routerinfo_t *ri, const char **msg, const char *source) desc = tor_strndup(ri->cache_info.signed_descriptor_body, desclen); nickname = tor_strdup(ri->nickname); + /* Tell if we're about to need to launch a test if we add this. */ + ri->needs_retest_if_added = + dirserv_should_launch_reachability_test(ri, ri_old); + r = router_add_to_routerlist(ri, msg, 0, 0); if (!WRA_WAS_ADDED(r)) { /* unless the routerinfo was fine, just out-of-date */ @@ -744,7 +748,7 @@ dirserv_add_descriptor(routerinfo_t *ri, const char **msg, const char *source) changed = smartlist_create(); smartlist_add(changed, ri); - control_event_descriptors_changed(changed); + routerlist_descriptors_added(changed, 0); smartlist_free(changed); if (!*msg) { *msg = ri->is_valid ? "Descriptor for valid server accepted" : @@ -3115,6 +3119,26 @@ dirserv_orconn_tls_done(const char *address, * skip testing. */ } +/** Called when we, as an authority, receive a new router descriptor either as + * an upload or a download. Used to decide whether to relaunch reachability + * testing for the server. */ +int +dirserv_should_launch_reachability_test(routerinfo_t *ri, routerinfo_t *ri_old) +{ + if (!authdir_mode_handles_descs(get_options(), ri->purpose)) + return 0; + if (!ri_old) { + /* New router: Launch an immediate reachability test, so we will have an + * opinion soon in case we're generating a consensus soon */ + return 1; + } + if (ri_old->is_hibernating && !ri->is_hibernating) { + /* It just came out of hibernation; launch a reachability test */ + return 1; + } + return 0; +} + /** Helper function for dirserv_test_reachability(). Start a TLS * connection to router, and annotate it with when we started * the test. */ diff --git a/src/or/dirserv.h b/src/or/dirserv.h index fc5a5549c5..d80d6f693f 100644 --- a/src/or/dirserv.h +++ b/src/or/dirserv.h @@ -100,6 +100,8 @@ void dirserv_orconn_tls_done(const char *address, uint16_t or_port, const char *digest_rcvd, int as_advertised); +int dirserv_should_launch_reachability_test(routerinfo_t *ri, + routerinfo_t *ri_old); void dirserv_single_reachability_test(time_t now, routerinfo_t *router); void dirserv_test_reachability(time_t now); int authdir_wants_to_reject_router(routerinfo_t *ri, const char **msg, diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index a9a9c78b89..bf034f4685 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1924,6 +1924,15 @@ routers_update_status_from_consensus_networkstatus(smartlist_t *routers, router->is_bad_directory = rs->is_bad_directory; router->is_bad_exit = rs->is_bad_exit; router->is_hs_dir = rs->is_hs_dir; + } else { + /* If we _are_ an authority, we should check wither this router + * is one that will cause us to need a reachability test. */ + routerinfo_t *old_router = + router_get_by_digest(router->cache_info.identity_digest); + if (old_router != router) { + router->needs_retest_if_added = + dirserv_should_launch_reachability_test(router, old_router); + } } if (router->is_running && ds) { download_status_reset(&ds->v2_ns_dl_status); diff --git a/src/or/or.h b/src/or/or.h index 572dc8b96d..cb2bbd75f2 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -1453,6 +1453,9 @@ typedef struct { * directory according to the authorities. */ unsigned int policy_is_reject_star:1; /**< True iff the exit policy for this * router rejects everything. */ + /** True if, after we have added this router, we should re-launch + * tests for it. */ + unsigned int needs_retest_if_added:1; /** Tor can use this router for general positions in circuits. */ #define ROUTER_PURPOSE_GENERAL 0 diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 5f98abe01b..968d5a1040 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -3276,11 +3276,6 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg, * the list. */ routerlist_insert(routerlist, router); if (!from_cache) { - if (authdir) { - /* launch an immediate reachability test, so we will have an opinion - * soon in case we're generating a consensus soon */ - dirserv_single_reachability_test(time(NULL), router); - } signed_desc_append_to_journal(&router->cache_info, &routerlist->desc_store); } @@ -3600,15 +3595,19 @@ routerlist_remove_old_routers(void) /** We just added a new set of descriptors. Take whatever extra steps * we need. */ -static void +void routerlist_descriptors_added(smartlist_t *sl, int from_cache) { tor_assert(sl); control_event_descriptors_changed(sl); - SMARTLIST_FOREACH(sl, routerinfo_t *, ri, + SMARTLIST_FOREACH_BEGIN(sl, routerinfo_t *, ri) { if (ri->purpose == ROUTER_PURPOSE_BRIDGE) learned_bridge_descriptor(ri, from_cache); - ); + if (ri->needs_retest_if_added) { + ri->needs_retest_if_added = 0; + dirserv_single_reachability_test(approx_time(), ri); + } + } SMARTLIST_FOREACH_END(ri); } /** diff --git a/src/or/routerlist.h b/src/or/routerlist.h index e31b07aef5..d71a737b88 100644 --- a/src/or/routerlist.h +++ b/src/or/routerlist.h @@ -115,6 +115,7 @@ was_router_added_t router_add_to_routerlist(routerinfo_t *router, was_router_added_t router_add_extrainfo_to_routerlist( extrainfo_t *ei, const char **msg, int from_cache, int from_fetch); +void routerlist_descriptors_added(smartlist_t *sl, int from_cache); void routerlist_remove_old_routers(void); int router_load_single_router(const char *s, uint8_t purpose, int cache, const char **msg); From d9e05505606046afa8b346acac5f1e379e6f6e6d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Sep 2010 22:10:32 -0400 Subject: [PATCH 7/7] Tweak some issues found by arma in bug911 review. --- src/or/dirserv.c | 6 +++--- src/or/networkstatus.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 523a92178e..8523335ec4 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -930,7 +930,7 @@ running_long_enough_to_decide_unreachable(void) /** If we tested a router and found it reachable _at least this long_ after it * declared itself hibernating, it is probably done hibernating and we just * missed a descriptor from it. */ -#define ALLOW_REACHABILITY_PUBLICATION_SKEW (60*60) +#define HIBERNATION_PUBLICATION_SKEW (60*60) /** Treat a router as alive if * - It's me, and I'm not hibernating. @@ -949,7 +949,7 @@ dirserv_set_router_is_running(routerinfo_t *router, time_t now) answer = ! we_are_hibernating(); } else if (router->is_hibernating && (router->cache_info.published_on + - ALLOW_REACHABILITY_PUBLICATION_SKEW) > router->last_reachable) { + HIBERNATION_PUBLICATION_SKEW) > router->last_reachable) { /* A hibernating router is down unless we (somehow) had contact with it * since it declared itself to be hibernating. */ answer = 0; @@ -3126,7 +3126,7 @@ int dirserv_should_launch_reachability_test(routerinfo_t *ri, routerinfo_t *ri_old) { if (!authdir_mode_handles_descs(get_options(), ri->purpose)) - return 0; + return 0; if (!ri_old) { /* New router: Launch an immediate reachability test, so we will have an * opinion soon in case we're generating a consensus soon */ diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index bf034f4685..2814f160ca 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1925,7 +1925,7 @@ routers_update_status_from_consensus_networkstatus(smartlist_t *routers, router->is_bad_exit = rs->is_bad_exit; router->is_hs_dir = rs->is_hs_dir; } else { - /* If we _are_ an authority, we should check wither this router + /* If we _are_ an authority, we should check whether this router * is one that will cause us to need a reachability test. */ routerinfo_t *old_router = router_get_by_digest(router->cache_info.identity_digest);