Opened 4 weeks ago

Last modified 8 days ago

#30578 needs_review defect

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

Reported by: nickm Owned by: asn
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: circuitpadding, 041-should, hopefully-easy, wtf-pad
Cc: asn, mikeperry Actual Points:
Parent ID: Points:
Reviewer: mikeperry Sponsor: Sponsor2


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 (7)

comment:1 Changed 4 weeks ago by nickm

Keywords: wtf-pad added

comment:2 Changed 3 weeks ago by teor

Sponsor: Sponsor2

comment:3 Changed 3 weeks 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 3 weeks 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 12 days 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:

comment:6 Changed 8 days ago by dgoulet

Reviewer: asn

comment:7 Changed 8 days ago by dgoulet

Reviewer: asnmikeperry
Note: See TracTickets for help on using tickets.