Opened 10 months ago

Closed 5 months ago

#24378 closed enhancement (implemented)

Prune the list of supported consensus methods

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop290, 034-triage-20180328, fast-fix
Cc: Actual Points:
Parent ID: Points: .5
Reviewer: teor Sponsor:

Description

We currently have 13 supported consensus methods.

In 0.3.3, it is likely that prop282 will add 1 more, and prop283 will add 2 more.

Maybe we should prune this list eventually, because it will let us simplify our code, and make votes smaller, less expensive to calculate, and reduce authority RAM requirements (due to fewer microdescs).

It has almost no impact on consensus size.

Here's how we could work out what to prune:

By mandatory feature:

We are currently locked into using consensus method 16 or later in the public network, because 0.2.9 and later require ntor keys, and 0.2.9 clients use microdescriptors by default.

We may add more mandatory features in 0.3.3 and 0.3.4.

By supported tor version:

On May 1, 2018, we will stop supporting 0.2.5, and only support 0.2.9 and later. This means that all supported non-alpha versions will support consensus methods 25 and later. (Or, if we count 0.2.9 alpha versions, it's 22 and later.)

Child Tickets

Change History (12)

comment:1 Changed 8 months ago by teor

See #10163, for how we did this last time.

comment:2 Changed 6 months ago by nickm

Keywords: 034-triage-20180328 added

comment:3 Changed 6 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:4 Changed 6 months ago by nickm

Keywords: prop290 fast-fix added; needs-proposal 034-removed-20180328 removed
Owner: set to nickm
Points: 2.5
Status: newaccepted

comment:5 Changed 6 months ago by nickm

Status: acceptedneeds_review

See my branch remove_old_consensus_methods_2018, available for review at https://github.com/torproject/tor/pull/45

To note:

  • There are a few places where I changed if (x) { to { because I felt that the block structure kept the code more organized.

comment:6 Changed 5 months ago by asn

Reviewer: isis

comment:7 Changed 5 months ago by teor

Reviewer: isisisis, teor

Looks good to me, I left a review on github with two nitpick changes:

  • fix some missing words in a commit message
  • please don't remove a unit test for a far future consensus method

We will also need a dir-spec change like this one:

[ As of 0.2.6.1-alpha, authorities no longer advertise or negotiate
          any consensus methods lower than 13. ]

I'm not sure if isis also wants to review this code.

Let's also make sure someone has run "make test-network-all" on this branch on an IPv6/mixed system, with tor-stable linked to 0.2.9.

comment:8 Changed 5 months ago by nickm

Added some fixup/squash commits to resolve your concerns above.

comment:9 Changed 5 months ago by nickm

Also see branch bug24378_spec in my torspec repo for the requested dir-spec changes.

comment:10 in reply to:  7 Changed 5 months ago by isis

Reviewer: isis, teorteor

Replying to teor:

I'm not sure if isis also wants to review this code.


Nope, your review looks good! Thanks, teor! I'll take review on #25818 in place of this.

comment:11 Changed 5 months ago by nickm

Status: needs_reviewmerge_ready

comment:12 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Both merged to 'master' and/or squashed as appropriate.

Note: See TracTickets for help on using tickets.