[tor-bugs] #16943 [Tor]: Implement prop250 (Random Number Generation During Tor Voting)

[Tor Bug Tracker & Wiki]

#16943: Implement prop250 (Random Number Generation During Tor Voting)
-------------------------+------------------------------------
 Reporter:  asn          |          Owner:  dgoulet
     Type:  enhancement  |         Status:  needs_review
 Priority:  High         |      Milestone:  Tor: 0.2.8.x-final
Component:  Tor          |        Version:
 Severity:  Normal       |     Resolution:
 Keywords:  tor-hs       |  Actual Points:
Parent ID:  #8244        |         Points:  large
  Sponsor:  SponsorR     |
-------------------------+------------------------------------
Changes (by dgoulet):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:34 teor]:
 > > > There are missed opportunities for const pointers in this commit.
 > >
 > > All fixed (hopefully).
 >
 > Except for the new smartlist_get_most_frequent_srv, which can take
 `const smartlist_t *sl`.
 > (It was added in 3b04ebdb94e29c0b2569ff8c64256f37f1389180.)

 Fixed 136da34.

 >
 > > > Nitpick: get_majority_srv_from_votes() uses `unsigned int current`
 for a boolean value. Tor typically uses int for boolean values.
 > >
 > > Should we really continue that "tradition"? :)
 >
 > I'm in favour of consistently using int, but it doesn't matter that
 much.
 > (We could decide to move to `stdbool.h`, and assert that our booleans
 are actually 0 or 1, but I think that's a topic for another ticket.)

 Fair enough. Fixed 279764a

 >
 > > > Why pick the microdesc consensus to update the state? Any particular
 reason?
 > > > Should we fall back to the NS consensus if there's no microdesc
 consensus or SR values?
 > > > {{{
 > > >   /* A new consensus has been created: pass it to the shared random
 subsystem
 > > >      to update the SR state. */
 > > >   sr_act_post_consensus(pending[FLAV_MICRODESC].consensus);
 > > > }}}
 > >
 > > It's the consensus that client uses so we use that consensus to set
 our SRV
 > > value post consensus thus having the same view as client. Does that
 make sense?
 > > You think we should use `FLAV_NS`?
 >
 > Most clients use the microdesc consensus, some still use NS.
 >
 > I don't mind which consensus we use, but:
 > * I think we should have a comment explaining the arbitrary choice, and

 I actually changed it to use the FLAV_NS which is the main one that if we
 can't
 create, problem! I also checked before using the consensus pointer that it
 does
 exists.

 See 63abee2

 > * I wonder what happens if one consensus fails, but the other doesn't
 (can this even happen?).

 Yeah, quite unsure but plausible from what I can see.

 [snip]

 > Now I see `7/724 TESTS FAILED. (1 skipped)`.
 >
 > 6 of these are the crashes above, the final one is:
 >

 Thanks to asn, we found out why the failures! See 30edde4.

 See branch: `prop250_final_v5` with the fixups.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16943#comment:35>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online