Opened 5 months ago

Last modified 4 weeks ago

#30578 needs_revision defect

The circuitpadding_circuitsetup_machine test: Re-enable, remove, re-document, or ___?

Reported by: nickm Owned by: asn
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: circuitpadding, hopefully-easy, wtf-pad, 042-deferred-20190918
Cc: asn, mikeperry Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor: Sponsor2

Description

Our code in test_circuitpadding.c says:

  /** Disabled unstable test until #29298 is implemented (see #29122) */
  //  TEST_CIRCUITPADDING(circuitpadding_circuitsetup_machine, TT_FORK),

But both #29298 and #29122 are closed now.

If this test will work now, let's enable it. If it is no longer useful, let's remove it. If it is disabled for some reason other than the one that's described in the comment, let's adjust the comment.

Child Tickets

Change History (12)

comment:1 Changed 5 months ago by nickm

Keywords: wtf-pad added

comment:2 Changed 5 months ago by teor

Sponsor: Sponsor2

comment:3 Changed 5 months ago by nickm

Owner: set to asn
Status: newassigned

Assigning to asn, at least as far as deciding which change to make.

comment:4 Changed 5 months ago by asn

Hm, I took a look and tried to naively activate the old test, it was still giving out failures.

Fixing this test is possible I think now that #29298 is done, but it will still require some non-trivial changes to the test and also to remember how this complicated old test was supposed to work. I might need some help with Mike who wrote the test. Will work on other things for now because I have lots of backlog and come back to this.

comment:5 Changed 4 months ago by asn

Status: assignedneeds_review

OK I tried another time to fix the test, but it seems to be a PITA. The test is testing some old complicated padding machines, and it's not accounting for some state transitions that are possible (in particular the relay machine transitions from BURST to GAP and sometimes sends padding really fast before the test accounts for it). Fixing it would require either huge changes to the test, or changes to the machine.

IMO we should just remove the test since it's just doing in-depth testing of these machines that are now obsolete and only used as test machines. As part of my branch I moved these machines to the unittest file since they were never used by anything other than the unittests anyway: https://github.com/torproject/tor/pull/1086

comment:6 Changed 4 months ago by dgoulet

Reviewer: asn

comment:7 Changed 4 months ago by dgoulet

Reviewer: asnmikeperry

comment:8 Changed 4 months ago by mikeperry

Reviewer: mikeperryasn

Ok, I fixed the test and resolved the flapping issue. Shifting the circuitsetup machine by 2ms ensures that the call of timers_advaance_and_run(1) from circuit_package_relay_cell_mock() does not trigger the circuitsetup relay side machine to send padding (which was the problem).

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

I am going to switch reviewer back to asn for the test flapping fix commits. The rest look good to me.

comment:9 Changed 4 months ago by mikeperry

(The reason for fixing the test is that it adds about 200 lines of coverage, taking circuitpadding from 92 to 94%).

comment:10 Changed 4 months ago by asn

Status: needs_reviewneeds_revision

Much better but still not perfect :(

Running it like this:

$ while ./src/test/test --debug circuitpadding/circuitpadding_circuitsetup_machine ; do :; done

still fails after a few seconds with:

Jun 26 09:57:44.066 [info] circpad_send_padding_cell_for_callback(): Callback: Sending padding to circuit (1) [length: 18446744073709551615]
Jun 26 09:57:44.066 [debug] relay_send_command_from_edge___real(): delivering 10 cell backward.
Jun 26 09:57:44.066 [info] circpad_machine_spec_transition__real(): Circpad machine 0 transitioning from 1 to 1
Jun 26 09:57:44.066 [info] circpad_machine_schedule_padding__real(): 	Padding in 540 usec
Jun 26 09:57:44.066 [info] circpad_machine_schedule_padding__real(): 	Padding in 0 sec, 540 usec
Jun 26 09:57:44.066 [info] circpad_deliver_recognized_relay_cell_events(): Got padding cell on origin circuit 1.
Jun 26 09:57:44.066 [debug] relay_send_command_from_edge___real(): delivering 10 cell forward.
Jun 26 09:57:44.066 [debug] relay_send_command_from_edge___real(): Sending a RELAY_EARLY cell; 4 remaining.
Jun 26 09:57:44.066 [info] circpad_machine_spec_transition__real(): Circpad machine 0 transitioning from 2 to 2
Jun 26 09:57:44.066 [info] circpad_machine_schedule_padding__real(): 	Padding in 4154 usec
Jun 26 09:57:44.066 [info] circpad_machine_schedule_padding__real(): 	Padding in 0 sec, 4154 usec
Jun 26 09:57:44.066 [info] circpad_deliver_recognized_relay_cell_events(): Got padding cell on non-origin circuit 0.
Jun 26 09:57:44.066 [info] circpad_send_padding_cell_for_callback(): Callback: Sending padding to origin circuit 1 (5) [length: 18446744073709551615]
FAIL src/test/test_circuitpadding.c:2322: assert(n_client_cells OP_EQ 2): 3 vs 2`

I'd be more comfortable if we don't rely on machine timing for this test given that this can vary wildly and is unpredictable in CI, etc.

comment:11 Changed 3 months ago by asn

Keywords: 041-should removed
Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final

I suggest we defer this to 042 since Mike's attempt did not make it work properly.

If we don't want to have this disabled test in 041 whatsoever, then feel free to apply my branch from comment:5 in 041, and we can try to fix the test in 042.

comment:12 Changed 4 weeks ago by nickm

Keywords: 042-deferred-20190918 added
Milestone: Tor: 0.4.2.x-finalTor: unspecified

Deferring various tickets from 0.4.2 to Unspecified.

Note: See TracTickets for help on using tickets.