Opened 2 years ago

Closed 2 years ago

#23623 closed enhancement (implemented)

hs: Cache current time period number and SRV start time

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, tor-sr, prop224
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:

Description

For each node in the consensus, we call node_set_hsdir_index() which gets the time period and srv start time to learn if we are in between TP and SRV or not.

These calls ultimately call get_voting_schedule() which allocates memory and prints a debug log everytime.

All of this is actually pretty heavy and they are values we can cache because they are all based on the timings of a new consensus (valid_after, valid_until, fresh_until).

It is also called every second by a tor with a hidden service.

Caching those and updating when we get a new consensus seems a light refactoring for a good performance improvement.

Child Tickets

Change History (16)

comment:1 Changed 2 years ago by dgoulet

Keywords: prop224 added

comment:2 Changed 2 years ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

comment:3 Changed 2 years ago by dgoulet

Reviewer: asn
Status: acceptedneeds_review

It was actually more straight forward than I thought.

Instead of keeping a star time of current round and time period and what not, we actually don't need to do such a thing but rather use the voting schedule static object instead. It is updated everytime we get a new consensus so using it is as close as we get to the latest.

Thus, now the SR subsystem only uses that object to compute it stuff.

Unit test is broken because we have an issue of timestamp being off because one function used to take a timestamp "now" but now it only uses the voting schedule timings so it needs to be fixed. However, I want to know if the approach seems sensible before going further.

See branch: ticket23623_032_01

comment:4 Changed 2 years ago by asn

Hmm, seems like we will only recompute voting schedule if we are a dirauth with this patch
because of how dirvote_recalculate_timing() works.

We should probably change that, and document the new voting schedule setting behavior in static voting_schedule_t voting_schedule;.

comment:5 Changed 2 years ago by asn

Status: needs_reviewneeds_revision

comment:6 Changed 2 years ago by dgoulet

See extra commit in branch ticket23623_032_01.

I have still yet to fix the tests. Will get on that soon.

comment:7 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Ok tests fixed! That was a bit tricky. Everytime we mock a consensus, we have to recalculate its voting schedule for this kind of test now.

See branch: ticket23623_032_01

comment:8 Changed 2 years ago by asn

Status: needs_reviewmerge_ready

LGTM. Did some tests connecting to hsv3s and it seems to work well.

comment:9 Changed 2 years ago by nickm

Why is the test_get_next_valid_after_time test removed? It's a little worrisome to remove unit tests. Do the same tests now get performed in some other way?

comment:10 Changed 2 years ago by dgoulet

Before that patch, the function get_next_valid_after_time(time_t now), which was renamed in this patch to dirvote_get_next_valid_after_time(void), was allocating a voting schedule using now and returning the voting_schedule.interval_starts of that newly created schedule.

We now use a static voting schedule and update it with dirvote_recalculate_timing(), the test became pointless because that function now only returns interval_starts which is from the static voting schedule. Thus it is not needed as the test have been modified to use a static object and update it as it goes.

comment:11 Changed 2 years ago by nickm

okay, that makes sense. I'm going to try merging this and looking at the test coverage.

comment:12 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Coverage looks fine. Merging forward and pushing!

comment:13 Changed 2 years ago by nickm

Resolution: implemented
Status: closedreopened

ug, wait, I spoke too soon. Not merged yet -- could you please write a changes file?

comment:14 Changed 2 years ago by nickm

Status: reopenedneeds_revision

comment:15 Changed 2 years ago by asn

Status: needs_revisionmerge_ready

Oops sorry about that. Pushed changes file in `ticket23623_032_01` in my repo.

comment:16 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to 0.3.2 and forward!

Note: See TracTickets for help on using tickets.