Opened 2 years ago

Closed 2 years ago

#22060 closed enhancement (implemented)

Remove deprecated options from 0.2.9.2-alpha

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: config
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: nickm Sponsor:

Description

We have a set of deprecated options in src/or/config.c in the option_deprecation_notes_ array.

We should OBSOLETE() them and remove the associated code. Let's do one option removal per commit.

#21522 and #21031 asks to not get rid of ClientDNSRejectInternalAddresses so let's not for this ticket.

Child Tickets

Change History (15)

comment:1 Changed 2 years ago by nickm

BTW, let's remove only one option per commit, in case we need to revert. :)

comment:2 in reply to:  1 Changed 2 years ago by dgoulet

Replying to nickm:

BTW, let's remove only one option per commit, in case we need to revert. :)

Yes this is what I meant with: "We should OBSOLETE() them and remove the associated code. Let's do one option removal per commit." :D

comment:3 Changed 2 years ago by dgoulet

Keywords: easy removed
Points: 0.20.5
Status: newneeds_review

Some options are more tricky to remove! The branch I present removes the following.

  • AllowInvalidNodes
  • AllowSingleHopCircuits
  • AllowSingleHopExits
  • ExcludeSingleHopRelays
  • FastFirstHopPK
  • CloseHSClientCircuitsImmediatelyOnTimeout
  • CloseHSServiceRendCircuitsImmediatelyOnTimeout
  • WarnUnsafeSocks
  • {Control,DNS,Dir,Socks,Trans,NATD,OR}ListenAddress

Two are still deprecated at this point. AllowDotExit is a bit more complex and would benefit from its own ticket once this is merged. The second is ClientDNSRejectInternalAddresses that is still used for test networks (see ticket in description).

For AllowSingleHopExits a very small patch is needed to the dir-spec.txt.

Spec: ticket22060_01
Tor: ticket22060_031_01

Extra points! There was a bug that got exposed with the removal of ORListenAddress. Within server_mode(), we used to look at ORPort_set || ORListenAddress where ORPort_set is set in parse_ports().

However, options_validate() is using server_mode() at the start to check if we need to look at the uname but then the ORPort_set is unset at that point because the port parsing is just after. Thus, there is an extra commit that fixes that.

comment:4 Changed 2 years ago by dgoulet

Owner: set to dgoulet
Status: needs_reviewaccepted

comment:5 Changed 2 years ago by dgoulet

Status: acceptedneeds_review

comment:6 Changed 2 years ago by dgoulet

(I'm weirdly always happy about code removal sometimes :)

16 files changed, 203 insertions(+), 897 deletions(-)

comment:8 Changed 2 years ago by nickm

Thanks! I've reviewed it on github.

comment:9 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

comment:10 Changed 2 years ago by dgoulet

Reviewer: nickm
Status: needs_revisionneeds_review

I've addressed everything. As for this comment (which I can not reply on Gitlab, probably a bug):

If this is a separate bug, I think maybe this should have its own changes file, and its own ticket number?

This bug was introduced with the cleanup because we get rid of the *ListenAddress interface. So because it was never released, I didn't go for a change file nor ticket. It's basically part of this patchset like fixing a test. Not good?

comment:11 Changed 2 years ago by nickm

I argue more about 5df1535dc on the ticket. I still think that branch should always be taken.

comment:12 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

comment:13 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Last fixup commits fixing the comment about AllowSingleHop situation.

Branch: ticket22060_031_01
Spec: ticket22060_01

comment:14 Changed 2 years ago by dgoulet

Status: needs_reviewmerge_ready

Actually probably better in merge_ready since nickm was the reviewer here.

comment:15 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

squashed and merged; thank you!

Note: See TracTickets for help on using tickets.