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

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 legacy/trac#29298 (moved) and legacy/trac#29122 (moved) 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.

changed milestone to %Tor: unspecified in legacy/trac

added 042-deferred-20190918 in Legacy / Trac circuitpadding in Legacy / Trac component::core tor/tor in Legacy / Trac hopefully-easy in Legacy / Trac milestone::Tor: unspecified in Legacy / Trac owner::asn in Legacy / Trac priority::medium in Legacy / Trac reviewer::asn in Legacy / Trac severity::normal in Legacy / Trac sponsor::2 in Legacy / Trac status::needs-revision in Legacy / Trac type::defect in Legacy / Trac wtf-pad in Legacy / Trac labels

Trac:
Keywords: N/A deleted, wtf-pad added

Trac:
Sponsor: N/A to Sponsor2

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

Trac:
Status: new to assigned
Owner: N/A to 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 legacy/trac#29298 (moved) 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.

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

Trac:
Status: assigned to needs_review

Trac:
Reviewer: N/A to asn

Trac:
Reviewer: asn to mikeperry

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.

Trac:
Reviewer: mikeperry to asn

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

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.

Trac:
Status: needs_review to needs_revision

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.

Trac:
Keywords: 041-should deleted, N/A added
Milestone: Tor: 0.4.1.x-final to Tor: 0.4.2.x-final

Deferring various tickets from 0.4.2 to Unspecified.

Trac:
Milestone: Tor: 0.4.2.x-final to Tor: unspecified
Keywords: N/A deleted, 042-deferred-20190918 added

mentioned in issue legacy/trac#31788 (moved)

moved from legacy/trac#30578 (moved)

added Bug label and removed 1 deleted label

unassigned @asn

added Deferred Padding + 1 deleted label and removed 1 deleted label

added 1 deleted label

removed milestone

added Technical Debt label and removed 1 deleted label

added Triage2022 label

added Icebox label and removed Triage2022 label