Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3263 closed defect (fixed)

only publish new descriptor if onion key has changed

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We call set_onion_key() whenever we call init_keys(). set_onion_key() calls mark_my_descriptor_dirty() which triggers a new descriptor publish.

The problem is that we call init_keys() in some cases where our new descriptor won't be any different (for example, when our SafeLogging config option has changed).

The fix is to only republish a descriptor when the onion key actually changes.

Child Tickets

Change History (6)

comment:1 Changed 8 years ago by arma

Status: newneeds_review

See my bug3263 branch.

In particular, it needs review of:

A) Should I be doing the key comparison inside the mutex? I think no.

B) Are there other things that init_keys() does that *do* want a new descriptor to be uploaded? I worry we're swinging the pendulum the other way and uncovering bugs that were masked by the "always republish" bug.

Issue "B" is why I've targeted this bugfix on 0.2.3.x.

comment:2 Changed 8 years ago by arma

(See also #1810)

comment:3 in reply to:  1 Changed 8 years ago by nickm

Replying to arma:

See my bug3263 branch.

In particular, it needs review of:

A) Should I be doing the key comparison inside the mutex? I think no.

Indeed no. set_onion_key is only called from the main thread, and is the only thing that can change onionkey.

B) Are there other things that init_keys() does that *do* want a new descriptor to be uploaded? I worry we're swinging the pendulum the other way and uncovering bugs that were masked by the "always republish" bug.

I had a quick look through the code, and the only iffy case I can find is that we call init_keys if options_transition_affects_workers() detects a change in public_server_mode, whereas option_transition_affects_descriptors() doesn't check public_server_mode. If you think that's safe, let's merge this. If you want to add a check for public_server_mode() transition, that's fine too.

comment:4 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Tweaked as described above and merged into master.

comment:5 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:6 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.