Make all statistics depend on ExtraInfoStatistics
Like #29017 (moved), when a user sets ExtraInfoStatistics 0, they probably don't want any statistics in their extra-info document.
- Show closed items
Activity
-
Newest first Oldest first
-
Show all activity Show comments only Show history only
- teor changed milestone to %Tor: 0.4.1.x-final
changed milestone to %Tor: 0.4.1.x-final
- teor added 040-deferred-20190220 actualpoints::2 asn-merge component::core tor/tor dgoulet-merge merge-after-0403-alpha milestone::Tor: 0.4.1.x-final network-team-roadmap-2019-Q1Q2 owner::teor points::0.1 priority::medium privcount resolution::fixed reviewer::nickm security-low severity::normal sponsor::V-can status::closed type::enhancement labels
added 040-deferred-20190220 actualpoints::2 asn-merge component::core tor/tor dgoulet-merge merge-after-0403-alpha milestone::Tor: 0.4.1.x-final network-team-roadmap-2019-Q1Q2 owner::teor points::0.1 priority::medium privcount resolution::fixed reviewer::nickm security-low severity::normal sponsor::V-can status::closed type::enhancement labels
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.
Trac:
Status: assigned to needs_reviewTrac:
Reviewer: N/A to nickmThis 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_readyReplying 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.
Trac:
Status: merge_ready to needs_revisionReplying 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 (moved) to address this technical debt, because I don't have time to do it in Sponsor V.
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 (moved)
- based on 0.3.3, to keep the same commit as #29017 (moved)
- rebased refactor from #29017 (moved)
- 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 (moved) and #29018 (moved)
Tests
The code passes
make test-network-all
withExtraInfoStatistics 1
andExtraInfoStatistics 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 (moved) util/map_anon_nofork test fails on macOS
- #29527 (moved) Division by zero: undefined behaviour in circuitpadding/circuitpadding_sample_distribution test
- #29530 (moved) 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 (moved) and master. But I'll need to wait for CI to make sure.
Trac:
Actualpoints: 0.1 to 2
Status: needs_revision to needs_review- merged changes from #29017 (moved)
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 (moved) 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?
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.
Trac:
Keywords: N/A deleted, 040-deferred-20190220 added
Milestone: Tor: 0.4.0.x-final to Tor: unspecifiedThe 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 (moved).)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_readyReplying 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.
Trac:
Status: merge_ready to needs_reviewLGTM. This can go in after #29017 (moved) has been merged.
Trac:
Keywords: N/A deleted, dgoulet-merge added
Status: needs_review to merge_readyReplying to nickm:
LGTM. This can go in after #29017 (moved) 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.
I'm acking 709, but only once 683 is merged as a part of #29017 (moved).
Replying to nickm:
I'm acking 709, but only once 683 is merged as a part of #29017 (moved).
#29017 (moved) has been merged to master. This ticket won't be backported, so we can merge now.
0.4.0.3-alpha was released on Friday, so these tickets can now be merged.
#29018 (moved) and #29806 (moved) can be merged by asn, and #29703 (moved) can be merged by teor as a backport.
Pushed in master. comment:23 indicates that no 040 backport is needed. Closing this.
Trac:
Resolution: N/A to fixed
Status: merge_ready to closed- Trac closed
closed
- Trac changed time estimate to 48m
changed time estimate to 48m
- Trac added 16h of time spent
added 16h of time spent
- teor mentioned in issue #29019 (moved)
mentioned in issue #29019 (moved)
- teor mentioned in issue #29703 (moved)
mentioned in issue #29703 (moved)
- teor mentioned in issue #29806 (moved)
mentioned in issue #29806 (moved)
- teor mentioned in issue #30441 (moved)
mentioned in issue #30441 (moved)
- teor mentioned in issue #30956 (moved)
mentioned in issue #30956 (moved)
- Trac moved to tpo/core/tor#29018 (closed)
moved to tpo/core/tor#29018 (closed)
- Trac mentioned in issue tpo/core/tor#30956 (closed)
mentioned in issue tpo/core/tor#30956 (closed)