Opened 3 years ago

Closed 3 years ago

#19012 closed defect (fixed)

Refactor code that looks at voted-on parameters during voting

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-nickm-says-yes
Cc: Actual Points: 0
Parent ID: #16943 Points: 0.1
Reviewer: asn Sponsor:

Description

The prop250 logic wants this, and I think I found a bug in the way we do it now. (#18363)

I have a branch params that isn't right and needs tweaking and tests.

Child Tickets

Change History (11)

comment:1 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 3 years ago by nickm

Actual Points: 0
Points: 0
Status: acceptedneeds_review

Now my params_v2 branch is no longer sketchy.

(This took about 60 minutes.)

comment:3 Changed 3 years ago by nickm

Keywords: 029-nickm-says-yes added

comment:4 Changed 3 years ago by nickm

Points: 00.1

comment:5 Changed 3 years ago by teor

Parent ID: #16943

comment:6 Changed 3 years ago by asn

Reviewer: asn

Useful patch! Assigning myself as the reviewer.

comment:7 Changed 3 years ago by nickm

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

Adding these to 0.2.9 since they (appear to be?) dependencies of stuff that's already in the milestone.

comment:8 Changed 3 years ago by asn

As a very very initial review, the branch won't compile because the function is never used. Do we assume this will go in as part of the prop250 branch which will immediately use the func?

comment:9 Changed 3 years ago by dgoulet

I've cherry-picked the changes from nickm's branch (params_v2) and put it in the master prop250 branch along with a second commit that actually uses it (5d0656ed0).

Small downside, the commit from params_v2 doesn't compile like asn said so it's resolved with the commit just after which uses it.

comment:10 Changed 3 years ago by asn

Status: needs_reviewmerge_ready

Nice readable code with tests included. Looks good to me.

I think we can consider this ready to merge as part of the prop250 branch?

comment:11 Changed 3 years ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

It has been merged with commit id 6927467 now upstream.

Note: See TracTickets for help on using tickets.