Opened 2 years ago
Last modified 21 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 Changed 2 years ago by
comment:2 follow-up: 4 Changed 2 years ago by
Ok, let's make channel_add_to_digest_map check for channel_is_client().
comment:3 Changed 2 years ago by
Status: | new → needs_revision |
---|
comment:4 Changed 2 years ago by
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.
comment:5 Changed 2 years ago by
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 21 months ago by
Keywords: | 033-triage-20180320 added |
---|
Marking all tickets reached by current round of 033 triage.
comment:7 Changed 21 months ago by
Keywords: | 033-removed-20180320 added |
---|
Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.
comment:8 Changed 21 months ago by
Milestone: | Tor: 0.3.3.x-final → Tor: 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.
Replying to teor:
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.