Bug28780 squashed expiretest #966
Conversation
Pull Request Test Coverage Report for Build 4988
|
If circuit padding wants to keep a circuit open and pathbias used to ignore it, pathbias should continue to ignore it. This may catch other purpose-change related miscounts (such as timeout measurement, cannibalization, onion service circuit transitions, and vanguards).
To ease debugging of miscount issues, attach vanguards with --loglevel DEBUG and obtain control port logs (or use any other control port CIRC and CIRC_MINOR event logging mechanism).
Because padding machine timers are UINT32_MAX in size, if some sort of network event doesn't happen on a padding-only circuit within that time, we can conclude it is deadlocked.
Inverting the logic makes it easier to see that there is only one place the function returns true to decide to keep ownership of a circuit. In all other cases it will release control and allow circuits to be closed.
…uits. Cast after division to avoid sign vs unsigned comparison.
src/core/or/circuitpadding.c
Outdated
| * last circpad_delay_t timespan, it's in some deadlock state. | ||
| * It's Ok to close now. */ | ||
| if (circ->padding_info[i]->last_cell_time_sec + | ||
| (CIRCPAD_DELAY_INFINITE/CIRCPAD_DELAY_UNITS_PER_SECOND)+1 |
I think we can simplify this a bit. Instead of defining the constant number CIRCPAD_DELAY_UNITS_PER_SECOND and the constant expression (CIRCPAD_DELAY_INFINITE/CIRCPAD_DELAY_UNITS_PER_SECOND)+1, instead let's define a single constant number with the result of the above expression and document exactly what it means.
| /* If we weren't marked dirty yet, let's pretend we're dirty now, to use | ||
| * that timer instead of the idle one (we're not supposed to look idle, | ||
| * after all) */ | ||
| if (!circ->timestamp_dirty) |
I really like this commit. But what does this timestamp_dirty thing does here? I was not expecting to see it and I'm not sure what timestamp_dirty does in general, or why we need to mod it. Perhaps some more comments?
Ah I guess it's so that circuit_expire_old_circuits_clientside() definitely applies.
I think we should find a place and write a detailed analysis of how we think that this "block mark for close, but close in the expiry function" codepath should work exactly.
More comments: f6d815f
More comments: f6d815f
…uits. Simplify age check.
…uits. Improve comments.
|
Superceded by PR #1015 |
No description provided.
The text was updated successfully, but these errors were encountered: