Opened 3 years ago

Closed 3 years ago

#22735 closed task (fixed)

prop224: HS desc overlap period func uses absolute times instead of slots

Reported by: asn Owned by: asn
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224 tor-hs spec-conformance
Cc: Actual Points:
Parent ID: #20657 Points: 1
Reviewer: dgoulet Sponsor: SponsorR-can

Description (last modified by asn)

The function that decides whether we are in HS desc overlap mode currently uses the following logic:

  tor_gmtime_r(&consensus->valid_after, &valid_after_tm);
  if (valid_after_tm.tm_hour > 0 && valid_after_tm.tm_hour < 12) {
    return 1;

While that logic is accurate wrt prop224 section 2.2.4, it's actually not a good idea since it uses absolute times, and it will fail to work as intended in the testnet.

We should refactor that logic to use the slot based system that we use for time periods and shared randomness, since semantically the HS desc overlap period is just the period between publishing a fresh SRV and starting the new time period.

We should also change the spec to reflect the new logic.

Child Tickets

Change History (13)

comment:1 Changed 3 years ago by asn

Description: modified (diff)
Keywords: tor-hs added

comment:2 Changed 3 years ago by dgoulet

Owner: set to asn
Status: newassigned

comment:3 Changed 3 years ago by dgoulet

Milestone: Tor: 0.3.2.x-finalTor: unspecified

We can't make those for 032 so for now they go in Unspecified.

comment:4 Changed 3 years ago by dgoulet

Parent ID: #21888

Also, this is not prop224 groundwork.

comment:5 Changed 3 years ago by nickm

Keywords: spec-conformance added
Priority: MediumHigh

comment:6 Changed 3 years ago by asn

Parent ID: #20657

comment:7 Changed 3 years ago by asn

Keywords: prop224-extra removed

Removing prop224-extra. This needs to happen since it really influences our testing abilities. We can't really accurately test our overlap logic if chutney needs 12 hours to go to overlap mode.

comment:8 Changed 3 years ago by asn

Another problem here is that our current time period function hs_get_time_period_num() also uses absolute numbers instead of a slot system, which means it does not work exactly as intended in chutney (time periods rotate too slow).

Will probably have to fix this first before fixing the root issue here.

comment:9 Changed 3 years ago by asn

Status: assignedneeds_review

Please check branch bug22735 in my repo for a fix to the original issue of this ticket, plus comment:8.

I made a gitlab MR here:

There is also a torspec branch that you can find at bug22735 in my torspec repo:

David, you probably want to merge this into your #20657 branch.

comment:10 Changed 3 years ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

Review done in oniongit.

I think we could wait until #20657 is merged then rebase it on master and submit it for upstream? I fear adding 6 more commits on top of #20657 could just make its merge longer?

comment:11 Changed 3 years ago by dgoulet

Scratch the above, once the branch is fixed, I'll merge it in #20657.

comment:12 Changed 3 years ago by asn

Status: needs_revisionneeds_review

Pushed branch with revisions in bug22735_v2. No gitlab MR yet, since it's simple enough. Let me know if you want one.

comment:13 Changed 3 years ago by dgoulet

Resolution: fixed
Status: needs_reviewclosed

lgtm! I've cherry-picked all the commits and merge them in #20657 branch.

Note: See TracTickets for help on using tickets.