Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adaptive padding rebased 0.3.6 squashed #461

Open

Conversation

Projects
None yet
5 participants
@mikeperry-tor
Copy link
Contributor

commented Oct 31, 2018

Strong recommend to review in git commit order (which is different than github's weirdly sorted order below).

mikeperry-tor and others added some commits Oct 23, 2018

Provide a smartlist reverse-order traversal.
We need this for padding negotiation so that we can have later machine
revisions supercede earlier ones.
Circuit padding header.
This is a good code review start point, to get an overview of the interfaces
and types used in circuit padding.
Circuit padding ProtoVer plumbing.
This helps us to determine if a middle node can pad to us or not.
Circuit padding cell event callbacks.
These callbacks allow the padding state machines to react to various types of
sent and received relay cells.
Circuit padding machine creation events.
These event callbacks allow circuit padding to decide when to attempt to
launch and negotiate new padding machines, and when to tear old ones down.
Add TODO file for padding work.
Note to self/others: don't merge this.
Improve circpad_distribution_sample() accuracy.
* Tighten and justify cutoff for approximation in GPD sampler.
* Use expm1(-xi log U) to evaluate U^{-xi} - 1 without precision loss.
* Use log1p(x) to evaluate log(1 + x) precisely even for x near 0.
* Avoid 0 by rejection sampling, not by choosing to return 0 instead.
  (This avoids the problematic 0 return from crypto_rand_double()
   into the log() without introducing distribution bias).
Circuit padding implementation.
This implements all of the event handling, state machines, and padding
decisions for circuit padding.

I recommend reviewing this after you look at the call-in points into it from
the rest of Tor.
Config option to specify specific MiddleNodes.
Hope is this will make it easier to test on the live tor network.

Does not need to be merged if we don't want to, but will come in handy
for researchers.
@coveralls

This comment has been minimized.

Copy link

commented Oct 31, 2018

Pull Request Test Coverage Report for Build 2808

  • 554 of 774 (71.58%) changed or added relevant lines in 10 files are covered.
  • 2666 unchanged lines in 14 files lost coverage.
  • Overall coverage increased (+0.2%) to 62.216%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/feature/nodelist/routerlist.c 1 2 50.0%
src/core/or/circuituse.c 3 5 60.0%
src/core/or/connection_edge.c 0 2 0.0%
src/core/or/relay.c 4 11 36.36%
src/core/or/circuitbuild.c 2 13 15.38%
src/core/or/circuitpadding.c 536 733 73.12%
Files with Coverage Reduction New Missed Lines %
src/core/or/circuitbuild.c 2 42.06%
src/lib/thread/compat_pthreads.c 4 91.78%
src/feature/dirparse/authcert_parse.c 18 68.93%
src/feature/dircache/consdiffmgr.c 63 90.09%
src/feature/dircache/dirserv.c 67 63.96%
src/feature/nodelist/authcert.c 122 28.07%
src/feature/nodelist/nodelist.c 127 61.42%
src/feature/dircache/dircache.c 132 69.83%
src/feature/relay/dns.c 155 17.89%
src/feature/dirparse/ns_parse.c 197 67.97%
Totals Coverage Status
Change from base Build 2687: 0.2%
Covered Lines: 44720
Relevant Lines: 71879

💛 - Coveralls
fixup! Circuit padding implementation.
Fix an appveyor fail.
Show resolved Hide resolved src/core/or/circuitpadding.h Outdated
Show resolved Hide resolved src/core/or/circuitpadding.h Outdated
Show resolved Hide resolved src/core/or/circuitpadding.c Outdated
Show resolved Hide resolved src/core/or/circuitpadding.c Outdated
Show resolved Hide resolved src/core/or/circuitpadding.c Outdated
Show resolved Hide resolved src/core/or/circuitpadding.c Outdated
@asn-d6
Copy link
Member

left a comment

review

Show resolved Hide resolved src/core/or/or.h Outdated
Show resolved Hide resolved src/core/or/circuitpadding.c
Show resolved Hide resolved src/core/or/circuitpadding.c Outdated
Show resolved Hide resolved src/core/or/circuitpadding.c Outdated

mikeperry-tor added some commits Nov 1, 2018

fixup! Circuit padding header.
Rename all the variables.
fixup! Circuit padding implementation.
Rename all the variables.
fixup! Circuit padding tests.
Rename all the variables.
fixup! Circuit padding implementation.
Factor end state cleanup code into its own function.
fixup! Circuit padding implementation.
Fix issues with bin<->usec conversion.
fixup! Circuit padding tests.
Better tests for bin conversion.
fixup! Circuit padding ProtoVer plumbing.
Padding support is for version >= 1.
fixup! Circuit padding implementation.
Clean up limit checks; also avoid overflow.
@nmathewson
Copy link
Contributor

left a comment

More questions/comments based on generic structures/declarations

@@ -49,6 +52,7 @@ static const struct {
{ PRT_HSREND, "HSRend" },
{ PRT_DESC, "Desc" },
{ PRT_MICRODESC, "Microdesc"},
{ PRT_PADDING, "Padding"},

This comment has been minimized.

Copy link
@nmathewson

nmathewson Nov 7, 2018

Contributor

Is this in the spec? This needs to be in the spec. These are crucial.

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Dec 7, 2018

Member

Done in my bug28142 torspec branch: https://github.com/asn-d6/torspec/tree/bug28142

@@ -83,6 +83,14 @@
++var ## _sl_idx) { \
var = (sl)->list[var ## _sl_idx];

#define SMARTLIST_FOREACH_REVERSE_BEGIN(sl, type, var) \

This comment has been minimized.

Copy link
@nmathewson

nmathewson Nov 8, 2018

Contributor

Let's have a unit test for this.

Also, let's document that it isn't compatible with SMARTLIST_DEL_CURRENT

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Dec 17, 2018

Member

Done in 3553d48 and 0a20957

Show resolved Hide resolved src/core/or/circuitpadding.h Outdated
typedef uint8_t circpad_machine_num_t;

/** Global state machine structure from the consensus */
typedef struct circpad_machine_t {

This comment has been minimized.

Copy link
@nmathewson

nmathewson Nov 8, 2018

Contributor

Let's have this called "machine_spec" and the other one called machine_state then.

Show resolved Hide resolved src/core/or/circuitpadding.c
Show resolved Hide resolved src/core/or/circuitpadding.c
circpad_send_padding_callback(tor_timer_t *timer, void *args,
const struct monotime_t *time)
{
circpad_machineinfo_t *mi = ((circpad_machineinfo_t*)args);

This comment has been minimized.

Copy link
@nmathewson

nmathewson Nov 8, 2018

Contributor

Let's also mention the lifetime rules here in the documentation for the timer fields.

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Dec 17, 2018

Member

Addressed in 04c059d

* Cache our consensus parameters upon consensus update.
*/
void
circpad_new_consensus_params(networkstatus_t *ns)

This comment has been minimized.

Copy link
@nmathewson

nmathewson Nov 8, 2018

Contributor

could this argument be const?

(Please look over the file to see what other arguments could be const)

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Dec 17, 2018

Member

Addressed in 04c059d

Show resolved Hide resolved src/core/or/circuitpadding.c Outdated
* Returns negative on error, 0 on success.
*/
static int
circpad_send_command_to_hop(origin_circuit_t *circ, int hopnum,

This comment has been minimized.

Copy link
@nmathewson

nmathewson Nov 8, 2018

Contributor

Can we split the parts of this file that interact with the network into their own module, and maybe abstract them somehow? I expect that would make testing easier.

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Dec 7, 2018

Member

I opened a ticket about this in https://trac.torproject.org/projects/tor/ticket/28777#ticket because I think trying to split the file now will be pretty hasty and won't give us a good interface. It will also cause lots of conflicts since me and Mike are both working on the same branch. Let's do it after the branch is merged IMO.

circ_client_machine->burst.transition_cancel_events =
CIRCPAD_EVENT_NONPADDING_SENT;

circ_client_machine->burst.transition_events[CIRCPAD_STATE_END] =

This comment has been minimized.

Copy link
@nmathewson

nmathewson Nov 8, 2018

Contributor

Can we split the parts of this file that do padding negotiation into their own module? This file is pretty large...

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Dec 7, 2018

Member

I opened a ticket about this in https://trac.torproject.org/projects/tor/ticket/28777#ticket because I think trying to split the file now will be pretty hasty and won't give us a good interface. It will also cause lots of conflicts since me and Mike are both working on the same branch. Let's do it after the branch is merged IMO.

mikeperry-tor added some commits Nov 8, 2018

fixup! Circuit padding header.
Allow up to 255 states instead of just 4.
fixup! Circuit padding implementation.
Allow up to 255 states instead of just 4.
fixup! Circuit padding tests.
Allow up to 255 states instead of just 4.
Show resolved Hide resolved src/core/or/circuitpadding.c Outdated

mikeperry-tor and others added some commits Nov 9, 2018

fixup! Circuit padding implementation.
Refactor delay sampling branches.
fixup! Circuit padding implementation.
Comment the machine rate limiting clamp.
fixup! Circuit padding implementation.
Simplify circpad_handle_padding_negotiate().
fixup! Circuit padding header.
Fix negotiation return value issue.
fixup! Circuit padding implementation.
Fix negotiation failure due to not checking return values.
fixup! Circuit padding header.
Invert transition array.
fixup! Circuit padding implementation.
Invert transition array.
fixup! Circuit padding tests.
Invert transition array.
Show resolved Hide resolved PADDING_TODO.txt
Show resolved Hide resolved PADDING_TODO.txt
}

if (i == state->histogram_len-1) {
if (state->token_removal != CIRCPAD_TOKEN_REMOVAL_NONE &&

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Nov 28, 2018

Member

I'm a bit confuse on how the token removal logic works. I don't think it corresponds to what 3.1.2 in prop254 says. In particular, why is there an explicit rule here about removing tokens from the infinity bin? Why is this not handled in the token removal functions?

Furthermore, we also remove tokens from bins in circpad_send_padding_cell_for_callback() regardless of what our token removal strategy is, even tho the spec says If token removal is enabled, when a padding packet is sent, a token is removed from the bin corresponding to the target delay..

Also, the spec seems to imply that special token removal rules only apply when the exact bin has no tokens left. I don't see where this logic is in the code. I just see us applying the token removal rule, when we send a non-padding cell.

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Dec 7, 2018

Author Contributor

I'm a bit confuse on how the token removal logic works. I don't think it corresponds to what 3.1.2 in prop254 says. In particular, why is there an explicit rule here about removing tokens from the infinity bin? Why is this not handled in the token removal functions?

The infinity bin has to have its tokens removed as soon as "infinite" delay is chosen. This is because there is no callback for it. For all other bins, tokens are removed upon the padding callback, or if a non-padding cell is sent before the callback. We could have defined the infinity token removal to happen only if no non-padding cell ever is sent.. But those semantics are both more complicated, and not as useful for having a fixed number of "infinite" padding delays in a state. Does this make sense?

Furthermore, we also remove tokens from bins in circpad_send_padding_cell_for_callback() regardless of what our token removal strategy is, even tho the spec says If token removal is enabled, when a padding packet is sent, a token is removed from the bin corresponding to the target delay..

This is correct, and the implementation does in fact match the spec. So long as the token removal strategy is not NONE, then we will have a mutable histogram in the machine info. For all token removal strategies other than NONE, the padding case is the same -- remove from exactly the bin corresponding to the chosen delay. This bin should not be empty by definition -- otherwise padding could not have been scheduled for this bin's delay value.. Hence the asserts before token removal upon actually sending the padding cell.

Also, the spec seems to imply that special token removal rules only apply when the exact bin has no tokens left. I don't see where this logic is in the code. I just see us applying the token removal rule, when we send a non-padding cell.

Every token removal rule other than NONE will try to remove from exactly the bin corresponding to the delay value that passed. It is only if this bin is empty do they apply their strategy. I could see an argument for factoring out this case and only applying the strategy-specific code if the target bin is in fact empty, but this mans either duplicating the target bin calculations, or passing the target bin weirdly between a bunch of the helper functions...

@torproject torproject deleted a comment from nmathewson Dec 11, 2018

@torproject torproject deleted a comment from nmathewson Dec 17, 2018

* to be considered to match the conditions.
*/
typedef struct circpad_machine_conditions_t {
/** Only apply the machine *if* the circuit has at least this many hops */

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Dec 17, 2018

Member

Addressed in 666368e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.