Opened 4 months ago

Closed 7 weeks ago

#30747 closed defect (fixed)

Unclear check for skipping scaling due to missing bandwidths or missing descriptors

Reported by: teor Owned by:
Priority: High Milestone: sbws: 1.1.x-final
Component: Core Tor/sbws Version: sbws: 1.1.0
Severity: Major Keywords: sbws-majority-blocker-maybe
Cc: juga Actual Points: 0.2
Parent ID: #30733 Points: 0.2
Reviewer: juga Sponsor:

Description

We skip old results when scaling, but there are a few things wrong with this check:

  1. It is hard to tell what this line does, because it mixes "and" and "or" without brackets: l.desc_bw_obs_last or l.desc_bw_obs_mean and l.desc_bw_avg
  2. We skip scaling if any input is missing, but then we check desc_bw_obs_type and ignore some inputs
  3. If we are missing a descriptor for long enough, we stop generating results for a relay. We could use a substitute value instead.
                if not(l.desc_bw_obs_last or l.desc_bw_obs_mean and l.desc_bw_avg):
                    log.debug("Skipping %s from scaling, because there were not "
                              "descriptor bandwidths.", l.nick)
                    continue
                if desc_bw_obs_type == TORFLOW_OBS_LAST:
                    desc_bw_obs = l.desc_bw_obs_last
                elif desc_bw_obs_type == TORFLOW_OBS_MEAN:
                    desc_bw_obs = l.desc_bw_obs_mean
    

https://github.com/torproject/sbws/blob/9065848247e49330560a2f73b044fb8c02338b48/sbws/lib/v3bwfile.py#L1244

Child Tickets

Change History (4)

comment:1 Changed 3 months ago by teor

Actual Points: 0.2
Keywords: sbws-majority-blocker-maybe added
Points: 0.2
Priority: MediumHigh
Reviewer: juga
Severity: NormalMajor
Status: newneeds_review

Python evaluates "and" before "or", so this code was doing:
if not(l.desc_bw_obs_last or (l.desc_bw_obs_mean and l.desc_bw_avg)):
which is obviously wrong.

Reference:
https://docs.python.org/3/reference/expressions.html#operator-precedence

See my pull request, which only skips the relay if we're actually going to use the attribute:
https://github.com/torproject/sbws/pull/359

comment:2 Changed 2 months ago by juga

Status: needs_reviewmerge_ready

lgtm

comment:3 Changed 2 months ago by gaba

Can we merge this? Anything else that needs to be done here?

comment:4 in reply to:  3 Changed 7 weeks ago by juga

Resolution: fixed
Status: merge_readyclosed

Replying to gaba:

Can we merge this? Anything else that needs to be done here?

No, just lack of time in the last month to work on sbws code.
Merged.

Note: See TracTickets for help on using tickets.