I recommend if you want a function that gets called after the new consensus is in place (i.e. you don't care about what the old consensus used to say), move your call to the bottom of networkstatus_set_current_consensus.

Quoting the oniongit comment here for ease of history: """ When this function is called, the consensus has not yet changed. It is still old_c. It will be new_c soon. This is your chance to take new actions.

So if you ignore new_c and just call stuff that looks at "the consensus", you're going to be looking at the current consensus, i.e. old_c.

I think this is a bug in 0.3.2 with scheduler_notify_networkstatus_changed() too, in that set_scheduler() can call select_scheduler() which calls scheduler_can_use_kist() which calls kist_scheduler_run_interval(NULL) -- so even though kist_scheduler_run_interval knows how to receive an ns as an argument, it doesn't get new_c here. """

See branch: bug24975_032_01

Important bug that affects any values set by the consensus for the KIST scheduler so we need to backport it.

Trac:
Status: assigned to needs_review

merged to 0.3.2 and forward.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Ok sorry, I fucked up and arma noticed so good!

I've pushed a revert and then a commit that does the right thing.

The problem was that the function was moved after but was still using get_latest_consensus() for the parameter of the "old consensus". After we set globally the new consensus, that doesn't work so well...

Solution is to go with a before and after function. To achieve such, the scheduler is a bit changed but only parameters that are actually never used are removed.

Branch: bug24975_032_01

Trac:
Status: closed to reopened
Reviewer: N/A to armadev
Resolution: fixed to N/A

Trac:
Status: reopened to needs_review

New branch looks ok.

The changelog says "This lead [sic] to the scheduler failing to notice any consensus parameters that might have changed between consensuses" but I think it isn't quite that bad -- the bug meant that you picked up the new consensus params on the following consensus fetch.

So it could count as a minor bugfix and go only on master if we wanted to be more conservative.

Speaking of conflicts, when this gets merged there will be a conflict with the new dos_consensus_has_changed() call, which can be put in either the notify_before or notify_after function, so long as it's done right.

Oh, and eventually all of those other "things we do with a new consensus" should work their way out of networkstatus_set_current_consensus() into one of these new functions.

I've merged to 0.3.2 and forward (on the theory that having only one version of this to maintain will make us happier). I put the dos_consensus_has_changed() call in notify_before but please let's change it as appropriate?

Leaving this open till somebody confirms the dos_consensus_has_changed() issue.

Replying to nickm:

I've merged to 0.3.2 and forward (on the theory that having only one version of this to maintain will make us happier). I put the dos_consensus_has_changed() call in notify_before but please let's change it as appropriate?

Leaving this open till somebody confirms the dos_consensus_has_changed() issue.

Yes I do confirm that it is properly placed. Using the new consensus it was needs to be done so good!

Trac:
Status: needs_review to closed
Resolution: N/A to fixed