netdir: New function to check consistency of a HasRelayIds
This function will be used to look up a relay by a set of LinkSpecs given from an incoming HsDesc or INTRODUCE2 message. It differs from other "lookup relay by IDs" functions in that it needs to be able to return "here's a relay", "couldn't found a relay", or "learned that this relay is impossible."
Closes #855 (closed): This is the only new API needed for ChanTarget validation, I think.
Merge request reports
Activity
assigned to @nickm
requested review from @gabi-250
954 Some(relay) => relay, 955 None => continue, 956 }; 957 if target 958 .identities() 959 .all(|wanted_id| match candidate.identity(wanted_id.id_type()) { 960 None => true, 961 Some(id) => id == wanted_id, 962 }) 963 { 964 return Ok(Some(candidate)); 965 } else { 966 return Err(RelayLookupError::Impossible); 967 } 968 } 969 Ok(None) - Comment on lines +952 to +969
I find this a bit easier to reason about with the loop turned into a
find_map+ early return:952 for id in target.identities() { 953 let candidate = match self.by_id(id) { 954 Some(relay) => relay, 955 None => continue, 956 }; 957 if target 958 .identities() 959 .all(|wanted_id| match candidate.identity(wanted_id.id_type()) { 960 None => true, 961 Some(id) => id == wanted_id, 962 }) 963 { 964 return Ok(Some(candidate)); 965 } else { 966 return Err(RelayLookupError::Impossible); 967 } 968 } 969 Ok(None) 952 let candidate = match target.identities().find_map(|id| self.by_id(id)) { 953 Some(relay) => relay, 954 None => return Ok(None), 955 }; 956 957 if target 958 .identities() 959 .all(|wanted_id| match candidate.identity(wanted_id.id_type()) { 960 None => true, 961 Some(id) => id == wanted_id, 962 }) 963 { 964 Ok(Some(candidate)) 965 } else { 966 Err(RelayLookupError::Impossible) 967 } -
Hmm, AFAICT
find_mapdoes the same thing as thematch self.by_id(id)from line 953: it callsself.by_id(id)for every identity intarget.identities(), until it finds a result that isSome. Just like the original code, this returns immediately after finding the first relay matching one of the target identities (returning eitherOk(Some(candidate))orErr(RelayLookupError::Impossible). changed this line in version 2 of the diff
Actually, as I was rewriting this, I found a case that the old algorithm didn't handle.
Suppose that there there are two relays R1 (with RsaId=RSA1 and Ed25519Id=None) and R2 (with RsaId=None and Ed25519Id=ED1).
Now if we ask for a relay with the identity (RsaId=RSA1 Ed25519Id=ED1), we will be given one relay or the other, since it does match the target identity set. But the fact that two different relays match the target identity set means that there is, in fact, a contradiction here.
(This situation is not possible in practice since in our current protocol, every relay must have an RSA identity. But if we were to add a third identity type, or make RSA optional, it would become possible, so I think it's best to revise to avoid this problem.)
I've pushed a fixup commit to address this issue and adopt an approach more like what you recommended. What do you think?
added 1 commit
- 1ebf19ef - fixup! netdir: New function to check consistency of a HasRelayIds
marked this merge request as draft from nickm/arti@1ebf19ef
added 1 commit
- 4a779807 - netdir: New function to check consistency of a HasRelayIds
enabled an automatic merge when the pipeline for 4a779807 succeeds
mentioned in commit dfa19998