[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)

[Tor Bug Tracker & Wiki]

#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