Opened 3 years ago

Closed 3 years ago

#18840 closed defect (fixed)

dir auths vote "package" lines out of order

Reported by: arma Owned by: arma
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: 029-nickm-says-yes, review-group-2, TorCoreTeam201605
Cc: Actual Points:
Parent ID: Points: small
Reviewer: nickm Sponsor:

Description

In proposal 227, we say

   Votes and consensuses may include any number of "package" lines,
   but no vote or consensus may include more than one "package" line
   with the same PACKAGENAME and VERSION values.  All "package"
   lines must be sorted by "PACKAGENAME VERSION", in
   lexical (strcmp) order.

but I just had moria1 set up its torrc with

RecommendedPackages shouldbesecond 0 http digest=digest
RecommendedPackages outoforder 0 http digest=digest

and sure enough, its vote now says

package shouldbesecond 0 http digest=digest
package outoforder 0 http digest=digest

I've looked through the code, and it looks like the lines do get sorted for the consensus. And maybe there is no problem with having them unsorted in the votes? In that case should we just update the proposal document so future me doesn't get confused? Or, better, should we change votes to sort them too, because it can't hurt and it might help?

Child Tickets

Change History (16)

comment:1 Changed 3 years ago by nickm

Points: small

comment:2 Changed 3 years ago by nickm

Keywords: 029-nickm-says-no added

Marking these tickets as ones I propose we do not include in 029.

comment:3 Changed 3 years ago by nickm

Keywords: 029-proposed 029-nickm-says-no removed

Nobody objected to these, so they aren't going into 029 milestone. Please re-add 029-proposed if you disagree, and let me know why.

comment:4 Changed 3 years ago by arma

Status: newneeds_review

One-line patch is in my bug18840 branch.

Tested and working on moria1.

Last edited 3 years ago by arma (previous) (diff)

comment:5 Changed 3 years ago by nickm

Keywords: 029-proposed added
Severity: NormalMinor

comment:6 Changed 3 years ago by nickm

Keywords: 029-nickm-says-yes added

comment:7 Changed 3 years ago by nickm

Keywords: review-group-2 added

Create a review-group-2 from (most of the) tickets in 0.2.8 or 0.2.9 or 029-nickm-says-yes listed as needs_review,

comment:8 Changed 3 years ago by nickm

Owner: set to arma
Status: needs_reviewassigned

Setting owner

comment:9 Changed 3 years ago by nickm

Reviewer: nickm
Status: assignedneeds_review

comment:10 Changed 3 years ago by nickm

Keywords: 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

Calling these "yes" because they are bugfixes.

comment:11 Changed 3 years ago by arma

Keywords: TorCoreTeam201605 added

comment:12 Changed 3 years ago by asn

Status: needs_reviewmerge_ready

Patch works for me. Packages in votes seem ordered correctly.

---

As an almost off-topic matter, the code managing packages for consensuses and votes seems to be of particularly low quality. See compute_consensus_package_lines() doing ad-hoc parsing. And see validate_recommended_package_line() doing more ad-hoc parsing and having wrong return value patterns. Fortunately, both of them are weakly tested in the unittests. Maybe we should do something about these functions....

comment:13 Changed 3 years ago by nickm

Hm. Think we should open another ticket for that, or try to fix it right now?

comment:14 Changed 3 years ago by asn

I think we can defer that for another ticket.

Opened #19179 for the refactoring part.

comment:15 Changed 3 years ago by nickm

Applied; thanks!

comment:16 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.