Opened 5 months ago

Closed 6 weeks ago

#22215 closed enhancement (implemented)

networkstatus_nickname_is_unnamed() can get ripped out

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-23
Cc: neel@…, Sebastian Actual Points:
Parent ID: #12898 Points:
Reviewer: nickm Sponsor:

Description

  /* Is it marked as owned-by-someone-else? */
  if (networkstatus_nickname_is_unnamed(nickname)) {
    log_info(LD_GENERAL, "The name %s is listed as Unnamed: there is some "
             "router that holds it, but not one listed in the current "
             "consensus.", escaped(nickname));
    return NULL;
  }

The Unnamed flag is long gone from the world, now that proposal 235 has been merged. So we can simplify node_get_by_nickname() by getting rid of this function, plus the discussions about the Named flag.

If we want to get adventuresome, I also see

    int *named_flag; /* Index of the flag "Named" for votes[j] */
    int *unnamed_flag; /* Index of the flag "Unnamed" for votes[j] */

in networkstatus_compute_consensus(). And more generally,

$ grep Named *.c
circuitbuild.c: * If <b>verbose_names</b> is false, give nicknames for Named routers and hex
dirserv.c:/*                 1  Historically used to indicate Named */
dirvote.c:    int *named_flag; /* Index of the flag "Named" for votes[j] */
dirvote.c:        if (!strcmp(fl, "Named"))
dirvote.c:    /* Named and Unnamed get treated specially */
dirvote.c:        if (!strcmp(fl, "Named")) {
networkstatus.c: * nickname, but the Named flag isn't set for that router. */
nodelist.c: * nickname, but the Named flag isn't set for that router. */
nodelist.c:                 "but none is listed as Named in the directory consensus. "
routerparse.c:      else if (!strcmp(tok->args[i], "Named"))

Child Tickets

Attachments (2)

0001-networkstatus_nickname_is_unnamed.patch (10.4 KB) - added by neel 2 months ago.
Patch to remove networkstatus_nickname_is_unnamed(), as well as named_flag and unnamed_flag, and checks for Named and Unnamed flags
0002-networkstatus_nickname_is_unnamed.patch (2.4 KB) - added by neel 6 weeks ago.
Updated patch to remove networkstatus_nickname_is_unnamed()

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 months ago by nickm

Parent ID: #12898

comment:2 Changed 5 months ago by asn

Milestone: Tor: 0.3.2.x-final

Triaging to 0.3.2. Feel free to put it in 0.3.1 if you feel we should do it faster.

comment:3 Changed 3 months ago by neel

Cc: neel@… added

comment:4 Changed 3 months ago by neel

I want to work on this ticket.

I found out that there are also is_named and is_unnamed flags in or.h. Should I remove them or keep them?

comment:5 Changed 3 months ago by nickm

Cc: Sebastian added

I think those are also removable, but it might make sense to talk to Sebastian (Sebastian on IRC) first -- I know he's done some work in this area before.

Changed 2 months ago by neel

Patch to remove networkstatus_nickname_is_unnamed(), as well as named_flag and unnamed_flag, and checks for Named and Unnamed flags

comment:6 Changed 2 months ago by neel

Hi,

I have created a patch 0001-networkstatus_nickname_is_unnamed.patch​ which removes networkstatus_nickname_is_unnamed(), the named_flag and unnamed_flag, and checks for Named and Unnamed flags.

Please tell me what you think of this patch.

Thanks,

Neel Chauhan

comment:7 Changed 2 months ago by neel

BTW, I did not remove is_named and is_unnamed as I saw more code using these variables, so I did not want to mess with them.

comment:8 Changed 2 months ago by nickm

Status: newneeds_review

comment:9 Changed 6 weeks ago by nickm

Keywords: review-group-23 added

Put 0.3.2 needs_review and merge_ready tickets into review-group-23.

comment:10 Changed 6 weeks ago by nickm

Reviewer: nickm

comment:11 Changed 6 weeks ago by nickm

Status: needs_reviewneeds_revision

The first two parts of this commit are fine, but the changes in dirvote.c need to become conditional, and only happen when a new consensus method is provided.

The problem is that the consensus-generation code in dirvote.c needs to be run in exactly the same way on all the authorities for all inputs, or else they can produce different outputs, which will make them not agree on the consensus.

comment:12 Changed 6 weeks ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

Changed 6 weeks ago by neel

Updated patch to remove networkstatus_nickname_is_unnamed()

comment:13 Changed 6 weeks ago by neel

I created an updated patch with the name 0002-networkstatus_nickname_is_unnamed.patch​ which does not include the dirvote.c code. Is this enough, or do I need to add more?

comment:14 Changed 6 weeks ago by nickm

looks good! I merged it, and removed a few other now-unnecessary functions.

comment:15 Changed 6 weeks ago by nickm

Resolution: implemented
Status: needs_revisionclosed
Note: See TracTickets for help on using tickets.