Opened 11 months ago

Last modified 7 months ago

#27299 new defect

hsv3: Clarify timing sources around hsv3 code

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs hsv3 refactoring easy technical-debt
Cc: Actual Points:
Parent ID: #23764 Points:
Reviewer: Sponsor:

Description

A big source of bugs and confusions (e.g. #26980, #26930) in the HSv3 code stem from the fact that it uses various timing sources to compute time periods, SRV, etc. Some parts of the code use time(NULL), others use the current consensus valid-after, and others use the voting-schedule.

The code is currently not clear in which timing source is used in each case. As an example, some functions take as input now but they only use it to get a live consensus to use its valid-after, but that may confuse a reader that the now is used as the time source (e.g. should_rotate_descriptors() that caused the #26930 confusion).

We should try to clarify and improve the function signatures around the HSv3 codebase on this regard.

Child Tickets

Change History (6)

comment:1 Changed 9 months ago by ffmancera

I am going to work on this patch. This seems to be huge because there is a lot of files using timing sources in src/features/hs. I am going to add some notes in the ticket if there is no problem.

I saw that we are using approx_time() which is an alias of time(NULL), but at the same time and in the same file, we are using both. As approx_time.h has different utilities for timing, we should use it instead of time(NULL). What do you think?

comment:2 Changed 8 months ago by teor

Parent ID: #23605
Sponsor: Sponsor8-can

I think we should use approx_time() consistently, because it saves CPU on some platforms.

Tentatively assigning to Sponsor 8.

comment:3 in reply to:  1 ; Changed 8 months ago by asn

Replying to ffmancera:

I am going to work on this patch. This seems to be huge because there is a lot of files using timing sources in src/features/hs. I am going to add some notes in the ticket if there is no problem.

Agreed that this is an abstract enough task that keeping notes will be useful.

I saw that we are using approx_time() which is an alias of time(NULL), but at the same time and in the same file, we are using both. As approx_time.h has different utilities for timing, we should use it instead of time(NULL). What do you think?

Yes, I think we should be using approx_time() instead of time(NULL) everywhere. I don't see a reason not to.

One of the problems with HS timing is that lots of the time sources come from the main event loop where time(NULL) is used.

comment:4 Changed 7 months ago by teor

Keywords: technical-debt added
Parent ID: #23605#23764
Sponsor: Sponsor8-can

We won't do this as part of sponsor 8, but we'll need this for #23764.

comment:5 in reply to:  3 ; Changed 7 months ago by ffmancera

Replying to asn:

I saw that we are using approx_time() which is an alias of time(NULL), but at the same time and in the same file, we are using both. As approx_time.h has different utilities for timing, we should use it instead of time(NULL). What do you think?

Yes, I think we should be using approx_time() instead of time(NULL) everywhere. I don't see a reason not to.

One of the problems with HS timing is that lots of the time sources come from the main event loop where time(NULL) is used.

Then we should refactor all time resources, using only approx_time() over all the code right? I have a patch for this specific task. I will do a pull request if you are fine with that :-)

Last edited 7 months ago by ffmancera (previous) (diff)

comment:6 in reply to:  5 Changed 7 months ago by asn

Replying to ffmancera:

Replying to asn:

I saw that we are using approx_time() which is an alias of time(NULL), but at the same time and in the same file, we are using both. As approx_time.h has different utilities for timing, we should use it instead of time(NULL). What do you think?

Yes, I think we should be using approx_time() instead of time(NULL) everywhere. I don't see a reason not to.

One of the problems with HS timing is that lots of the time sources come from the main event loop where time(NULL) is used.

Then we should refactor all time resources, using only approx_time() over all the code right? I have a patch for this specific task. I will do a pull request if you are fine with that :-)

You mean ALL over the codebase? Not sure how to evaluate the correctness of such a refactoring. Do you think there is a clear way to show that it does not break anything or change any behavior?

Note: See TracTickets for help on using tickets.