Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#962 closed defect (fixed)

bandwidth change makes relay publish but authorities discard

Reported by: arma Owned by:
Priority: Low Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version: 0.2.0.31
Severity: Keywords:
Cc: arma, Sebastian, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by nickm)

Apr 01 09:45:39.145 [info] dirserv_add_descriptor(): Added descriptor from 'fluxe3' (source: 78.47.18.110): Valid server updated.
Apr 01 18:52:28.976 [info] dirserv_add_descriptor(): Not replacing descriptor from 'fluxe3' (source: 78.47.18.110); differences are cosmetic.

shahn> I also reloaded it at 22:52 on April 1st

what exactly did you change?

shahn> RelayBandwidthRate 150 KBytes (was at 10000 or something insanely high)
shahn> and RelayBandwidthBurst 200 KBytes (was at 15000)

see options_transition_affects_descriptor() in config.c
If

  • you changed your bandwidthrate but maxadvertisedbandwidth still
  • trumps, no need to republish.

as part of an XXX
then compare to router_differences_are_cosmetic()
any differences between these two functions will cause bugs like yours.
in the latter, it does

/* Did bandwidth change a lot? */

if ((r1->bandwidthcapacity < r2->bandwidthcapacity/2)

(r2->bandwidthcapacity < r1->bandwidthcapacity/2))

return 0;

in the former, it says "did they change at all"

what the relay should do is make a new descriptor, and then call
router-differences-are-cosmetic on it, and revert if cosmetic.
that way we're using the same function, rather than trying to keep the
checks the same in both cases.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (7)

comment:1 Changed 10 years ago by arma

shahn> But I think it's still not the real fix, which would be to actually
check if the authority liked our descriptor

once we move to microdescriptors, much of this will become moot, i think
since authorities won't need to say "he didn't publish in the last 24 hours,

screw him"

though really, they don't need to say that now either. if the authority

finds you reachable still, who cares if you didn't publish a new descriptor.
shahn> Do clients like descriptors that are older than 24 hours?
shahn> or does anyone care?

clients don't
but when we switch to microdescriptors, clients won't think about

descriptors any more.
shahn> yeah. But... it's kind of going to take a while for none of the
clients to think about them anymore

comment:2 Changed 10 years ago by Sebastian

I've been looking into this a bit more. There are quite a few differences
between the two functions, and (for the 0.2.1.x timeframe) we should
look into making the two functions more similar. Should relays dump
the check for a changed bandwithrate, calculate the new
bandwithcapacity and then decide based on a comparison with the
current descriptor? Or should we make the authorities behave more
like clients do, and thus fix older clients without making them upgrade?

Should we focus less on these issues, and make sure that a client
notices when its descriptor wasn't accepted, and republishes sooner?
(This seems to be more appealing, somehow, because this means that
when we decide to change stuff in 0.2.2.x on the authority side, we don't
re-introduce the problems here.) We could then leave everything as-is
for now, and make sure that 0.2.2.x uses the same logic.

Also, I've been wondering about our use of Bandwithcapacity. It seems
that the whole concept breaks down when someone lowers their
bandwithrate significantly - the history up until the sighup occurred is
meaningless.

comment:3 Changed 10 years ago by Sebastian

armadev: so here's the change to make authorities way more lenient.

This will mean even if the clients don't upgrade now, they'll not fall out
of the consensus.

http://sebastianhahn.net/tor/0001-Directory-authorities-should-accept-a-descriptor-as.patch
not checking bandwithrate in options_transition_affects_descriptor

actually makes sense. Currently, the bandwithcapacity has no reason to
change just because some options changed.

Putting a check there wouldn't change a thing
I think in the future we don't actually want to get rid of

options_transition_affects_descriptor. We want to get rid of all the parts
that the descriptor comparison function has, and then call both.

comment:4 Changed 10 years ago by arma

Added the temporary fix in r19259 and r19261. The plan for 0.2.2.x should be
to teach relays to recognize when their descriptor was dropped, so they can
retry sooner.

comment:5 Changed 9 years ago by nickm

Description: modified (diff)
Milestone: post 0.2.1.xTor: 0.2.3.x-final

comment:6 Changed 7 years ago by nickm

Resolution: Nonefixed
Status: newclosed

The "temporary fix" permanent. We also already have code to make relays retry when their descriptor is fixed. This is an ex-bug!

comment:7 Changed 7 years ago by nickm

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