Opened 14 months ago

Closed 9 months ago

#23814 closed defect (fixed)

Remove non-exponential backoff directory download implementation

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-31
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn, teor Sponsor:

Description (last modified by teor)

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.

Child Tickets

TicketStatusOwnerSummaryComponent
#24113closedWe stop trying to download an md after 8 failed triesCore Tor/Tor

Change History (16)

comment:1 Changed 14 months ago by teor

Description: modified (diff)

comment:2 Changed 11 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 11 months ago by nickm

Status: acceptedneeds_review

Code in my branch ticket23814.

comment:4 Changed 11 months ago by nickm

Keywords: review-group-31 added

comment:5 Changed 11 months ago by asn

Status: needs_reviewneeds_revision

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.

comment:7 Changed 11 months ago by asn

Reviewer: asn

comment:8 Changed 11 months ago by nickm

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.

comment:9 Changed 11 months ago by nickm

Status: needs_revisionneeds_review

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

comment:10 Changed 10 months ago by asn

Status: needs_reviewmerge_ready

Changes look good to me.

comment:11 Changed 10 months ago by teor

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.

comment:12 Changed 10 months ago by nickm

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

comment:13 Changed 10 months ago by nickm

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.

comment:14 Changed 10 months ago by teor

Reviewer: asnasn, teor

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

comment:15 Changed 10 months ago by teor

Status: merge_readyneeds_review

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

comment:16 Changed 10 months ago by teor

Status: needs_reviewmerge_ready

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. We can target #23354 for 0.3.4.

comment:17 Changed 9 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

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

Note: See TracTickets for help on using tickets.