Opened 4 months ago

Closed 3 months ago

#30173 closed enhancement (fixed)

Ensure circuit padding can be safely disabled from consensus

Reported by: mikeperry Owned by:
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: wtf-pad, tor-relay, tor-cell, padding, 041-proposed
Cc: Actual Points: 0.33
Parent ID: #28634 Points: 1
Reviewer: asn Sponsor: Sponsor2-can

Description

Right now, to "disable" circuit padding, we have to set consensus parameters circpad_global_allowed_cells=1 and circpad_global_max_padding_pct=1, which actually only sets a limit of 1% of our traffic to be padding. Additionally, each padding cell that is *not* sent because this limit is reached causes an info log line to be written.

We need another way to fully disable padding, for A/B performance testing. And we should rate limit that info log so relays logging at info don't print a line for every single padding cell they don't send.

Child Tickets

Change History (17)

comment:1 Changed 4 months ago by nickm

Milestone: Tor: 0.4.1.x-final

comment:2 Changed 4 months ago by mikeperry

Status: newneeds_review

https://github.com/torproject/tor/pull/965

That PR contains fixes for #28693, #30173, and #29203. I recommend that the same reviewer review all three tickets -- they are extremely similar and related code (hence all in the same branch with the same test; doing these separetely would involve needless extra mental context switching and extra communication).

comment:3 Changed 4 months ago by mikeperry

Actual Points: 0.33

comment:4 Changed 4 months ago by teor

Status: needs_reviewneeds_revision

Both Travis and Appveyor CIs failed on https://github.com/torproject/tor/pull/965

comment:5 Changed 4 months ago by teor

These tickets all have code in the same pull request, so I'm assuming they're all 0.4.1.

comment:6 Changed 4 months ago by mikeperry

Status: needs_revisionneeds_review

Ok the CI issues were a couple test-only memory leaks and a handful of practracker issues, none of which appeared locally for me for some reason.

Fixed in the PR. Spec changes at https://github.com/torproject/torspec/pull/78

Last edited 4 months ago by mikeperry (previous) (diff)

comment:7 Changed 4 months ago by asn

Reviewer: asn

comment:8 Changed 4 months ago by mikeperry

Points: 1

comment:9 Changed 4 months ago by asn

Status: needs_reviewneeds_revision

Functionality looks good to me! I left a small comment in the PR about a place where we could tidy up the code a bit more. After this, we are good to go.

comment:10 Changed 4 months ago by mikeperry

Status: needs_revisionneeds_review

Pushed two fixups on to the PR, but practracter updates caused a conflict. New PR: https://github.com/torproject/tor/pull/997

comment:11 Changed 4 months ago by asn

Status: needs_reviewneeds_revision

Hm, seems like CI is broken because of at least src/core/or/circuitpadding.c:1111:1: error: no previous prototype for ‘circpad_is_padding_allowed’ [-Werror=missing-prototypes] . Not sure if there are other errors too.

I also let two other comments in the PR.

After that, I think we are good to go.

comment:12 Changed 4 months ago by mikeperry

Status: needs_revisionneeds_review

https://github.com/torproject/tor/pull/1003 - had to make a new PR due to practracker conflicts. I fixed the CI issue and the naming issue, but left the is_network_dormant() check separate, since it requires its own log line and is a different mechanism that causes it to happen.

comment:13 Changed 4 months ago by asn

Status: needs_reviewmerge_ready

LGTM!

Version 0, edited 4 months ago by asn (next)

comment:14 Changed 4 months ago by asn

Status: merge_readyneeds_review

comment:15 in reply to:  13 Changed 4 months ago by mikeperry

Replying to asn:

LGTM!

Before we go into merge_ready, can you also look into comment:22:ticket:28693 and see if we need to add a torrc option for reduced padding as well? If not, let's merge ready this.

I think that was just a random cypherpunk. The branch provides both CircuitPadding and ReducedCircuitPadding torrc options already, and has unit tests for them. I asked for clarification on that ticket from them, but I don't expect to get any.

comment:16 Changed 3 months ago by asn

Status: needs_reviewmerge_ready

Ugh. You are right, the option was already in. I somehow missed it. Sorry about that.

LGTM!

comment:17 Changed 3 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged.

Note: See TracTickets for help on using tickets.