Remove non-exponential backoff directory download implementation

We're not meant to use this code any more~~, but it was accidentally used for microdescs up to 0.3.2.~~ Let's remove it.

changed milestone to %Tor: 0.3.3.x-final

added component::core tor/tor milestone::Tor: 0.3.3.x-final owner::nickm priority::medium resolution::fixed review-group-31 reviewer::asn reviewer::teor severity::normal status::closed type::defect labels

Trac:
Description: We're not meant to use this code any more, but it was accidentally used for microdescs up to 0.3.2. Let's remove it.

to

We're not meant to use this code any more~~, but it was accidentally used for microdescs up to 0.3.2.~~ Let's remove it.

Trac:
Owner: N/A to nickm
Status: new to accepted

Code in my branch ticket23814.

Trac:
Status: accepted to needs_review

Trac:
Keywords: N/A deleted, review-group-31 added

Hey, not very familiar with this part of the codebase, but did an attempt to review it as part of r-g-31.

Commit e0049ef022b8bf LGTM. I liked the trick with bf74194f unintend that code section on its own commit. I think we should also remove max_failures from download_status_is_ready() tho.

I'm a bit curious on why we needed to do 5b55e15 if the function works fine and we had tests for it. Is it to simplify the code? Should we do it as part of another ticket, or we feel fine doing it in this one? Code looks reasonable anyhow. We should also remove mention of max_delay from func doc of next_random_exponential_delay().

Marking this as needs_rev for the trivial fixes above. Feel free to put it in merge_ready after that.

Trac:
Status: needs_review to needs_revision

Trac:
Reviewer: N/A to asn
Cc: N/A to N/A

I'm a bit curious on why we needed to do 5b55e15 if the function works fine and we had tests for it. Is it to simplify the code? Should we do it as part of another ticket, or we feel fine doing it in this one? Code looks reasonable anyhow. We should also remove mention of max_delay from func doc of next_random_exponential_delay().

I'm fine removing max_delay it in this ticket -- My rationale was that with the removal of , it was never anything besides INT_MAX, and so wasn't actually serving any purpose.

Working on the other issues you raise above.

Okay, I've tried to remove max_failures. In doing so, I discovered that a bunch of testing options were now obsolete, and so obsoleted them. See ticket23814 again? This is complicated enough that I don't think it should go right back into merge_ready

Trac:
Status: needs_revision to needs_review

Changes look good to me.

Trac:
Status: needs_review to merge_ready

Is this going in 0.3.4? If it's not urgent, I'd like to review it. I should have time around the middle of next week.

I'd been thinking of 0.3.3, but 034 might be smarter? Either way, it can wait for another review

teor -- still planning to have a look at this? I'm happy to hold off on the merge if so; you know a lot about this code.

Yes, likely towards end of this week. I keep having to do extra features for experimental PrivCount.

Trac:
Reviewer: asn to asn, teor

Putting this in needs_review, so I actually see it in my review ticket queue.

Trac:
Status: merge_ready to needs_review

I've read and run the code, it looks good, and it passes make check test-network-all.

I think we should merge it in 0.3.3, because it might fix the microdesc download issue in #24113 (moved). We can target #23354 (moved) for 0.3.4.

Trac:
Status: needs_review to merge_ready

okay, I think you're right. Merging in 0.3.3.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

mentioned in issue #24113 (moved)

mentioned in issue #25347 (moved)

moved to tpo/core/tor#23814 (closed)