Opened 18 months ago

Last modified 15 months ago

#24911 needs_revision defect

Remove the known digest check from channel_check_for_duplicates()

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: refactor, redundant, technical-debt, 033-triage-20180320, 033-removed-20180320
Cc: Actual Points:
Parent ID: #23423 Points:
Reviewer: Sponsor:

Description

This is in arma's commit f5ff9f23 in his bug24898-more branch.

This comment is wrong, the check is already implemented in channel_add_to_digest_map():

+    /* XXX we might want to replace this logic with a call to
+     * channel_is_client() instead. Or maybe we can change
+     * channel_identity_mapso it refuses to add any channels that are
+     * client channels? */

So we can remove the check entirely.

Child Tickets

Change History (8)

comment:1 in reply to:  description Changed 18 months ago by arma

Replying to teor:

the check is already implemented in channel_add_to_digest_map()

Really? Where in channel_add_to_digest_map does it check if channel_is_client() and abort?

I see a check for chan->identity_digest, but it's not obvious to me that that's the same thing, especially wrt, say, handshake-proto=2 connections. It would sure be simpler if we could decide to make them the same thing though.

comment:2 Changed 18 months ago by teor

Ok, let's make channel_add_to_digest_map check for channel_is_client().

comment:3 Changed 18 months ago by teor

Status: newneeds_revision

comment:4 in reply to:  2 Changed 18 months ago by arma

Replying to teor:

Ok, let's make channel_add_to_digest_map check for channel_is_client().

Actually, for whatever version we decide to backport the more-accurate channel_is_client() to, I think we would be wise to work towards having an invariant where, when channel_is_client is true, conn->identity_digest is zero. I think we're not too far away from having that be true now, and it would sure simplify things for us rather than forcing us to sprinkle in tiny confusing exception clauses everywhere.

Last edited 18 months ago by arma (previous) (diff)

comment:5 Changed 17 months ago by teor

I think this would help - should we even be storing v1 and v2 client certificate fingerprints anyway?

But there are still two separate questions we can ask, and we should keep them separate:

  • channel_is_client(): does the remote end act like a client (use v1 or v2, or not authenticate and use v3 or v4)
  • …is_known_relay(): was the remote end authenticated and in a recent consensus?

There is a small gap between these two cases, filled with unpublished relays.

comment:6 Changed 15 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:7 Changed 15 months ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:8 Changed 15 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: unspecified

These tickets were marked as removed, and nobody has said that they can fix them. Let's remember to look at 033-removed-20180320 as we re-evaluate our triage process, to see whether we're triaging out unnecessarily, and to evaluate whether we're deferring anything unnecessarily. But for now, we can't do these: we need to fix the 033-must stuff now.

Note: See TracTickets for help on using tickets.