Opened 8 years ago

Closed 5 years ago

#2678 closed enhancement (wontfix)

bridgedb /16 filtering needs review

Reported by: karsten Owned by:
Priority: Medium Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords:
Cc: nickm, aagbsn, kaner, Matthew.Finkel@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Someone should review aagbsn's filter16 branch:

https://github.com/aagbsn/bridgedb/tree/filter16

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by karsten

Cc: nickm aagbsn kaner added

A few comments:

  • Do we want to enforce that we only return one bridge in the same /16 for all current distributors, that is, https, email, and file buckets? aagbsn says yes for the former 2, but is unsure regarding the latter (buckets). kaner, what's your opinion here?
  • aagbsn's patch removes bridges from the results that are in the same /16 as other bridges, but it doesn't add new bridges to the result.  Shouldn't we try to return the requested number of bridges? aagbsn is concerned about making it easier for people to mine BridgeDB, whereas I'm concerned that people won't understand why they only get 2 bridges when they usually get 3.
  • Should we avoid removing bridges in the same /16 if that would mean we cannot return the requested number of bridges (say, we have 3 bridges in a ring with two being in the same /16; should we return just 2 bridges or all 3)? aagbsn thinks we should return just 2 bridges. I would like to hear nickm's opinion here.
  • The new unit tests contain IP addresses starting with 123.321. which are obviously not valid. The unit tests still succeed, but we should change the addresses to something valid.

comment:2 in reply to:  1 ; Changed 8 years ago by kaner

Replying to karsten:

  • Do we want to enforce that we only return one bridge in the same /16 for all current distributors, that is, https, email, and file buckets? aagbsn says yes for the former 2, but is unsure regarding the latter (buckets). kaner, what's your opinion here?

My opinion: We want to return all bridges for file buckets verbatim, without filtering for /16s.

comment:3 in reply to:  2 ; Changed 8 years ago by karsten

Replying to kaner:

My opinion: We want to return all bridges for file buckets verbatim, without filtering for /16s.

Sounds reasonable, because whatever distributes the bridges from a bucket should do the additional filtering, if desired. OK.

comment:4 in reply to:  3 Changed 8 years ago by aagbsn

Replying to karsten:

Replying to kaner:

My opinion: We want to return all bridges for file buckets verbatim, without filtering for /16s.

Sounds reasonable, because whatever distributes the bridges from a bucket should do the additional filtering, if desired. OK.

Agreed. Bridgedb should not make assumptions about how the bridges are allocated from buckets.

comment:5 in reply to:  1 Changed 8 years ago by aagbsn

Replying to karsten:

A few comments:

  • Do we want to enforce that we only return one bridge in the same /16 for all current distributors, that is, https, email, and file buckets? aagbsn says yes for the former 2, but is unsure regarding the latter (buckets). kaner, what's your opinion here?
  • aagbsn's patch removes bridges from the results that are in the same /16 as other bridges, but it doesn't add new bridges to the result.  Shouldn't we try to return the requested number of bridges? aagbsn is concerned about making it easier for people to mine BridgeDB, whereas I'm concerned that people won't understand why they only get 2 bridges when they usually get 3.

Agreed that people may be confused, though this may also be the case when bridges are blocked (see the filterblocked patches #1837 & #1608).

  • Should we avoid removing bridges in the same /16 if that would mean we cannot return the requested number of bridges (say, we have 3 bridges in a ring with two being in the same /16; should we return just 2 bridges or all 3)? aagbsn thinks we should return just 2 bridges. I would like to hear nickm's opinion here.
  • The new unit tests contain IP addresses starting with 123.321. which are obviously not valid. The unit tests still succeed, but we should change the addresses to something valid.

Fixed.

comment:6 Changed 6 years ago by sysrqb

Is there anything I can work on for this ticket? Is this bug completed? Waiting for Nick's input?

comment:7 Changed 6 years ago by aagbsn

IIRC this was implemented but due to changes added for pluggable transport and multiple or-addresses it needs rethinking.

comment:8 Changed 6 years ago by sysrqb

Cc: Matthew.Finkel@… added

hrm, ok. I know there's a comment in the bridge selection code that says it does this, but it seems like it's misplaced. But I do think I remember seeing a check for this somewhere else, though. I have a feeling, in any case, we're going to want to come back to this as we get further into #7520.

comment:9 in reply to:  8 Changed 5 years ago by isis

Resolution: wontfix
Status: newclosed

Replying to sysrqb:

hrm, ok. I know there's a comment in the bridge selection code that says it does this, but it seems like it's misplaced. But I do think I remember seeing a check for this somewhere else, though. I have a feeling, in any case, we're going to want to come back to this as we get further into #7520.


That comment is on this line, FWIW.

And yes, the comment is misplaced. The line directly underneath it just takes all the bridges in the splitter. I'm not sure if implementing this is still a good idea, given that we have volunteer bridge operators running bridges very close to each other, esp. for PTs, and implementing this would likely severely decrease the bridges available to a user.

Note: See TracTickets for help on using tickets.