ChanMgr: Add more contextual info to error types.
Now each type has a peer. In some cases this peer is just an address, whereas in others (where key is relevant or address isn't yet narrowed down) it's a full OwnedChanTarget.
Merge request reports
Activity
assigned to @nickm
requested review from @Diziet
26 ChanTimeout, 29 #[error("Channel for {peer} timed out")] 30 ChanTimeout { 31 /// Who we were trying to talk to 32 peer: OwnedChanTarget, 33 }, 27 34 28 35 /// A protocol error while making a channel 29 #[error("Protocol error while opening a channel.")] 36 #[error("Protocol error while opening a channel with {peer}")] 30 37 Proto { 31 38 /// The underlying error 32 39 #[source] 33 40 source: tor_proto::Error, 41 /// Who we were trying to talk to 42 peer: OwnedChanTarget, - Comment on lines +41 to +42
I wondered whether this should be
Sensitive. According to the docs I can find, we are using this only for "target" addresses, which I think means only ultimate circuit targets, not channel targets. Is this right?The changelog says "we aim to protect more information moving forward". So here would be an opportunity to do this.
I vaguely remember previous discussions about where
Sensitiveought to be applied, and I think we must have come to a conclusion about whether relevant fields in error enums ought to beSensitive<T>(my guess is that the answer was "yes")It would be nice if there were some written-down comprehensive policy about this, linked to from the toplevel
safelog/README.md, where currently there are a couple fo sentences. As it is I feel rather at sea. 
I wondered whether this should be
Sensitive. According to the docs I can find, we are using this only for "target" addresses, which I think means only ultimate circuit targets, not channel targets. Is this right?So far we're only using
Sensitivefor the targets of streams: neither circuits nor channels. There's a case to be made that we should use it for more, but that case isn't completely clear.I think we must have come to a conclusion about whether relevant fields in error enums ought to be
Sensitive<T>(my guess is that the answer was "yes")Agreed.
It would be nice if there were some written-down comprehensive policy about this, linked to from the toplevel
safelog/README.md, where currently there are a couple of sentences.Agreed; I think that it's going to take a whole-team discussion to get the actual policy right though. I suggest we make a separate ticket, list all the kinds of info that we're thinking about, and revisit once we have a rough consensus. Are you okay with that?
-
(opened #514 (closed) to review "what is sensitive anyway")
150 153 /// 151 154 /// This is not an `Into` implementation because we don't want to call it 152 155 /// accidentally when we actually do have clock skew information. 153 pub(crate) fn from_proto_no_skew(source: tor_proto::Error) -> Self { 156 pub(crate) fn from_proto_no_skew<T: ChanTarget + ?Sized>( 157 source: tor_proto::Error, 158 peer: &T, 159 ) -> Self { - Comment on lines -153 to +159
I think the generics here are doing us a disservice, possibly. In the next commit you change the code so that the two call sites to this function take a different concrete type.
If instead this function took a concrete type involving one address, the overly-vague earlier code would have been impossible. I think this might mean that the type in the error enum ought to be more specific.
And, I find the the
restrict_addrfunction you introduce here rather odd. It feels like a wart.What do you think? (If having thought about it in these terms you still think it's best the way it is, I'm OK with that.)
-
I think the generics here are doing us a disservice, possibly. In the next commit you change the code so that the two call sites to this function take a different concrete type.
If instead this function took a concrete type involving one address, the overly-vague earlier code would have been impossible. I think this might mean that the type in the error enum ought to be more specific.
Hmm. So you think we should add a new type representing a single address and key set, distinct from an OwnedChanTarget in that it represents only a single connection method to a relay rather than a set of possible connection methods?
I think that's a good idea. If you're okay with it, I'll make a separate ticket for this change, since it also touches on #428 (closed) and on future work for #69 (closed), and doing it properly will require a bunch of changes elsewhere.
-
So you think we should add a new type representing a single address and key set, distinct from an OwnedChanTarget in that it represents only a single connection method to a relay rather than a set of possible connection methods?
Err, maybe? I don't know precisely what I am suggesting :-). (Sorry, very vague today.) If you think that's a good idea then please go ahead.
Dealing with this as a separate ticket SGTM in any case.
added 14 commits
-
9a44d105...6878e3a2 - 11 commits from branch
tpo/core:main - bc9db9b8 - ChanMgr: Add more contextual info to error types.
- dd491931 - ChanMgr: errors: attribute errors to correct address.
- 7213c4a4 - Add a semver note.
Toggle commit list-
9a44d105...6878e3a2 - 11 commits from branch
mentioned in commit 357606a4