#26110 closed defect (fixed)

CIRC_BW fields vague

Reported by: atagar Owned by:
Priority: Low Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: spec
Cc: mikeperry Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

Hi lovely network team folks. Yesterday Mike documented CIRC_BW's new fields...

https://gitweb.torproject.org/torspec.git/commit/?id=fbb38ec

This is great, but this isn't quite enough for me to grok what they are. "Bytes read and written on a circuit" is simple and clear, but "the total length of the unused overhead portion of valid relay cells" doesn't make sense to me.

I'll submit a patch in a minute I find clearer giving a rough guess at what I think these are, *but* my description will no doubt be inaccurate. Hopefully together we'll be able to come up with something both clear and correct. :)

Child Tickets

Change History (12)

comment:1 Changed 21 months ago by nickm

Cc: mikeperry added
Keywords: spec added
Milestone: Tor: 0.3.4.x-final

comment:2 Changed 21 months ago by atagar

Cc: mikeperry removed
Keywords: spec removed
Milestone: Tor: 0.3.4.x-final
Status: newneeds_review

Proposed patch pushed to the ticket26110 branch of my repo...

https://gitweb.torproject.org/user/atagar/torspec.git/commit/?h=ticket26110

Network team, what are your thoughts?

comment:3 Changed 21 months ago by atagar

Cc: mikeperry added
Keywords: spec added
Milestone: Tor: 0.3.4.x-final

Damn trac. Didn't mean to revert Nick's changes. Adding 'em back.

comment:4 Changed 21 months ago by nickm

Reviewer: mikeperry

Setting Mike as the logical reviewer.

comment:5 Changed 21 months ago by atagar

Stem support for these new fields added: https://gitweb.torproject.org/stem.git/commit/?id=aa692e6

Only thing pending this are the attribute descriptions (filled in TBD for now).

comment:6 Changed 20 months ago by mikeperry

Status: needs_reviewneeds_revision

There's a couple things that are not quite right here, as a consequence of what I needed from these fields:

Circuit data falls into two categories: delivered payloads and overhead.

Technically, there are two high level categories: "validated by Tor, and unvalidated by Tor." Data that is not checked for correctness or fails a check for correctness is not counted as valid delivered bytes *or* overhead. Of that data that is checked for correctness and is correct, it falls into delivered payloads and overhead, and is counted in those fields.

+  The sum of DeliveredBytesRead and OverheadBytesRead SHOULD match BytesRead,
+  and same for their written counterparts.

As a result of the first issue, the closest thing we can say here is:

+  The sum of DeliveredBytesRead and OverheadBytesRead MUST be less than or equal to BytesRead,
+  and same for their written counterparts. This sum represents the total validated bytes sent
+  on the circuit.

I am not sure how we want to explain the hierarchy of valid/invalid and delivered/overhead. It seems a little tricky.

comment:7 Changed 20 months ago by atagar

Thanks Mike. There's probably some context here I'm missing. Lets step back a sec: what are these fields useful for? Are they useful to control port users, or were they added for a specific Network team task?

comment:8 Changed 20 months ago by mikeperry

Status: needs_revisionneeds_review

Ok atagar, how is this: https://gitweb.torproject.org/user/mikeperry/torspec.git/commit/?h=ticket26110
(it is a fixup to your commit).

For those who missed our IRC conversation -- the fields were defined the way they are so that they could be used both to measure overhead due to extra space in cells, as well as "invalid" overhead due to cells that Tor has decided to drop/ignore due to protocol errors or padding. Defining the delivered+overhead values as covering only valid data allows us to determine overhead due to slack cell space as well as invalid/dropped cells.

Last edited 20 months ago by mikeperry (previous) (diff)

comment:9 Changed 20 months ago by atagar

Status: needs_reviewmerge_ready

Thanks Mike! Looks good to me.

comment:10 Changed 20 months ago by mikeperry

Woops - I remembered one more detail. The READ and WRITE fields do not include circuit headers, and the Delivered and Overhead fields do not include relay cell headers. I pushed another fixup for this to that branch.

comment:11 Changed 20 months ago by atagar

Reviewer: mikeperrynickm

comment:12 Changed 20 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.