Ticket33436 #1763
Ticket33436 #1763
Conversation
The 'voting_schdule_get_start_of_next_interval' function isn't actually dirauth-specific.
This is an automated commit, generated by this command:
./scripts/maint/rename_c_identifier.py \
voting_schedule_recalculate_timing dirauth_sched_recalculate_timing \
voting_schedule_get_start_of_next_interval voting_sched_get_start_of_interval_after \
voting_schedule_get_next_valid_after_time dirauth_sched_get_next_valid_after_time
Most of this function was about recreating a voting schedule on demand if it didn't exist yet or was not up-to-date. I've made that into its own function.
This is not as clean a patch as I would like: see the comment on ASSUME_AUTHORITY_SCHEDULING. This issue here is that the unit tests sometimes assume that we are going to be looking at the dirauth options and behaving like a dirauth, but without setting the options to turn is into one. This isn't an issue for actually running Tor, as far as I can tell with chutney.
Pull Request Test Coverage Report for Build 8211
|
| @@ -20,50 +20,6 @@ | |||
| * Vote scheduling | |||
Perhaps it's now time to update the file-level \brief comment of voting_schedule.c to indicate that this file should only be used by dirauth now?
good idea.
| **/ | ||
| #define DEFAULT_NETWORK_VOTING_INTERVAL (3600) | ||
|
|
||
| /* This is an unpleasing workaround for tests. Our unit tests assume that we |
Hmm agreed that this is suboptimal. I wonder if another approach would be to mock authdir_mode() in the relevant tests, since this macro is always used next to authdir_mode().
This way it would be ultimately true that only dirauths can get into voting_schedule.c in a way that we could also soft-assert against it?
We could mock dirauth mode or set V3AuthoritativeDirectory here, but the problem is how pervasive in the tests it would need to be. All of our consensus-generation tests need to reach this code, but most of them don't actually set dirauth mode.
Without this kludge, there are 28 tests that fail and would need to get tweaked. To me that seemed higher than I thought it was reasonable to deal with, but I could try to tweak them all anyway. Do you think I should go ahead with that?
(Also, it's not just authority-only functions. For example, consider the test hs_service/intro2_handling. It tries to get a subcredential, so it calls into the shared_random_client.c code. In reality that would never happen without a consensus, but in our test it happens without a consensus, and so would trigger a soft assertion.)
ACK makes sense. Let's leave it as is then.
| @@ -242,8 +238,14 @@ sr_state_get_start_time_of_current_protocol_run(void) | |||
| networkstatus_t *ns = networkstatus_get_live_consensus(approx_time()); | |||
I think we should update the comment above this to reflect the new tristate reality.
| } else { | ||
| beginning_of_curr_round = get_start_time_of_current_round(); | ||
| tor_assert_nonfatal_unreached_once(); |
Hm. The soft assert here makes me wonder...
I assume you are soft-asserting here because all the HS-relevant code that calls this function has already checked that the consensus is live before getting n here. That's true. However there is a conceivable TOCTOU race-condition that can trigger this code path; if the consensus was live but about to expire during check, but while moving to this function the second has passed, and now it's not live anymore, so we get to this third state and soft-assert.
Is this something that should concern us? The problem is that if this soft-assert ever triggers we might not be sure if it's the edge case above or a different bug.
Hm. Maybe instead of using a live consensus we should use the latest consensus we have whether it's live or not. Would that help here, do you think?
I think that could work indeed. And then we can have a final case where we return the default value and do the soft-assert?
Also we seem to be doing this quadraple if check twice in the code. Can we abstract it somehow?
I've tried to make the changes for this on 16f6b65 . Doing so made the two functions diverge slightly, so I'm not sure about the abstraction you suggest, but I think this behavior is more sensible.
In this case, when we're looking for the voting interval, we should try looking at the _latest_ consensus if we have one. When we're looking for the start of the current voting period, we can use our existing fallback logic without complaint, since the voting interval code will already have given us a reasonable voting interval, and we want to have a round starting time based on the current time.
No description provided.
The text was updated successfully, but these errors were encountered: