Bug24977 final #146
Bug24977 final #146
Conversation
Here is how this changes the HSv3 client-side and service-side: For service side we already required live consensus to upload descriptors (see 9e900d1) so we should never get there without a live consensus. For the client-side we now require a live consensus to attempt to connect to HS. While this changes the client behavior in principle, it doesn't really change it, because we always required live consensus to set HSDir indices, so before this patch a client with no live consensus would try to compute responsible HSDirs without any HSDir indices and bug out. This makes the client behavior more consistent, by requiring a live consensus (and hence a semi-synced clock) for the client to connect to an HS entirely. The alternative would have been to allow setting HSDir indices with a non-live consensus, but this would cause the various problems outlined by commit b89d2fa.
We currently only do the check when we are about to use the HSDir indices.
src/or/voting_schedule.c
Outdated
| * newer consensus, make sure we recalculate the voting schedule. */ | ||
| networkstatus_t *consensus = networkstatus_get_live_consensus(approx_time()); | ||
| if (consensus && | ||
| consensus->valid_after != voting_schedule.live_consensus_valid_after) { |
There was a problem hiding this comment.
Maybe we want to use a else if somehow so we avoid all this code if the first condition was hit which would recalculate the voting schedule in the first place?
There was a problem hiding this comment.
Good point. Took the goto approach in 82bb4f3 to avoid the extra indentation of else if. Let me know if you hate it, and I will just use else if.
src/or/voting_schedule.c
Outdated
|
|
||
| /* Also make sure we are not using an outdated voting schedule. If we have a | ||
| * newer consensus, make sure we recalculate the voting schedule. */ | ||
| networkstatus_t *consensus = networkstatus_get_live_consensus(approx_time()); |
There was a problem hiding this comment.
I would const that if possible.
Also, approx_time() is used twice in this function so probably a time_t now is desirable?
src/or/voting_schedule.c
Outdated
| } | ||
|
|
||
| if (need_to_recalculate_voting_schedule) { | ||
| voting_schedule_recalculate_timing(get_options(), approx_time()); |
There was a problem hiding this comment.
Hold on a sec. Don't we have a circular loop here? get_voting_schedule() can reach this point and then voting_schedule_recalculate_timing() calls get_voting_schedule() again so we do this whole path over and over again?
I actually wonder if the right approach would be to have a getter for the voting_schedule static object that makes sure it is not out of date. So no where we should use that object directly but rather get the ref through the getter assuring us that it is updated?
There was a problem hiding this comment.
Hmmm, trying to understand this better: How can get_voting_schedule() reach voting_schedule_get_next_valid_after_time()? They seem disconnected to me.
No?
There was a problem hiding this comment.
Oh oops... my brain failed to parse that voting_schedule_get_start_of_next_interval() (in get_voting_schedule()) != voting_schedule_get_next_valid_after_time() :P...
NVM.
src/or/nodelist.c
Outdated
|
|
||
| /* We don't even have a recent consensus: this is a NOP */ | ||
| networkstatus_t *ns = networkstatus_get_live_consensus(approx_time()); | ||
| if (!ns) { |
There was a problem hiding this comment.
Seems we can't call this without a live consensus. Should we BUG() here?
There was a problem hiding this comment.
Good point. Done in aa951c7.
src/or/nodelist.h
Outdated
| @@ -29,6 +29,7 @@ const node_t *node_get_by_hex_id(const char *identity_digest, | |||
| node_t *nodelist_set_routerinfo(routerinfo_t *ri, routerinfo_t **ri_old_out); | |||
| node_t *nodelist_add_microdesc(microdesc_t *md); | |||
| void nodelist_set_consensus(networkstatus_t *ns); | |||
| void ensure_nodelist_freshness(void); | |||
There was a problem hiding this comment.
I propose we namespace this one starting with nodelist_.
There was a problem hiding this comment.
Good point. Done in aa951c7.
|
Closed on trac |
No description provided.