Opened 2 years ago

Closed 2 years ago

#22149 closed defect (implemented)

prop140: Update dir-spec with prop140 protocols

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201705
Cc: Actual Points: .3
Parent ID: #13339 Points: .3
Reviewer: Sponsor: Sponsor4

Description


Child Tickets

Change History (9)

comment:1 Changed 2 years ago by nickm

Status: newaccepted

comment:2 Changed 2 years ago by nickm

Actual Points: .3
Keywords: TorCoreTeam201705 added
Status: acceptedneeds_review

See my dirspec branch prop140_completed

comment:3 Changed 2 years ago by catalyst

Looks reasonable to me at first glance. I haven't exhaustively checked how faithfully the new dir-spec.txt reflects the prop140 content. I just noticed that the following diff chunk seems like a possible typo, though.

@@ -216,7 +217,7 @@ Status: Accepted
 
   The following parameters govern how relays and clients use this protocol.
 
-     min-consensuses-age-to-cache-for-diff
+b     min-consensuses-age-to-cache-for-diff
        (min 0, max 744, default 6)
      max-consensuses-age-to-cache-for-diff
        (min 0, max 8192, default 72)

comment:4 Changed 2 years ago by ahf

Status: needs_reviewmerge_ready

Generally looks good. Two questions:

1) This patch marks prop140 as closed, but do we support the /tor/status-vote/current/consensus/diff/<HASH>/<FPRLIST>.z paths already?

2) This is in the nitpick category. Do we want to change the delimiter in X-Or-Diff-From-Consensus to use "," and optionally " " like in the rest of HTTP?

Catalyst also commented on a possible typo above.

comment:5 in reply to:  4 Changed 2 years ago by nickm

Replying to ahf:

Generally looks good. Two questions:

1) This patch marks prop140 as closed, but do we support the /tor/status-vote/current/consensus/diff/<HASH>/<FPRLIST>.z paths already?

Not until we merge one of the branches in #22149 ; I'm not planning to merge this branch until #22149 is done

2) This is in the nitpick category. Do we want to change the delimiter in X-Or-Diff-From-Consensus to use "," and optionally " " like in the rest of HTTP?

Huh. I think right now it allows either in the code; I guess we should change the spec to match.

Catalyst also commented on a possible typo above.

Agreed, will fix before merge.

comment:6 Changed 2 years ago by ahf

The code does:

3661   smartlist_split_string(hex_digests, hdr, " ",
3662                          SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, -1);

So no ",".

comment:7 Changed 2 years ago by nickm

ok. IMO it's fine to have it start accepting commas, since currently nothing sends more than one X-Or-Diff-From-Consensus value anyway.

comment:8 Changed 2 years ago by nickm

branch updated; I should do a corresponding update for the comma issue once the other prop#278 and prop#140 stuff is merged

comment:9 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

squashed and merged

Note: See TracTickets for help on using tickets.