Opened 5 years ago

Closed 4 years ago

#12752 closed defect (fixed)

stem: Microdescript bandwidth considered unmeasured by default

Reported by: asn Owned by: Sebastian
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-spec
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Reading https://stem.torproject.org/api/descriptor/router_status_entry.html#stem.descriptor.router_status_entry.RouterStatusEntryMicroV3 gave me the impression that measured would contain the bandwidth referenced in microdescriptor consensuses (since AFAIK the bandwidth in consensuses is measured), however it seems that stem puts the bandwidth in bandwidth (considering it unmeasured):

The code of stem is here:

    if w_key == "Bandwidth":
      if not (w_value and w_value.isdigit()):
        if not validate:
          return

        raise ValueError("%s 'Bandwidth=' entry needs to have a numeric value: w %s" % (desc._name(), value))

      desc.bandwidth = int(w_value)
    elif w_key == "Measured":
      if not (w_value and w_value.isdigit()):
        if not validate:
          return

        raise ValueError("%s 'Measured=' entry needs to have a numeric value: w %s" % (desc._name(), value))

      desc.measured = int(w_value)
    elif w_key == "Unmeasured":
      if validate and w_value != "1":
        raise ValueError("%s 'Unmeasured=' should only have the value of '1': w %s" % (desc._name(), value))

      desc.is_unmeasured = True
    else:
      desc.unrecognized_bandwidth_entries.append(w_entry)

which basically saves stuff to bandwidth except if an explicit Measured appears which makes it save to measured. OTOH, Tor assumes measured by default, and only adds the Unmeasured string if it's not:

      /*     Now the weight line. */
      if (rs_out.has_bandwidth) {
        int unmeasured = rs_out.bw_is_unmeasured &&
          consensus_method >= MIN_METHOD_TO_CLIP_UNMEASURED_BW;
        smartlist_add_asprintf(chunks, "w Bandwidth=%d%s\n",
                               rs_out.bandwidth_kb,
                               unmeasured?" Unmeasured=1":"");
      }

I think the correct fix here is to assume that microdedescriptor bandwidth is measured, except if the Unmeasured=1 string is present.

Child Tickets

Change History (9)

comment:1 Changed 5 years ago by atagar

Component: StemTor

Hi George, really sorry about the long delay. I'm still catching up on tickets. According to the dir-spec on this...

  • 'Bandwidth=INT' is 'an estimate of the bandwidth of this relay' though it's really nebulous about what that means.
  • 'Measured=INT' is the vote of a bandwidth authority.
  • 'Unmeasured=1' has a specific meaning, which is that the 'Bandwidth=INT' isn't based on three or more measurements.

Stem's docs on this are...

:var int bandwidth: bandwidth claimed by the relay (in kb/s)
:var int measured: bandwidth measured to be available by the relay
:var bool is_unmeasured: bandwidth measurement isn't based on three or more
  measurements

On reading this again I suspect the behavior is as follows...

  • Votes from bandwidth authorities have 'Measured=' values.
  • These votes are transformed into 'Bandwidth=' for the consensus, with the 'Unmeasured=1' if it's not coming from 3 or more authorities (though the wording on this is vague enough that it could mean 3 or more samplings by a single authority).

Either way, I'm happy to revise Stem if the dir-spec is clarified. Sending this over to the Tor component.

comment:2 in reply to:  1 Changed 5 years ago by nickm

Replying to atagar:

Hi George, really sorry about the long delay. I'm still catching up on tickets. According to the dir-spec on this...

  • 'Bandwidth=INT' is 'an estimate of the bandwidth of this relay' though it's really nebulous about what that means.

That's correct. From a client's POV, the units and accuracy of this value are unspecified intentionally, so that we could make some uniform change to the units and/or measurement scheme as needed.

  • 'Measured=INT' is the vote of a bandwidth authority.
  • 'Unmeasured=1' has a specific meaning, which is that the 'Bandwidth=INT' isn't based on three or more measurements.

Stem's docs on this are...

:var int bandwidth: bandwidth claimed by the relay (in kb/s)
:var int measured: bandwidth measured to be available by the relay
:var bool is_unmeasured: bandwidth measurement isn't based on three or more
  measurements

On reading this again I suspect the behavior is as follows...

  • Votes from bandwidth authorities have 'Measured=' values.

Right.

  • These votes are transformed into 'Bandwidth=' for the consensus, with the 'Unmeasured=1' if it's not coming from 3 or more authorities (though the wording on this is vague enough that it could mean 3 or more samplings by a single authority).

Yes.

Either way, I'm happy to revise Stem if the dir-spec is clarified. Sending this over to the Tor component.

What revisions to dir-spec do you suggest?

comment:3 Changed 5 years ago by atagar

Questions that this spec leaves for me are...

  • The 'Bandwidth=INT' entries exist in both votes and the consensus. What do they mean in the votes? If the consensus value is derived from the 'Measured=INT' entries then that's not said.
  • Does 'Unmeasured' mean we didn't have 3 or more bandwidth authorities voting, or does it mean the bandwidth authorities think it's too new?

George is the original filer of this ticket and might have other questions.

comment:4 Changed 5 years ago by nickm

Keywords: tor-spec added
Milestone: Tor: 0.2.6.x-final

comment:5 Changed 4 years ago by Sebastian

Owner: changed from atagar to Sebastian
Status: newassigned

If this requires any changes in tor, we should probably defer this to 0.2.7. Will poke

comment:6 Changed 4 years ago by Sebastian

Status: assignedneeds_review

For the first question, is that not adequately explained by this?

If 3 or more authorities provide a Measured= keyword for
   a router, the authorities produce a consensus containing a "w"
   Bandwidth= keyword equal to the median of the Measured= votes.

for your second point, I added a patch in branch bug12752 in my torspec repo.

comment:7 Changed 4 years ago by Sebastian

after discussion with atagar, added a pointer to section 3.4.2 in the "w" line description

comment:8 Changed 4 years ago by atagar

Looks good to me. Thanks Sebastian!

comment:9 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thanks!

Note: See TracTickets for help on using tickets.