Opened 3 years ago

Closed 3 years ago

#20001 closed enhancement (implemented)

Infer running and valid from presence in consensus

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop272, TorCoreTeam201608
Cc: Actual Points: .1
Parent ID: Points: .2
Reviewer: Sponsor:

Description

We no longer include in the consensus any non-running nodes.

We are no longer _supposed_ to include in the consensus any non-Valid nodes.

Let us update Tor to infer these flags as true from their presence in the consensus. This could work with prop#266 as another means to kill off obsolete clients.

Child Tickets

TicketTypeStatusOwnerSummary
#20002defectclosednickmNever include non-Valid nodes in consensus.

Change History (20)

comment:1 Changed 3 years ago by nickm

This is part of proposal 272.

comment:2 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

Please have a look at branch "ticket20001" in my public repository. We shouldn't actually merge this till #20002 is also done and deployed, and we may want to do it in conjunction with #19958.

comment:3 Changed 3 years ago by nickm

Actual Points: .1
Status: acceptedneeds_review

comment:4 Changed 3 years ago by nickm

Keywords: prop272 added; needs-proposal removed

comment:5 Changed 3 years ago by nickm

Keywords: TorCoreTeam201608 added

comment:6 Changed 3 years ago by nickm

Keywords: review-group-8 added

comment:7 Changed 3 years ago by andrea

Status: needs_reviewmerge_ready

This one looks fine to me, but shouldn't be merged until the authorities actually behave this way. Marking it merge_ready for now but sequence it after #20002 is merged and running on auths.

comment:8 Changed 3 years ago by andrea

Alternate but more complex suggestion to avoid the deployment dependency: check the consensus method.

comment:9 Changed 3 years ago by nickm

Checking the consensus method would be easy; method 24 for Valid and method 5 (which is always available) for Running.

comment:10 Changed 3 years ago by nickm

Status: merge_readyneeds_review

Branch updated; review would be welcome. :)

I've also pushed a prop272_part2 branch for torspec.

comment:11 Changed 3 years ago by dgoulet

Spec change looks good. Maybe also close prop272?

comment:12 Changed 3 years ago by nickm

Will do when I merge.

How does the updated ticket20001 branch look?

comment:13 Changed 3 years ago by nickm

(Actually, even after reivew, I don't want to merge this till *after* proposal 264 is in.)

comment:14 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

Review:

  • The following change is wrong and probably accidental:
    -        rs->is_flagged_running = 1;
    +        rs->is_flagged_running = 1 *;
    

Also please squash both commits as they are overwriting each other.

  • I don't see the MIN_METHOD_FOR_EXCLUDING_INVALID_NODES consensus method defined anywhere. However, it seems like it's included in the #20002 branch so this is just a rebase issue.
  • As a step further, does this mean that we can eventually deprecate the is_valid and is_flagged_running flags, and assume that they are set? When can this be done? I guess we can already do this for is_valid but we need to wait till MIN_METHOD_FOR_EXCLUDING_INVALID_NODES becomes standard to remove is_flagged_running.
  • Perhaps there could also be a unittest for this new functionality?

Marking this as needs_revision for the fist comment.

Last edited 3 years ago by asn (previous) (diff)

comment:15 in reply to:  14 Changed 3 years ago by nickm

Replying to asn:

Review:

[...]

Thanks!

I've fixed the typo issues and squashed the commits in a new branch, ticket20001_v2. It has also been rebased to master. I've added a unit test in a second commit.

  • As a step further, does this mean that we can eventually deprecate the is_valid and is_flagged_running flags, and assume that they are set? When can this be done? I guess we can already do this for is_valid but we need to wait till MIN_METHOD_FOR_EXCLUDING_INVALID_NODES becomes standard to remove is_flagged_running.

I think you have those backwards, but yeah. We could do that eventually.

comment:16 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

comment:17 Changed 3 years ago by asn

Status: needs_reviewmerge_ready

Looks good to me. Hooray for unittest.

comment:18 Changed 3 years ago by nickm

Okay, this can sit in merge_ready till #19958 goes in.

comment:19 Changed 3 years ago by nickm

Keywords: review-group-8 removed

comment:20 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merging, now that #19958 is in.

Note: See TracTickets for help on using tickets.