Opened 3 months ago

Last modified 32 hours ago

#28636 new defect

Address WTF-PAD comments by Nick (2018-11-27 over IRC)

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: wtf-pad, tor-relay, tor-cell, padding, 041-proposed
Cc: nickm Actual Points:
Parent ID: #28632 Points: 2
Reviewer: Sponsor: Sponsor2

Description (last modified by asn)

Nick expressed the following concerns/comments about the branch:

  • Figure out how dormant mode interacts with the padding code.
    15:27 <+nickm> okay.  short version is that tor can enter a "dormant" state when it is inactive for a really long time, or when the controller tells it to do so.
    15:28 <+nickm> we should probably figure out whether being dormant prevents you from sendig padding and vice versa...
    15:28 <+nickm> ... and whether it should prevent you from sending padding (and vice versa)...
    
  • Do type checking when downcasting the vcarious probability distributions.
  • <+nickm> Also there are all these generic dist_ops functions that you could use ... but the API seems to encourage you not to use them. having wrapper functions instead that call dist->dist_ops.func(dist, ...) would be neat
  • <+nickm> oh, one list thing: I think src/lib/math is not allowed to include lib/crypt_ops, since that would introduce a circularity. Better make sure that it doesn't

Child Tickets

Change History (11)

comment:1 Changed 3 months ago by asn

Description: modified (diff)

comment:2 Changed 3 months ago by teor

Summary: Address comments by Nick (2018-11-27 over IRC)Address WTF-PAD comments by Nick (2018-11-27 over IRC)

comment:3 Changed 3 months ago by nickm

To be more clear: I'd be happy to help with some/all of these, and I am not 100% sure that they all need to go in before WTF-pad is merged if any of them will be very hard... though it should really go in before it's turned on by default.

comment:4 in reply to:  3 Changed 3 months ago by asn

Replying to nickm:

To be more clear: I'd be happy to help with some/all of these, and I am not 100% sure that they all need to go in before WTF-pad is merged if any of them will be very hard... though it should really go in before it's turned on by default.

Yep, agreed. That's why I made this ticket a child of #28632 and not of #28142.
Like you said, we MUST address the above before turning it on by default, but not necessarily before merging the big branch.

comment:5 Changed 3 months ago by asn

Some logs from Riastradh on how to fix the probdistr stuff:

 21:49 < Riastradh> nickm: You rightly identified that the intended way to get at functions like uniform_sample is via the dist_ops!  See bin_cdfs and bin_samples in test_prob_distr.c for an 
                    example of how to use them generically -- these (and the test_psi_dist_sample function) were why I built that abstraction.
 21:50 < Riastradh> nickm: I did not build wrapper functions because I just needed it for the tests and I didn't want to spend any more time than necessary architecting an abstraction without 
                    knowing whether it would bear fruit beyond its erstwhile solitary use case.
 21:52 < Riastradh> nickm: If you want to make broader use than the switch in circpad_distribution_sample, I would encourage a dist_sample(dist) { return dist->ops->sample(dist); } function, and 
                    dist_cdf(dist, x) { return dist->ops->cdf(dist, x); }, &c.
 21:58 < Riastradh> nickm: And if you did this, I would encorage hiding the member functions uniform_sample, uniform_cdf, &c., in prob_distr.c.  Indeed, you could do that right now in 
                    circpad_distribution_sample with no adverse effects.
 22:01 < Riastradh> nickm: As for potential circularities between crypto and math, it would be reasonable to separate the purely computational functions in prob_distr.c from the struct dist 
                    abstraction which uses crypto_rand_*; one could go in math and the other could go in crypto or something downstream of both math and crypto.
 22:01 < Riastradh> nickm: I didn't do that splitting up because I'm not familiar with the large-scale organization of the code base so I didn't want to commit to particular organizational 
                    suggestions that would turn out to be moot.
 22:03 < Riastradh> nickm: If you want to split up prob_distr.c, nothing _above_ `Public API for probability distributions' depends on anything below it, so that's the easy point.  
                    (sample_geometric should maybe go above that instead of below it.)

comment:6 Changed 5 weeks ago by asn

Description: modified (diff)

comment:7 Changed 5 weeks ago by asn

Milestone: Tor: 0.4.0.x-finalTor: unspecified

comment:8 Changed 5 weeks ago by mikeperry

Keywords: 041-proposed added

comment:9 Changed 4 weeks ago by nickm

Sponsor: Sponsor2

comment:10 Changed 3 weeks ago by mikeperry

So during one of Nick's code reviews, there was some discussion about renaming the circuit machine definitions and state structs to spec and state.. But we didn't pick good names for the machine state because it is confusing with the states of the machines.

I like either circuit_machine_mutable_t or circuit_machine_mutable_info_t or circuit_machine_runtime_t or circuit_machine_runtime_info_t rather than the circuit_machine_state_t we landed on from the review.

comment:11 Changed 32 hours ago by asn

Points: 2
Note: See TracTickets for help on using tickets.