Run dirvote_act() as a scheduled event. #62
Conversation
This change should have no behavioral effect: it just uses macros to describe the current control flow.
This is remarkably simple, given the macros in the last commit.
Closes ticket25937.
src/or/dirvote.c
Outdated
|
|
||
| #define IF_TIME_FOR_NEXT_ACTION(when_field, done_field) \ | ||
| if (voting_schedule.when_field < now && !voting_schedule.done_field) do { | ||
| #define ENDIF } while(0); |
Our check-space might not like this while(0) here? Dunno, but my OCD eyes picked it up :P.
| dirvote_act(const or_options_t *options, time_t now) | ||
| { | ||
| if (!authdir_mode_v3(options)) | ||
| return; | ||
| return TIME_MAX; |
Shouldn't -1 be returned here? As in ultimately, this value will probably be used for rescheduling the callback and if not in v3 mode, the callback should just stop no?
I don't think -1 means "stop forever"; it means "try again". TIME_MAX seemed like a reasonable way to say "never".
True, sorry PERIODIC_EVENT_NO_UPDATE = -1 means don't update last timestamp and try again next period.
But then we might trigger this BUG():
+ if (BUG(next == TIME_MAX)) {
+ return 3600;
+ }
... which in theory should never be triggered since the callback should never run if authdir_mode_v3() returns false so catching that BUG is good. And returning 3600 allows us to try again to vote in an hour if maybe tor state recovered? We should just add a comment there to explain why 3600 but also why the BUG() considering that dirvote_act() can legitimately return TIME_MAX.
cd45c16 has some comments to try to explain this. Please let me know if that isn't what you had in mind.
src/or/dirvote.c
Outdated
| } ENDIF | ||
|
|
||
| tor_assert_nonfatal_unreached(); | ||
| return now + 1; |
Again, shouldn't we stop the callback to be rescheduled here considering we should never get here?
src/or/main.c
Outdated
| return 3600; | ||
| } | ||
| if (BUG(next <= now)) { | ||
| return 1; |
Probably I would comment here mentionning that chances are the clock changed so then we rescheduled a second after the callback to update everything with the new clock?
So, I think this is impossible for the same reason it couldn't happen in #25948 -- the "next" value here is based on the input time to dirvote_act(), so unless dirvote_act() is buggy, it can't be earlier than the input.
Indeed.
src/or/main.c
Outdated
| time_t next = TIME_MAX; | ||
| if (authdir_mode_v3(options)) { | ||
| next = dirvote_act(options, now); | ||
| } |
src/or/config.c
Outdated
| dirvote_recalculate_timing(options, time(NULL)); | ||
| reschedule_dirvote(options); | ||
| } |
Hmmm... I'm not entirely sure this will work.
This code path is about when we are NOT a authority and we become one. Meaning that the dirvote event was not enabled before so the reschedule won't do anything.
And at the end of the code path, the periodic_events_on_new_options() is called which rescan the whole list and will the enable dirvote_act(). So all in all, we shouldn't do that here.
Hm, I think you're right. I think I was wondering what we should do if some other option changes (like V3AuthVotingInterval or V3AuthVoteDelay or V3AuthDistDelay). In that case, we'd want to reschedule, right?
cf0818e tries to dtrt here
Assert that the dirvote callback can't be reached on non-authorities.
but actually compile.
More comments on time calculations.
|
We merged an updated and squashed version of this as afd4fc6 |
No description provided.
The text was updated successfully, but these errors were encountered: