Opened 3 years ago

Closed 3 years ago

#21641 closed defect (implemented)

Prop274: Rotate onion keys less frequently

Reported by: nickm Owned by: ahf
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201703
Cc: Actual Points: 2.1
Parent ID: Points: 2
Reviewer: nickm Sponsor: Sponsor4


Let's implement proposal 274: This proposal will save a fair amount of bandwidth now, and even more once we have consensus diffs.

Child Tickets

Change History (7)

comment:1 Changed 3 years ago by nickm

Owner: set to ahf
Status: newassigned

comment:2 Changed 3 years ago by nickm

Keywords: TorCoreTeam201703 added

comment:3 Changed 3 years ago by ahf

Status: assignedaccepted

comment:4 Changed 3 years ago by ahf

Actual Points: 2.1
Status: acceptedneeds_review

I've added a patch set that is ready for review in my Gitlab branch 'bugs/21641':

comment:5 Changed 3 years ago by nickm

Reviewer: nickm
Status: needs_reviewneeds_revision

Looks good!


  • When a time interval (like MIN/MAX/DEFAULT_ONION_KEY_LIFETIME) has a unit other than seconds, put the unit in the name. (eg, "MIN_ONION_KEY_LIFETIME_DAYS"). Otherwise people tend to assume seconds. Same for DEFAULT/MIN onion_key_grace_period.
  • Maybe we should check whether the onion key is expired more frequently than the get_onion_key_lifetime() interval? -- otherwise, if the interval changes in the consensus, we won't reach the interior of rotate_onion_key_callback(). Same for the other callback.
  • The documentation for expire_old_onion_keys should make it clear that the function doesn't perform a grace period check.
  • This branch needs a changes file.

(Also please remember when you make these changes, fixup commits are easier to review than a completely new or rebased branch.)

comment:6 Changed 3 years ago by ahf

Status: needs_revisionneeds_review

I've updated my branch with a set of fixup commits and one additional "real" commit.

comment:7 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

LGTM; Squashed and merged. This should begin having a modest effect on directory bandwidth immediately, which will grow as more relays upgrade. We should keep an eye on upgraded relays, of course, to make sure that they don't have any weird side effects from the increased interval.

Note: See TracTickets for help on using tickets.