Opened 7 years ago

Closed 6 years ago

#4994 closed enhancement (implemented)

Be willing to use microdescs even if one bridge runs 0.2.2

Reported by: arma Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: microdescriptors tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In #4013 we fixed a big bug where clients would ask 0.2.2 bridges for microdescriptors, and then just fail. But we fixed it by falling back to normal descriptors if we ever get a hint that we have an 0.2.2 bridge. That fix is adequate for 0.2.3, but by the time we get to the 0.2.4 timeframe, 0.2.2 bridges will be rare. The fact that you fall back if you've ever seen an 0.2.2 bridge will make those clients rare and unusual (not to mention wasteful).

The better behavior, once 0.2.2 bridges are more gone, is to learn to avoid 0.2.2 bridges when asking microdescriptor requests, and only fall back to normal descriptors if none of your bridges can handle microdescriptors.

Child Tickets

Change History (11)

comment:1 Changed 7 years ago by arma

Status: newneeds_revision

See the bug4013-part2 branch in my git repository for some patches. They'll need to be rebased and deconflicted since we added another commit in the middle.

comment:2 Changed 7 years ago by arma

Status: needs_revisionneeds_review

git, you're my hero:

$ git rebase master
First, rewinding head to replay your work on top of it...
Applying: use microdescriptors if *any* of our bridges can handle them
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging src/or/microdesc.c
Auto-merging src/or/directory.c
Auto-merging src/or/circuitbuild.h
Auto-merging src/or/circuitbuild.c
Applying: generalize choose_random_entry()'s dirinfo parameter
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging src/or/directory.c
Auto-merging src/or/circuitbuild.h
Auto-merging src/or/circuitbuild.c

I added a changes file, tested it, and pushed it to my feature4994 branch.

comment:3 in reply to:  2 Changed 7 years ago by arma

Replying to arma:

git, you're my hero

You're not my hero anymore, git. You left me with a mishmash of the different branches, while mentioning no conflicts. You even left dirty dishes in the sink.

I pushed a squashed version to feature4994-try2 that makes it more robust.

We again have the oscillation issue that concerned nickm on #4013. I think it's not so bad this time though, since we only revert to normal descriptors when all our microdesc-speaking bridges fall out.

comment:4 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

There's going to be a few interesting problem here, unless there's something in your patch that I missed.

When a client starts up, which consensus will it download? It doesn't know its nodes' versions yet, so it doesn't know if any of its bridges have microdescriptors or not. It won't know until it gets some consensus and sees the "p" lines there. But if it asks for the wrong one, it needs to turn around and download the other.

Oh hey, I don't think that non-microdesc bridges will serve microdesc-consensuses, right? So we'd better restrict those downloads too. Also, if we try to download the microdesc consensus from our bridges and fail, we'll never actually learn their versions in any way that node_t will see, so we'll never learn that they don't support microdescs.

comment:5 Changed 7 years ago by nickm

Keywords: tor-client added

comment:6 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:7 Changed 6 years ago by andrea

Status: needs_revisionneeds_review

I've rebased arma's feature4994-try2 against current master in my feature4994-rebased branch; nickm, please review.

comment:8 Changed 6 years ago by nickm

Priority: normalmajor

comment:9 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

The choose_random_entry API seems a bit wrong here -- we have separate notions of "entry" and "dirguard", and we should really never be calling "choose_random_entry" when what we're going to ask for is directory information. The only thing to make sure of then that choose_random_dirguard() will return a bridge when bridges are configured.

Let me see if this makes sense to fix...

comment:10 Changed 6 years ago by nickm

Status: needs_revisionneeds_review

Totally untested fix at "feature4994-rebased" in my public repository. armadev, athena -- comments?

comment:11 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

arma and andrea like it -- merging!

Note: See TracTickets for help on using tickets.