Make all statistics depend on ExtraInfoStatistics

[teor]

Like #29017, when a user sets ExtraInfoStatistics 0, they probably don't want any statistics in their extra-info document.

[teor]

See my pull request ​https://github.com/torproject/tor/pull/639

Notes:

GeoIP file hashes might not be sensitive, but the other statistics are reasonably sensitive, particularly for private bridges.

ServerTransportPlugin lines aren't strictly statistics, but they are still potentially sensitive.

We turn off statistics in the extra-info file if we ever fail to parse the file. So we should really include all optional chunks under the write_stats_to_extrainfo check.

[teor]

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

[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?

[teor]

Replying 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?

I could tweak the existing tests to run with ExtraInfoStatistics 0 and 1. That should improve coverage compared to master.

[teor]

Replying to teor:

Replying 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?

I could tweak the existing tests to run with ExtraInfoStatistics 0 and 1. That should improve coverage compared to master.

Actually, there are no existing tests that build an extrainfo document. Instead, the existing tests mock the relevant functions, replacing them with static descriptor strings.

These static descriptor strings are also out of date. I opened #29521 to address this technical debt, because I don't have time to do it in Sponsor V.

[teor]

Branch

I tried to merge the changes from 0.3.3 forward to master, but the merge was really complicated.

So I opened a new pull request ​https://github.com/torproject/tor/pull/709
It contains the:

• merged changes from #29017
  • based on 0.3.3, to keep the same commit as #29017
• rebased refactor from #29017
  • because the merge was complex
• rebased commits from this ticket ( ​https://github.com/torproject/tor/pull/639 )
  • because the merge was complex
• new unit tests for #29017 and #29018

Tests

The code passes make test-network-all with ExtraInfoStatistics 1 and ExtraInfoStatistics 0 set in chutney's common.i.

I discovered some bugs on my local machine while writing this code.
I don't know if they'll affect CI:

• #29529 util/map_anon_nofork test fails on macOS
• #29527 Division by zero: undefined behaviour in circuitpadding/circuitpadding_sample_distribution test
• #29530 Some address/get_if_addrs* tests fail when the network is unreachable

TODO

I think there might be some memory leaks in the new unit tests, I'm not sure if I caught them all. I'll check in the morning once CI has finished.

I think the coverage should have increased a bit compared with #29017 and master. But I'll need to wait for CI to make sure.

[teor]

Replying to teor:

I think there might be some memory leaks in the new unit tests, I'm not sure if I caught them all. I'll check in the morning once CI has finished.

I found and fixup'd one memory leak. Hopefully it's the last one. (I need to install a newer version of clang to get good leak detection locally.)

I think the coverage should have increased a bit compared with #29017 and master. But I'll need to wait for CI to make sure.

Coveralls stopped working about 20 hours ago. Maybe it's back now?

[nickm]

Deferring 51 tickets from 0.4.0.x-final. Tagging them with 040-deferred-20190220 for visibility. These are the tickets that did not get 040-must, 040-can, or tor-ci.

[teor]

This task is on the 0.4.1 roadmap as part of Sponsor V.

[teor]

The coveralls bot is back:
​https://github.com/torproject/tor/pull/709#issuecomment-465408842

These unit tests increase the overall coverage by 0.2%.

I looked at the uncovered lines: many of them are error cases or mocking. Some of them are from other pull requests.

I decided not to unit test the new router_build_fresh_unsigned_routerinfo() function: it requires too much state. (See #29521.)

[nickm]

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

[nickm]

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.

Oops, I meant to say that on another ticket.

[nickm]

LGTM. This can go in after #29017 has been merged.

[dgoulet]

Replying to nickm:

LGTM. This can go in after #29017 has been merged.

Is this ACK for the PR 639 or 709. They are MASSIVELY different and the 709 one is huge. I just want to make sure what was reviewed.

[nickm]

I'm acking 709, but only once 683 is merged as a part of #29017.

[teor]

Replying to nickm:

I'm acking 709, but only once 683 is merged as a part of #29017.

#29017 has been merged to master.
This ticket won't be backported, so we can merge now.

[teor]

Oh right, asn has to merge this ticket.

[teor]

Fix keywords.

[teor]

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

[teor]

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

[teor]

This ticket is a large refactor, it's not going to be backported to 0.4.0.

[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.

[asn]

Pushed in master. comment:23 indicates that no 040 backport is needed.
Closing this.