PaddingStatistics should be disabled when ExtraInfoStatistics is 0

The man page entry for PaddingStatistics says that it is disabled when ExtraInfoStatistics is 0. But that isn't the case: the statistics are published regardless of ExtraInfoStatistics.

As far as I can tell, PaddingStatistics are collected on bridges, but the man page says "Relays only."

changed milestone to %Tor: 0.3.4.x-final

added 033-backport-unreached 034-backport 035-backport 040-can actualpoints::1.3 component::core tor/tor consider-backport-after-0404 dgoulet-merge fast-fix milestone::Tor: 0.3.4.x-final network-team-roadmap-2019-Q1Q2 owner::teor points::0.2 postfreeze-ok priority::low resolution::fixed reviewer::nickm severity::normal sponsor::V-can status::closed type::defect version::tor 0.3.1.1-alpha labels

See https://github.com/torproject/tor/pull/638

Trac:
Actualpoints: N/A to 0.2
Status: assigned to needs_review

Improving the reliability and usability of statistics can fall under Sponsor V.

Trac:
Sponsor: N/A to SponsorV-can

Trac:
Reviewer: N/A to nickm

This looks fine to me. A quick question before I merge: I see that coverage has fallen here. Do you think you would time to add a test for this stuff?

Trac:
Status: needs_review to merge_ready

I tried to start testing router_build_fresh_descriptor(), but it was a huge function that looked really hard to test. So I refactored it into smaller functions.

Now I've done the refactor, I can test extrainfo_dump_to_string() using just router_build_fresh_unsigned_routerinfo() and router_build_fresh_unsigned_extrainfo(). But router_build_fresh_unsigned_routerinfo() is also large, so I might re-use some of the existing fake test routerinfos.

In the interim, feel free to review my work in progress refactor in https://github.com/torproject/tor/pull/638

I don't expect that we'll backport this refactor at all: it ended up being much larger than I expected. So maybe we'll just have the tests on master.

Trac:
Actualpoints: 0.2 to 1.1
Status: merge_ready to needs_review

I also opened #29053 (moved) to follow-up some other issues that make this code hard to test. But I doubt we'll have time for it in Sponsor V.

Mark some tickets as postfreeze-ok, to indicate that I think they are okay to accept in 0.4.0 post-freeze. Does not indicate that they are all necessary to do postfreeze.

Trac:
Keywords: N/A deleted, postfreeze-ok added

I like the refactoring here; it's looking good.

I agree that it's fine (and wise) to have the tests and refactoring only on master, and backport the original commit only.

Putting this into needs_revision since I don't know if Teor considers the branch done. Teor, when you're finished, please put it into needs_review and I'll have a last look?

Trac:
Status: needs_review to needs_revision

I force-pushed to this pull request, so it only contains the backport to 0.3.3:

• https://github.com/torproject/tor/pull/638

I want to deal with the refactor and the extra unit tests along with the changes in #29018 (moved). They're more complicated, and they won't be backported.

Trac:
Status: needs_revision to merge_ready
Actualpoints: 1.1 to 1.3

Oops, this should have gone in needs_review.

Trac:
Status: merge_ready to needs_review

Bug triage of 0.4.0 tickets. These are now in the "CAN" section. Lower priority than "040-must".

Trac:
Priority: Medium to Low
Keywords: N/A deleted, 040-can added

This seems okay for 0.4.0. I'm not 100% sure on backporting any farther, but I wouldn't object much.

Trac:
Status: needs_review to merge_ready

Replying to nickm:

This seems okay for 0.4.0. I'm not 100% sure on backporting any farther, but I wouldn't object much.

My feeling is that getting it in 0.3.5 would be nice. Putting it in 0.3.4 seems less important, and 0.3.3 is not supported in 3 days' time.

I'll check it against our backport policy when I do my next backport run.

Trac:
Keywords: N/A deleted, dgoulet-merge added

Merged to 040 and master. Moving this to 035 Milestone for backport decision.

Trac:
Milestone: Tor: 0.4.0.x-final to Tor: 0.3.5.x-final

These merge_ready tickets did not get backported to 0.3.3, and 0.3.3 is now unsupported.

Trac:
Keywords: 033-backport deleted, 033-backport-unreached added

Trac:
Keywords: N/A deleted, network-team-roadmap-2019-Q1Q2 added

These tickets are low-risk changes that were marked for backport after the release of 0.4.0.2-alpha. I want them to get testing in at least one alpha release, so I'll look at them after 0.4.0.4-alpha is released.

Trac:
Keywords: N/A deleted, consider-backport-after-0404-alpha added

Drop the -alpha from backport tags

Trac:
Keywords: consider-backport-after-0404-alpha deleted, consider-backport-after-0404 added

Merged to 0.3.4 and merged forward.

Merged #23790 (moved), #29665 (moved), #29017 (moved), #27199 (moved), #29144 (moved), #13221 (moved), #28698 (moved).

Trac:
Milestone: Tor: 0.3.5.x-final to Tor: 0.3.4.x-final

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

Replying to teor:

As far as I can tell, PaddingStatistics are collected on bridges, but the man page says "Relays only."

For the historical record: bridge relays are a kind of relay, so "relays only" includes bridges.

(If we want to change this terminology, that's fine, but I bet there will be plenty more places, from way back, where it's used this way.)

closed

changed time estimate to 1h 36m

added 10h 24m of time spent

mentioned in issue #29018 (moved)

mentioned in issue #29053 (moved)

mentioned in issue #29144 (moved)

mentioned in issue #29665 (moved)

moved to tpo/core/tor#29017 (closed)

mentioned in issue tpo/core/tor#29018 (closed)