Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#18815 closed enhancement (implemented)

Clients don't use optimistic data to fetch their first consensus, because we told them to ask the consensus whether they should

Reported by: arma Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-accepted, review-group-1
Cc: Actual Points: very small
Parent ID: Points: very small
Reviewer: Sponsor:

Description

static int
optimistic_data_enabled(void)
{ 
  const or_options_t *options = get_options();
  if (options->OptimisticData < 0) {
    /* XXX023 consider having auto default to 1 rather than 0 before
     * the 0.2.3 branch goes stable. See bug 3617. -RD */
    const int32_t enabled =
      networkstatus_get_param(NULL, "UseOptimisticData", 0, 0, 1);
    return (int)enabled;
  }
  return options->OptimisticData;
}

OptimisticData is "auto" by default, aka -1, so we look at networkstatus_get_param, but since there *is* no consensus on first boot, we pick 0.

For one, this means we're opting not to save a round-trip on the Tor user's first bootstrap (the one where they judge Tor first).

For another, it would seem that we're telling the directory mirror whether this is our first bootstrap, because all the other clients do it differently.

For three, it increases the set of possible behaviors by normal clients that we need to consider during bootstrapping, which is how I found it (working on #18809).

We should make it default default to 1 when there's no consensus.

See https://trac.torproject.org/projects/tor/ticket/3617#comment:8 where it seems I looked into the future and saw this bug coming.

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by nickm

Keywords: 029-accepted added; 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

Sure, this is a one-line fix.

comment:2 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 3 years ago by nickm

Actual Points: very small
Points: very small
Status: acceptedneeds_review

tor patch in my branch bug18815.

torspec patch in my branch bug18815.

comment:4 Changed 3 years ago by nickm

Keywords: review-group-1 added

comment:5 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

Changes file doesn't mention the "fix on" tor version. Anyway, simple enough. Both patches look good to me!

Tested in chutney as well with success.

comment:6 Changed 3 years ago by sysrqb

Same, looks good to me.

comment:7 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed
Type: defectenhancement

Thanks for the review; merged!

(I claim that this is not actually a bugfix but a feature, and so it doesn't need a "fix on" version.)

comment:8 Changed 2 years ago by nickm

Historical note: this was a duplicate of #6899 all along.

Note: See TracTickets for help on using tickets.