#20823 closed defect (fixed)

[controller, prop271] GETINFO support for new guard selection logic

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-guard regression TorCoreTeam201612
Cc: Actual Points: .1 so far
Parent ID: #20822 Points: 1
Reviewer: Sponsor:

Description


Child Tickets

Change History (10)

comment:1 Changed 11 months ago by dgoulet

Keywords: tor-guard added

comment:2 Changed 11 months ago by nickm

Priority: MediumHigh

comment:3 Changed 11 months ago by nickm

Keywords: regression added

comment:4 Changed 11 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:5 Changed 11 months ago by nickm

Keywords: TorCoreTeam201612 added
Points: 1

comment:6 Changed 11 months ago by nickm

Actual Points: .1 so far
Status: acceptedneeds_review

I've done a minimal version of this in a branch called "bug20823". It's a branch on top of prop271_030_v1. It tries to shove the new guards into the GETINFO API that the old guards supported.

I think there should also be a non-legacy controller GETINFO API that exposes this information more sensibly.

comment:7 Changed 10 months ago by chelseakomlo

Hey,

This is a small patch, overall it looks good!

I do get a test failure when I run make test:

entrynodes/retry_unreachable: [forking] 
  FAIL src/test/test_entrynodes.c:2302: assert(g1->is_reachable OP_EQ GUARD_REACHABLE_NO): 2 vs 0
  [retry_unreachable FAILED]

as well as a bug assertion:

dir/purpose_needs_anonymity_returns_true_by_default: Dec 13 20:52:30.319 [warn] purpose_needs_anonymity(): Bug: Called with dir_purpose=0, router_purp
ose=0 (on Tor 0.3.0.0-alpha-dev 7a7240716eef1f54)
OK

Not sure if you get these as well? I don't see these on master.

One point about how we switch on whether we are running the legacy or post-271 algorithm.

If you define two functions- one for the legacy behavior and one for the post-271 behavior, then when you remove the legacy algorithm, you won't need to change the post-271 method signature or implementation when you remove support for legacy in the future.

So it could look something like:

STATIC char *                                                                   
legacy_getinfo_helper_format_single_entry_guard(const entry_guard_t *e) 

STATIC char *                                                                   
getinfo_helper_format_single_entry_guard(const entry_guard_t *e) 

for the method signatures. The caller would then look like:

SMARTLIST_FOREACH_BEGIN(guards, const entry_guard_t *, e) {
  char *cp;
  #ifdef ENABLE_LEGACY_GUARD_ALGORITHM
    cp = legacy_getinfo_helper_format_single_entry_guard(e);
  #else 
    cp = getinfo_helper_format_single_entry_guard(e);
  #endif
  smartlist_add(sl, cp);
} SMARTLIST_FOREACH_END(e);


Again, this is minor and it might be less work to leave this as is.

comment:8 Changed 10 months ago by chelseakomlo

Status: needs_reviewneeds_revision

comment:9 in reply to:  7 Changed 10 months ago by chelseakomlo

I do get a test failure when I run make test:

Apologies- after cleaning & re-checking out the branch, all tests are passing.

comment:10 Changed 10 months ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Merging; thanks for the re-check!

Note: See TracTickets for help on using tickets.