related to the new overload detection code
armadev
so dgoulet and ahf have been near that code recently
armadev
 double fraction = (double) overload_onionskin_assessment.n_ntor_dropped /
armadev
                   (double) overload_onionskin_assessment.n_ntor_requested;
armadev
so it would seem 'dropped' is higher than 'requested'
armadev
rep_hist_note_circuit_handshake_requested() is where one of them gets incremented
armadev
and rep_hist_note_circuit_handshake_dropped() is for the other
armadev
so, look through for places that call the _dropped() when nobody called _requested()
armadev
this one will be fun because you get to learn more about the "incoming create cell comes in, gets handed to cpuworker" logic
armadev
 if (!channel_is_client(chan)) {
armadev
   /* remember create types we've seen, but don't remember them from
armadev
    * clients, to be extra conservative about client statistics. */
armadev
   rep_hist_note_circuit_handshake_requested(create_cell->handshake_type);
armadev
 }
armadev
is your hint
armadev
do we only count requested ones when !channel_is_client(),
armadev
but do we count dropped ones always?
andy6
I'm glad you sent me that because I was looking at something completely different
armadev
feel free to paste all of my backlog here onto the ticket
armadev
it will help the person reviewing your patch
andy6
Thanks
armadev
also i bet geko will be interested in seeing this patched, because the network health team is trying to understand why relays are saying they are overloaded,
armadev
and if the relays are counting wrong that makes the rest of the analysis harder
armadev
it looks like every guard node (as in, relay that often sees client circuits) will count wrong
andy6
So I'm looking at this and I'm possibly being very dumb here but the if statement you sent me only triggers the requested method. It doesn't trigger the dropped method at all.
armadev
yep
armadev
we count one in one place, and the other in a different place
andy6
They are both in for loops in test_overload_onionskin_ntor
andy6
The places dropped is called
andy6
But both the dropped calls are proceeded by requested calls
armadev
woah, you're looking at the unit tests
armadev
i am looking at the actual tor code
andy6
So I am
andy6
I just searched the whole project for that method name
armadev
ok
armadev
the important files are:
armadev
src/feature/stats/rephist.c where the counts are modified
armadev
src/core/or/command.c where the _requested is called
loki_val has left IRC (Remote host closed the connection)
andy6
Yeah I am there now
armadev
src/feature/relay/onion_queue.c where the _dropped is called
armadev
just those three, mercifully
armadev
let me know when you want another hint :)
andy6
So were calling dropped when have_room_for_onionskin in onion_pending_add is false. Were calling onion_pending_add from assign_onionskin_to_cpuworker in cpuworker.c if total_pending_tasks >= max_pending_tasks.
armadev
yep!
ggus
could someone fix gettor@tpo email?
armadev
ggus: i verified that gettor@torproject.org email gets successfully delivered to gettor@rdsys-frontend.torproject.org
Disconnected
You have joined the channel
andy6 has joined (~andy6@c-174-51-54-86.hsd1.co.comcast.net)
Topic: This is the channel for developing Tor. Feel free to watch, but please take commentary to #tor-project and usage questions to #tor. | Relay operator discussion at #tor-relays | Commit and bug notifications -> #tor-bots | "it's a simple fix, just have to work out which lines to change" -- teor
ChanServ set the topic at: Aug 14, 2022 at 1:53 PM
Mode: +ntcR
Created at: Sep 15, 2008 at 9:00 PM
andy6
So if channel_is_client returns true and total_pending_tasks >= max_pending_tasks I think we will call dropped without calling requested
armadev
yep!
ribe has left IRC (Ping timeout: 480 seconds)
andy6
Do you think I should change it so that if total pending >= max pending and channel_is_client returns false we just call requested?
andy6
I'm thinking of adding this within the total pending >= max pending if statment
armadev
no, because then requested would be a weird combination of "not client + dropped"
armadev
i think there are two possible fixes. one of them is to drop that if channel_is_client check, and just count both correctly
armadev
the other is to, inside onion_pending_add(),
andy6
Okay I will get it fixed up and a merge request out.
armadev
also check if channel_is_client(circ->p_chan)
armadev
i think the second fix is probably the better one
armadev
that is, don't count either of them if it's a client channel
andy6
Thank you for being so helpful by the way. This is way more help than you get from most projects.
armadev
since that was pretty clearly the original intent, with the comment around the _requested() call
Disconnected
You have joined the channel
andy6 has joined (~andy6@c-174-51-54-86.hsd1.co.comcast.net)
Topic: This is the channel for developing Tor. Feel free to watch, but please take commentary to #tor-project and usage questions to #tor. | Relay operator discussion at #tor-relays | Commit and bug notifications -> #tor-bots | "it's a simple fix, just have to work out which lines to change" -- teor
ChanServ set the topic at: Aug 14, 2022 at 1:53 PM
Mode: +ntcR
Created at: Sep 15, 2008 at 9:00 PM
armadev
andy6: ok. let me know when you have a patch
armadev
i predict you will run into a confusing compile error,
armadev
because the channel_is_client() function isn't known by anything in onion_queue.c
armadev
when you get there let me know
andy6
I've got my fix together
andy6
Is it just a case of specifying the correct namespace?
armadev
maybe. can you post your fix somewhere? either as a git branch you push to some git repo, or just as a 'git diff' posted to paste.debian.net
armadev
also, 'does it build' is a good middle step
richard has left IRC (Ping timeout: 480 seconds)
richard has joined (~richard@8VQAAC5QD.tor-irc.dnsbl.oftc.net)
ChanServ has changed mode: +v richard
meejah has left IRC (Remote host closed the connection)
andy6
Right now it does not build. I need to get a reference to channel_is_client into onion_pending_add
armadev
yep! so the way you do that in c is with includes
armadev
go up to the top of onion_queue.c
armadev
and add #include "core/or/channel.h" in the list
armadev
that's because the channel_is_client() function is in core/or/channel.c and its prototype is listed, for other people to use, in core/or/channel.h
bentham has left IRC (Remote host closed the connection)
bentham has joined (~bentham@0002691c.user.oftc.net)
andy6
Thanks
andy6
So right now I've added the opposite check of the one in command_process_create_cell to onion_pending_add
andy6
So if the one in command_process_create_cell fails then the one in onion_pending_add will trigger but both shouldn't trigger.
andy6
if(channel_is_client(circ->p_chan)){
andy6
   rep_hist_note_circuit_handshake_requested(onionskin->handshake_type);
andy6
 }
andy6
Is now added to onion_pending_add.
andy6
It also compiled with that include statement.
armadev
hm, don't you want the same check, not the opposite?
armadev
you want failed to only increment if requested also increments
armadev
oh i see, you are adding to requested inside onion_pending_add
armadev
i think the goal of the original code was: never count anything regarding a channel that comes from a client
armadev
that is, don't count requested, don't count dropped
andy6
I'm possibly getting this wrong here but I want the check to activate if the check in command_process_create_cell fails. That way you don't get the check in command_process_create_cell to add 1 requested and the check in onion_pending_add to add 1 as well
andy6
So if the check fails we don't want to count dropped
andy6
So what do you think about this instead:
andy6
if (!have_room_for_onionskin(queue_idx) && !channel_is_client(chan)) {
andy6
#define WARN_TOO_MANY_CIRC_CREATIONS_INTERVAL (60)
andy6
   static ratelim_t last_warned =
andy6
     RATELIM_INIT(WARN_TOO_MANY_CIRC_CREATIONS_INTERVAL);
andy6
   rep_hist_note_circuit_handshake_dropped(queue_idx);
andy6
That way the dropped call wont activate unless the requested activated in command_process_create_cel
andy6
That way the dropped call wont activate unless the requested activated in command_process_create_cel
andy6
The check is !channel_is_client(circ->p_chan) as you mentioned earlier as well.
armadev
closer, but, if you do it that way then you won't actually drop the cell, if !channel_is_client().
armadev
you want to still drop it, just, not count it in the dropped stat
armadev
otherwise you will end up adding a onionskin when have_room_for_onionskin() says there's no room
armadev
that would make sense if you *also* wanted to change the design to "never drop circuit create requests if they're from a client"
armadev
but that is a pretty drastic change
andy6
So looking at the dropped method it just seems to increment things
andy6
If I wrap just the call to dropped in the channel_is_client check whatever code is actually dropping these connections should still work but we wont count them in the dropped stats
armadev
yep
andy6
 if (!have_room_for_onionskin(queue_idx) ) {
andy6
#define WARN_TOO_MANY_CIRC_CREATIONS_INTERVAL (60)
andy6
   static ratelim_t last_warned =
andy6
     RATELIM_INIT(WARN_TOO_MANY_CIRC_CREATIONS_INTERVAL);
andy6
   if(!channel_is_client(circ->p_chan)){
andy6
     rep_hist_note_circuit_handshake_dropped(queue_idx);
andy6
   }
andy6
   if (queue_idx == ONION_HANDSHAKE_TYPE_NTOR) {
andy6
     char *m;
andy6
     if ((m = rate_limit_log(&last_warned, approx_time()))) {
andy6
       log_warn(LD_GENERAL,
andy6
                "Your computer is too slow to handle this many circuit "
andy6
                "creation requests! Please consider using the "
andy6
                "MaxAdvertisedBandwidth config option or choosing a more "
andy6
                "restricted exit policy.%s",
andy6
                m);
andy6
       tor_free(m);
andy6
     }
andy6
   }
andy6
   tor_free(tmp);
andy6
   return -1;
andy6
 }
andy6
Like this?
armadev
whoops, that's a lot of lines to paste. in the future, when it's more than a few, put them on https://paste.debian.net/
andy6
Not a problem
armadev
but yes, that sounds good. two little changes to do, and then it should be ready:
armadev
(a) add some spaces on the new 'if' line so the spacing is like the other lines
armadev
(b) add a comment explaining that you're explicitly avoiding counting create cells from clients, because see the matching check in that-other-function
armadev
in command_process_create_cell()
20:56 armadev
that way when people see the new code in onion_pending_add() they won't be as surprised
armadev
and also that way if somebody wants to change this one day, they know to go look at command_process_create_cell() and maybe they'll need to change something there too.