Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#22702 closed defect (fixed)

Never send a consensus which the downloader already has

Reported by: nickm Owned by: ahf
Priority: High Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay directory regression
Cc: Actual Points: 3
Parent ID: Points: 3
Reviewer: Sponsor: Sponsor4

Description

FWICT, we have a regression in our new consensus code: we no longer check the if-modified-since date when serving consensuses, since that field isn't set in the consensus_cache_entry_t case of .

Additionally, if somebody says X-Or-Diff-From the consensus which we currently have, then instead of telling them "That's up-to-date", we also send the current consensus. That's also bad.

These problems make the most trouble in test networks, where the consensus update rate is so fast that relays frequently try to download a consensus before it's there.

Diagnosed with help from ahf.

Child Tickets

Change History (10)

comment:1 Changed 16 months ago by nickm

Owner: set to ahf
Status: newassigned

comment:2 Changed 16 months ago by ahf

Additionally, if somebody says X-Or-Diff-From the consensus which we currently have, then instead of telling them "That's up-to-date", we also send the current consensus. That's also bad.

What is meant here by "which we currently have"? Prop#140 allows a client to send multiple hashes of the consensuses they have, but our current request sending code only ever sends one hash, but our request receive path handles multiple hashes (which is according to the proposal).

Should we check if the digest_sha3_as_signed (from networkstatus_t) in our currently "live" consensus document, of the given client requested flavour, matches any one of the set of hashes the client sends to us in the X-Or-Diff-From-Consensus header - or should we only handle the case where a client sends one hash that happens to match the digest_sha3_as_signed? Or something entirely different?

comment:3 Changed 16 months ago by ahf

comment:4 in reply to:  2 Changed 16 months ago by nickm

Replying to ahf:

Additionally, if somebody says X-Or-Diff-From the consensus which we currently have, then instead of telling them "That's up-to-date", we also send the current consensus. That's also bad.

What is meant here by "which we currently have"? Prop#140 allows a client to send multiple hashes of the consensuses they have, but our current request sending code only ever sends one hash, but our request receive path handles multiple hashes (which is according to the proposal).

If they include a hash that indicates that they would accept a diff _from_ the most recent consensus, then they have the most recent consensus, and we should not send it to them.

Should we check if the digest_sha3_as_signed (from networkstatus_t) in our currently "live" consensus document, of the given client requested flavour, matches any one of the set of hashes the client sends to us in the X-Or-Diff-From-Consensus header - or should we only handle the case where a client sends one hash that happens to match the digest_sha3_as_signed? Or something entirely different?

I think it's an "any" condition: if they say they have the most recent consensus, we shouldn't send them any diff.

comment:5 Changed 16 months ago by ahf

Great. I'll implement that on top of the current patch and restart the measurement - should give us enough data by tonight (UTC).

comment:6 Changed 16 months ago by ahf

Status: assignedneeds_review

comment:7 Changed 16 months ago by nickm

Status: needs_reviewneeds_revision

Requested a function rename.

Also, this needs a changes file.

comment:8 Changed 16 months ago by ahf

Status: needs_revisionneeds_review

I've added a fixup commit and added a changes file.

comment:9 Changed 16 months ago by nickm

Points: 13
Resolution: fixed
Status: needs_reviewclosed

Looks good, except that this branch was based on master. Next time you're fixing a bug on 0.3.1, remember to base it on maint-0.3.1.

This time, it rebased cleanly. I've squashed and rebased as ahf_bugs_22702_squashed, and I'm merging it to maint-0.3.1 and forward. Thanks!

comment:10 Changed 16 months ago by nickm

Actual Points: 3
Note: See TracTickets for help on using tickets.