Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6363 closed enhancement (implemented)

Make directory authorities vote on "a" lines in consensus network status documents

Reported by: ln5 Owned by: ln5
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: SponsorG20120930 023-da tor-auth
Cc: phobos, karsten Actual Points:
Parent ID: #4564 Points:
Reviewer: Sponsor:

Description


Child Tickets

TicketTypeStatusOwnerSummary
#6598enhancementclosedln5Update dir-spec with "a" lines

Change History (24)

comment:1 Changed 7 years ago by ln5

Component: - Select a componentTor Directory Authority

comment:2 Changed 7 years ago by ln5

Milestone: Sponsor G: September 30, 2012
Type: defectenhancement

comment:3 Changed 7 years ago by karsten

Keywords: SponsorG20120930 added
Milestone: Sponsor G: September 30, 2012

Switching from using milestones to keywords for sponsor deliverables. See #6365 for details.

comment:4 Changed 7 years ago by ln5

Status: newaccepted

Pushed an initial implementation of voting on and producing "a" lines
in votes and consensus. Branch bug6363 in my public repo.

No microdescriptors at this point. Please review now or later.

comment:5 Changed 7 years ago by nickm

method 13 needs to become method 14 -- 13 just got used by bug6404.

Will need microdesc support before it's done.

Does this need to be deployed and producing "a" lines for September? If so, it needs to go in 0.2.3.x, unfortunately. ("No" is the answer that would make me happie.)

The logic for setting the 3rd argument for compare_vote_rs is pretty complicated! Some comments might help me out.

The new memset() in networkstatus_compute_consensus() is redundant with #6514, already fixed.

comment:6 in reply to:  5 ; Changed 7 years ago by ln5

Replying to nickm:

method 13 needs to become method 14 -- 13 just got used by bug6404.

OK, that's now been fixed in a7716707, same branch.

Will need microdesc support before it's done.

Yes.

Does this need to be deployed and producing "a" lines for September? If so, it needs to go in 0.2.3.x, unfortunately. ("No" is the answer that would make me happie.)

External (SponsorG) deadline is September 30. Why does it have to go
in 0.2.3? Wouldn't the 0.2.4.1 (around September 7) be a good release
for this?

The logic for setting the 3rd argument for compare_vote_rs is pretty complicated! Some comments might help me out.

I'm afraid I don't understand. Do you mean that the places where we're
calling compare_vote_rs() are hard to understand? The

consensus_method >= MIN_METHOD_FOR_A_LINES

in compute_routerstatus_consensus()?

The new memset() in networkstatus_compute_consensus() is redundant with #6514, already fixed.

Fixed in e6225a33, same branch.

comment:7 in reply to:  6 ; Changed 7 years ago by nickm

Status: acceptedneeds_revision

Replying to ln5:

Replying to nickm:

method 13 needs to become method 14 -- 13 just got used by bug6404.

OK, that's now been fixed in a7716707, same branch.

Will need microdesc support before it's done.

Yes.

Does this need to be deployed and producing "a" lines for September? If so, it needs to go in 0.2.3.x, unfortunately. ("No" is the answer that would make me happie.)

External (SponsorG) deadline is September 30. Why does it have to go
in 0.2.3? Wouldn't the 0.2.4.1 (around September 7) be a good release
for this?

Because the authorities won't all upgrade to 0.2.4.x on that timeframe.

The logic for setting the 3rd argument for compare_vote_rs is pretty complicated! Some comments might help me out.

I'm afraid I don't understand. Do you mean that the places where we're
calling compare_vote_rs() are hard to understand? The

consensus_method >= MIN_METHOD_FOR_A_LINES

in compute_routerstatus_consensus()?

yes, those.

The new memset() in networkstatus_compute_consensus() is redundant with #6514, already fixed.

Fixed in e6225a33, same branch.

thanks

comment:8 in reply to:  7 Changed 7 years ago by ln5

Cc: phobos karsten added
Status: needs_revisionaccepted

Replying to nickm:

Does this need to be deployed and producing "a" lines for September? If so, it needs to go in 0.2.3.x, unfortunately. ("No" is the answer that would make me happie.)

External (SponsorG) deadline is September 30. Why does it have to go
in 0.2.3? Wouldn't the 0.2.4.1 (around September 7) be a good release
for this?

Because the authorities won't all upgrade to 0.2.4.x on that timeframe.

Oh. I see. I guess this is a question for Andrew and maybe Karsten then.

Andrew, Karsten: Is there a way to avoid having to have "clients talk
to public relays" _deployed_ by the end of September?

The logic for setting the 3rd argument for compare_vote_rs is pretty complicated! Some comments might help me out.

I'm afraid I don't understand. Do you mean that the places where we're
calling compare_vote_rs() are hard to understand? The

consensus_method >= MIN_METHOD_FOR_A_LINES

in compute_routerstatus_consensus()?

yes, those.

Comments added in 4aa76832, pushed to the same branch.

comment:9 Changed 7 years ago by phobos

What's this 'deployed' speak? We have to something in a release, not deployed.

comment:10 Changed 7 years ago by ln5

Branch task4564_bug6363_mds in my repo contains code for generating
microdescs and microdesc consensuses (150964c6).

Please let me know if this is completely crazy.

Thanks.

comment:11 Changed 7 years ago by nickm

Status: acceptedneeds_revision

I think the approach in that commit is slightly wrong. You need to generate _both_ types of microdescriptors, and include an m line for each. The first kind of microdesc (with no a line) will get chosen if the method chosen is one without a-line support; the second will get chosen if the method is one that _does_ have a-line support.

comment:12 Changed 7 years ago by ln5

<ln5> ok, this makes us always be able to produce a microdesc consensus. my
      code would make dir auths not capable of method N+1 not produce a
      microdesc consensus due to lack of signatures.  [16:32]

comment:13 Changed 7 years ago by ln5

Status: needs_revisionneeds_review

Pushed to bug6363_md_hackery_squashed (based on task4564 containing
all of my ipv6 stuff (based on master)).

Failed to merge the right things to maint-0.2.3. Will try again
tomorrow when brain hopefully works again. (Damn you, git. I love
you.)

comment:14 Changed 7 years ago by ln5

Please see bug6363_for_review in my repo.

comment:15 Changed 7 years ago by ln5

Status: needs_reviewneeds_revision

Nick found that the voting on "a" lines doesn't follow the spec.
I'll look into that tomorrow.

comment:16 Changed 7 years ago by nickm

Other than that, it mostly looked okay.

One suggestion on the table format. Right now it says:

   {MIN_METHOD_FOR_MICRODESC, MIN_METHOD_FOR_MANDATORY_MICRODESC},
   {MIN_METHOD_FOR_A_LINES, MAX_SUPPORTED_CONSENSUS_METHOD},

but that's a little off; we don't care about "MANDATORY_MICRODESC" except for the fact that it preceded MIN_METHOD_FOR_A_LINES. So maybe instead it should say:

   {MIN_METHOD_FOR_MICRODESC, MIN_METHOD_FOR_A_LINES - 1},
   {MIN_METHOD_FOR_A_LINES, MAX_SUPPORTED_CONSENSUS_METHOD},

comment:17 Changed 7 years ago by ln5

Status: needs_revisionneeds_review

I believe that the voting issue is fixed in the following commits (bug6363_5535):

1f4ba9f7 * Implement voting on "a" lines according to specification.
7d60367e * Add tor_addr_port_alloc().
c2f1a8f1 * Remove trailing semicolon from #define TOR_ADDR_NULL.

comment:18 in reply to:  16 Changed 7 years ago by ln5

Replying to nickm:

but that's a little off; we don't care about "MANDATORY_MICRODESC" except for the fact that it preceded MIN_METHOD_FOR_A_LINES. So maybe instead it should say:

Fixed in

5128cb47 * Express the end of the first microdesc consensus format more clearly.

(branch bug6363_5535).

comment:19 Changed 7 years ago by ln5

Fixed two issues found by Nick:

be54e523 | * Rename tor_addr_port_alloc().
2790248f | * Use the correct comparison function.

comment:20 Changed 7 years ago by nickm

I squashed and reordered the branch "bug6363_5535" to make "bug6363_5535_rebase_p3" in my public repository. I then extracted the bug6363 commits into a branch "bug6363_only". I'm planning to re-review and merge.

comment:21 Changed 7 years ago by ln5

See bug6363_only-ln for some small things.

comment:22 Changed 7 years ago by nickm

Keywords: 023-da added
Resolution: implemented
Status: needs_reviewclosed

They look okay to me. Merging onto master.

comment:23 Changed 7 years ago by nickm

Keywords: tor-auth added

comment:24 Changed 7 years ago by nickm

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