From 1dc21b87904b527140446674876076f29c78b89f Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 30 Aug 2017 15:29:41 +0300 Subject: prop224: Simplify HSDir set change algo. Our logic for detecting hsdir set changes was needlessly compicated: we had to sort smartlists and compare them. Instead, we can simplify things by employing the following logic: "We should reupload our descriptor if the latest HSDir set contains nodes that were not previously there" --- src/or/hs_service.c | 32 +++++++++++--------------------- src/or/hs_service.h | 2 +- 2 files changed, 12 insertions(+), 22 deletions(-) (limited to 'src/or') diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 5ff118222d..0fba0d7f90 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -1546,7 +1546,6 @@ service_desc_note_upload(hs_service_descriptor_t *desc, const node_t *hsdir) if (!smartlist_contains_string(desc->previous_hsdirs, b64_digest)) { smartlist_add_strdup(desc->previous_hsdirs, b64_digest); - smartlist_sort_strings(desc->previous_hsdirs); } } @@ -2366,9 +2365,8 @@ STATIC int service_desc_hsdirs_changed(const hs_service_t *service, const hs_service_descriptor_t *desc) { - int retval = 0; + int should_reupload = 0; smartlist_t *responsible_dirs = smartlist_new(); - smartlist_t *b64_responsible_dirs = smartlist_new(); /* No desc upload has happened yet: it will happen eventually */ if (!desc->previous_hsdirs || !smartlist_len(desc->previous_hsdirs)) { @@ -2379,32 +2377,24 @@ service_desc_hsdirs_changed(const hs_service_t *service, hs_get_responsible_hsdirs(&desc->blinded_kp.pubkey, desc->time_period_num, service->desc_next == desc, 0, responsible_dirs); - /* Make a second list with their b64ed identity digests, so that we can - * compare it with out previous list of hsdirs */ + /* Check if any new hsdirs have been added to the responsible hsdirs set: + * Iterate over the list of new hsdirs, and reupload if any of them is not + * present in the list of previous hsdirs. + */ SMARTLIST_FOREACH_BEGIN(responsible_dirs, const routerstatus_t *, hsdir_rs) { char b64_digest[BASE64_DIGEST_LEN+1] = {0}; digest_to_base64(b64_digest, hsdir_rs->identity_digest); - smartlist_add_strdup(b64_responsible_dirs, b64_digest); - } SMARTLIST_FOREACH_END(hsdir_rs); - - /* Sort this new smartlist so that we can compare it with the other one */ - smartlist_sort_strings(b64_responsible_dirs); - /* Check whether the set of HSDirs changed */ - if (!smartlist_strings_eq(b64_responsible_dirs, desc->previous_hsdirs)) { - log_info(LD_GENERAL, "Received new dirinfo and set of hsdirs changed!"); - retval = 1; - } else { - log_debug(LD_GENERAL, "No change in hsdir set!"); - } + if (!smartlist_contains_string(desc->previous_hsdirs, b64_digest)) { + should_reupload = 1; + break; + } + } SMARTLIST_FOREACH_END(hsdir_rs); done: smartlist_free(responsible_dirs); - SMARTLIST_FOREACH(b64_responsible_dirs, char*, s, tor_free(s)); - smartlist_free(b64_responsible_dirs); - - return retval; + return should_reupload; } /* Return 1 if the given descriptor from the given service can be uploaded diff --git a/src/or/hs_service.h b/src/or/hs_service.h index 57717fc927..26f0bc0002 100644 --- a/src/or/hs_service.h +++ b/src/or/hs_service.h @@ -133,7 +133,7 @@ typedef struct hs_service_descriptor_t { /** List of the responsible HSDirs (their b64ed identity digest) last time we * uploaded this descriptor. If the set of responsible HSDirs is different * from this list, this means we received new dirinfo and we need to - * reupload our descriptor. This list is always sorted lexicographically. */ + * reupload our descriptor. */ smartlist_t *previous_hsdirs; } hs_service_descriptor_t; -- cgit v1.2.3-54-g00ecf From b9f849bdee20f36264fe061498536e0d3a8616a6 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 29 Aug 2017 16:02:01 +0300 Subject: prop224: Clear list of prev hsdirs before we upload all descs. This fixes a serious bug in our hsdir set change logic: We used to add nodes in the list of previous hsdirs everytime we uploaded to a new hsdir and we only cleared the list when we built a new descriptor. This means that our prev_hsdirs list could end up with 7 hsdirs, if for some reason we ended up uploading our desc to 7 hsdirs before rebuilding our descriptor (e.g. this can happen if the set of hsdirs changed). After our previous hdsir set had 7 nodes, then our old algorithm would always think that the set has changed since it was comparing a smartlist with 7 elements against a smartlist with 6 elements. This commit fixes this bug, by clearning the prev_hsdirs list before we upload to all hsdirs. This makes sure that our prev_hsdirs list always contains the latest hsdirs! --- src/or/hs_service.c | 4 ++++ src/test/test_hs_common.c | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) (limited to 'src/or') diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 0fba0d7f90..218d49ace3 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -2316,6 +2316,10 @@ upload_descriptor_to_all(const hs_service_t *service, hs_get_responsible_hsdirs(&desc->blinded_kp.pubkey, desc->time_period_num, for_next_period, 0, responsible_dirs); + /** Clear list of previous hsdirs since we are about to upload to a new + * list. Let's keep it up to date. */ + service_desc_clear_previous_hsdirs(desc); + /* For each responsible HSDir we have, initiate an upload command. */ SMARTLIST_FOREACH_BEGIN(responsible_dirs, const routerstatus_t *, hsdir_rs) { diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index 6bb03f39d6..9980892951 100644 --- a/src/test/test_hs_common.c +++ b/src/test/test_hs_common.c @@ -613,10 +613,10 @@ test_desc_reupload_logic(void *arg) tt_int_op(service_desc_hsdirs_changed(service, desc), OP_EQ, 1); tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6); - /* Now order another upload and see how we end up with 7 hsdirs */ + /* Now order another upload and see that we keep having 6 prev hsdirs */ upload_descriptor_to_all(service, desc, 0); /* Check that previous hsdirs were populated */ - tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 7); + tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6); /* Now restore the HSDir hash ring to its original state by swapping back aaron for nora */ -- cgit v1.2.3-54-g00ecf