Opened 5 weeks ago

Closed 4 weeks ago

Last modified 8 days ago

#29806 closed defect (fixed)

Ignore bandwidth file lines with "vote=0"

Reported by: juga Owned by: teor
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-bwauth, 040-backport, 035-backport, 034-backport, merge-after-0403-alpha, authority-test-completed, sbws-bwauth-test-completed
Cc: Actual Points: 0.5
Parent ID: Points: 1
Reviewer: nickm, teor Sponsor:

Child Tickets

Change History (33)

comment:1 Changed 5 weeks ago by juga

It probably should be backported to the minimum Tor version dirauths are running.

comment:2 Changed 5 weeks ago by juga

Keywords: 035-backport added

comment:3 Changed 5 weeks ago by juga

Owner: set to juga
Status: newassigned

comment:4 Changed 5 weeks ago by juga

Status: assignedneeds_review

comment:5 Changed 5 weeks ago by juga

Status: needs_reviewneeds_revision

CI failed

comment:6 Changed 5 weeks ago by teor

Keywords: 034-backport added

Marking for backport to 0.3.4, some directory authorities are still running 0.3.4:
https://consensus-health.torproject.org/#authorityversions
(In general, we ask directory authorities to be on the two latest stable versions. At the moment, that's 0.3.4 and 0.3.5.)

Juga, please rebase your patch on 0.3.4 before review.
The parsing code changed between 0.3.4 and 0.3.5, so you may need a pull request for 0.3.4, and another pull request for 0.3.5.

comment:7 Changed 5 weeks ago by juga

Parent ID: #28563

Add parent since this might need to be reviewed with it

comment:8 Changed 5 weeks ago by juga

Status: needs_revisionneeds_review

comment:9 Changed 5 weeks ago by nickm

Reviewer: nickm

comment:10 Changed 5 weeks ago by nickm

Status: needs_reviewneeds_revision

This looks okay to me, but I would like to see a unit test to make sure that we *don't* ignore lines that say "vote=1" or "unmeasured=1" or "unmeasured=0".

comment:11 Changed 5 weeks ago by teor

Keywords: 040-backport added

comment:12 Changed 5 weeks ago by juga

Status: needs_revisionneeds_review

Added tests, modified one to remove things that were not needed and pushed in 035 the change teor suggested in 034

comment:13 Changed 5 weeks ago by nickm

lgtm.

comment:14 Changed 5 weeks ago by nickm

Status: needs_reviewmerge_ready

comment:15 Changed 5 weeks ago by teor

There is a changes file.
The spec is in #29813.
The CI passed on both pull requests, but 809 still shows travis as pending.
That seems to be a GitHub bug.

The branches have fixups, so I need to do a squash.

The branches have the same effect, but the code changes are in different files. I was concerned about merge conflicts between 0.3.4 and 0.3.5. I did a merge from the 0.3.4 branch to 0.3.5, and copied over the code changes to the new directory structure.

Here are the pull requests:

comment:16 Changed 5 weeks ago by teor

Oops, there is an extra argument in dirserv_read_measured_bandwidths() in 0.3.5.
Pushed a commit to 0.3.5 and re-did the master merge.

comment:17 Changed 5 weeks ago by nickm

Keywords: asn-merge added

comment:18 in reply to:  16 ; Changed 5 weeks ago by teor

Replying to teor:

Oops, there is an extra argument in dirserv_read_measured_bandwidths() in 0.3.5.
Pushed a commit to 0.3.5 and re-did the master merge.

There is a 4th argument to dirserv_read_measured_bandwidths() in master.

If we backport any more calls to dirserv_read_measured_bandwidths(), we're going to have to be really careful.

So we need to merge:

Edit: add 0.4.0 to the list.

Last edited 5 weeks ago by teor (previous) (diff)

comment:19 Changed 5 weeks ago by teor

asn, please don't merge this into 0.4.0 until next week: nickm is doing an alpha.

comment:20 Changed 5 weeks ago by teor

Reviewer: nickmnickm, teor

comment:21 Changed 5 weeks ago by teor

Keywords: merge-after-0403-alpha added

Tag these tickets for merge after 0.4.0.3-alpha, to make searches easier.

comment:22 Changed 4 weeks ago by teor

0.4.0.3-alpha was released on Friday, so these tickets can now be merged.

#29018 and #29806 can be merged by asn, and #29703 can be merged by teor as a backport.

comment:23 Changed 4 weeks ago by teor

Hi asn,

Here's what you need to do to merge two different pull requests to 0.4.0 and master:
(You don't want to merge to 0.3.4 or 0.3.5. That's my job, in a few months time, after a lot of testing!)

  1. Merge ​​https://github.com/torproject/tor/pull/821 to maint-0.4.0
  2. Merge ​​https://github.com/torproject/tor/pull/822 to master
  3. Merge forward

Here are the exact steps on the command-line:

$ cd ../maint-0.4.0
$ git merge tor-github/pr/821
Auto-merging src/test/test_dir.c
Auto-merging src/feature/dirauth/bwauth.c
Merge made by the 'recursive' strategy.
 changes/ticket29806          |  7 ++++++
 src/feature/dirauth/bwauth.c |  8 ++++++-
 src/test/test_dir.c          | 57 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 1 deletion(-)
 create mode 100644 changes/ticket29806
$ cd ../tor
$ git merge tor-github/pr/822
Merge made by the 'recursive' strategy.
 changes/ticket29806          |  7 ++++++
 src/feature/dirauth/bwauth.c |  8 +++++-
 src/test/test_dir.c          | 59 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100644 changes/ticket29806
$ git-merge-forward.sh
  [+] Fetching origin...success
[+] Handling branch 
release-0.2.9 Handling branch 
  [+] Switching branch to release-0.2.9...success
  [+] Merging branch origin/release-0.2.9...success
  [+] Merging branch maint-0.2.9 into release-0.2.9...success
[+] Handling branch 
maint-0.3.4 Handling branch 
  [+] Switching branch to maint-0.3.4...success
  [+] Merging branch origin/maint-0.3.4...success
  [+] Merging branch maint-0.2.9 into maint-0.3.4...success
[+] Handling branch 
release-0.3.4 Handling branch 
  [+] Switching branch to release-0.3.4...success
  [+] Merging branch origin/release-0.3.4...success
  [+] Merging branch maint-0.3.4 into release-0.3.4...success
[+] Handling branch 
maint-0.3.5 Handling branch 
  [+] Switching branch to maint-0.3.5...success
  [+] Merging branch origin/maint-0.3.5...success
  [+] Merging branch maint-0.3.4 into maint-0.3.5...success
[+] Handling branch 
release-0.3.5 Handling branch 
  [+] Switching branch to release-0.3.5...success
  [+] Merging branch origin/release-0.3.5...success
  [+] Merging branch maint-0.3.5 into release-0.3.5...success
[+] Handling branch 
maint-0.4.0 Handling branch 
  [+] Switching branch to maint-0.4.0...success
  [+] Merging branch origin/maint-0.4.0...success
  [+] Merging branch maint-0.3.5 into maint-0.4.0...success
[+] Handling branch 
release-0.4.0 Handling branch 
  [+] Switching branch to release-0.4.0...success
  [+] Merging branch origin/release-0.4.0...success
  [+] Merging branch maint-0.4.0 into release-0.4.0...success
[+] Handling branch 
master Handling branch 
  [+] Switching branch to master...success
  [+] Merging branch origin/master...success
  [+] Merging branch maint-0.4.0 into master...success

These forward merges were clean merges:

  [+] Merging branch maint-0.4.0 into release-0.4.0...success
  [+] Merging branch maint-0.4.0 into master...success

The rest of the merges did not add a merge commit, because there were no changes.

If you want to see what the merge looks like, I pushed the updated maint-0.4.0, release-0.4.0, and master to my GitHub:
https://github.com/teor2345/tor/branches

I added some instructions to the merge policy.
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/MergePolicy?action=diff&version=10

comment:24 Changed 4 weeks ago by teor

Owner: changed from juga to teor
Status: merge_readyassigned

Oops, the 0.3.5 pull request doesn't work when merged to 0.4.0:
https://ci.appveyor.com/project/teor2345/tor/builds/23353398/job/ocl4y7is4jgfbqyu#L2460
(I thought I tested that? Maybe one of the other CI failures hid it?)

I'll try again.

comment:25 Changed 4 weeks ago by teor

Status: assignedneeds_revision

comment:26 in reply to:  18 Changed 4 weeks ago by teor

Status: needs_revisionmerge_ready

Ok, all fixed now.

When I backport, I will merge:

When asn merges to mainline, he needs to merge:

The merge forward to master is a clean merge, we should check it passes CI at:
https://github.com/torproject/tor/pull/822

comment:27 Changed 4 weeks ago by teor

Keywords: consider-backport-after-0405-alpha consider-backport-after-authority-test consider-backport-after-sbws-bwauth-test added

Backport triage tags

comment:28 Changed 4 weeks ago by teor

The CI passed on PR 847 and the new PR 822.

comment:29 Changed 4 weeks ago by asn

Thanks for the instructions teor.

I pushed to 040 and master. I think I also messed up somehow and I also accidentally pushed to release-0.3.4... Perhaps it was a wrong merge-forward from yesterday that left trash? Sorry about that, I hope it doesn't cause you trouble.

I opened #29905 for some improvements to the git scripts that would help me against such PEBCAKs.

Last edited 4 weeks ago by asn (previous) (diff)

comment:30 Changed 4 weeks ago by asn

Keywords: asn-merge removed

comment:31 Changed 4 weeks ago by teor

Here is the status of the backports/merges for this ticket:

  • maint-0.3.4: not merged
  • release-0.3.4: merged tor-github/pr/820 to maint-0.3.4 as ee43bc706c, and merged forward to release-0.3.4 as f8304cc6fb
  • maint-0.3.5: not merged
  • release-0.3.5: not merged
  • maint-0.4.0: merged tor-github/pr/847 as 06951cb3fc
  • release-0.4.0: merged maint-0.4.0 forward as fa9e0963cf
  • master: merged maint-0.4.0 forward as 2790ee3685

We could revert the commit, but that's messy. (We don't want to revert the merge, because then getting the commits back in the release is hard.)

This is a 1-line change, with associated debug log, memory management, and unit tests. So we decided to backport now.

We need to make sure this change is tested on a torflow authority, and a sbws authority, before the next 0.3.4 or 0.3.5 release.

comment:32 Changed 4 weeks ago by teor

Actual Points: 0.5
Milestone: Tor: 0.4.1.x-finalTor: 0.3.4.x-final
Resolution: fixed
Status: merge_readyclosed

Merged pr/820 to maint-0.3.4 and pr/821 to maint-0.3.5, and merged forward.

comment:33 Changed 8 days ago by teor

Keywords: authority-test-completed sbws-bwauth-test-completed added; consider-backport-after-0405-alpha consider-backport-after-authority-test consider-backport-after-sbws-bwauth-test removed
Parent ID: #28563

Update tags and un-parent, because trac is fussy.

Note: See TracTickets for help on using tickets.