Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket27247 #310

Merged
merged 2 commits into from Sep 14, 2018
Merged

Ticket27247 #310

merged 2 commits into from Sep 14, 2018

Conversation

Labels
None yet
Projects
None yet
4 participants
@nmathewson
Copy link
Contributor

@nmathewson nmathewson commented Sep 7, 2018

No description provided.

nmathewson added 2 commits Sep 7, 2018
We already had fallback code for "dir/status-vote/current/consensus"
to read from disk if we didn't have a cached_dir_t available.  But
there's a function in networkstatus_t that does it for us, so let's
do that.
There are three reasons we use a cached_dir_t to hold a consensus:
  1. to serve that consensus to a client
  2. to apply a consensus diff to an existing consensus
  3. to send the consensus to a controller.

But case 1 is dircache-only.  Case 2 and case 3 both fall back to
networkstatus_read_cached_consensus().  So there's no reason for us
to store this as a client.  Avoiding this saves about 23% of our RAM
usage, according to our experiments last month.

This is, semantically, a partial revert of e5c608e.

Fixes bug 27247; bugfix on 0.3.0.1-alpha.
@coveralls
Copy link

@coveralls coveralls commented Sep 8, 2018

Coverage Status

Coverage decreased (-0.0009%) to 59.831% when pulling 33a0c61 on nmathewson:ticket27247 into 9f0e8d8 on torproject:master.

Copy link
Contributor

@teor2345 teor2345 left a comment

Hi, this branch looks good, and I think it's fine to revert parts of e5c608e - clients shouldn't need to cache consensuses in RAM, because they'll never serve them. I also checked that routerstatus_format_entry() only uses parsed descriptors and routerstatuses.

I would happily merge this as-is, but I have some questions.

Microdesc Consensus GETINFO

Is there a microdesc-consensus equivalent for GETINFO dir/status-vote/current/consensus?
(I couldn't find one.)

Because this branch drops all flavours of cached consensus, but only refactors the GETINFO code for the ns consensus.

Consensus Request Headers

When we are setting the consensus request headers, we use the parsed consensus, then fall back to the RAM cached consensus:

if (flav != -1) {
/* IF we have a parsed consensus of this type, we can do an
* if-modified-time based on it. */
networkstatus_t *v;
v = networkstatus_get_latest_consensus_by_flavor(flav);
if (v) {
/* In networks with particularly short V3AuthVotingIntervals,
* ask for the consensus if it's been modified since half the
* V3AuthVotingInterval of the most recent consensus. */
time_t ims_delay = DEFAULT_IF_MODIFIED_SINCE_DELAY;
if (v->fresh_until > v->valid_after
&& ims_delay > (v->fresh_until - v->valid_after)/2) {
ims_delay = (v->fresh_until - v->valid_after)/2;
}
if_modified_since = v->valid_after + ims_delay;
if (v->valid_after >= approx_time() - max_age_for_diff) {
memcpy(or_diff_from, v->digest_sha3_as_signed, DIGEST256_LEN);
or_diff_from_is_set = 1;
}
}
} else {
/* Otherwise it might be a consensus we don't parse, but which we
* do cache. Look at the cached copy, perhaps. */
cached_dir_t *cd = dirserv_get_consensus(resource);
/* We have no method of determining the voting interval from an
* unparsed consensus, so we use the default. */
if (cd) {
if_modified_since = cd->published + DEFAULT_IF_MODIFIED_SINCE_DELAY;
if (cd->published >= approx_time() - max_age_for_diff) {
memcpy(or_diff_from, cd->digest_sha3_as_signed, DIGEST256_LEN);
or_diff_from_is_set = 1;
}
}
}

Should we add a further fall back to load and parse the consensus file before every consensus request? The fallback would only happen on clients with FetchUselessDescriptors 1.

If we do add this fallback, it will save bandwidth, but cost CPU and RAM.

(How many clients set FetchUselessDescriptors 1? Are there a significant number? If so, we should try to save this bandwidth. If not, it probably doesn't matter.)

Or is there some sensible default published time we could use for a consensus file?

@@ -2341,9 +2341,7 @@ getinfo_helper_dir(control_connection_t *control_conn,
*answer = tor_strdup(consensus->dir);
Copy link
Contributor

@teor2345 teor2345 Sep 14, 2018

We could add dir_server_mode() to the conditions in if (we_want_to_fetch_flavor(get_options(), FLAV_NS)) {.

But I think the current code works as-is, and it is is more future-proof.

@torproject-pusher torproject-pusher merged commit 33a0c61 into torproject:master Sep 14, 2018
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment