[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #21027 [Core Tor/Tor]: tor_bug_occurred_(): Bug: src/or/entrynodes.c:816: entry_guard_add_to_sample_impl: Non-fatal assertion !(have_sampled_guard_with_id(gs, rsa_id_digest)) failed. (on Tor 0.3.0.0-alpha-dev 8b75261b6dc341de)
#21027: tor_bug_occurred_(): Bug: src/or/entrynodes.c:816:
entry_guard_add_to_sample_impl: Non-fatal assertion
!(have_sampled_guard_with_id(gs, rsa_id_digest)) failed. (on Tor 0.3.0.0
-alpha-dev 8b75261b6dc341de)
-------------------------------------------------+-------------------------
Reporter: asn | Owner: nickm
Type: defect | Status:
| needs_revision
Priority: High | Milestone: Tor:
| 0.3.0.x-final
Component: Core Tor/Tor | Version:
Severity: Normal | Resolution:
Keywords: tor-guard, tor-guards-revamp, | Actual Points:
regression, review-group-16 |
Parent ID: | Points: 1
Reviewer: | Sponsor:
-------------------------------------------------+-------------------------
Comment (by asn):
Hello, as I said also in IRC, I think that the provided patch addresses
the issue. I tested this on my Tor Browser with the bridges from
comment:17 and I verified that they are all treated as separate bridges
indeed, and they get sampled to SAMPLED_GUARDS properly.
That said, I agree with the comments of David about the code/comment
quality:
- The function documentation for
`get_closest_configured_bridge_by_addr_port_digest()` seems to be
incomplete and incorrect. What does `tolerate` mean? Does it actualy
`tolerate a bridge with an unknown digest if the digest is provided`? I
don't see this in the code. Also, if anything, the new function seems less
tolerable than the old one in general.
Also, is that the important difference of this new function for our
purposes? IIUC, the important part for us is that it will only return a
bridge if the addrport really matches; whereas the old
`get_configured_bridge_by_addr_port_digest()` would return a bridge that
matches the digest even if the addrport was completely different. Is that
what you tried to express in the comment? Can we perhaps make it more
clear?
- Also, as David said,
`get_closest_configured_bridge_by_addr_port_digest()` is kind of an
ambiguous name. Can we clarify it? I couldn't think of a better name;
these two functions are quite similar semantically with some small
esoteric differences.
Alternatively, could we merge these two functions together and use a
func argument to disambiguate?
All in all the patch seems good with regards to functionality, but perhaps
we should take care of the above to improve code quality and
maintainability.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21027#comment:25>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs